sound: Use bus_topo_lock() where appropriate

[christos]

Summary

Lock around uses of devclass_*() and replace leftover
CTLFLAG_NEEDGIANTs with CTLFLAG_MPSAFE.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

[markj]

Anything which is calling devclass_get_maxunit() should be holding bus_topo_lock, AKA, Giant. This change is incomplete.

[imp]

In D46700#1064689, @markj wrote:

Anything which is calling devclass_get_maxunit() should be holding bus_topo_lock, AKA, Giant. This change is incomplete.

Quibble: Giant is a detail now that consumers need not know. So removing NEEDSGIANT is good, so long as one explicitly uses bus_topo_lock() around references to devclass_get_maxunit the whole loop in sysctl_hw_snd_maxautovchans.

I should add asserts to devclass_get_maxunit()... It's not strictly required for it, but to use the results, one must hold the topo lock.

[christos]

Extend previous patch.

[markj]

sys/dev/sound/pcm/channel.c

68

bus_topo lock is for the newbus subsystem, i.e., bits of the kernel which control device driver attach, detach, etc..

Its use here doesn't make sense. A store to an integer value is already atomic.

114

Ditto.

170

Ditto.

190

Ditto.

[christos]

sys/dev/sound/pcm/channel.c

68

Is the change from CTLFLAG_NEEDGIANT to CTLFLAG_MPSAFE for sysctls that also only modify integers okay?

[markj]

sys/dev/sound/pcm/channel.c

68

"Maybe."

What is the purpose of the lock? Are you worried about multiple threads concurrently writing a value? Then there's no real problem - all of the stores will be atomic, and the last thread to write the value will win.

But, what about the code which reads this value? Is it reading the value multiple times in a function? If so, does it matter if the value changes in between reads? If so, then you need a lock to synchronize readers and writers.

A concrete example: feeder_register() loads the value chn_latency_profile multiple times. That code is wrong if chn_latency_profile changes in between loads. How to fix it? Either:

• use a lock to serialize readers and writers,
• tell the compiler that the value is to be loaded only once:

int val = atomic_load_int(&chn_latency_profile);
if (val < CHN_LATENCY_PROFILE_MIN || val > CHN_LATENCY_PROFILE_MAX)
    atomic_store_int(&chn_latency_profile, CHN_LATENCY_PROFILE_DEFAULT);

It is usually better to use a mutex. Code which uses mutexes is usually easier to read and understand.

Now, that said, I think this particular example is a bit bogus: the sysctl_hw_snd_latency_profile() handler returns EINVAL if the value is outside of those bounds, and I don't see any other way to change the value. So I think that statement can just be replaced with an assertion that the latency profile is between max and min. But, the general idea is there.

[christos]

Lock only around uses of devclass_get_maxunit(). Will write a separate patches
to 1) add assertions around reads of global variables, 2) remove
initializations from feeder.c.

[christos]

Get rid of locking inside sysctl_hw_snd_default_unit().

[markj]

sys/dev/sound/pcm/feeder_rate.c

284

Extra newline here.

sys/dev/sound/pcm/sound.c

126–127

This is a bit dodgy. You're setting two variables here, and the readers are not locked at all, as far as I can see. This sysctl handler can race if it runs at the same time as pcm_setstatus(), which is called from various driver attach routines, which are usually invoked with Giant held.

Rather than relying on Giant, the sound subsystem should perhaps have some global driver init lock that can serialize sysctl updates. However, driver attach is a rare event, so these races are very unlikely to be problematic; it's a low-priority problem.

[christos]

sys/dev/sound/pcm/sound.c

126–127

At EuroBSDCon we were discussing with @imp whether it's better to get rid of bus_topo_* calls in general in sound(4), and instead use a global driver lock which we'll create on sound(4) load. Is this what you're referring to?

[markj]

sys/dev/sound/pcm/sound.c

126–127

Something like that, yeah. In general you want/need *something* to synchronize setting global variables (usually via sysctl) and consumers (drivers at initialization time). In the past, that was provided by Giant. Today, it's buggy, but the races are hard to hit so no one notices, but that makes the code brittle and hard to reason about.

[christos]

Use new global driver lock instead of bus_topo_*.

However, sound(4) still relies on the PCM_GIANT_* macros. Should those be
replaced with the new lock as well?

[christos]

@markj any opinion (see previous comment)?

[christos]

Retire PCM_GIANT_ENTER() and PCM_GIANT_LEAVE/PCM_GIANT_EXIT() and replace with
2 two PCM_SND_LOCK() and PCM_SND_UNLOCK() macros (I'm open to propositions for
a better name).

[christos]

sys/dev/sound/pcm/sound.h

353 ↗

(On Diff #144990)

@markj Is it better to initialize the lock as MTX_RECURSE or just check that mtx_owned(snd_mtx) == 0 here?

[christos]

Use sx(9) instead of mtx(9) so that we can sleep with the lock held.

[christos]

Use a shared lock instead. Fixes some bugs I missed (didn't realize I wasn't
using a non-MPSAFE driver when testing...).

[christos]

After discussion with markj@, keep PCM_GIANT_* and just use the new
SND_LOCK for uses of devclass_get_maxunit() and where needed, global variable
setting in sysctl handlers.

[christos]

Bump.

[christos]

Rebase on top of recent patches.

[christos]

Use SND_LOCK() in sysctl_hw_snd_vchans_enable().

[christos]

Is this good to go?

[markj]

sys/dev/sound/pci/hda/hdaa.c

6463 ↗

(On Diff #151625)

You still want the explicit bus_topo_lock() around calls to newbus functions like this one. Right now it's acquired implicitly because the hdaa_sysctl_reconfig sysctl isn't marked MPSAFE, but we're not supposed to rely on that. See commit 96e500a3f9f0e - you're basically undoing a portion of it here.

[christos]

sys/dev/sound/pci/hda/hdaa.c

6463 ↗

(On Diff #151625)

Does that also apply to all the other places I use SND_LOCK() below, around uses of devlcass_*()?

[markj]

sys/dev/sound/pci/hda/hdaa.c

6463 ↗

(On Diff #151625)

Formally yes, see https://reviews.freebsd.org/D46700#1064689 and https://reviews.freebsd.org/D46700#1064707 .

[christos]

Address Mark's comments.

[imp]

Code changes look good, I wonder if the two highlighted comments are now obsolete. There's no more giant locked sound devices and now that we use bus functions to lock newbus access, we shouldn't need it for anything else, right?

sys/dev/sound/pcm/mixer.c

1460

Is this obsolete now?

sys/dev/sound/pcm/sound.c

574

Is this now obsolete?

[christos]

In D46700#1128765, @imp wrote:

Code changes look good, I wonder if the two highlighted comments are now obsolete. There's no more giant locked sound devices and now that we use bus functions to lock newbus access, we shouldn't need it for anything else, right?

There are Giant locked drivers still, they are the ones that do not set SD_F_MPSAFE.

[imp]

In D46700#1128872, @christos wrote:

In D46700#1128765, @imp wrote:

Code changes look good, I wonder if the two highlighted comments are now obsolete. There's no more giant locked sound devices and now that we use bus functions to lock newbus access, we shouldn't need it for anything else, right?

There are Giant locked drivers still, they are the ones that do not set SD_F_MPSAFE.

Ah, OK. I thought we'd sunsetted them. Is there a reason not to plan this sunset for them?

[christos]

In D46700#1128873, @imp wrote:

In D46700#1128872, @christos wrote:

In D46700#1128765, @imp wrote:

Code changes look good, I wonder if the two highlighted comments are now obsolete. There's no more giant locked sound devices and now that we use bus functions to lock newbus access, we shouldn't need it for anything else, right?

There are Giant locked drivers still, they are the ones that do not set SD_F_MPSAFE.

Ah, OK. I thought we'd sunsetted them. Is there a reason not to plan this sunset for them?

I once sent an email in freebsd-arch@ [1] to purge some old drivers, which IIRC are all Giant locked [2], and some people reacted against this.

[1] https://lists.freebsd.org/archives/freebsd-arch/2024-April/000641.html
[2] Actually, most of our drivers except the popular ones are Giant locked. These are the ones that are not: snd_hdsp, snd_hdspe, snd_hda, snd_emu10kx, snd_uaudio, snd_dummy.

sys/dev/sound/pcm/mixer.c

mixer_oss_mixerinfo(struct cdev *i_dev, oss_mixerinfo *mi)
 
         d = NULL;
         m = NULL;
 
         /*
          * There's a 1:1 relationship between mixers and PCM devices, so
          * begin by iterating over PCM devices and search for our mixer.
          */
+        bus_topo_lock();
         for (i = 0; pcm_devclass != NULL &&
             i < devclass_get_maxunit(pcm_devclass); i++) {
                 d = devclass_get_softc(pcm_devclass, i);
                 if (!PCM_REGISTERED(d)) {
                         if ((mi->dev == -1 && i == snd_unit) || mi->dev == i) {
                                 mixer_oss_mixerinfo_unavail(mi, i);
+                                bus_topo_unlock();
                                 return (0);
                         } else
                                 continue;
                 }
 
                 /* XXX Need Giant magic entry */
 
                 /* See the note in function docblock. */
                 PCM_UNLOCKASSERT(d);
                 PCM_LOCK(d);
 
                 if (!((d->mixer_dev == i_dev && mi->dev == -1) ||
                     mi->dev == i)) {
                         PCM_UNLOCK(d);
                         continue;
                 }
 
                 if (d->mixer_dev->si_drv1 == NULL) {
                         mixer_oss_mixerinfo_unavail(mi, i);
                         PCM_UNLOCK(d);
+                        bus_topo_unlock();
                         return (0);
                 }
 
                 m = d->mixer_dev->si_drv1;
                 mtx_lock(m->lock);
 
                 /*
                  * At this point, the following synchronization stuff
for (i = 0; pcm_devclass != NULL &&
 
                 snprintf(mi->devnode, sizeof(mi->devnode), "/dev/mixer%d", i);
                 mi->legacy_device = i;
 
                 mtx_unlock(m->lock);
 
                 PCM_UNLOCK(d);
 
+                bus_topo_unlock();
                 return (0);
         }
+        bus_topo_unlock();
 
         return (EINVAL);
 }
 
 /*
  * Allow the sound driver to use the mixer lock to protect its mixer
  * data:
  */