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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 25 Jun 2018 12:58:56 +0200,
Shreyas NC wrote:
> 
> From: Sanyog Kale <sanyog.r.kale@intel.com>
> 
> Currently, the stream concept is limited to single Master and one
> or more Codecs.
> 
> This patch extends the concept to support multiple Master(s)
> sharing the same reference clock and synchronized in the hardware.
> Modify sdw_stream_runtime to support a list of sdw_master_runtime
> for the same. The existing reference to a single m_rt is removed
> in the next patch.
> 
> Typically to lock, one would acquire a global lock and then lock
> bus instances. In this case, the caller framework(ASoC DPCM)
> guarantees that stream operations on a card are always serialized.
> So, there is no race condition and hence no need for global lock.
> 
> Bus lock(s) are acquired to reconfigure the bus while the stream
> is set-up.
> So, we add sdw_acquire_bus_lock()/sdw_release_bus_lock() APIs which
> are used only to reconfigure the bus.
> 
> Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
> ---
>  drivers/soundwire/bus.h       |  2 ++
>  drivers/soundwire/stream.c    | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/soundwire/sdw.h |  4 ++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 3b15c4e..b6cfbdf 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -99,6 +99,7 @@ struct sdw_slave_runtime {
>   * this stream, can be zero.
>   * @slave_rt_list: Slave runtime list
>   * @port_list: List of Master Ports configured for this stream, can be zero.
> + * @stream_node: sdw_stream_runtime master_list node
>   * @bus_node: sdw_bus m_rt_list node
>   */
>  struct sdw_master_runtime {
> @@ -108,6 +109,7 @@ struct sdw_master_runtime {
>  	unsigned int ch_count;
>  	struct list_head slave_rt_list;
>  	struct list_head port_list;
> +	struct list_head stream_node;
>  	struct list_head bus_node;
>  };
>  
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 8974a0f..eb942c6 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -747,6 +747,7 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
>  		return NULL;
>  
>  	stream->name = stream_name;
> +	INIT_LIST_HEAD(&stream->master_list);
>  	stream->state = SDW_STREAM_ALLOCATED;
>  
>  	return stream;
> @@ -1234,6 +1235,46 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave,
>  	return NULL;
>  }
>  
> +/**
> + * 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?

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


thanks,

Takashi