On 01/12/2022 14:32, Richard Fitzgerald wrote:
On 29/11/2022 15:44, Pierre-Louis Bossart wrote:
Don't hold sdw_dev_lock while calling the peripheral driver probe() and remove() callbacks.
Holding sdw_dev_lock around the probe() and remove() calls causes a theoretical mutex inversion which lockdep will assert on. The peripheral driver probe will probably register a soundcard, which will take ALSA and ASoC locks. During
It's extremely unlikely that a peripheral driver would register a sound card, this is what machine drivers do.
Which leads me to the question: is this a real problem?
Yes, try turning on lockdep checking and you will get an assert. During probe the existing code takes sdw_dev_lock and then calls the codec driver probe, so you will get a mutex sequence like:
sdw_dev_lock -> controls_rw_sem -> pcm_mutex
but in normal operation the ALSA/ASoC code will take its mutexes first and call runtime_resume which then takes the sdw_dev_lock, so you get
pcm_mutex -> sdw_dev_lock
and lockdep will assert on that opposite ordering. The full assert is at the end of this email.
Humm, you lost me with the reference to runtime_resume. I don't fully understand how it's possible to invoke pm_runtime during probe. pm_runtime should only enabled during the codec update_status() which can only be done once the probe completes.
I am fine with the changes that you are suggesting, the introduction of the sdw_dev_lock was probably too conservative and it'd be fine to only protect what is required.
However we do have lockdep enabled
I wonder whether this is because the Cirrus devices use full DP prepare, so there will be a DP prepare interrupt during the attempt to prepare the dailink. The lockdep assert was when sdw_update_slave_status() tried to take sdw_dev_lock.
If the Realtek codecs only use Soundwire interrupts for jack detect you probably won't see a sdw_dev_lock inside a pcm_mutex.
Ok, I think I understand this now. lockdep tries to prove locking correctness without requiring that a bad lock order actually happened. This makes sense, because some deadlocks might be extremely difficult to reproduce and lockdep wouldn't help much if you had to reproduce it to get an assert.
There's a lot of detail in Documentation/locking/lockdep-design.rst, but in summary it is looking for a relationship between three locks that is theoretically incompatible. So if it sees: a) L1->L2 b) L2->L3 c) L3->L1 this will assert because the relationship of L1 and L2 on L3 is opposite to the relationship of L1 to L2. IOW lockdep is assuming the possibility of L2->L3->L1. You could look at that another way and say that lockdep is telling you that even though you didn't intend L2->L3->L1 could ever happen there is a risk lurking in the code.
What we've seen is three normal call chains that give these orderings:
a) pcm_mutex -> sdw_dev_lock (when starting up a playback) b) controls_rwsem -> pcm_mutex (a write to a DAPM MUX control re-evaluates DAPM power states of the DPCM links) c) sdw_dev_lock -> controls_rwsem (when snd_soc_register_component() adds ALSA controls)
controls_rwsem is the "L3" here, the lock that has a relationship to two other locks that is incompatible to those two locks' relationship to each other in (a).
The theoretical case sdw_dev_lock->controls_rwsem->pcm_mutex should be impossible, but it's always nicer to fix the problem that try to make it a special case "safe to ignore".
The only places sdw_dev_lock can be taken before the ALSA/ASoC locks is during probe() and remove(). At other times it would only be taken as a result of an ALSA/ASoC activity so the ALSA/ASoC locks would be taken first.