Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Mark Brown]

On Mon, Jan 17, 2011 at 08:35:27PM -0600, Misael Lopez Cruz wrote:
> Multiple calls to dapm_power_widgets() can create a race condition
> causing power list to be corrupted. Those scenarios can occur in
> multistream usecases, stream start/stop along with simultaneous
> update power calls (mixer, mux, dapm_sync, stream_event).

> A new 'dapm_mutex' is added to soc_card, which is held while power
> list is created.

This needs a bit more analysis - why are we getting multiple
simultaneous calls to dapm_power_widgets() and is that itself safe?  The
ASoC locking model has always been to have a big lock around the entire
card rather than to lock subcomponents, and for example this isn't going
to make sure we're consistent with register map accesses from other
parts of the code.

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Mark Brown]

On Tue, Jan 18, 2011 at 01:35:37PM +0200, Peter Ujfalusi wrote:

> Hmm, the patch looks quite similar to the patch I've sent back in November:
> http://mailman.alsa-project.org/pipermail/alsa-devel/2010-November/033362.html

My thought exactly.

> On 01/18/11 12:39, ext Mark Brown wrote:

> > This needs a bit more analysis - why are we getting multiple
> > simultaneous calls to dapm_power_widgets() and is that itself safe?  The
> > ASoC locking model has always been to have a big lock around the entire
> > card rather than to lock subcomponents, and for example this isn't going
> > to make sure we're consistent with register map accesses from other
> > parts of the code.

> As far as I have traced back than, it was due to the use of dapm_sync in
> machine drivers. For me it looked that all other paths were satisfactory
> protected.
> In my case  the use of SOC_DAPM_PIN_SWITCH() instead of the custom call
> fixed the race (since this will take a card mutex, AFAIK).

I *suspect* that we're in a similar case and either the locking has been
broken by the multi-component stuff or there's other cases that need
protection, probably the jack detection code.

> PS: I might received this mail unintentionally, since I was not on the
> TO/CC, and alsa-devel neither.

> Should alsa-devel be on the CC?

Yes, I bounced on the second copy - Misael had sent two copies of the
mail, one omitting the CC, and I'd replied to the first one before I saw
the second.

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Peter Ujfalusi]

Resend, since alsa-devel address was mistyped ;)

On 01/18/11 13:39, ext Mark Brown wrote:
> > I *suspect* that we're in a similar case and either the locking has been
> > broken by the multi-component stuff or there's other cases that need
> > protection, probably the jack detection code.
The snd_soc_dapm_sync is just a wrapper for dapm_power_widgets, right?
It (snd_soc_dapm_sync) is used in soc-core:soc_post_component_init.
But there is no need for it, since the snd_soc_dapm_new_widgets will
call dapm_power_widgets at the end. so we can remove the
snd_soc_dapm_sync from there.
For the rest:
I would replace the snd_soc_dapm_sync calls in soc-dapm.c with
dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP); calls.
Modify the snd_soc_dapm_sync to use the codec->mutex around the
dapm_power_widgets call.
In that way I think all race conditions going to be covered without
adding new mutex.

What do you think?
I can make a patch...

--
Péter

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Mark Brown]

On Tue, Jan 18, 2011 at 02:08:51PM +0200, Peter Ujfalusi wrote:
> On 01/18/11 13:39, ext Mark Brown wrote:

> > > I *suspect* that we're in a similar case and either the locking has been
> > > broken by the multi-component stuff or there's other cases that need
> > > protection, probably the jack detection code.

> The snd_soc_dapm_sync is just a wrapper for dapm_power_widgets, right?
> It (snd_soc_dapm_sync) is used in soc-core:soc_post_component_init.

You're jumping into the analysis mid-way here again...

> Modify the snd_soc_dapm_sync to use the codec->mutex around the
> dapm_power_widgets call.

This doesn't sound like the right solution in a multi-CODEC system, DAPM
will affect multiple devices within the system.

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Peter Ujfalusi]

On 01/18/11 14:26, ext Mark Brown wrote:
>> The snd_soc_dapm_sync is just a wrapper for dapm_power_widgets, right?
>> It (snd_soc_dapm_sync) is used in soc-core:soc_post_component_init.
> 
> You're jumping into the analysis mid-way here again...

Unfortunately yes I did :(

>> Modify the snd_soc_dapm_sync to use the codec->mutex around the
>> dapm_power_widgets call.
> 
> This doesn't sound like the right solution in a multi-CODEC system, DAPM
> will affect multiple devices within the system.

Yes, you are right.

But I think the snd_soc_dapm_sync call in soc_post_component_init can be
removed, since it has been taken care of the snd_soc_dapm_new_widgets
call...
Surely it is not solving the problem, in fact it has nothing to do with
the problem ;)

-- 
Péter

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Jarkko Nikula]

On Tue, 18 Jan 2011 14:47:17 +0200
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> But I think the snd_soc_dapm_sync call in soc_post_component_init can be
> removed, since it has been taken care of the snd_soc_dapm_new_widgets
> call...

Looks like it can be removed from machine init callbacks too since the
snd_soc_dapm_new_widgets is called after machine initialization in
soc_post_component_init.

-- 
Jarkko

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Mark Brown]

On Tue, Jan 18, 2011 at 02:47:17PM +0200, Peter Ujfalusi wrote:

> But I think the snd_soc_dapm_sync call in soc_post_component_init can be
> removed, since it has been taken care of the snd_soc_dapm_new_widgets
> call...

Yes.

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Peter Ujfalusi]

On Tuesday 18 January 2011 14:08:51 ext Peter Ujfalusi wrote:
> Resend, since alsa-devel address was mistyped ;)
> 
> On 01/18/11 13:39, ext Mark Brown wrote:
> > > I *suspect* that we're in a similar case and either the locking has
> > > been broken by the multi-component stuff or there's other cases that
> > > need protection, probably the jack detection code.
> 
> The snd_soc_dapm_sync is just a wrapper for dapm_power_widgets, right?
> It (snd_soc_dapm_sync) is used in soc-core:soc_post_component_init.
> But there is no need for it, since the snd_soc_dapm_new_widgets will
> call dapm_power_widgets at the end. so we can remove the
> snd_soc_dapm_sync from there.
> For the rest:
> I would replace the snd_soc_dapm_sync calls in soc-dapm.c with
> dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP); calls.
> Modify the snd_soc_dapm_sync to use the codec->mutex around the
> dapm_power_widgets call.
> In that way I think all race conditions going to be covered without
> adding new mutex.
> 
> What do you think?
> I can make a patch...

Or not.. soc-jack.c:snd_soc_jack_report also calls snd_soc_dapm_sync, while 
keeping the codec->mutex locked...
Should machine drivers also take the mutex when they call snd_soc_dapm_sync?

-- 
Péter

Re: [PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Mark Brown]

On Tue, Jan 18, 2011 at 02:34:13PM +0200, Peter Ujfalusi wrote:

> Or not.. soc-jack.c:snd_soc_jack_report also calls snd_soc_dapm_sync, while 
> keeping the codec->mutex locked...
> Should machine drivers also take the mutex when they call snd_soc_dapm_sync?

In general yes.

[PATCH] ASoC: dapm: Fix race condition in widgets power list creation

[Misael Lopez Cruz]

Multiple calls to dapm_power_widgets() can create a race condition
causing power list to be corrupted. Those scenarios can occur in
multistream usecases, stream start/stop along with simultaneous
update power calls (mixer, mux, dapm_sync, stream_event).

A new 'dapm_mutex' is added to soc_card, which is held while power
list is created.

Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
---
 include/sound/soc.h  |    1 +
 sound/soc/soc-core.c |    1 +
 sound/soc/soc-dapm.c |    2 ++
 3 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 7e65b01..3dfc608 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -614,6 +614,7 @@ struct snd_soc_card {
 
 	struct list_head list;
 	struct mutex mutex;
+	struct mutex dapm_mutex;
 
 	bool instantiated;
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 1dc4b11..583a821 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3124,6 +3124,7 @@ static int snd_soc_register_card(struct snd_soc_card *card)
 	INIT_LIST_HEAD(&card->list);
 	card->instantiated = 0;
 	mutex_init(&card->mutex);
+	mutex_init(&card->dapm_mutex);
 
 	mutex_lock(&client_mutex);
 	list_add(&card->list, &card_list);
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 3d310af..faee456 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -940,6 +940,7 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event)
 	/* Check which widgets we need to power and store them in
 	 * lists indicating if they should be powered up or down.
 	 */
+	mutex_lock(&card->dapm_mutex);
 	list_for_each_entry(w, &dapm->widgets, list) {
 		switch (w->id) {
 		case snd_soc_dapm_pre:
@@ -974,6 +975,7 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event)
 			break;
 		}
 	}
+	mutex_unlock(&card->dapm_mutex);
 
 	/* If there are no DAPM widgets then try to figure out power from the
 	 * event type.
-- 
1.6.3.3