From nobody Thu May 23 00:58:06 2024 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Vl8t673S0z5LNqT; Thu, 23 May 2024 00:58:06 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Vl8t665gbz4lwS; Thu, 23 May 2024 00:58:06 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1716425886; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=J9kJqu8u6C6Shcv9wInSfKAp1g24dJ1NtKI8nkUWt2g=; b=f8rQZZqoVUwdEBAll2vkQaTW4TsHz4I2sxBS0HVbc65LTns1we3Zm8FyLvElJc5IHMleOX 0vwrIODoJ+UlkVKDg30a/5p6Z4N/HV3m1lrBxNK6ixIotaHsDIMl0UNdl7eykZIfnZp7vc A/8egiHvs1EV72jl6HfyExOtf+yNeq2INylb0b24c4VCczO2cF/VCs0Pa2MJcalllU8rbv ckrC0N5yhQCN9UGL34L/YAXqVin87nmg3OLtACYTGhxo8VhnxqmGQqAJdmFl5dXJBvr52V xNOmWQIxIqSFMNaASMzPNuq/nzBCnupXRK3UrlLEBuUbf0vO7M7yHQlLgFKOZQ== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1716425886; a=rsa-sha256; cv=none; b=DbCLoeuXd8s8v0e5X7Tq/6GdZmQBjdN0sKBtpS5XyDXAEC95vjfncs5LuqBnE2kuf7UAkt UY3Zk3YF9fQGmq424UUYb1zx3A5Eu57iFY3E3I2Gqt1OPu5Zg3rfYgExPeGSYqIENTbDwU G7wGHndtv10DuzgazxazZmpcFXnMeppkxQ6Z+LGHe/WOqMfYHxZ0GHKY47S4kZHHYigftz gaHUu/xjbZ8RJZ6Ml0cnG0RXXnLzw2uVL8OqPZM2vxCRfx+bGI9p+VabYFMavycut0MCpz R3I/K3a7tN4Ak6Mpdj9Av5KC0VjzLYXhf0g92fW2FfVvdTnBeH5WSWg/5Qx1aQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1716425886; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=J9kJqu8u6C6Shcv9wInSfKAp1g24dJ1NtKI8nkUWt2g=; b=mMLasV2NSMnapgnPzYEW92fLs0UbhQELDGB1rio0d6+haX4nBJte/dS0u9cu4S9HJde8V3 hMgr5MC95Eez2t10jDjNFt50LBPwrlqWxOG5pH1acmjrtKK8zvrj+g5EwA6mMxnktNY/VG RxxixaTBk0LKb8lAAp3sBK6HiqR4oqVJ4pbYcRLns46DqjaF5v1W8kvZE2tHUZkSzzdQ5d DBDDkski3on5sOBacSdcJAVyhcgxVheWunzMfeCjIgvdlj+lX3ZyP0+w76PM1m2jc8/SLD PdNP13V4st54qdEGwOYRPN1HYbRWzyQocqH3HTvj472NCUJrGmlMtOBlCgr7FA== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4Vl8t65hf0zSTN; Thu, 23 May 2024 00:58:06 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.17.1/8.17.1) with ESMTP id 44N0w60t071898; Thu, 23 May 2024 00:58:06 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.17.1/8.17.1/Submit) id 44N0w6Xp071895; Thu, 23 May 2024 00:58:06 GMT (envelope-from git) Date: Thu, 23 May 2024 00:58:06 GMT Message-Id: <202405230058.44N0w6Xp071895@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Christos Margiolis Subject: git: 5d980fadf73d - main - sound: Handle unavailable devices in various OSS IOCTLs List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: christos X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 5d980fadf73df64a1e0eda40a93170ed76ce6f14 Auto-Submitted: auto-generated The branch main has been updated by christos: URL: https://cgit.FreeBSD.org/src/commit/?id=5d980fadf73df64a1e0eda40a93170ed76ce6f14 commit 5d980fadf73df64a1e0eda40a93170ed76ce6f14 Author: Christos Margiolis AuthorDate: 2024-05-23 00:57:17 +0000 Commit: Christos Margiolis 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); }