[PATCH 1/3] ASoC: core - Add card mutex locking subclasses

[PATCH 1/3] ASoC: core - Add card mutex locking subclasses

[Liam Girdwood]

This is the first part of a change that is intended to improve
ASoC locking protection for DAPM and PCM operations.

We currently use the codec->mutex for locking some DAPM operations.
This can't guarantee correctness now with the recent multi-component
and forth coming Dynamic PCM updates.

This part of the series adds a mutex class for the soc_card mutex. The
SND_SOC_CARD_CLASS_INIT class is used for card initialisation only whilst the
SND_SOC_CARD_CLASS_PCM class is used for all the DAPM and forth coming Dynamic
PCM operations. The new mutex classes are required otherwise we will see a false
positive mutex deadlock warning between the card initialisation and the DAPM
operations (something that would never deadlock in real life).

Signed-off-by: Liam Girdwood <lrg@ti.com>
---
 include/sound/soc.h  |    5 +++++
 sound/soc/soc-core.c |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 82bd773..f2b5ad1 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -288,6 +288,11 @@ enum snd_soc_pcm_subclass {
 	SND_SOC_PCM_CLASS_BE	= 1,
 };
 
+enum snd_soc_card_subclass {
+	SND_SOC_CARD_CLASS_INIT	= 0,
+	SND_SOC_CARD_CLASS_PCM	= 1,
+};
+
 int snd_soc_codec_set_sysclk(struct snd_soc_codec *codec, int clk_id,
 			     int source, unsigned int freq, int dir);
 int snd_soc_codec_set_pll(struct snd_soc_codec *codec, int pll_id, int source,
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 29dbbd7..2038522 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1416,7 +1416,7 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card)
 	struct snd_soc_dai_link *dai_link;
 	int ret, i, order;
 
-	mutex_lock(&card->mutex);
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_INIT);
 
 	if (card->instantiated) {
 		mutex_unlock(&card->mutex);
-- 
1.7.5.4

[PATCH 2/3] ASoC: dapm - Use card mutex for DAPM ops instead of codec mutex.

[Liam Girdwood]

It has now become necessary to use the card mutex instead of the codec
mutex to lock the DAPM operations. This is due to the recent multi
component support and forth coming Dynamic PCM updates.

Currently we lock DAPM operations with the codec mutex of the calling
RTD context. However, DAPM operations can span the whole card context 
and all components.

This patch updates the DAPM operations that use the codec mutex to
now use the card mutex PCM subclass for all DAPM ops.

Signed-off-by: Liam Girdwood <lrg@ti.com>
---
 sound/soc/soc-dapm.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index c9b088d..faeb28d 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -2335,6 +2335,7 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dapm_widget_list *wlist = snd_kcontrol_chip(kcontrol);
 	struct snd_soc_dapm_widget *widget = wlist->widgets[0];
 	struct snd_soc_codec *codec = widget->codec;
+	struct snd_soc_card *card = codec->card;
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
@@ -2361,7 +2362,7 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 		/* old connection must be powered down */
 		connect = invert ? 1 : 0;
 
-	mutex_lock(&codec->mutex);
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
 
 	change = snd_soc_test_bits(widget->codec, reg, mask, val);
 	if (change) {
@@ -2383,7 +2384,7 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 		}
 	}
 
-	mutex_unlock(&codec->mutex);
+	mutex_unlock(&card->mutex);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_put_volsw);
@@ -2432,6 +2433,7 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dapm_widget_list *wlist = snd_kcontrol_chip(kcontrol);
 	struct snd_soc_dapm_widget *widget = wlist->widgets[0];
 	struct snd_soc_codec *codec = widget->codec;
+	struct snd_soc_card *card = codec->card;
 	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
 	unsigned int val, mux, change;
 	unsigned int mask, bitmask;
@@ -2452,7 +2454,7 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 		mask |= (bitmask - 1) << e->shift_r;
 	}
 
-	mutex_lock(&codec->mutex);
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
 
 	change = snd_soc_test_bits(widget->codec, e->reg, mask, val);
 	if (change) {
@@ -2474,7 +2476,7 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 		}
 	}
 
-	mutex_unlock(&codec->mutex);
+	mutex_unlock(&card->mutex);
 	return change;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double);
@@ -2511,6 +2513,7 @@ int snd_soc_dapm_put_enum_virt(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dapm_widget_list *wlist = snd_kcontrol_chip(kcontrol);
 	struct snd_soc_dapm_widget *widget = wlist->widgets[0];
 	struct snd_soc_codec *codec = widget->codec;
+	struct snd_soc_card *card = codec->card;
 	struct soc_enum *e =
 		(struct soc_enum *)kcontrol->private_value;
 	int change;
@@ -2520,7 +2523,7 @@ int snd_soc_dapm_put_enum_virt(struct snd_kcontrol *kcontrol,
 	if (ucontrol->value.enumerated.item[0] >= e->max)
 		return -EINVAL;
 
-	mutex_lock(&codec->mutex);
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
 
 	change = widget->value != ucontrol->value.enumerated.item[0];
 	if (change) {
@@ -2533,7 +2536,7 @@ int snd_soc_dapm_put_enum_virt(struct snd_kcontrol *kcontrol,
 		}
 	}
 
-	mutex_unlock(&codec->mutex);
+	mutex_unlock(&card->mutex);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_virt);
@@ -2598,6 +2601,7 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dapm_widget_list *wlist = snd_kcontrol_chip(kcontrol);
 	struct snd_soc_dapm_widget *widget = wlist->widgets[0];
 	struct snd_soc_codec *codec = widget->codec;
+	struct snd_soc_card *card = codec->card;
 	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
 	unsigned int val, mux, change;
 	unsigned int mask;
@@ -2616,7 +2620,7 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 		mask |= e->mask << e->shift_r;
 	}
 
-	mutex_lock(&codec->mutex);
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
 
 	change = snd_soc_test_bits(widget->codec, e->reg, mask, val);
 	if (change) {
@@ -2638,7 +2642,7 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 		}
 	}
 
-	mutex_unlock(&codec->mutex);
+	mutex_unlock(&card->mutex);
 	return change;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_put_value_enum_double);
@@ -2675,7 +2679,7 @@ int snd_soc_dapm_get_pin_switch(struct snd_kcontrol *kcontrol,
 	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
 	const char *pin = (const char *)kcontrol->private_value;
 
-	mutex_lock(&card->mutex);
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
 
 	ucontrol->value.integer.value[0] =
 		snd_soc_dapm_get_pin_status(&card->dapm, pin);
@@ -2698,7 +2702,7 @@ int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol,
 	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
 	const char *pin = (const char *)kcontrol->private_value;
 
-	mutex_lock(&card->mutex);
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
 
 	if (ucontrol->value.integer.value[0])
 		snd_soc_dapm_enable_pin(&card->dapm, pin);
@@ -2990,11 +2994,11 @@ static void soc_dapm_stream_event(struct snd_soc_dapm_context *dapm,
 int snd_soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream,
 			      struct snd_soc_dai *dai, int event)
 {
-	struct snd_soc_codec *codec = rtd->codec;
+	struct snd_soc_card *card = rtd->card;
 
-	mutex_lock(&codec->mutex);
-	soc_dapm_stream_event(&codec->dapm, stream, dai, event);
-	mutex_unlock(&codec->mutex);
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
+	soc_dapm_stream_event(&card->dapm, stream, dai, event);
+	mutex_unlock(&card->mutex);
 	return 0;
 }
 
-- 
1.7.5.4

Re: [PATCH 2/3] ASoC: dapm - Use card mutex for DAPM ops instead of codec mutex.

[Mark Brown]

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

On Fri, Mar 02, 2012 at 06:11:11PM +0000, Liam Girdwood wrote:

Thanks for looking at this stuff, it's needed attention for a long time
but it's never been a practical problem and it's always depressed me too
much every time I've thought about looking at it myself.

> This patch updates the DAPM operations that use the codec mutex to
> now use the card mutex PCM subclass for all DAPM ops.

Hrm, this makes "PCM" look misnamed...  should we have a DAPM context,
or perhaps even just a separate lock for DAPM?  If we can have a
separate lock that'd be good, it's usually much simpler (at least for
me) to reason about multiple locks interacting than nesting on the same
lock but it's not always reasonable to split the locks, like with the
PCM stuff calling back into itself.

It looks like we're also missing a lock in snd_soc_dapm_sync() here, we
need to lock that as well as the updaters otherwise we might change the
lists underneath the DAPM run which doesn't work so well.  I might've
missed that, though (and currently we're not exactly 100% on taking that
lock when we should).  That's why snd_soc_jack_report() has the CODEC
mutex for example.

We'll also need to make sure the locking for the register I/O is sorted,
it's OK for the devices using regmap as that locks for us but currently
anything using the ASoC level cache relies on the CODEC mutex being held
for register I/O.  This isn't quite so risky as it was when we had the
advanced caches since the data structure is just a plain array but
writes may still go AWOL and that'd be a nightmare to debug.  I'm
worrying that right now something is relying on the fact that DAPM takes
the CODEC mutex to protect it.  But probably it's OK to fix that up
separately I think, we've got plenty of problems there and either
killing all the ASoC level caches or adding a new lock at the cache
level seem like the way to go both of which are basically orthogonal to
what's going on here.

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

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

Re: [PATCH 2/3] ASoC: dapm - Use card mutex for DAPM ops instead of codec mutex.

[Liam Girdwood]

On Sun, 2012-03-04 at 14:09 +0000, Mark Brown wrote:
> On Fri, Mar 02, 2012 at 06:11:11PM +0000, Liam Girdwood wrote:
> 
> Thanks for looking at this stuff, it's needed attention for a long time
> but it's never been a practical problem and it's always depressed me too
> much every time I've thought about looking at it myself.
> 
> > This patch updates the DAPM operations that use the codec mutex to
> > now use the card mutex PCM subclass for all DAPM ops.
> 
> Hrm, this makes "PCM" look misnamed...  should we have a DAPM context,
> or perhaps even just a separate lock for DAPM?  If we can have a
> separate lock that'd be good, it's usually much simpler (at least for
> me) to reason about multiple locks interacting than nesting on the same
> lock but it's not always reasonable to split the locks, like with the
> PCM stuff calling back into itself.

A separate DAPM mutex and subclass (for init and PCM ops) would be
required here but I'm fine with that too. 

I do also need the card mutex changes for Dynamic PCM though since we
need to lock the card PCM operations for Dynamic PCM devices.

> 
> It looks like we're also missing a lock in snd_soc_dapm_sync() here, we
> need to lock that as well as the updaters otherwise we might change the
> lists underneath the DAPM run which doesn't work so well.  I might've
> missed that, though (and currently we're not exactly 100% on taking that
> lock when we should).  That's why snd_soc_jack_report() has the CODEC
> mutex for example.
> 

Yeah, we are missing one or two - looking through that now. Some of the
DAPM init functions also need locking here too.

> We'll also need to make sure the locking for the register I/O is sorted,
> it's OK for the devices using regmap as that locks for us but currently
> anything using the ASoC level cache relies on the CODEC mutex being held
> for register I/O.  This isn't quite so risky as it was when we had the
> advanced caches since the data structure is just a plain array but
> writes may still go AWOL and that'd be a nightmare to debug.  I'm
> worrying that right now something is relying on the fact that DAPM takes
> the CODEC mutex to protect it.  But probably it's OK to fix that up
> separately I think, we've got plenty of problems there and either
> killing all the ASoC level caches or adding a new lock at the cache
> level seem like the way to go both of which are basically orthogonal to
> what's going on here.

Agree, adding a new cache lock is best here, but regmap is better. It
may be possible to use snd_soc_update_bits_locked() for DAPM codec
access though instead of adding a new lock.

Fwiw, looking at mutex holders in soc/codecs/ gives us :-

grep -rn "mutex_lock(" sound/soc/codecs/
sound/soc/codecs/tlv320dac33.c:252:	mutex_lock(&dac33->mutex);
sound/soc/codecs/tlv320dac33.c:379:	mutex_lock(&dac33->mutex);
sound/soc/codecs/tlv320dac33.c:743:	mutex_lock(&dac33->mutex);
sound/soc/codecs/tlv320dac33.c:914:	mutex_lock(&dac33->mutex);
sound/soc/codecs/tlv320aic3x.c:162:	mutex_lock(&widget->codec->mutex);
sound/soc/codecs/wm8903.c:464:	mutex_lock(&codec->mutex);
sound/soc/codecs/wm8994.c:725:	mutex_lock(&wm8994->accdet_lock);
sound/soc/codecs/wm8994.c:743:	mutex_lock(&wm8994->accdet_lock);
sound/soc/codecs/wm8994.c:3186:				mutex_lock(&codec->mutex);
sound/soc/codecs/wm8994.c:3230:	mutex_lock(&wm8994->accdet_lock);
sound/soc/codecs/wm8994.c:3266:			mutex_lock(&codec->mutex);
sound/soc/codecs/wm8994.c:3279:			mutex_lock(&codec->mutex);
sound/soc/codecs/wm8994.c:3405:	mutex_lock(&wm8994->accdet_lock);
sound/soc/codecs/tpa6130a2.c:133:	mutex_lock(&data->mutex);
sound/soc/codecs/tpa6130a2.c:199:	mutex_lock(&data->mutex);
sound/soc/codecs/tpa6130a2.c:232:	mutex_lock(&data->mutex);
sound/soc/codecs/wm8731.c:139:	mutex_lock(&codec->mutex);
sound/soc/codecs/wl1273.c:53:	mutex_lock(&core->lock);
sound/soc/codecs/wl1273.c:151:	mutex_lock(&core->lock);
sound/soc/codecs/wm8962.c:1581:	mutex_lock(&codec->mutex);
sound/soc/codecs/max98095.c:1903:	mutex_lock(&codec->mutex);
sound/soc/codecs/max98095.c:2057:	mutex_lock(&codec->mutex);
sound/soc/codecs/wm8958-dsp2.c:870:		mutex_lock(&codec->mutex);
sound/soc/codecs/wm8958-dsp2.c:882:		mutex_lock(&codec->mutex);
sound/soc/codecs/wm8958-dsp2.c:903:	mutex_lock(&codec->mutex);
sound/soc/codecs/twl6040.c:686:	mutex_lock(&priv->mutex);

Some of which initially look like private mutexs.

Liam

Re: [PATCH 2/3] ASoC: dapm - Use card mutex for DAPM ops instead of codec mutex.

[Mark Brown]

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

On Mon, Mar 05, 2012 at 02:48:02PM +0000, Liam Girdwood wrote:

> A separate DAPM mutex and subclass (for init and PCM ops) would be
> required here but I'm fine with that too. 

What's the path where init and PCM ops end up fighting with each other
for DAPM?  I can't think of one right now.

> I do also need the card mutex changes for Dynamic PCM though since we
> need to lock the card PCM operations for Dynamic PCM devices.

Yes, they do need to have something.

> Fwiw, looking at mutex holders in soc/codecs/ gives us :-
> 
> grep -rn "mutex_lock(" sound/soc/codecs/

> sound/soc/codecs/twl6040.c:686:	mutex_lock(&priv->mutex);

> Some of which initially look like private mutexs.

Yes, we only really need to worry about the ones that are taking a
subsystem level mutex here - if they're locking a driver-local mutex
they shouldn't need to worry about what the subsystem does.

The twl6040 one looks like it doesn't work anyway as we're only taking
the lock at one point which is itself guaranteed to be called only from
one context.

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

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

Re: [PATCH 2/3] ASoC: dapm - Use card mutex for DAPM ops instead of codec mutex.

[Liam Girdwood]

On Mon, 2012-03-05 at 15:26 +0000, Mark Brown wrote:
> On Mon, Mar 05, 2012 at 02:48:02PM +0000, Liam Girdwood wrote:
> 
> > A separate DAPM mutex and subclass (for init and PCM ops) would be
> > required here but I'm fine with that too. 
> 
> What's the path where init and PCM ops end up fighting with each other
> for DAPM?  I can't think of one right now.
> 

PCM ops and DAPM don't fight with each other, there is a dependency with
the snd_card->control_rwsem. The calling ordering is different for init
that it is for kcontrol updates that causes the warning below :-

[   24.726867] ======================================================
[   24.733367] [ INFO: possible circular locking dependency detected ]
[   24.739959] 3.3.0-rc4+ #72 Not tainted
[   24.742919] -------------------------------------------------------
[   24.743927] alsactl/3986 is trying to acquire lock:
[   24.754455]  (&card->dapm_mutex){+.+.+.}, at: [<bf1f2858>] snd_soc_dapm_mixer_update_power+0x38/0x5c [snd_soc_core]
[   24.765075] 
[   24.765075] but task is already holding lock:
[   24.772979]  (&card->controls_rwsem){++++.+}, at: [<bf0519fc>] snd_ctl_ioctl+0x214/0xbc4 [snd]
[   24.778411] 
[   24.778411] which lock already depends on the new lock.
[   24.782165] 
[   24.782165] 
[   24.790771] the existing dependency chain (in reverse order) is:
[   24.798645] 
[   24.798645] -> #1 (&card->controls_rwsem){++++.+}:
[   24.805267]        [<c0098ab8>] __lock_acquire+0x1380/0x19dc
[   24.806579]        [<c0099698>] lock_acquire+0xa0/0x130
[   24.816741]        [<c04e08e8>] down_write+0x3c/0x4c
[   24.816741]        [<bf051280>] snd_ctl_add+0x70/0x1d0 [snd]
[   24.827331]        [<bf1f1534>] snd_soc_dapm_new_widgets+0x480/0x6ac [snd_soc_core]
[   24.827972]        [<bf311a38>] abe_mixer_add_widgets+0x194/0x218 [snd_soc_omap_abe]
[   24.839508]        [<bf310578>] abe_probe+0x4c8/0x6cc [snd_soc_omap_abe]
[   24.851348]        [<bf1eb914>] snd_soc_instantiate_cards+0x868/0x1210 [snd_soc_core]
[   24.851348]        [<bf1ec534>] snd_soc_register_dais+0x150/0x1f0 [snd_soc_core]
[   24.865783]        [<bf3a2960>] asoc_mcpdm_probe+0x14c/0x1d4 [snd_soc_omap_mcpdm]
[   24.874816]        [<c031fc18>] platform_drv_probe+0x28/0x2c
[   24.875427]        [<c031e4a4>] driver_probe_device+0xc0/0x2c4
[   24.882751]        [<c031e74c>] __driver_attach+0xa4/0xa8
[   24.889862]        [<c031ca0c>] bus_for_each_dev+0x60/0x8c
[   24.894531]        [<c031e074>] driver_attach+0x28/0x30
[   24.903167]        [<c031dc94>] bus_add_driver+0x1a4/0x288
[   24.903167]        [<c031ecdc>] driver_register+0x88/0x140
[   24.915008]        [<c031ff40>] platform_driver_register+0x54/0x68
[   24.922760]        [<bf3a5014>] 0xbf3a5014
[   24.922760]        [<c0008734>] do_one_initcall+0x48/0x190
[   24.932861]        [<c00a4554>] sys_init_module+0xeac/0x1da0
[   24.934997]        [<c0014780>] ret_fast_syscall+0x0/0x3c
[   24.944549] 
[   24.944549] -> #0 (&card->dapm_mutex){+.+.+.}:
[   24.948974]        [<c04db2e8>] print_circular_bug+0x68/0x2b8
[   24.952301]        [<c0098d14>] __lock_acquire+0x15dc/0x19dc
[   24.952301]        [<c0099698>] lock_acquire+0xa0/0x130
[   24.968353]        [<c04e0154>] mutex_lock_nested+0x4c/0x308
[   24.968353]        [<bf1f2858>] snd_soc_dapm_mixer_update_power+0x38/0x5c [snd_soc_core]
[   24.975769]        [<bf3110ec>] abe_put_switch+0x80/0xd0 [snd_soc_omap_abe]
[   24.983581]        [<bf052014>] snd_ctl_ioctl+0x82c/0xbc4 [snd]
[   24.996673]        [<c012851c>] do_vfs_ioctl+0x8c/0x590
[   24.999206]        [<c0128aa0>] sys_ioctl+0x80/0x88
[   25.007324]        [<c0014780>] ret_fast_syscall+0x0/0x3c
[   25.011810] 
[   25.011810] other info that might help us debug this:
[   25.013031] 
[   25.021453]  Possible unsafe locking scenario:
[   25.021453] 
[   25.027282]        CPU0                    CPU1
[   25.032440]        ----                    ----
[   25.037200]   lock(&card->controls_rwsem);
[   25.040435]                                lock(&card->dapm_mutex);
[   25.048126]                                lock(&card->controls_rwsem);
[   25.055084]   lock(&card->dapm_mutex);
[   25.059051] 
[   25.059051]  *** DEADLOCK ***
[   25.059051] 
[   25.059051] 2 locks held by alsactl/3986:
[   25.066955]  #0:  (&card->power_lock){+.+...}, at: [<bf0519dc>] snd_ctl_ioctl+0x1f4/0xbc4 [snd]
[   25.078704]  #1:  (&card->controls_rwsem){++++.+}, at: [<bf0519fc>] snd_ctl_ioctl+0x214/0xbc4 [snd]

Liam

Re: [PATCH 2/3] ASoC: dapm - Use card mutex for DAPM ops instead of codec mutex.

[Mark Brown]

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

On Mon, Mar 05, 2012 at 03:55:10PM +0000, Liam Girdwood wrote:

> PCM ops and DAPM don't fight with each other, there is a dependency with
> the snd_card->control_rwsem. The calling ordering is different for init
> that it is for kcontrol updates that causes the warning below :-

control_rwsem isn't in mainline...

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

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

Re: [PATCH 2/3] ASoC: dapm - Use card mutex for DAPM ops instead of codec mutex.

[Liam Girdwood]

On Mon, 2012-03-05 at 16:45 +0000, Mark Brown wrote:
> On Mon, Mar 05, 2012 at 03:55:10PM +0000, Liam Girdwood wrote:
> 
> > PCM ops and DAPM don't fight with each other, there is a dependency with
> > the snd_card->control_rwsem. The calling ordering is different for init
> > that it is for kcontrol updates that causes the warning below :-
> 
> control_rwsem isn't in mainline...

Sorry, my typo controls_rwsem. Mostly used in sound/core/control.c

Re: [PATCH 2/3] ASoC: dapm - Use card mutex for DAPM ops instead of codec mutex.

[Mark Brown]

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

On Mon, Mar 05, 2012 at 05:05:12PM +0000, Liam Girdwood wrote:
> On Mon, 2012-03-05 at 16:45 +0000, Mark Brown wrote:

> > control_rwsem isn't in mainline...

> Sorry, my typo controls_rwsem. Mostly used in sound/core/control.c

Oh, ick.  Right.  This is a bit nasty.  So, the ALSA core is assuming
that we won't lock against ourselves during probe, which is actually not
that unreasonable given that the driver model guarantees that probe
isn't going to get called multiple times.  Now, if Grant's probe
deferral stuff makes it in to 3.4 (which would be nice anyway and is
looking likely) we can actually pretty much do that as the driver core
will do the waiting for things to come up bit for us which is the main
thing we're worried about here.

That said there are cases where we want to do a DAPM run due to
interrupts.  Currently we're actually just suppressing all those DAPM
runs so perhaps we'd be home free, we'd just need to worry about widget
status updates and that's much more localised.  But let's just add the
nested mutex for now (well, I guess for 3.5 at this point - Linus is
threatening to open the merge window this week so probably shouldn't be
pushing much new stuff in now) and take another look later.

The other nicer thing to do would be to fix this at an ALSA level - the
controls_rwsem looks like a good candidate for something like RCU, it's
a read mostly list with very infrequent updates so even a rwsem is more
heavyweight than we need.  The index offset stuff does make things very
slightly more complex though I think it's tractable.

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

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

[PATCH 3/3] ASoC: dapm - lock mixer & mux update power with card mutex.

[Liam Girdwood]

Both snd_soc_dapm_mux_update_power() and snd_soc_dapm_mixer_update_power() can
be called internally within DAPM core (with card mutex held) and externally.

Provide some wrappers so that external users of both functions do not have to
remeber and hold the card mutex.

Signed-off-by: Liam Girdwood <lrg@ti.com>
---
 sound/soc/soc-dapm.c |   36 ++++++++++++++++++++++++++++++------
 1 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index faeb28d..07790b5 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1718,7 +1718,7 @@ static inline void dapm_debugfs_cleanup(struct snd_soc_dapm_context *dapm)
 #endif
 
 /* test and update the power status of a mux widget */
-int snd_soc_dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
+static int soc_dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
 				 struct snd_kcontrol *kcontrol, int mux, struct soc_enum *e)
 {
 	struct snd_soc_dapm_path *path;
@@ -1757,10 +1757,22 @@ int snd_soc_dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
 
 	return 0;
 }
+
+int snd_soc_dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
+				 struct snd_kcontrol *kcontrol, int mux, struct soc_enum *e)
+{
+	struct snd_soc_card *card = widget->dapm->card;
+	int ret;
+
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
+	ret = soc_dapm_mux_update_power(widget, kcontrol, mux, e);
+	mutex_unlock(&card->mutex);
+	return ret;
+}
 EXPORT_SYMBOL_GPL(snd_soc_dapm_mux_update_power);
 
 /* test and update the power status of a mixer or switch widget */
-int snd_soc_dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
+static int soc_dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
 				   struct snd_kcontrol *kcontrol, int connect)
 {
 	struct snd_soc_dapm_path *path;
@@ -1789,6 +1801,18 @@ int snd_soc_dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
 
 	return 0;
 }
+
+int snd_soc_dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
+				   struct snd_kcontrol *kcontrol, int connect)
+{
+	struct snd_soc_card *card = widget->dapm->card;
+	int ret;
+
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_PCM);
+	ret = soc_dapm_mixer_update_power(widget, kcontrol, connect);
+	mutex_unlock(&card->mutex);
+	return ret;
+}
 EXPORT_SYMBOL_GPL(snd_soc_dapm_mixer_update_power);
 
 /* show dapm widget status in sys fs */
@@ -2378,7 +2402,7 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 			update.val = val;
 			widget->dapm->update = &update;
 
-			snd_soc_dapm_mixer_update_power(widget, kcontrol, connect);
+			soc_dapm_mixer_update_power(widget, kcontrol, connect);
 
 			widget->dapm->update = NULL;
 		}
@@ -2470,7 +2494,7 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 			update.val = val;
 			widget->dapm->update = &update;
 
-			snd_soc_dapm_mux_update_power(widget, kcontrol, mux, e);
+			soc_dapm_mux_update_power(widget, kcontrol, mux, e);
 
 			widget->dapm->update = NULL;
 		}
@@ -2532,7 +2556,7 @@ int snd_soc_dapm_put_enum_virt(struct snd_kcontrol *kcontrol,
 
 			widget->value = ucontrol->value.enumerated.item[0];
 
-			snd_soc_dapm_mux_update_power(widget, kcontrol, widget->value, e);
+			soc_dapm_mux_update_power(widget, kcontrol, widget->value, e);
 		}
 	}
 
@@ -2636,7 +2660,7 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 			update.val = val;
 			widget->dapm->update = &update;
 
-			snd_soc_dapm_mux_update_power(widget, kcontrol, mux, e);
+			soc_dapm_mux_update_power(widget, kcontrol, mux, e);
 
 			widget->dapm->update = NULL;
 		}
-- 
1.7.5.4