sound: Use bus_topo_lock() where needed

[christos]

Summary

Lock around uses of devclass_get_maxunit() and modification of global
variables in sysctl handlers.

While here, replace leftover CTLFLAG_NEEDGIANTs with CTLFLAG_MPSAFE.

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[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

64

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.

110

Ditto.

166

Ditto.

186

Ditto.

[christos]

sys/dev/sound/pcm/channel.c

64

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

[markj]

sys/dev/sound/pcm/channel.c

64

"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

283

Extra newline here.

sys/dev/sound/pcm/sound.c

185

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.