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.
