sound: Untangle dsp_cdevs[] and dsp_unit2name() confusion

[christos]

Summary

Before de8c0d15a64fa ("sound: Get rid of snd_clone and use
DEVFS_CDEVPRIV(9)"), sound(4) would create one device for each allocated
channel. The device names would be chosen from dsp_cdevs[], and created
with dsp_unit2name(). Additionally, dsp_cdevs[] was also used to match
these devices names, as well as OSSv4 aliases in dsp_clone().

Since sound(4) does not create separate devices for each channel
anymore, the meaning and use dsp_cdevs[] has changed. Part of it no
longer corresponds to devices at all, but instead is used to create
channel names, and another part is used to match only OSSv4 aliases in
dsp_clone().

To address this confusion, separate dsp_cdevs[] into a dsp_aliases[]
array, and move dsp_unit2name() to pcm/channel.c and rename it to
chn_mkname().

While here, get rid of the SND_DEV_DSPHW_* channel types, and simply use
the existing PCMDIR_* constants as the channel types. There is no need
to duplicate the same meaning twice.

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[christos]

Make some cases fallthrough in chn_init() switch statement.

[dev_submerge.ch]

This simplifies the code quite a bit, nice! Some nitpicks in the inline comments.

sys/dev/sound/pcm/channel.c

1178–1186

Is there still a reason to keep the chn_names array like this when all access goes through chn_mkname()? Could be mapped in a switch maybe?

1221–1222

If I read style(9) correctly, there should be fallthrough comment:

Elements in a switch statement that cascade should have a FALLTHROUGH comment.

1225–1226

Same here.

sys/dev/sound/pcm/dsp.c

2240–2244

Is there a need to create the channel name here? I don't see devname or buf used in any way, except for a check that it's not NULL. Like this it's just a costly check of the channel type, no?

[christos]

Address Florian's comments:

• Get rid of chn_names[] and integrate it into a switch statement in chn_mkname().
• Refactor dsp_oss_engineinfo() to not use chn_mkname(). Should give us some performance boost as well.

[christos]

s/rec/record/ in chn_mkname().

[dev_submerge.ch]

sys/dev/sound/pcm/channel.c

1188

IMHO these would be more readable as "dsp%d.play.%d" quasi template strings. Not really important though.

sys/dev/sound/pcm/dsp.c

2258–2259

Shouldn't we continue to iterate through the pcm units here?

[christos]

sys/dev/sound/pcm/channel.c

1188

This way we'd duplicate dsp%d...%d in all the switch cases, though.

sys/dev/sound/pcm/dsp.c

2258–2259

Yes, I updated the diff in my local tree yesterday, but forgot to update it here as well.

[christos]

Remove redundant chn_mkname() call in vchan_passthrough().

[dev_submerge.ch]

sys/dev/sound/pcm/channel.c

1188

Alternatively, move the separating dots into the printf literal: "dsp%d.%s.%d"

sys/dev/sound/pcm/dsp.c

2258–2259

Ok, but don't we also have to unlock the CHN_LOCK(ch); here?

[dev_submerge.ch]

sys/dev/sound/pcm/dsp.c

2258–2259

My bad, forget about last comment - the question is whether ch is set to NULL when the channel is not found?

[christos]

sys/dev/sound/pcm/channel.c

1188

I agree with this.

sys/dev/sound/pcm/dsp.c

2258–2259

It is.

[christos]

Address Florian's comments for chn_mkname().

[dev_submerge.ch]

sys/dev/sound/pcm/dsp.c

2258–2259

Indeed, it's set to NULL by the SLIST implementation. Is that behavior guaranteed or documented somewhere? Don't know about FreeBSD src customs, but in other projects I wouldn't rely on that if it's not part of the documented interface.

[christos]

sys/dev/sound/pcm/dsp.c

2258–2259

I see that it's documented for TAILQ_FOREACH, but I am not sure why it's not for the other queue(3) types.
@des @imp Any info on this?

[emaste]

sys/dev/sound/pcm/channel.c

1221–1222

I guess style(9) should be more explicit - FALLTHROUGH is needed when it otherwise looks like a break is missing, but not for multiple cases that share the same body. Note that style(9)'s FALLTHROUGH example has code for case 'a': and the FALLTHROUGH indicates that it's intentional to fall through to the case 'b': code. So leave these ones out.

Submitted D47242 for a style.9 update.

[christos]

Remove "FALLTHROUGH".

[dev_submerge.ch]

@emaste: Thanks for the clarification on style(9)'s FALLTHROUGH part.

@christos: Sorry for the misleading comment.

[christos]

So is the patch good to go?

[dev_submerge.ch]

In D47199#1077635, @christos wrote:

So is the patch good to go?

I'm still undecided on the CHN_FOREACH() postcondition of ch == NULL. Tried to find precedence, but I only see implementations that set additional variables like found = true;. Although unlikely to change, this may result in a really hard to spot error when the postcondition doesn't hold.

[christos]

In D47199#1077896, @dev_submerge.ch wrote:

In D47199#1077635, @christos wrote:

So is the patch good to go?

I'm still undecided on the CHN_FOREACH() postcondition of ch == NULL. Tried to find precedence, but I only see implementations that set additional variables like found = true;. Although unlikely to change, this may result in a really hard to spot error when the postcondition doesn't hold.

Both imp@ and markj@ confirmed that this behavior is more or less guaranteed, and plenty of code relies on that assumption already.