[alsa-devel] [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 at nokia.com wrote:
> > > From: Eero Nurkkala <ext-eero.nurkkala at 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