git: 5d980fadf73d - main - sound: Handle unavailable devices in various OSS IOCTLs
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Thu, 23 May 2024 00:58:06 UTC
The branch main has been updated by christos:
URL: https://cgit.FreeBSD.org/src/commit/?id=5d980fadf73df64a1e0eda40a93170ed76ce6f14
commit 5d980fadf73df64a1e0eda40a93170ed76ce6f14
Author: Christos Margiolis <christos@FreeBSD.org>
AuthorDate: 2024-05-23 00:57:17 +0000
Commit: Christos Margiolis <christos@FreeBSD.org>
CommitDate: 2024-05-23 00:57:17 +0000
sound: Handle unavailable devices in various OSS IOCTLs
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.
Additionally, notify applications that a device is
unavailable/unregistered instead of skipping it. The current
implementations of SNDCTL_AUDIOINFO, SNDCTL_MIXERINFO and
SNDCTL_CARDINFO break applications that expect to get information about
a device that is skipped. Related discussion can be found here:
https://reviews.freebsd.org/D45135#1029526
It has to be noted, that other applications, apart from mixer(8), suffer
from this.
PR: 277615
Sponsored by: The FreeBSD Foundation
MFC after: 1 day
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D45256
---
lib/libmixer/mixer.3 | 7 +-
sys/dev/sound/pcm/dsp.c | 23 +++++-
sys/dev/sound/pcm/mixer.c | 201 ++++++++++++++++++++++++----------------------
sys/dev/sound/pcm/mixer.h | 2 -
sys/dev/sound/pcm/sound.c | 41 ++++++----
5 files changed, 155 insertions(+), 119 deletions(-)
diff --git a/lib/libmixer/mixer.3 b/lib/libmixer/mixer.3
index af2a8ff135a8..a3593898ed68 100644
--- a/lib/libmixer/mixer.3
+++ b/lib/libmixer/mixer.3
@@ -20,7 +20,7 @@
.\" THE SOFTWARE.
.\"
-.Dd January 19, 2023
+.Dd May 22, 2024
.Dt MIXER 3
.Os
.Sh NAME
@@ -395,7 +395,10 @@ Playback and recording.
.Pp
The
.Fn mixer_get_nmixers
-function returns the total number of mixer devices in the system.
+function returns the maximum mixer unit number.
+Although this might sound as incorrect behavior, given that one would expect
+"nmixers" to refer to the total number of active mixers, it is more intuitive
+for applications that want to loop through all mixer devices.
.Pp
The
.Fn MIX_ISDEV
diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
index 4ee5d7c6738c..c737df33b97d 100644
--- a/sys/dev/sound/pcm/dsp.c
+++ b/sys/dev/sound/pcm/dsp.c
@@ -2040,6 +2040,19 @@ dsp_unit2name(char *buf, size_t len, struct pcm_channel *ch)
return (NULL);
}
+static void
+dsp_oss_audioinfo_unavail(oss_audioinfo *ai, int unit)
+{
+ bzero(ai, sizeof(*ai));
+ ai->dev = unit;
+ snprintf(ai->name, sizeof(ai->name), "pcm%d (unavailable)", unit);
+ ai->pid = -1;
+ ai->card_number = unit;
+ ai->port_number = unit;
+ ai->mixer_dev = -1;
+ ai->legacy_device = unit;
+}
+
/**
* @brief Handler for SNDCTL_AUDIOINFO.
*
@@ -2084,8 +2097,14 @@ dsp_oss_audioinfo(struct cdev *i_dev, oss_audioinfo *ai, bool ex)
unit < devclass_get_maxunit(pcm_devclass); unit++) {
d = devclass_get_softc(pcm_devclass, unit);
if (!PCM_REGISTERED(d)) {
- d = NULL;
- continue;
+ if ((ai->dev == -1 && unit == snd_unit) ||
+ ai->dev == unit) {
+ dsp_oss_audioinfo_unavail(ai, unit);
+ return (0);
+ } else {
+ d = NULL;
+ continue;
+ }
}
PCM_UNLOCKASSERT(d);
diff --git a/sys/dev/sound/pcm/mixer.c b/sys/dev/sound/pcm/mixer.c
index 130333c7c442..0cf6a9f42f8f 100644
--- a/sys/dev/sound/pcm/mixer.c
+++ b/sys/dev/sound/pcm/mixer.c
@@ -105,11 +105,6 @@ static struct cdevsw mixer_cdevsw = {
.d_name = "mixer",
};
-/**
- * Keeps a count of mixer devices; used only by OSSv4 SNDCTL_SYSINFO ioctl.
- */
-int mixer_count = 0;
-
static eventhandler_tag mixer_ehtag = NULL;
static struct cdev *
@@ -701,22 +696,13 @@ mixer_delete(struct snd_mixer *m)
snd_mtxfree(m->lock);
kobj_delete((kobj_t)m, M_MIXER);
- --mixer_count;
-
return (0);
}
struct snd_mixer *
mixer_create(device_t dev, kobj_class_t cls, void *devinfo, const char *desc)
{
- struct snd_mixer *m;
-
- m = mixer_obj_create(dev, cls, devinfo, MIXER_TYPE_SECONDARY, desc);
-
- if (m != NULL)
- ++mixer_count;
-
- return (m);
+ return (mixer_obj_create(dev, cls, devinfo, MIXER_TYPE_SECONDARY, desc));
}
int
@@ -769,8 +755,6 @@ mixer_init(device_t dev, kobj_class_t cls, void *devinfo)
pdev->si_drv1 = m;
snddev->mixer_dev = pdev;
- ++mixer_count;
-
if (bootverbose) {
for (i = 0; i < SOUND_MIXER_NRDEVICES; i++) {
if (!(m->devs & (1 << i)))
@@ -839,8 +823,6 @@ mixer_uninit(device_t dev)
d->mixer_dev = NULL;
- --mixer_count;
-
return 0;
}
@@ -1411,6 +1393,17 @@ mixer_sysuninit(void *p)
SYSINIT(mixer_sysinit, SI_SUB_DRIVERS, SI_ORDER_MIDDLE, mixer_sysinit, NULL);
SYSUNINIT(mixer_sysuninit, SI_SUB_DRIVERS, SI_ORDER_MIDDLE, mixer_sysuninit, NULL);
+static void
+mixer_oss_mixerinfo_unavail(oss_mixerinfo *mi, int unit)
+{
+ bzero(mi, sizeof(*mi));
+ mi->dev = unit;
+ snprintf(mi->id, sizeof(mi->id), "mixer%d (n/a)", unit);
+ snprintf(mi->name, sizeof(mi->name), "pcm%d:mixer (unavailable)", unit);
+ mi->card_number = unit;
+ mi->legacy_device = unit;
+}
+
/**
* @brief Handler for SNDCTL_MIXERINFO
*
@@ -1454,8 +1447,13 @@ mixer_oss_mixerinfo(struct cdev *i_dev, oss_mixerinfo *mi)
for (i = 0; pcm_devclass != NULL &&
i < devclass_get_maxunit(pcm_devclass); i++) {
d = devclass_get_softc(pcm_devclass, i);
- if (!PCM_REGISTERED(d) || PCM_DETACHING(d))
- continue;
+ if (!PCM_REGISTERED(d) || PCM_DETACHING(d)) {
+ if ((mi->dev == -1 && i == snd_unit) || mi->dev == i) {
+ mixer_oss_mixerinfo_unavail(mi, i);
+ return (0);
+ } else
+ continue;
+ }
/* XXX Need Giant magic entry */
@@ -1463,89 +1461,96 @@ mixer_oss_mixerinfo(struct cdev *i_dev, oss_mixerinfo *mi)
PCM_UNLOCKASSERT(d);
PCM_LOCK(d);
- if (d->mixer_dev != NULL && d->mixer_dev->si_drv1 != NULL &&
- ((mi->dev == -1 && d->mixer_dev == i_dev) ||
+ if (!((d->mixer_dev == i_dev && mi->dev == -1) ||
mi->dev == i)) {
- m = d->mixer_dev->si_drv1;
- mtx_lock(m->lock);
-
- /*
- * At this point, the following synchronization stuff
- * has happened:
- * - a specific PCM device is locked.
- * - a specific mixer device has been locked, so be
- * sure to unlock when existing.
- */
- bzero((void *)mi, sizeof(*mi));
- mi->dev = i;
- snprintf(mi->id, sizeof(mi->id), "mixer%d", i);
- strlcpy(mi->name, m->name, sizeof(mi->name));
- mi->modify_counter = m->modify_counter;
- mi->card_number = i;
- /*
- * Currently, FreeBSD assumes 1:1 relationship between
- * a pcm and mixer devices, so this is hardcoded to 0.
- */
- mi->port_number = 0;
-
- /**
- * @todo Fill in @sa oss_mixerinfo::mixerhandle.
- * @note From 4Front: "mixerhandle is an arbitrary
- * string that identifies the mixer better than
- * the device number (mixerinfo.dev). Device
- * numbers may change depending on the order the
- * drivers are loaded. However the handle should
- * remain the same provided that the sound card
- * is not moved to another PCI slot."
- */
-
- /**
- * @note
- * @sa oss_mixerinfo::magic is a reserved field.
- *
- * @par
- * From 4Front: "magic is usually 0. However some
- * devices may have dedicated setup utilities and the
- * magic field may contain an unique driver specific
- * value (managed by [4Front])."
- */
-
- mi->enabled = device_is_attached(m->dev) ? 1 : 0;
- /**
- * The only flag for @sa oss_mixerinfo::caps is
- * currently MIXER_CAP_VIRTUAL, which I'm not sure we
- * really worry about.
- */
- /**
- * Mixer extensions currently aren't supported, so
- * leave @sa oss_mixerinfo::nrext blank for now.
- */
-
- /**
- * @todo Fill in @sa oss_mixerinfo::priority (requires
- * touching drivers?)
- * @note The priority field is for mixer applets to
- * determine which mixer should be the default, with 0
- * being least preferred and 10 being most preferred.
- * From 4Front: "OSS drivers like ICH use higher
- * values (10) because such chips are known to be used
- * only on motherboards. Drivers for high end pro
- * devices use 0 because they will never be the
- * default mixer. Other devices use values 1 to 9
- * depending on the estimated probability of being the
- * default device.
- */
-
- snprintf(mi->devnode, sizeof(mi->devnode), "/dev/mixer%d", i);
- mi->legacy_device = i;
+ PCM_UNLOCK(d);
+ continue;
+ }
- mtx_unlock(m->lock);
+ if (d->mixer_dev->si_drv1 == NULL) {
+ mixer_oss_mixerinfo_unavail(mi, i);
+ PCM_UNLOCK(d);
+ return (0);
}
+ m = d->mixer_dev->si_drv1;
+ mtx_lock(m->lock);
+
+ /*
+ * At this point, the following synchronization stuff
+ * has happened:
+ * - a specific PCM device is locked.
+ * - a specific mixer device has been locked, so be
+ * sure to unlock when existing.
+ */
+ bzero((void *)mi, sizeof(*mi));
+ mi->dev = i;
+ snprintf(mi->id, sizeof(mi->id), "mixer%d", i);
+ strlcpy(mi->name, m->name, sizeof(mi->name));
+ mi->modify_counter = m->modify_counter;
+ mi->card_number = i;
+ /*
+ * Currently, FreeBSD assumes 1:1 relationship between
+ * a pcm and mixer devices, so this is hardcoded to 0.
+ */
+ mi->port_number = 0;
+
+ /**
+ * @todo Fill in @sa oss_mixerinfo::mixerhandle.
+ * @note From 4Front: "mixerhandle is an arbitrary
+ * string that identifies the mixer better than
+ * the device number (mixerinfo.dev). Device
+ * numbers may change depending on the order the
+ * drivers are loaded. However the handle should
+ * remain the same provided that the sound card
+ * is not moved to another PCI slot."
+ */
+
+ /**
+ * @note
+ * @sa oss_mixerinfo::magic is a reserved field.
+ *
+ * @par
+ * From 4Front: "magic is usually 0. However some
+ * devices may have dedicated setup utilities and the
+ * magic field may contain an unique driver specific
+ * value (managed by [4Front])."
+ */
+
+ mi->enabled = device_is_attached(m->dev) ? 1 : 0;
+ /**
+ * The only flag for @sa oss_mixerinfo::caps is
+ * currently MIXER_CAP_VIRTUAL, which I'm not sure we
+ * really worry about.
+ */
+ /**
+ * Mixer extensions currently aren't supported, so
+ * leave @sa oss_mixerinfo::nrext blank for now.
+ */
+
+ /**
+ * @todo Fill in @sa oss_mixerinfo::priority (requires
+ * touching drivers?)
+ * @note The priority field is for mixer applets to
+ * determine which mixer should be the default, with 0
+ * being least preferred and 10 being most preferred.
+ * From 4Front: "OSS drivers like ICH use higher
+ * values (10) because such chips are known to be used
+ * only on motherboards. Drivers for high end pro
+ * devices use 0 because they will never be the
+ * default mixer. Other devices use values 1 to 9
+ * depending on the estimated probability of being the
+ * default device.
+ */
+
+ snprintf(mi->devnode, sizeof(mi->devnode), "/dev/mixer%d", i);
+ mi->legacy_device = i;
+
+ mtx_unlock(m->lock);
+
PCM_UNLOCK(d);
- if (m != NULL)
- return (0);
+ return (0);
}
return (EINVAL);
diff --git a/sys/dev/sound/pcm/mixer.h b/sys/dev/sound/pcm/mixer.h
index 6c5c8f3ec3fe..7139a766b392 100644
--- a/sys/dev/sound/pcm/mixer.h
+++ b/sys/dev/sound/pcm/mixer.h
@@ -69,8 +69,6 @@ u_int32_t mix_getchild(struct snd_mixer *m, u_int32_t dev);
void *mix_getdevinfo(struct snd_mixer *m);
struct mtx *mixer_get_lock(struct snd_mixer *m);
-extern int mixer_count;
-
#define MIXER_CMD_DIRECT 0 /* send command within driver */
#define MIXER_CMD_CDEV 1 /* send command from cdev/ioctl */
diff --git a/sys/dev/sound/pcm/sound.c b/sys/dev/sound/pcm/sound.c
index 072d42c4e989..8d97dcd60231 100644
--- a/sys/dev/sound/pcm/sound.c
+++ b/sys/dev/sound/pcm/sound.c
@@ -766,7 +766,12 @@ sound_oss_sysinfo(oss_sysinfo *si)
*/
si->nummidis = 0;
si->numtimers = 0;
- si->nummixers = mixer_count;
+ /*
+ * Set this to the maximum unit number so that applications will not
+ * break if they try to loop through all mixers and some of them are
+ * not available.
+ */
+ si->nummixers = devclass_get_maxunit(pcm_devclass);
si->numcards = devclass_get_maxunit(pcm_devclass);
si->numaudios = devclass_get_maxunit(pcm_devclass);
/* OSSv4 docs: Intended only for test apps; API doesn't
@@ -797,23 +802,29 @@ sound_oss_card_info(oss_card_info *si)
for (i = 0; pcm_devclass != NULL &&
i < devclass_get_maxunit(pcm_devclass); i++) {
d = devclass_get_softc(pcm_devclass, i);
- if (!PCM_REGISTERED(d))
- continue;
-
if (i != si->card)
continue;
- PCM_UNLOCKASSERT(d);
- PCM_LOCK(d);
-
- strlcpy(si->shortname, device_get_nameunit(d->dev),
- sizeof(si->shortname));
- strlcpy(si->longname, device_get_desc(d->dev),
- sizeof(si->longname));
- strlcpy(si->hw_info, d->status, sizeof(si->hw_info));
- si->intr_count = si->ack_count = 0;
-
- PCM_UNLOCK(d);
+ if (!PCM_REGISTERED(d)) {
+ snprintf(si->shortname, sizeof(si->shortname),
+ "pcm%d (n/a)", i);
+ strlcpy(si->longname, "Device unavailable",
+ sizeof(si->longname));
+ si->hw_info[0] = '\0';
+ si->intr_count = si->ack_count = 0;
+ } else {
+ PCM_UNLOCKASSERT(d);
+ PCM_LOCK(d);
+
+ strlcpy(si->shortname, device_get_nameunit(d->dev),
+ sizeof(si->shortname));
+ strlcpy(si->longname, device_get_desc(d->dev),
+ sizeof(si->longname));
+ strlcpy(si->hw_info, d->status, sizeof(si->hw_info));
+ si->intr_count = si->ack_count = 0;
+
+ PCM_UNLOCK(d);
+ }
return (0);
}