Re: [PATCH] soundwire: bus_type: Avoid lockdep assert in sdw_drv_probe()

[Richard Fitzgerald]

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.