sound: Make sure chn_init() produces unique unit numbers

[christos]

Summary

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[dev_submerge.ch]

This only works when the vchans per type are sorted by unit, in ascending order, right? Aren't the vchans sorted in descending order?

Also I would mention the dependency on D46549 in the commit message, and maybe add a comment.

sys/dev/sound/pcm/channel.c

1205–1206

Why a while() loop?

[christos]

In D46550#1063518, @dev_submerge.ch wrote:

This only works when the vchans per type are sorted by unit, in ascending order, right? Aren't the vchans sorted in descending order?

VCHANs are also sorted in ascending order in the channel list stored in snddev_info since vchan_create() calls pcm_chn_add(), which in turn calls CHN_INSERT_SORT_ASCEND(). The descending sort happens in the children list of pcm_channel.

Also I would mention the dependency on D46549 in the commit message, and maybe add a comment.

Will add it.

[christos]

In D46550#1063996, @christos wrote:

In D46550#1063518, @dev_submerge.ch wrote:

This only works when the vchans per type are sorted by unit, in ascending order, right? Aren't the vchans sorted in descending order?

VCHANs are also sorted in ascending order in the channel list stored in snddev_info since vchan_create() calls pcm_chn_add(), which in turn calls CHN_INSERT_SORT_ASCEND(). The descending sort happens in the children list of pcm_channel.

To add to this: This patch works only when snddev_info's (not just VCHANs) channel list is sorted in ascending order. If pcm_chn_add() is (for some reason) modified to sort channels in descending order, then with this patch, after a clean module load, we'd end up all with channel units after 0 being 1. The reason for this is that if we start with unit=0 and a layout like p2, p1, p0, we'll hit 0 (i.e the first same unit) last, and we'll only increment unit once. Even though I do not think someone will modify pcm_chn_add(), I will improve the patch to be more robust and work in both orders.

[christos]

So, making this work when the list is sorted in descending order is quite more involved... The only viable solution I can think of is the following: 1) change from SLIST to TAILQ so that we can have TAILQ_FOREACH_REVERSE(), 2) introduce a CHN_FOREACH_REVERSE() macro, 3) store the sort type (i.e ascending or descending) in snddev_info, 4) check this sort type in chn_init(), and if it's ascending order, do what the patch does already, otherwise use CHN_FOREACH_REVERSE() instead instead of CHN_FOREACH(), so that the same code can keep working.

However... is there a point in solving this in the first place? The sorting is hardcoded in pcm_chn_add() and as long as someone does not deliberately change it, there shouldn't be any problems. After all, there is no good reason to sort this list in descending order. I think the fix will introduce enough complexity for a case that will most likely never occur. I think a comment in pcm_chn_add() and chn_init() should do. @dev_submerge.ch What do you think?

sys/dev/sound/pcm/channel.c

1205–1206

I've changed this to an if.

[christos]

An alternative approach using unr(9). Works with both ascending and descending order: https://reviews.freebsd.org/D46680

[dev_submerge.ch]

In D46550#1063996, @christos wrote:

In D46550#1063518, @dev_submerge.ch wrote:

This only works when the vchans per type are sorted by unit, in ascending order, right? Aren't the vchans sorted in descending order?

VCHANs are also sorted in ascending order in the channel list stored in snddev_info since vchan_create() calls pcm_chn_add(), which in turn calls CHN_INSERT_SORT_ASCEND(). The descending sort happens in the children list of pcm_channel.

I was looking at the wrong channel list, sorry for the noise.

In D46550#1064017, @christos wrote:

However... is there a point in solving this in the first place? The sorting is hardcoded in pcm_chn_add() and as long as someone does not deliberately change it, there shouldn't be any problems. After all, there is no good reason to sort this list in descending order. I think the fix will introduce enough complexity for a case that will most likely never occur. I think a comment in pcm_chn_add() and chn_init() should do. @dev_submerge.ch What do you think?

I know you already did the unr(9) solution, but for the records: It would be ok if this part here requires ascending order in the channel list, as long as that requirement is explained in a comment in pcm_chn_add().

[christos]

Go with D46680