[alsa-devel] [PATCH] ASoC: Use codec mutex in dapm_set_pga()
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; }
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?
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
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
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.
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
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.
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.
participants (3)
-
Eero Nurkkala -
ext-eero.nurkkala＠nokia.com
-
Mark Brown