| Message ID | 1529924340-30065-4-git-send-email-shreyas.nc@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> > +/** > > + * 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
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
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 :) > > 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. > > > > +/** > > > > + * 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... > Ok --Shreyas
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
On Tue, Jun 26, 2018 at 11:46:35AM +0200, Takashi Iwai wrote: > 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? > Yes > > > > > 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. > Hmmh, yes. The only way we could think of to protect this is to use a global lock which we wanted to avoid. We did give this a thought and since today no other subsytem can use this, we went ahead with the way it is today. Any suggestions on how to go about this ? --Shreyas
On Tue, 26 Jun 2018 11:59:32 +0200, Shreyas NC wrote: > > On Tue, Jun 26, 2018 at 11:46:35AM +0200, Takashi Iwai wrote: > > 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? > > > > Yes > > > > > > > > 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. > > > > Hmmh, yes. The only way we could think of to protect this is to use a global > lock which we wanted to avoid. > We did give this a thought and since today no other subsytem > can use this, we went ahead with the way it is today. > Any suggestions on how to go about this ? If so, you have to give an explicit big fat warning for the usage in the function description. Takashi
On Tue, Jun 26, 2018 at 12:16:42PM +0200, Takashi Iwai wrote: > On Tue, 26 Jun 2018 11:59:32 +0200, > Shreyas NC wrote: > > > > On Tue, Jun 26, 2018 at 11:46:35AM +0200, Takashi Iwai wrote: > > > 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? > > > > > > > Yes > > > > > > > > > > > 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. > > > > > > > Hmmh, yes. The only way we could think of to protect this is to use a global > > lock which we wanted to avoid. > > We did give this a thought and since today no other subsytem > > can use this, we went ahead with the way it is today. > > Any suggestions on how to go about this ? > > If so, you have to give an explicit big fat warning for the usage in > the function description. > Ok, I had added it in the commit log and documentation. It does make sense to add it in the function description. Thanks! --Shreyas
On Tue, 26 Jun 2018 12:22:01 +0200, Shreyas NC wrote: > > On Tue, Jun 26, 2018 at 12:16:42PM +0200, Takashi Iwai wrote: > > On Tue, 26 Jun 2018 11:59:32 +0200, > > Shreyas NC wrote: > > > > > > On Tue, Jun 26, 2018 at 11:46:35AM +0200, Takashi Iwai wrote: > > > > 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? > > > > > > > > > > Yes > > > > > > > > > > > > > > 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. > > > > > > > > > > Hmmh, yes. The only way we could think of to protect this is to use a global > > > lock which we wanted to avoid. > > > We did give this a thought and since today no other subsytem > > > can use this, we went ahead with the way it is today. > > > Any suggestions on how to go about this ? > > > > If so, you have to give an explicit big fat warning for the usage in > > the function description. > > > > Ok, I had added it in the commit log and documentation. > It does make sense to add it in the function description. Yes. It needs a clear sign showing that the concurrency has to be resolved in the caller side. Takashi
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); + } +} + +/** + * 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); + } +} + static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) { struct sdw_master_runtime *m_rt = stream->m_rt; diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 962971e..ccd8dcdf 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -769,6 +769,9 @@ struct sdw_stream_params { * @state: Current state of the stream * @type: Stream type PCM or PDM * @m_rt: Master runtime + * @master_list: List of Master runtime(s) in this stream. + * master_list can contain only one m_rt per Master instance + * for a stream */ struct sdw_stream_runtime { char *name; @@ -776,6 +779,7 @@ struct sdw_stream_runtime { enum sdw_stream_state state; enum sdw_stream_type type; struct sdw_master_runtime *m_rt; + struct list_head master_list; }; struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name);