[alsa-devel] [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Takashi Iwai]

On Tue, 26 Jun 2018 11:23:59 +0200,
Shreyas NC wrote:
> 
> On Tue, Jun 26, 2018 at 10:34:17AM +0200, Takashi Iwai wrote:
> > On Tue, 26 Jun 2018 10:22:01 +0200,
> > Shreyas NC wrote:
> > > 
> > > > > +/**
> > > > > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > > > > + *
> > > > > + * @stream: SoundWire stream
> > > > > + *
> > > > > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > > > > + * stream to reconfigure the bus.
> > > > > + */
> > > > > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > > > > +{
> > > > > +	struct sdw_master_runtime *m_rt = NULL;
> > > > > +	struct sdw_bus *bus = NULL;
> > > > > +
> > > > > +	/* Iterate for all Master(s) in Master list */
> > > > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > > > +		bus = m_rt->bus;
> > > > > +
> > > > > +		mutex_lock(&bus->bus_lock);
> > > > > +	}
> > > > > +}
> > > > 
> > > > So it's nested locks?  Then you'd need some more trick to deal with
> > > > the lockdep.  I guess you'll get the false-positive deadlock detection
> > > > by this code when the mutex lock debug is enabled.
> > > > 
> > > > Also, is the linked order assured not to lead to a real deadlock?
> > > >
> > > 
> > > Hi Takashi,
> > > 
> > > Thanks for the review :)
> > > 
> > > A multi link SoundWire stream consists of a list of Master runtimes and
> > > more importantly only one master runtime per SoundWire bus instance.
> > > 
> > > So, these mutexes are actually different mutex locks(one per bus instance)
> > > and are not nested.
> > 
> > You take a mutex lock inside a mutex lock, so they are nested.
> > If they take the very same lock, it's called a "deadlock" instead.
> > 
> 
> Ok, myy bad, I misunderstood the comment :(
> 
> I forgot to add that I did check with mutex debug enabled and lockdep did
> not complain though :)

You didn't test the actual concurrent calls because of FE's mutex
below, right?


> > > In SDW we have a bus instance per Master (link). In multi-link case, a
> > > stream may have multiple Masters, thus we need to lock all bus instances
> > > before we operate on them.
> > > 
> > > Now since these are invoked from a stream (pcm ops) they will be always
> > > serialized and DPCM ensures we are never racing.
> > > 
> > > We did add this note here and in Documentation to make it explicit.
> > 
> > Well, my question is whether the order to take the multiple locks is
> > always assured.  You're calling like:
> > 
> > 	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> > 		mutex_lock();
> > 
> > And it's a linked-list.  If a stream has a link of masters like
> > M1->M2->M3 while another stream has a link like M2->M1->M3, it'll lead
> > to a deadlock with the concurrent calls above.
> > 
> 
> These are called from PCM stream ops context and the DPCM holds
> lock(fe->card->mutex) which serializes these operations.
> So, in the scenario you have mentioned, we would not have
> concurrent calls to this function.

The implementation of soundwire bus is basically independent from ASoC
or whatever else.  That is, any other drivers may use this API, and
it'll be busted.


Takashi