[PATCH] ASoC: dapm: Fix locking during codec shutdown

[PATCH] ASoC: dapm: Fix locking during codec shutdown

[Liam Girdwood]

Codec shutdown performs a DAPM power sequence that might cause conflicts
and/or race conditions if another stream power event is running simultaneously.
Use card's dapm mutex to protect any potential race condition between them.

Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Liam Girdwood <lrg@ti.com>
---
 sound/soc/soc-dapm.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 0f078de..b485960 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3578,10 +3578,13 @@ EXPORT_SYMBOL_GPL(snd_soc_dapm_free);
 
 static void soc_dapm_shutdown_codec(struct snd_soc_dapm_context *dapm)
 {
+	struct snd_soc_card *card = dapm->card;
 	struct snd_soc_dapm_widget *w;
 	LIST_HEAD(down_list);
 	int powerdown = 0;
 
+	mutex_lock(&card->dapm_mutex);
+
 	list_for_each_entry(w, &dapm->card->widgets, list) {
 		if (w->dapm != dapm)
 			continue;
@@ -3604,6 +3607,8 @@ static void soc_dapm_shutdown_codec(struct snd_soc_dapm_context *dapm)
 			snd_soc_dapm_set_bias_level(dapm,
 						    SND_SOC_BIAS_STANDBY);
 	}
+
+	mutex_unlock(&card->dapm_mutex);
 }
 
 /*
-- 
1.7.9.5

Re: [PATCH] ASoC: dapm: Fix locking during codec shutdown

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 563 bytes --]

On Fri, Jul 06, 2012 at 04:57:05PM +0100, Liam Girdwood wrote:
> Codec shutdown performs a DAPM power sequence that might cause conflicts
> and/or race conditions if another stream power event is running simultaneously.
> Use card's dapm mutex to protect any potential race condition between them.

There's a bigger problem here if we're managing to run into this - the
shutdown function is only supposed to be being called while the system
is in the middle of shutting down so we've probably got a bunch of other
races further up the stack to worry about too...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: dapm: Fix locking during codec shutdown

[Liam Girdwood]

On Fri, 2012-07-06 at 17:02 +0100, Mark Brown wrote:
> On Fri, Jul 06, 2012 at 04:57:05PM +0100, Liam Girdwood wrote:
> > Codec shutdown performs a DAPM power sequence that might cause conflicts
> > and/or race conditions if another stream power event is running simultaneously.
> > Use card's dapm mutex to protect any potential race condition between them.
> 
> There's a bigger problem here if we're managing to run into this - the
> shutdown function is only supposed to be being called while the system
> is in the middle of shutting down so we've probably got a bunch of other
> races further up the stack to worry about too...

I don't think there is anything explicitly stopping a power off from
happening during a stream shutdown atm without this patch. It was
reported to me that this would fail 1 in 20 times on a certain device
and that the above patch fixed the issue (1200 tests completed OK after
the patch).

I do think we need this patch atm since we are accessing the same
resources as the stream event.

Liam

Re: [PATCH] ASoC: dapm: Fix locking during codec shutdown

[Mark Brown]

On Fri, Jul 06, 2012 at 05:36:15PM +0100, Liam Girdwood wrote:

> I don't think there is anything explicitly stopping a power off from
> happening during a stream shutdown atm without this patch. It was
> reported to me that this would fail 1 in 20 times on a certain device
> and that the above patch fixed the issue (1200 tests completed OK after
> the patch).

> I do think we need this patch atm since we are accessing the same
> resources as the stream event.

So how is suspend and resume safe, for example?  It's the same general
path through the PM core.

Re: [PATCH] ASoC: dapm: Fix locking during codec shutdown

[Liam Girdwood]

On Fri, 2012-07-06 at 17:43 +0100, Mark Brown wrote:
> On Fri, Jul 06, 2012 at 05:36:15PM +0100, Liam Girdwood wrote:
> 
> > I don't think there is anything explicitly stopping a power off from
> > happening during a stream shutdown atm without this patch. It was
> > reported to me that this would fail 1 in 20 times on a certain device
> > and that the above patch fixed the issue (1200 tests completed OK after
> > the patch).
> 
> > I do think we need this patch atm since we are accessing the same
> > resources as the stream event.
> 
> So how is suspend and resume safe, for example?  It's the same general
> path through the PM core.

suspend and resume are safe as they call stream_event() that holds the
DAPM mutex.

Fwiw, I've searched my email and found Misa's analysis here :-

<device is preparing to reboot>
soc_dapm_shutdown_codec() : Insert widget A, B, C, ... in "down_list"
soc_dapm_shutdown_codec() : Run power sequence via dapm_seq_run()
dapm_seq_run(): Move widget A, B, C, ... from "down_list" to "pending"
list for coalesced changes
dapm_seq_run_coalesced(): Iterating over A

<system sound indicating device reboot closes>
dapm_power_widgets() : Run a STREAM_STOP event a inserts widget B in a
different "down_list" (whose next element is not C, but other widget X
with diff reg address)

dapm_seq_run_coalesced() : Iterates over B and expected C next, but got
the widget X added by dapm_power_widgets() above
dapm_seq_run_coalesced() : Widget X doesn't share same reg address as
widget A, B, ... hence BUG().

dapm_power_widgets() uses card's power_mutex, but
soc_dapm_shutdown_codec() does not, so above scenario is possible. IMO,
just protecting codec shutdown with card's power_mutex should suffice as
in attached patch.

Liam

Re: [PATCH] ASoC: dapm: Fix locking during codec shutdown

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 629 bytes --]

On Fri, Jul 06, 2012 at 06:10:13PM +0100, Liam Girdwood wrote:
> On Fri, 2012-07-06 at 17:43 +0100, Mark Brown wrote:

> > So how is suspend and resume safe, for example?  It's the same general
> > path through the PM core.

> suspend and resume are safe as they call stream_event() that holds the
> DAPM mutex.

So that's safe with regard to DAPM...  it's not the specific example
that's concerning me here so much as the fact that I don't think we're
doing anything else to make sure shutdown is safe with other things that
might be going on when shutdown is called.  There's probably a huge raft
of issues that might occur...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: dapm: Fix locking during codec shutdown

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 401 bytes --]

On Fri, Jul 06, 2012 at 04:57:05PM +0100, Liam Girdwood wrote:
> Codec shutdown performs a DAPM power sequence that might cause conflicts
> and/or race conditions if another stream power event is running simultaneously.
> Use card's dapm mutex to protect any potential race condition between them.

Applied, but I do think this needs a much closer look - there must be
some other issues lurking here.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]