[alsa-devel] [PATCH] ASoC: Use codec mutex in dapm_set_pga()
ext-eero.nurkkala at nokia.com
Thu Oct 1 13:56:24 CEST 2009
On Thu, 2009-10-01 at 13:01 +0200, Nurkkala Eero.An (EXT-Offcode/Oulu)
> 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);) ->
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(),
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 =)
More information about the Alsa-devel