[PATCH] ASoC: Use codec mutex in dapm_set_pga()

[PATCH] ASoC: Use codec mutex in dapm_set_pga()

[ext-eero.nurkkala]

From: Eero Nurkkala <ext-eero.nurkkala@nokia.com>

codec->mutex seems required if widget->* values
are altered. Moreover, snd_soc_dapm_put_volsw()
may alter widget->saved_value. dapm_set_pga()
uses widget->saved_value in a for loop which now
has a distant chance of getting out of control.

Signed-off-by: Eero Nurkkala <ext-eero.nurkkala@nokia.com>
---
 sound/soc/soc-dapm.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index f79711b..d6fb6c4 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -349,6 +349,7 @@ static int dapm_set_pga(struct snd_soc_dapm_widget *widget, int power)
 		unsigned int mask = (1 << fls(max)) - 1;
 		unsigned int invert = mc->invert;
 
+		mutex_lock(&widget->codec->mutex);
 		if (power) {
 			int i;
 			/* power up has happended, increase volume to last level */
@@ -373,6 +374,7 @@ static int dapm_set_pga(struct snd_soc_dapm_widget *widget, int power)
 			}
 			widget->muted = 1;
 		}
+		mutex_unlock(&widget->codec->mutex);
 	}
 	return 0;
 }
-- 
1.6.0.4

Re: [PATCH] ASoC: Use codec mutex in dapm_set_pga()

[Mark Brown]

On Thu, Oct 01, 2009 at 09:17:36AM +0300, ext-eero.nurkkala@nokia.com wrote:
> From: Eero Nurkkala <ext-eero.nurkkala@nokia.com>

> codec->mutex seems required if widget->* values
> are altered. Moreover, snd_soc_dapm_put_volsw()
> may alter widget->saved_value. dapm_set_pga()
> uses widget->saved_value in a for loop which now
> has a distant chance of getting out of control.

Hrm.  This doesn't look right.  dapm_set_pga() is only called from
within the DAPM power updates so we should've taken the codec mutex much
further up the stack otherwise all the DAPM list walking and updating
will have issues.  Was this just from code inspection or are you running
into actual issues?

Re: [PATCH] ASoC: Use codec mutex in dapm_set_pga()

[Eero Nurkkala]

On Thu, 2009-10-01 at 12:37 +0200, ext Mark Brown wrote:
> On Thu, Oct 01, 2009 at 09:17:36AM +0300, ext-eero.nurkkala@nokia.com wrote:
> > From: Eero Nurkkala <ext-eero.nurkkala@nokia.com>
> 
> > codec->mutex seems required if widget->* values
> > are altered. Moreover, snd_soc_dapm_put_volsw()
> > may alter widget->saved_value. dapm_set_pga()
> > uses widget->saved_value in a for loop which now
> > has a distant chance of getting out of control.
> 
> Hrm.  This doesn't look right.  dapm_set_pga() is only called from
> within the DAPM power updates so we should've taken the codec mutex much
> further up the stack otherwise all the DAPM list walking and updating
> will have issues.  Was this just from code inspection or are you running
> into actual issues?

No, codex mutex is not taken further up the stack.
I ran lockdep for the case also. I also placed the mutexes
so that they're always taken in the same function.

I still think the described scenario can happen. Or could
you point where the mutex is taken earlier? If it was,
I would've deadlocked every time....Maybe I'm missing
some info.

BTW, what's the reasoning for codec mutex anyway?

(Yes, it was just code inspection).

- Eero

Re: [PATCH] ASoC: Use codec mutex in dapm_set_pga()

[Eero Nurkkala]

On Thu, 2009-10-01 at 13:01 +0200, Nurkkala Eero.An (EXT-Offcode/Oulu)
wrote:
> On Thu, 2009-10-01 at 12:37 +0200, ext Mark Brown wrote:
> > On Thu, Oct 01, 2009 at 09:17:36AM +0300, ext-eero.nurkkala@nokia.com wrote:
> > > From: Eero Nurkkala <ext-eero.nurkkala@nokia.com>
> > 
> > > codec->mutex seems required if widget->* values
> > > are altered. Moreover, snd_soc_dapm_put_volsw()
> > > may alter widget->saved_value. dapm_set_pga()
> > > uses widget->saved_value in a for loop which now
> > > has a distant chance of getting out of control.
> > 
> > Hrm.  This doesn't look right.  dapm_set_pga() is only called from
> > within the DAPM power updates so we should've taken the codec mutex much
> > further up the stack otherwise all the DAPM list walking and updating
> > will have issues.  Was this just from code inspection or are you running
> > into actual issues?
> 
> No, codex mutex is not taken further up the stack.
> I ran lockdep for the case also. I also placed the mutexes
> so that they're always taken in the same function.
> 
> I still think the described scenario can happen. Or could
> you point where the mutex is taken earlier? If it was,
> I would've deadlocked every time....Maybe I'm missing
> some info.
> 
> BTW, what's the reasoning for codec mutex anyway?
> 
> (Yes, it was just code inspection).
> 
> - Eero

Actually, in this case: (kernel 2.6.28)

snd_soc_dapm_put_volsw() (mutex_lock(&widget->codec->mutex);) ->
dapm_mixer_update_power() ->
dapm_power_widgets() ->
dapm_set_pga()

and with this patch it would deadlock. (would try to take
the mutex twice).

But if it didn't come from snd_soc_dapm_put_volsw(),
but instead:
close_delayed_work() ->
snd_soc_dapm_stream_event() ->
dapm_power_widgets() ->
dapm_set_pga()
then the codec->mutex is not taken,
and the bug presented in the patch is out there, no?

Eg. if snd_soc_dapm_put_volsw() is called, it can alter
widget->saved_value meanwhile damp_set_pga() is being
using the same value in a for loop.

So yes, my patch makes things worse, but this use of
codec mutex doesn't appear very clear to me. (possibly
everybody else knows it far better =)

- Eero

Re: [PATCH] ASoC: Use codec mutex in dapm_set_pga()

[Mark Brown]

On Thu, Oct 01, 2009 at 02:56:24PM +0300, Eero Nurkkala wrote:

> Actually, in this case: (kernel 2.6.28)

That's rather old...

> But if it didn't come from snd_soc_dapm_put_volsw(),
> but instead:
> close_delayed_work() ->
> snd_soc_dapm_stream_event() ->
> dapm_power_widgets() ->
> dapm_set_pga()
> then the codec->mutex is not taken,
> and the bug presented in the patch is out there, no?

Right, so close_delayed_work() needs to lock the card while it runs.

Re: [PATCH] ASoC: Use codec mutex in dapm_set_pga()

[Eero Nurkkala]

On Thu, 2009-10-01 at 14:09 +0200, ext Mark Brown wrote:
> On Thu, Oct 01, 2009 at 02:56:24PM +0300, Eero Nurkkala wrote:
> 
> > Actually, in this case: (kernel 2.6.28)
> 
> That's rather old...
> 
Yes, but I've been mirroring things with 2.6.31..

> > But if it didn't come from snd_soc_dapm_put_volsw(),
> > but instead:
> > close_delayed_work() ->
> > snd_soc_dapm_stream_event() ->
> > dapm_power_widgets() ->
> > dapm_set_pga()
> > then the codec->mutex is not taken,
> > and the bug presented in the patch is out there, no?
> 
> Right, so close_delayed_work() needs to lock the card while it runs.

Can't really do that, because snd_soc_dapm_stream_event()
will try to take codec mutex again. Tried the following:

--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -2071,9 +2071,9 @@ int snd_soc_dapm_stream_event(struct snd_soc_codec
*codec,
 			}
 		}
 	}
-	mutex_unlock(&codec->mutex);
 
 	dapm_power_widgets(codec, event);
+	mutex_unlock(&codec->mutex);
 	dump_dapm(codec, __func__);
 	return 0;
 }


Does that make any sense?

- Eero

Re: [PATCH] ASoC: Use codec mutex in dapm_set_pga()

[Mark Brown]

On Thu, Oct 01, 2009 at 03:34:46PM +0300, Eero Nurkkala wrote:

> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -2071,9 +2071,9 @@ int snd_soc_dapm_stream_event(struct snd_soc_codec
> *codec,
>  			}
>  		}
>  	}
> -	mutex_unlock(&codec->mutex);
>  
>  	dapm_power_widgets(codec, event);
> +	mutex_unlock(&codec->mutex);
>  	dump_dapm(codec, __func__);
>  	return 0;
>  }
> 

> Does that make any sense?

Looks plausible, I'd need to check properly for gotchas but it's not
like we're not taking the mutex in that path already and
dapm_power_widgets() certainly ought to have the lock.  Looks like this
has been there since DAPM was originally merged.

Re: [PATCH] ASoC: Use codec mutex in dapm_set_pga()

[Mark Brown]

On Thu, Oct 01, 2009 at 02:01:09PM +0300, Eero Nurkkala wrote:

> I still think the described scenario can happen. Or could
> you point where the mutex is taken earlier? If it was,
> I would've deadlocked every time....Maybe I'm missing
> some info.

Any of the control paths down from user space should be taking it.

> BTW, what's the reasoning for codec mutex anyway?

It protects all the data on the card - we've got a lot of
read/modify/write cycles going on, plus things like the power state
transitions which need to be run single threaded otherwise they'll get
therribly confused.  We could do something finer grained but it's never
been an issue.