dsp: Make witness happy

[aokblast]

Summary

chn_poll() may hold both rdch and wrch channel locks while calling
chn_trigger(rdch). chn_trigger() switches the lock order from
"channel -> dsp dev" to "dsp dev -> channel" by temporarily dropping
the channel lock before acquiring the dsp lock.

However, only rdch was unlocked during the transition while wrch
remained locked. Since wrch is also a channel lock and witness had
already established the lock order requirement:

dsp dev -> channel

witness reports a lock order reversal when pcm_lock() is acquired while
wrch is still held.

Avoid holding rdch and wrch simultaneously during chn_trigger()
lock-order switching by only keeping the channel locks when needed.

The issue can be reliably reproduced by starting pipewire,
pipewire-pulse, and pavucontrol.

[christos]

Please explain in a commit message what this change does. What does WITNESS complain about? Why are you changing the order of dsp_lock_chans()? And why did you remove the dsp_lock_chans() call in dsp_poll()?

[christos]

Also in the commit title it should be "sound", not "dsp".

[aokblast]

Elaborate on commit msg

[aokblast]

In D57009#1306393, @christos wrote:

Please explain in a commit message what this change does. What does WITNESS complain about? Why are you changing the order of dsp_lock_chans()? And why did you remove the dsp_lock_chans() call in dsp_poll()?

Thanks! The lock order change is unnecessary. We only have to lock read and write channel separately.

[christos]

I suppose you are referring to a seemingly "harmless" LOR coming from chn_trigger(). I've been aware of this for a while already. That being said, I don't see how this change fixes it. You are essentially doing the exact same thing.

[christos]

I understand that the change here locks the channels we actually want, instead of both of them. However, the effect is the same; if priv has both channels allocated, then your code will also lock both of them, just not at the same time. But I don't understand why doing this fixes the issue. Could you elaborate further?

[aokblast]

In D57009#1306459, @christos wrote:

I understand that the change here locks the channels we actually want, instead of both of them. However, the effect is the same; if priv has both channels allocated, then your code will also lock both of them, just not at the same time. But I don't understand why doing this fixes the issue. Could you elaborate further?

I know it is a little bit hard to understand as I also spend hours to read witness output. Let me explain it with more details:

1. when calling chn_poll(rdch, e, td), we hold both the rdch and wrch locks at the same time.
2. In chn_trigger, we are expected to have lock order "dsp dev" -> "channel". chn_trigger switch the lock order by using chn_unlock -> pcm_lock -> chn_lock.

However, when chn_trigger(rdch) (called by chn_poll(rdch) try to switch the lock order. Witness detects another lock order reverse (wrch) that is unrelated to the call stack.
It is because wrch is also a channel lock and also called by chn_trigger previous - that is, it requires lock order "dsp dev" -> channel lock but we called pcm_lock without unlock wrch channel (the channel is wrch but we are handling chn_trigger(rdch)).

It can be stably reproduced on my laptop by using the following sequence:

1. Start pipewire
2. Start pipewire-pulse
3. Start pavucontrol

[christos]

In D57009#1306470, @aokblast wrote:

In D57009#1306459, @christos wrote:

I understand that the change here locks the channels we actually want, instead of both of them. However, the effect is the same; if priv has both channels allocated, then your code will also lock both of them, just not at the same time. But I don't understand why doing this fixes the issue. Could you elaborate further?

I know it is a little bit hard to understand as I also spend hours to read witness output. Let me explain it with more details:

1. when calling chn_poll(rdch, e, td), we hold both the rdch and wrch locks at the same time.
2. In chn_trigger, we are expected to have lock order "dsp dev" -> "channel". chn_trigger switch the lock order by using chn_unlock -> pcm_lock -> chn_lock.

However, when chn_trigger(rdch) (called by chn_poll(rdch) try to switch the lock order. Witness detects another lock order reverse (wrch) that is unrelated to the call stack.
It is because wrch is also a channel lock and also called by chn_trigger previous - that is, it requires lock order "dsp dev" -> channel lock but we called pcm_lock without unlock wrch channel (the channel is wrch but we are handling chn_trigger(rdch)).

It can be stably reproduced on my laptop by using the following sequence:

1. Start pipewire
2. Start pipewire-pulse
3. Start pavucontrol

Right, this makes sense now. It would really helpful to explain this in the commit message.

[aokblast]

Elaborate commit msg again

[christos]

LGTM. Please change the commit title from "dsp: Make witness happy" to "sound: Fix lock order reversal in dsp_poll()".

[christos]

"sound: Fix lock order reversal in dsp_poll()"

This should be the final commit title. I edited the previous comment.