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

[Liam Girdwood <lrg@ti.com>]

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