From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances Date: Mon, 25 Jun 2018 14:38:14 +0200 Message-ID: References: <1529924340-30065-1-git-send-email-shreyas.nc@intel.com> <1529924340-30065-4-git-send-email-shreyas.nc@intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id D0EB126720C for ; Mon, 25 Jun 2018 14:38:14 +0200 (CEST) In-Reply-To: <1529924340-30065-4-git-send-email-shreyas.nc@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Shreyas NC Cc: alsa-devel@alsa-project.org, patches.audio@intel.com, gregkh@linuxfoundation.org, pierre-louis.bossart@linux.intel.com, vkoul@kernel.org, sanyog.r.kale@intel.com List-Id: alsa-devel@alsa-project.org On Mon, 25 Jun 2018 12:58:56 +0200, Shreyas NC wrote: > > From: Sanyog Kale > > 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 > Signed-off-by: Vinod Koul > Signed-off-by: Shreyas NC > --- > 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