Re: [alsa-devel] [PATCH] ASoC: dapm: Fix race condition in widgets power list creation
On Tue, Jan 18, 2011 at 01:35:37PM +0200, Peter Ujfalusi wrote:
Hmm, the patch looks quite similar to the patch I've sent back in November: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-November/033362.ht...
My thought exactly.
On 01/18/11 12:39, ext Mark Brown wrote:
This needs a bit more analysis - why are we getting multiple simultaneous calls to dapm_power_widgets() and is that itself safe? The ASoC locking model has always been to have a big lock around the entire card rather than to lock subcomponents, and for example this isn't going to make sure we're consistent with register map accesses from other parts of the code.
As far as I have traced back than, it was due to the use of dapm_sync in machine drivers. For me it looked that all other paths were satisfactory protected. In my case the use of SOC_DAPM_PIN_SWITCH() instead of the custom call fixed the race (since this will take a card mutex, AFAIK).
I *suspect* that we're in a similar case and either the locking has been broken by the multi-component stuff or there's other cases that need protection, probably the jack detection code.
PS: I might received this mail unintentionally, since I was not on the TO/CC, and alsa-devel neither.
Should alsa-devel be on the CC?
Yes, I bounced on the second copy - Misael had sent two copies of the mail, one omitting the CC, and I'd replied to the first one before I saw the second.
Resend, since alsa-devel address was mistyped ;)
On 01/18/11 13:39, ext Mark Brown wrote:
I *suspect* that we're in a similar case and either the locking has been broken by the multi-component stuff or there's other cases that need protection, probably the jack detection code.
The snd_soc_dapm_sync is just a wrapper for dapm_power_widgets, right? It (snd_soc_dapm_sync) is used in soc-core:soc_post_component_init. But there is no need for it, since the snd_soc_dapm_new_widgets will call dapm_power_widgets at the end. so we can remove the snd_soc_dapm_sync from there. For the rest: I would replace the snd_soc_dapm_sync calls in soc-dapm.c with dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP); calls. Modify the snd_soc_dapm_sync to use the codec->mutex around the dapm_power_widgets call. In that way I think all race conditions going to be covered without adding new mutex.
What do you think? I can make a patch...
-- Péter
On Tue, Jan 18, 2011 at 02:08:51PM +0200, Peter Ujfalusi wrote:
On 01/18/11 13:39, ext Mark Brown wrote:
I *suspect* that we're in a similar case and either the locking has been broken by the multi-component stuff or there's other cases that need protection, probably the jack detection code.
The snd_soc_dapm_sync is just a wrapper for dapm_power_widgets, right? It (snd_soc_dapm_sync) is used in soc-core:soc_post_component_init.
You're jumping into the analysis mid-way here again...
Modify the snd_soc_dapm_sync to use the codec->mutex around the dapm_power_widgets call.
This doesn't sound like the right solution in a multi-CODEC system, DAPM will affect multiple devices within the system.
On 01/18/11 14:26, ext Mark Brown wrote:
The snd_soc_dapm_sync is just a wrapper for dapm_power_widgets, right? It (snd_soc_dapm_sync) is used in soc-core:soc_post_component_init.
You're jumping into the analysis mid-way here again...
Unfortunately yes I did :(
Modify the snd_soc_dapm_sync to use the codec->mutex around the dapm_power_widgets call.
This doesn't sound like the right solution in a multi-CODEC system, DAPM will affect multiple devices within the system.
Yes, you are right.
But I think the snd_soc_dapm_sync call in soc_post_component_init can be removed, since it has been taken care of the snd_soc_dapm_new_widgets call... Surely it is not solving the problem, in fact it has nothing to do with the problem ;)
On Tue, 18 Jan 2011 14:47:17 +0200 Peter Ujfalusi peter.ujfalusi@nokia.com wrote:
But I think the snd_soc_dapm_sync call in soc_post_component_init can be removed, since it has been taken care of the snd_soc_dapm_new_widgets call...
Looks like it can be removed from machine init callbacks too since the snd_soc_dapm_new_widgets is called after machine initialization in soc_post_component_init.
On Tue, Jan 18, 2011 at 02:47:17PM +0200, Peter Ujfalusi wrote:
But I think the snd_soc_dapm_sync call in soc_post_component_init can be removed, since it has been taken care of the snd_soc_dapm_new_widgets call...
Yes.
On Tuesday 18 January 2011 14:08:51 ext Peter Ujfalusi wrote:
Resend, since alsa-devel address was mistyped ;)
On 01/18/11 13:39, ext Mark Brown wrote:
I *suspect* that we're in a similar case and either the locking has been broken by the multi-component stuff or there's other cases that need protection, probably the jack detection code.
The snd_soc_dapm_sync is just a wrapper for dapm_power_widgets, right? It (snd_soc_dapm_sync) is used in soc-core:soc_post_component_init. But there is no need for it, since the snd_soc_dapm_new_widgets will call dapm_power_widgets at the end. so we can remove the snd_soc_dapm_sync from there. For the rest: I would replace the snd_soc_dapm_sync calls in soc-dapm.c with dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP); calls. Modify the snd_soc_dapm_sync to use the codec->mutex around the dapm_power_widgets call. In that way I think all race conditions going to be covered without adding new mutex.
What do you think? I can make a patch...
Or not.. soc-jack.c:snd_soc_jack_report also calls snd_soc_dapm_sync, while keeping the codec->mutex locked... Should machine drivers also take the mutex when they call snd_soc_dapm_sync?
On Tue, Jan 18, 2011 at 02:34:13PM +0200, Peter Ujfalusi wrote:
Or not.. soc-jack.c:snd_soc_jack_report also calls snd_soc_dapm_sync, while keeping the codec->mutex locked... Should machine drivers also take the mutex when they call snd_soc_dapm_sync?
In general yes.
participants (3)
-
Jarkko Nikula -
Mark Brown -
Peter Ujfalusi