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.

Ditto.

Ditto.

Ditto.

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

"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.

Extra newline here.

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.

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?

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.

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