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

[Shreyas NC <shreyas.nc@intel.com>]

> > +/**
> > + * 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.

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.

> > +/**
> > + * 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 :)

--Shreyas