sound: Fix oss_sysinfo->nummixers

[christos]

Summary

mixer(8)'s -a option is used to print information about all mixer
devices in the system. To do this, it loops from 0 to
mixer_get_nmixers(), and tries to open "/dev/mixer%d". However, this
approach doesn't work when there are disabled/unregistered mixers in the
system, or when an audio device simply doesn't have a mixer.

mixer_get_nmixers() calls SNDCTL_SYSINFO and returns
oss_sysinfo->nummixers, whose value is the number of currently _enabled_
mixers only. Taking the bug report mentioned below (277615) as an
example, suppose a system with 8 mixer devices total, but 3 of them are
either disabled or non-existent, which means they will not show up under
/dev, meaning we have 5 enabled mixer devices, which is also what the
value of oss_sysinfo->nummixers will be. What mixer(8) will do is loop
from 0 to 5 (instead of 8), and start calling mixer_open() on
/dev/mixer0, up to /dev/mixer4, and as is expected, the first call will
fail right away, hence the error shown in the bug report.

To fix this, modify oss_sysinfo->nummixers to hold the value of the
maximum unit in the system, which, although not necessarily "correct",
is more intuitive for applications that will want to use this value to
loop through all mixer devices.

It has to be noted, that other applications, apart from mixer(8), suffer
from this.

PR: 277615
Sponsored by: The FreeBSD Foundation
MFC after: 3 days

[christos]

@dev_submerge.ch Can you test that this and D45151 fix the problem? I cannot reproduce it right now.

[dev_submerge.ch]

In D45150#1029901, @christos wrote:

@dev_submerge.ch Can you test that this and D45151 fix the problem? I cannot reproduce it right now.

The mixer does see devices now that come after the unavailable ones. But it shows no device description for it.

We're missing a piece of the puzzle, as mixer_oss_mixerinfo() still skips unavailable devices. Which means SNDCTL_MIXERINFO returns an error instead of a device with enabled == 0, thus breaking the ossinfo utility from audio/oss, and probably other software using the same approach.

[christos]

In D45150#1029983, @dev_submerge.ch wrote:

In D45150#1029901, @christos wrote:

@dev_submerge.ch Can you test that this and D45151 fix the problem? I cannot reproduce it right now.

The mixer does see devices now that come after the unavailable ones. But it shows no device description for it.

We're missing a piece of the puzzle, as mixer_oss_mixerinfo() still skips unavailable devices. Which means SNDCTL_MIXERINFO returns an error instead of a device with enabled == 0, thus breaking the ossinfo utility from audio/oss, and probably other software using the same approach.

That's right. So before I implement this I want to make sure we agree on what each oss_mixerinfo field should contain:

dev	loop counter
id	mixer<loop counter> (unavailable)
name	Perhaps something like pcm<loop_counter>:mixer (unavailable)
modify_counter	0
card_number	loop counter
port_number	0
enabled	0
caps	Not supported.
nrext	Not supported.
priority	Not supported.
devnode	Nothing.
legacy_device	loop counter

I am not really sure whether we want to include the mixer name in id and name or simply leave it blank, or unavailable.

Also, in oss_mixerinfo() there are two cases where we can skip a device:

1. if (!PCM_REGISTERED(d) || PCM_DETACHING(d))
2. if (d->mixer_dev != NULL && d->mixer_dev->si_drv1 != NULL && ((mi->dev == -1 && d->mixer_dev == i_dev) || mi->dev == i))

I think it's better to handle both cases the same way, that is, return a structure with the values described above, instead of handling them differently.

[christos]

Also it seems like this fallback mechanism is needed by a few other ioctls, so I think it's better to fix all of them in a single patch.

[dev_submerge.ch]

In D45150#1030167, @christos wrote:

In D45150#1029983, @dev_submerge.ch wrote:

We're missing a piece of the puzzle, as mixer_oss_mixerinfo() still skips unavailable devices. Which means SNDCTL_MIXERINFO returns an error instead of a device with enabled == 0, thus breaking the ossinfo utility from audio/oss, and probably other software using the same approach.

That's right. So before I implement this I want to make sure we agree on what each oss_mixerinfo field should contain:

dev	loop counter
id	mixer<loop counter> (unavailable)
name	Perhaps something like pcm<loop_counter>:mixer (unavailable)
modify_counter	0
card_number	loop counter
port_number	0
enabled	0
caps	Not supported.
nrext	Not supported.
priority	Not supported.
devnode	Nothing.
legacy_device	loop counter

I am not really sure whether we want to include the mixer name in id and name or simply leave it blank, or unavailable.

I would include the mixer name both in id and name, as those fields may be shown to the user in a list of mixer devices. It's more obvious than leaving them blank. I think your suggested strings are fine, except that id is only 16 bytes. Maybe mixer<loop counter> (n/a)?

The other values look good to me.

Also, in oss_mixerinfo() there are two cases where we can skip a device:

1. if (!PCM_REGISTERED(d) || PCM_DETACHING(d))
2. if (d->mixer_dev != NULL && d->mixer_dev->si_drv1 != NULL && ((mi->dev == -1 && d->mixer_dev == i_dev) || mi->dev == i))

I think it's better to handle both cases the same way, that is, return a structure with the values described above, instead of handling them differently.

Not sure I understand the question here. I think I would use ((mi->dev == -1 && d->mixer_dev == i_dev) || mi->dev == i) as an outer if to select the requested device, and then the other indicators to decide whether the requested device is available or not. Does that make sense?

[christos]

In D45150#1030208, @dev_submerge.ch wrote:

In D45150#1030167, @christos wrote:

In D45150#1029983, @dev_submerge.ch wrote:

We're missing a piece of the puzzle, as mixer_oss_mixerinfo() still skips unavailable devices. Which means SNDCTL_MIXERINFO returns an error instead of a device with enabled == 0, thus breaking the ossinfo utility from audio/oss, and probably other software using the same approach.

That's right. So before I implement this I want to make sure we agree on what each oss_mixerinfo field should contain:

dev	loop counter
id	mixer<loop counter> (unavailable)
name	Perhaps something like pcm<loop_counter>:mixer (unavailable)
modify_counter	0
card_number	loop counter
port_number	0
enabled	0
caps	Not supported.
nrext	Not supported.
priority	Not supported.
devnode	Nothing.
legacy_device	loop counter

I am not really sure whether we want to include the mixer name in id and name or simply leave it blank, or unavailable.

I would include the mixer name both in id and name, as those fields may be shown to the user in a list of mixer devices. It's more obvious than leaving them blank. I think your suggested strings are fine, except that id is only 16 bytes. Maybe mixer<loop counter> (n/a)?

The other values look good to me.

Also, in oss_mixerinfo() there are two cases where we can skip a device:

1. if (!PCM_REGISTERED(d) || PCM_DETACHING(d))
2. if (d->mixer_dev != NULL && d->mixer_dev->si_drv1 != NULL && ((mi->dev == -1 && d->mixer_dev == i_dev) || mi->dev == i))

I think it's better to handle both cases the same way, that is, return a structure with the values described above, instead of handling them differently.

Not sure I understand the question here. I think I would use ((mi->dev == -1 && d->mixer_dev == i_dev) || mi->dev == i) as an outer if to select the requested device, and then the other indicators to decide whether the requested device is available or not. Does that make sense?

D45256. We can go ahead with this patch I think.

[dev_submerge.ch]

The patch is fine as is, but it does not fully address the issue described in the commit message, only D45256 does actually resolve it. You could either merge the two commits (and commit messages) when D45256 is ready, or adapt the commit messages to describe what part of the issue is addressed in each.

[christos]

Merged with D45256.