Now, we:

• acquire the mutex,
• wait for the busy lock,
• acquire the busy lock,
• release the mutex,
• acquire the mutex.

IMO it would be simpler to just make PCM_ACQUIRE() always perform a PCM_WAIT() as well.

You don't need to hold the mutex in order to manipulate these flags?

Some comment in sound.h describing the locking protocol would be useful.

This is a preparatory patch for D60128, so it does look a bit more tedious/ugly here, but it makes the changes there easier, and there is no need to change PCM_ACQUIRE either.

We could, but we are busied at this point.

Regarding the document, I'll think about it. If I do it, it'll definitely be once the locking refactor is done.

We could, but we are busied at this point.

I know we could, I'm asking if it's necessary. What is the locking protocol for this field? You seem to be asserting that the busy lock is sufficient to modify flags, but look at the SNDCTL_DSP_SETDUPLEX ioctl handler: it does a read-modify-write of the flags field with only the mutex held, so now it is possible for these two code paths to race, when it wasn't possible before.

Your change is implicitly asserting that the busy lock is sufficient to modify flags. I can't tell if that's intentional or not, as there is no comment anywhere explaining the locking protocol (see the comment "Description of a process..." in sys/proc.h for an example of what I mean), nor any mention of this in the review description.

I think the locking protocol in sound(4) is a quite messy in general, it's not just here. This patch is meant to prepare the ground for more follow-up refactors. As it stands now, the sx and the lock more or less do the same thing, but the sx can sleep, so we tend to use it in larger operations.

Refactoring must be done on stable ground. You must understand the current state of affairs before changing it.

In the change description you wrote, "keep PCM_LOCK only where it actually guards something." Here I am suggesting that it actually guards something, namely d->flags. In particular it looks like this change is introducing race conditions, and I'm trying to point that out; that's why I gave an example where d->flags is updated without the sx lock. So why remove the locking here? If you're going to do that as part of a "refactor," you need to prove that it's a correct change. A good refactor will also add locking assertions whenever possible.

Please either fix the patch or tell me why I'm wrong. Please also keep in mind that much of this code is reachable by unprivileged users and that race conditions here can easily lead to security vulnerabilities. Correctness of multi-threaded code is a tricky subject and requires more rigour.