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