[alsa-devel] [PATCH] ASoC: dapm: Fix locking during codec shutdown
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); }
/*
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...
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
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.
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
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...
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.
participants (2)
-
Liam Girdwood -
Mark Brown