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