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

[Takashi Iwai <tiwai@suse.de>]

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.

> 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.

> > > +/**
> > > + * sdw_release_bus_lock: Release bus lock for all Master runtime(s)
> > > + *
> > > + * @stream: SoundWire stream
> > > + *
> > > + * Release the previously held bus_lock after reconfiguring the bus.
> > > + */
> > > +static void sdw_release_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_unlock(&bus->bus_lock);
> > > +	}
> > 
> > ... and this looks bad.  The loop for unlocking should be traversed
> > reversely.
> > 
> 
> Yes in principle I agree locking should be in reverse, but as explained
> above in this case, it does not matter much :)

It does matter when you dealing with the multiple nested mutexes...


thanks,

Takashi