[PATCH 1/2] ASoC: core - Add platform component mutex.

[PATCH 1/2] ASoC: core - Add platform component mutex.

[Liam Girdwood]

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);
-- 
1.7.5.4

[PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Liam Girdwood]

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);
-- 
1.7.5.4

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 294 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Liam Girdwood]

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

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 749 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Liam Girdwood]

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

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1253 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Tabi Timur-B04825]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Liam Girdwood]

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

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Timur Tabi]

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.




-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 835 bytes --]

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?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Tabi Timur-B04825]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Shawn Guo]

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

-- 
Regards,
Shawn

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Liam Girdwood]

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 

Re: [PATCH 2/2] ASoC: DAPM - Make sure DAPM widget IO ops hold the component mutex.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 640 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] ASoC: core - Add platform component mutex.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 186 bytes --]

On Tue, Mar 06, 2012 at 06:16:18PM +0000, Liam Girdwood wrote:
> Add mutex support for platform IO operations. e.g. can be used
> for platform DAPM widget IO ops.

Applied both, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]