sound: Remove nmix variable from mixer_oss_mixerinfo()

[christos]

Summary

nmix is used to compare against mixerinfo->dev, which is a user-supplied
value to select the mixer device (if not -1, in which case we'll select
the default one) we want to fetch the information of. It is also used to
set mixerinfo->dev in case it is -1.

However, nmix is at best redundant, since we have the loop counter
already (i), and confusing at worst.

For example, suppose a system with 3 mixer devices. We call
SNDCTL_MIXERINFO with mixerinfo->dev=1, meaning we want to get
information for /dev/mixer1. Suppose /dev/mixer0 detaches while inside
the loop, so we'll hit the loop's "continue" case, and nmix won't get
incremented (i.e will stay 0 for now). At this point nmix counts 1
device less, so when it reaches 1, we'll be fetching /dev/mixer2's
information instead of /dev/mixer1's.

Simply remove nmix and use the loop counter to both set mixerinfo->dev
and check against it in case a non -1 value is supplied.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

[emaste]

This seems sensible; I'd expect mixer 1 to refer to /dev/mixer1 not whatever the second mixer happens to be. I don't think a race condition like you describe is even required, right? Removing the device providing /dev/mixer0 means the system will always be in this state?

[christos]

In D45135#1029480, @emaste wrote:

This seems sensible; I'd expect mixer 1 to refer to /dev/mixer1 not whatever the second mixer happens to be. I don't think a race condition like you describe is even required, right? Removing the device providing /dev/mixer0 means the system will always be in this state?

I would also expect the behavior to be what you are describing (1 means /dev/mixer1 and so on). Regarding the race, it maybe be very hard to hit it, but I think even for correctness/semantics it's good to get rid of nmix.

[emaste]

Regarding the race, it maybe be very hard to hit it

I just mean that I think the race doesn't even matter because the issue is identical if you remove the device any time prior to the mixer_oss_mixerinfo call?

[christos]

In D45135#1029486, @emaste wrote:

Regarding the race, it maybe be very hard to hit it

I just mean that I think the race doesn't even matter because the issue is identical if you remove the device any time prior to the mixer_oss_mixerinfo call?

Yes.

[dev_submerge.ch]

Maybe unrelated to this particular piece of code, but I did some tests with two USB sound cards. When I remove the first one (pcm0), the mixer application reveals some flaws:

<flo@current:~> mixer -a
mixer: /dev/mixer0: no such mixer

<flo@current:~> mixer
pcm1:mixer: (play/rec) (default)
    vol       = 0.75:0.75     pbk
    pcm       = 0.75:0.75     pbk

Note that while the mixer default pcm is correct (pcm1), there's no sound card identifier like when I have both plugged in:

<flo@current:~> mixer -a
pcm0:mixer: <Roland EDIROL UA-25EX> on uaudio0 (play/rec)
    vol       = 0.75:0.75     pbk
    pcm       = 0.75:0.75     pbk
pcm1:mixer: <RME Babyface (23607206)> on uaudio1 (play/rec) (default)
    vol       = 0.75:0.75     pbk
    pcm       = 0.75:0.75     pbk

<flo@current:~> mixer
pcm1:mixer: <RME Babyface (23607206)> on uaudio1 (play/rec) (default)
    vol       = 0.75:0.75     pbk
    pcm       = 0.75:0.75     pbk

Possibly related bug report.

[christos]

This comment was removed by christos.

[christos]

In D45135#1029509, @dev_submerge.ch wrote:

Maybe unrelated to this particular piece of code, but I did some tests with two USB sound cards. When I remove the first one (pcm0), the mixer application reveals some flaws:

<flo@current:~> mixer -a
mixer: /dev/mixer0: no such mixer

<flo@current:~> mixer
pcm1:mixer: (play/rec) (default)
    vol       = 0.75:0.75     pbk
    pcm       = 0.75:0.75     pbk

Note that while the mixer default pcm is correct (pcm1), there's no sound card identifier like when I have both plugged in:

<flo@current:~> mixer -a
pcm0:mixer: <Roland EDIROL UA-25EX> on uaudio0 (play/rec)
    vol       = 0.75:0.75     pbk
    pcm       = 0.75:0.75     pbk
pcm1:mixer: <RME Babyface (23607206)> on uaudio1 (play/rec) (default)
    vol       = 0.75:0.75     pbk
    pcm       = 0.75:0.75     pbk

<flo@current:~> mixer
pcm1:mixer: <RME Babyface (23607206)> on uaudio1 (play/rec) (default)
    vol       = 0.75:0.75     pbk
    pcm       = 0.75:0.75     pbk

Possibly related bug report.

This seems to be a mixer(3) and mixer(8) issue.

This is how mixer(8) handles the -a option:

	/* Print all mixers and exit. */
	if (aflag) {
		if ((n = mixer_get_nmixers()) < 0)
			errx(1, "no mixers present in the system");
		for (i = 0; i < n; i++) {
			(void)snprintf(buf, sizeof(buf), "/dev/mixer%d", i);
			if ((m = mixer_open(buf)) == NULL)
				errx(1, "%s: no such mixer", buf);
			initctls(m);
			if (sflag)
				printrecsrc(m, oflag);
			else {
				printall(m, oflag);
				if (oflag)
					printf("\n");
			}
			(void)mixer_close(m);
		}
		return (0);
	}

mixer_get_nmixers() returns the oss_sysinfo->nummixers, which, in 277615's case at least, is 5, but we have 8 sound cards, just 3 of them do not have a mixer. So now in mixer(8) we are looping from 0 to 5 and we start opening from /dev/mixer0 to /dev/mixer4, and this is why we're failing to open /dev/mixer0 as his error message reports.

I think a solution can be to get the number of sound cards (i.e oss_sysinfo->numcards) as opposed to the number of mixers, loop through them and if we fail to open a mixer, continue to the next one without printing any error/warning. I can add a mixer_get_ncards() routine to do this.

[dev_submerge.ch]

In D45135#1029526, @christos wrote:

mixer_get_nmixers() returns the oss_sysinfo->nummixers, which, in 277615's case at least, is 5, but we have 8 sound cards, just 3 of them do not have a mixer. So now in mixer(8) we are looping from 0 to 5 and we start opening from /dev/mixer0 to /dev/mixer4, and this is why we're failing to open /dev/mixer0 as his error message reports.

I think a solution can be to get the number of sound cards (i.e oss_sysinfo->numcards) as opposed to the number of mixers, loop through them and if we fail to open a mixer, continue to the next one without printing any error/warning. I can add a mixer_get_ncards() routine to do this.

That solution somewhat contradicts the comment "only for test apps" there on oss_sysinfo->numcards, see D45136. I think it would be better to go from /dev/mixer0 up to some reasonable /dev/mixerN, and stop when we reach a number of oss_sysinfo->nummixers usable mixer devices.

Also, we need an "official" way of doing this, there's other mixer software like audio/mixertui in ports.

But this is unrelated, as you explained, so let's give the patch here a go. Maybe add to the commit message that it fixes the issue brought up by @emaste, it's valuable info.