[alsa-devel] [PATCH 1/2] ASoC: core - Add platform component mutex.
Add mutex support for platform IO operations. e.g. can be used for platform DAPM widget IO ops.
Signed-off-by: Liam Girdwood lrg@ti.com --- include/sound/soc.h | 1 + sound/soc/soc-core.c | 1 + 2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 82bd773..2ebf787 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -718,6 +718,7 @@ struct snd_soc_platform { int id; struct device *dev; struct snd_soc_platform_driver *driver; + struct mutex mutex;
unsigned int suspended:1; /* platform is suspended */ unsigned int probed:1; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 7978f6c..c90bb01 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3382,6 +3382,7 @@ int snd_soc_register_platform(struct device *dev, platform->dapm.dev = dev; platform->dapm.platform = platform; platform->dapm.stream_event = platform_drv->stream_event; + mutex_init(&platform->mutex);
mutex_lock(&client_mutex); list_add(&platform->list, &platform_list);
Currently not all DAPM widget IO ops are holding their component mutex (codec or platform). Make sure this is now held for DAPM widget IO operations.
Signed-off-by: Liam Girdwood lrg@ti.com --- sound/soc/soc-dapm.c | 34 ++++++++++++++++++++++++++++------ 1 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index dcd1160..a837977 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -206,7 +206,23 @@ static int soc_widget_write(struct snd_soc_dapm_widget *w, int reg, int val) return -1; }
-static int soc_widget_update_bits(struct snd_soc_dapm_widget *w, +static inline void soc_widget_lock(struct snd_soc_dapm_widget *w) +{ + if (w->codec) + mutex_lock(&w->codec->mutex); + else if (w->platform) + mutex_lock(&w->platform->mutex); +} + +static inline void soc_widget_unlock(struct snd_soc_dapm_widget *w) +{ + if (w->codec) + mutex_unlock(&w->codec->mutex); + else if (w->platform) + mutex_unlock(&w->platform->mutex); +} + +static int soc_widget_update_bits_locked(struct snd_soc_dapm_widget *w, unsigned short reg, unsigned int mask, unsigned int value) { bool change; @@ -219,18 +235,24 @@ static int soc_widget_update_bits(struct snd_soc_dapm_widget *w, if (ret != 0) return ret; } else { + soc_widget_lock(w); ret = soc_widget_read(w, reg); - if (ret < 0) + if (ret < 0) { + soc_widget_unlock(w); return ret; + }
old = ret; new = (old & ~mask) | (value & mask); change = old != new; if (change) { ret = soc_widget_write(w, reg, new); - if (ret < 0) + if (ret < 0) { + soc_widget_unlock(w); return ret; + } } + soc_widget_unlock(w); }
return change; @@ -847,7 +869,7 @@ int dapm_reg_event(struct snd_soc_dapm_widget *w, else val = w->off_val;
- soc_widget_update_bits(w, -(w->reg + 1), + soc_widget_update_bits_locked(w, -(w->reg + 1), w->mask << w->shift, val << w->shift);
return 0; @@ -1105,7 +1127,7 @@ static void dapm_seq_run_coalesced(struct snd_soc_dapm_context *dapm, "pop test : Applying 0x%x/0x%x to %x in %dms\n", value, mask, reg, card->pop_time); pop_wait(card->pop_time); - soc_widget_update_bits(w, reg, mask, value); + soc_widget_update_bits_locked(w, reg, mask, value); }
list_for_each_entry(w, pending, power_list) { @@ -1235,7 +1257,7 @@ static void dapm_widget_update(struct snd_soc_dapm_context *dapm) w->name, ret); }
- ret = snd_soc_update_bits(w->codec, update->reg, update->mask, + ret = soc_widget_update_bits_locked(w, update->reg, update->mask, update->val); if (ret < 0) pr_err("%s DAPM update failed: %d\n", w->name, ret);
On Tue, Mar 06, 2012 at 06:16:19PM +0000, Liam Girdwood wrote:
- if (w->codec)
mutex_lock(&w->codec->mutex);
Actually for CODECs we don't need to hold the CODEC mutex for I/O if we've pushed the cache down into regmap - regmap does all the locking for us. I'll send a followup patch.
On Tue, 2012-03-06 at 20:03 +0000, Mark Brown wrote:
On Tue, Mar 06, 2012 at 06:16:19PM +0000, Liam Girdwood wrote:
- if (w->codec)
mutex_lock(&w->codec->mutex);Actually for CODECs we don't need to hold the CODEC mutex for I/O if we've pushed the cache down into regmap - regmap does all the locking for us. I'll send a followup patch.
I know, but this patch did check for regmap usage in soc_widget_update_bits_locked(). With your fix we are doing the regmap test twice.
Liam
On Wed, Mar 07, 2012 at 10:11:01AM +0000, Liam Girdwood wrote:
On Tue, 2012-03-06 at 20:03 +0000, Mark Brown wrote:
Actually for CODECs we don't need to hold the CODEC mutex for I/O if we've pushed the cache down into regmap - regmap does all the locking for us. I'll send a followup patch.
I know, but this patch did check for regmap usage in soc_widget_update_bits_locked(). With your fix we are doing the regmap test twice.
Yes, we are but it's a simple comparison with integer so not the end of the world. I'd much rather have the check in the locking code so it's clear what's expected instead of split between one of the call sites and the locking function where it might easily get missed if someone adds a new user.
On Wed, 2012-03-07 at 10:54 +0000, Mark Brown wrote:
On Wed, Mar 07, 2012 at 10:11:01AM +0000, Liam Girdwood wrote:
On Tue, 2012-03-06 at 20:03 +0000, Mark Brown wrote:
Actually for CODECs we don't need to hold the CODEC mutex for I/O if we've pushed the cache down into regmap - regmap does all the locking for us. I'll send a followup patch.
I know, but this patch did check for regmap usage in soc_widget_update_bits_locked(). With your fix we are doing the regmap test twice.
Yes, we are but it's a simple comparison with integer so not the end of the world. I'd much rather have the check in the locking code so it's clear what's expected instead of split between one of the call sites and the locking function where it might easily get missed if someone adds a new user.
Ah, I'm thinking with a different perspective here ;-)
My reasoning is that the widget lock will guarantee the component lock. With the regmap test we don't guarantee a component lock and subsequent new widget lock users may depend on this lock.
Liam
On Wed, Mar 07, 2012 at 11:13:25AM +0000, Liam Girdwood wrote:
On Wed, 2012-03-07 at 10:54 +0000, Mark Brown wrote:
Yes, we are but it's a simple comparison with integer so not the end of the world. I'd much rather have the check in the locking code so it's clear what's expected instead of split between one of the call sites and the locking function where it might easily get missed if someone adds a new user.
Ah, I'm thinking with a different perspective here ;-)
My reasoning is that the widget lock will guarantee the component lock. With the regmap test we don't guarantee a component lock and subsequent new widget lock users may depend on this lock.
I don't understand what you mean by "component lock" here. Do you mean CODEC/platform lock or something else? In any case it's stuff like the above assumption which makes me want to be explicit here - either we should take the lock in all paths or we always skip the lock in regmap paths. Either is fine for me, I'd just rather keep it straight in my head.
Actually given the number of indirections here (there's similar "is it a platform" stuff for the I/O too) we probably want to create some base class object before we start adding more stuff anyway.
On Tue, Mar 6, 2012 at 12:16 PM, Liam Girdwood lrg@ti.com wrote:
Currently not all DAPM widget IO ops are holding their component mutex (codec or platform). Make sure this is now held for DAPM widget IO operations.
Signed-off-by: Liam Girdwood lrg@ti.com
This patch breaks the P1022DS, which uses the WM8776 as a codec. The MPC8610HPCD, which is identical to the P1022DS but uses the CS4270 codec instead, works fine.
I'm guessing it's some kind of deadlock, because as soon as I start playback, the system halts. Not even Ctrl-C works.
On Fri, 2012-03-09 at 17:26 +0000, Tabi Timur-B04825 wrote:
On Tue, Mar 6, 2012 at 12:16 PM, Liam Girdwood lrg@ti.com wrote:
Currently not all DAPM widget IO ops are holding their component mutex (codec or platform). Make sure this is now held for DAPM widget IO operations.
Signed-off-by: Liam Girdwood lrg@ti.com
This patch breaks the P1022DS, which uses the WM8776 as a codec. The MPC8610HPCD, which is identical to the P1022DS but uses the CS4270 codec instead, works fine.
I'm guessing it's some kind of deadlock, because as soon as I start playback, the system halts. Not even Ctrl-C works.
Can you switch on the mutex debugging kernel config here. I've just had a quick look and the WM8776 and its not holding the codec mutex or calling snd_soc_update_bits_locked() so it must deadlock via another path.
Thanks
Liam
Liam Girdwood wrote:
Can you switch on the mutex debugging kernel config here. I've just had a quick look and the WM8776 and its not holding the codec mutex or calling snd_soc_update_bits_locked() so it must deadlock via another path.
Enabling the debug options didn't reveal anything, unfortunately.
When I start playback, here's the code flow:
dapm_seq_run_coalesced:1138 soc_widget_update_bits_locked:244 soc_widget_lock:211 codec=wm8776.1-001a platform=
The numbers are line numbers of the printk statements I added.
What happens is that the thread hangs on:
mutex_lock(&w->codec->mutex);
in soc_widget_lock().
After a couple minutes, I see this:
INFO: task speaker-test:2624 blocked for more than 120 seconds. "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. speaker-test D 0fced718 0 2624 2623 0x00000000 Call Trace: [e6f27ab0] [c0038d5c] console_unlock+0x22c/0x29c (unreliable) [e6f27b70] [c0008520] __switch_to+0x70/0x144 [e6f27b90] [c04ab830] __schedule+0x224/0x4ec [e6f27be0] [c04aa6a8] mutex_lock_nested+0x188/0x394 [e6f27c40] [c0395c40] soc_widget_update_bits_locked+0x2b0/0x2c4 [e6f27c80] [c0397734] dapm_seq_run_coalesced.clone.24+0x1b8/0x1c4 [e6f27cc0] [c03977e4] dapm_seq_run.clone.25+0xa4/0x3c0 [e6f27d10] [c0397eb4] dapm_power_widgets+0x3b4/0x67c [e6f27d70] [c0399480] snd_soc_dapm_stream_event+0x74/0xdc [e6f27d90] [c039abb0] soc_pcm_prepare+0x108/0x1dc [e6f27dc0] [c0378248] snd_pcm_do_prepare+0x24/0x5c [e6f27dd0] [c0377e3c] snd_pcm_action_single+0x4c/0xb4 [e6f27df0] [c0379218] snd_pcm_action_nonatomic+0x7c/0xa0 [e6f27e10] [c037b678] snd_pcm_common_ioctl1+0x8f8/0x10d8 [e6f27e60] [c037c568] snd_pcm_playback_ioctl1+0x40/0x6cc [e6f27ea0] [c00f0e20] do_vfs_ioctl+0xa4/0x7d0 [e6f27f10] [c00f158c] sys_ioctl+0x40/0x88 [e6f27f40] [c000f6e4] ret_from_syscall+0x0/0x3c --- Exception: c01 at 0xfced718 LR = 0xfd76a24 INFO: lockdep is turned off.
On Fri, Mar 09, 2012 at 01:11:26PM -0600, Timur Tabi wrote:
Liam Girdwood wrote:
Can you switch on the mutex debugging kernel config here. I've just had a quick look and the WM8776 and its not holding the codec mutex or calling snd_soc_update_bits_locked() so it must deadlock via another path.
Enabling the debug options didn't reveal anything, unfortunately.
The major difference between your two boards is that CS4270 doesn't use DAPM while WM8776 does. Though if one of them were going to break I'd really expect it to be the CS4270, obviously non-DAPM CODECs are a real corner case (to the point where I think you're the only active user of such a device) and certainly all Liam's TI reference systems have DAPM CODECs so this is really surprising... I wonder if PowerPC mutexes are less forgiving here?
Mark Brown wrote:
I wonder if PowerPC mutexes are less forgiving here?
I'm still debugging this (although I have higher priority tasks to worry about, unfortunately), but it appears that the mutex is already taken when that code is executed. Unfortunately, I have been unable to figure out where. One second the 'count' value is 1, and then it suddenly becomes '0', and then mutex_lock() is called.
On Sun, Mar 11, 2012 at 12:55:59PM +0000, Mark Brown wrote:
On Fri, Mar 09, 2012 at 01:11:26PM -0600, Timur Tabi wrote:
Liam Girdwood wrote:
Can you switch on the mutex debugging kernel config here. I've just had a quick look and the WM8776 and its not holding the codec mutex or calling snd_soc_update_bits_locked() so it must deadlock via another path.
Enabling the debug options didn't reveal anything, unfortunately.
The major difference between your two boards is that CS4270 doesn't use DAPM while WM8776 does. Though if one of them were going to break I'd really expect it to be the CS4270, obviously non-DAPM CODECs are a real corner case (to the point where I think you're the only active user of such a device) and certainly all Liam's TI reference systems have DAPM CODECs so this is really surprising... I wonder if PowerPC mutexes are less forgiving here?
I'm not sure this is PowerPC specific. I'm running ARM platform and seeing this commit also breaks my imx-sgtl5000 (SGTL5000 codec) driver that I'm submitting.
============================================= [ INFO: possible recursive locking detected ] 3.3.0-rc4+ #159 Not tainted --------------------------------------------- aplay/395 is trying to acquire lock: (&codec->mutex){+.+...}, at: [<802f7414>] soc_widget_update_bits_locked+0x88/0x 1a0
but task is already holding lock: (&codec->mutex){+.+...}, at: [<802f9df4>] snd_soc_dapm_stream_event+0x38/0xc0
On Mon, 2012-03-12 at 17:12 +0800, Shawn Guo wrote:
On Sun, Mar 11, 2012 at 12:55:59PM +0000, Mark Brown wrote:
On Fri, Mar 09, 2012 at 01:11:26PM -0600, Timur Tabi wrote:
Liam Girdwood wrote:
Can you switch on the mutex debugging kernel config here. I've just had a quick look and the WM8776 and its not holding the codec mutex or calling snd_soc_update_bits_locked() so it must deadlock via another path.
Enabling the debug options didn't reveal anything, unfortunately.
The major difference between your two boards is that CS4270 doesn't use DAPM while WM8776 does. Though if one of them were going to break I'd really expect it to be the CS4270, obviously non-DAPM CODECs are a real corner case (to the point where I think you're the only active user of such a device) and certainly all Liam's TI reference systems have DAPM CODECs so this is really surprising... I wonder if PowerPC mutexes are less forgiving here?
I'm not sure this is PowerPC specific. I'm running ARM platform and seeing this commit also breaks my imx-sgtl5000 (SGTL5000 codec) driver that I'm submitting.
============================================= [ INFO: possible recursive locking detected ] 3.3.0-rc4+ #159 Not tainted
aplay/395 is trying to acquire lock: (&codec->mutex){+.+...}, at: [<802f7414>] soc_widget_update_bits_locked+0x88/0x 1a0
but task is already holding lock: (&codec->mutex){+.+...}, at: [<802f9df4>] snd_soc_dapm_stream_event+0x38/0xc0
Timur, Shawn, thanks for your debug.
snd_soc_dapm_stream_event() should not hold the codec mutex here. Something is out of sync between my branch and Mark's, maybe I've missed something out on a recent patch.
Liam
On Mon, Mar 12, 2012 at 09:27:52AM +0000, Liam Girdwood wrote:
snd_soc_dapm_stream_event() should not hold the codec mutex here. Something is out of sync between my branch and Mark's, maybe I've missed something out on a recent patch.
Summarising IRC discussion for the list: you identified that the issue is that the CODEC locking change actually depends on some of the later locking changes in your branch which didn't get in for 3.4. Therefore we agreed that I'll revert this change for 3.4 (nothing actually uses it yet in 3.4) as suggested further up the thread and reapply them for 3.5 with the other changes that they need.
participants (5)
-
Liam Girdwood -
Mark Brown -
Shawn Guo -
Tabi Timur-B04825 -
Timur Tabi