[alsa-devel] [PATCH] ASoC: OMAP machines: Fix kernel crash due to changes in core
Calling soc_dapm_sync() after adding DAPM widgets/routes will lead to kernel crash caused by unitialized widget->power_check callback (NULL pointer dereference).
Call snd_soc_dapm_new_widgets(dapm); instead of soc_dapm_sync which will initialize the widgets, and will call the dapm_power_widgets (soc_dapm_sync is a wrapper for dapm_power_widgets).
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Cc: Anuj Aggarwal anuj.aggarwal@ti.com Cc: Janusz Krzysztofik jkrzyszt@tis.icnet.pl Cc: Jarkko Nikula jarkko.nikula@bitmer.com Cc: Gražvydas Ignotas notasas@gmail.com Cc: Misael Lopez Cruz misael.lopez@ti.com --- sound/soc/omap/am3517evm.c | 2 +- sound/soc/omap/ams-delta.c | 2 +- sound/soc/omap/n810.c | 2 +- sound/soc/omap/omap3pandora.c | 4 ++-- sound/soc/omap/osk5912.c | 2 +- sound/soc/omap/rx51.c | 9 ++++++--- sound/soc/omap/sdp3430.c | 2 +- sound/soc/omap/sdp4430.c | 2 +- sound/soc/omap/zoom2.c | 2 +- 9 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/sound/soc/omap/am3517evm.c b/sound/soc/omap/am3517evm.c index 48af0f8..f9b995f 100644 --- a/sound/soc/omap/am3517evm.c +++ b/sound/soc/omap/am3517evm.c @@ -107,7 +107,7 @@ static int am3517evm_aic23_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_enable_pin(dapm, "Line In"); snd_soc_dapm_enable_pin(dapm, "Mic In");
- snd_soc_dapm_sync(dapm); + snd_soc_dapm_new_widgets(dapm);
return 0; } diff --git a/sound/soc/omap/ams-delta.c b/sound/soc/omap/ams-delta.c index 0aa475f..10d3d41 100644 --- a/sound/soc/omap/ams-delta.c +++ b/sound/soc/omap/ams-delta.c @@ -569,7 +569,7 @@ static int ams_delta_cx20442_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_disable_pin(dapm, "Speaker"); snd_soc_dapm_disable_pin(dapm, "AGCIN"); snd_soc_dapm_disable_pin(dapm, "AGCOUT"); - snd_soc_dapm_sync(dapm); + snd_soc_dapm_new_widgets(dapm);
/* Add virtual switch */ ret = snd_soc_add_controls(codec, ams_delta_audio_controls, diff --git a/sound/soc/omap/n810.c b/sound/soc/omap/n810.c index c10d356..bda2aa4 100644 --- a/sound/soc/omap/n810.c +++ b/sound/soc/omap/n810.c @@ -282,7 +282,7 @@ static int n810_aic33_init(struct snd_soc_pcm_runtime *rtd) /* Set up N810 specific audio path audio_map */ snd_soc_dapm_add_routes(dapm, audio_map, ARRAY_SIZE(audio_map));
- snd_soc_dapm_sync(dapm); + snd_soc_dapm_new_widgets(dapm);
return 0; } diff --git a/sound/soc/omap/omap3pandora.c b/sound/soc/omap/omap3pandora.c index 3ae87fd..35368fa 100644 --- a/sound/soc/omap/omap3pandora.c +++ b/sound/soc/omap/omap3pandora.c @@ -176,7 +176,7 @@ static int omap3pandora_out_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_add_routes(dapm, omap3pandora_out_map, ARRAY_SIZE(omap3pandora_out_map));
- return snd_soc_dapm_sync(dapm); + return snd_soc_dapm_new_widgets(dapm); }
static int omap3pandora_in_init(struct snd_soc_pcm_runtime *rtd) @@ -199,7 +199,7 @@ static int omap3pandora_in_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_add_routes(dapm, omap3pandora_in_map, ARRAY_SIZE(omap3pandora_in_map));
- return snd_soc_dapm_sync(dapm); + return snd_soc_dapm_new_widgets(dapm); }
static struct snd_soc_ops omap3pandora_ops = { diff --git a/sound/soc/omap/osk5912.c b/sound/soc/omap/osk5912.c index 5978332..4a13c45 100644 --- a/sound/soc/omap/osk5912.c +++ b/sound/soc/omap/osk5912.c @@ -107,7 +107,7 @@ static int osk_tlv320aic23_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_enable_pin(dapm, "Line In"); snd_soc_dapm_enable_pin(dapm, "Mic Jack");
- snd_soc_dapm_sync(dapm); + snd_soc_dapm_new_widgets(dapm);
return 0; } diff --git a/sound/soc/omap/rx51.c b/sound/soc/omap/rx51.c index 7164db5..a4676a0 100644 --- a/sound/soc/omap/rx51.c +++ b/sound/soc/omap/rx51.c @@ -317,7 +317,7 @@ static int rx51_aic34_init(struct snd_soc_pcm_runtime *rtd) if (err < 0) return err;
- snd_soc_dapm_sync(dapm); + snd_soc_dapm_new_widgets(dapm);
/* AV jack detection */ err = snd_soc_jack_new(codec, "AV Jack", @@ -346,8 +346,11 @@ static int rx51_aic34b_init(struct snd_soc_dapm_context *dapm) if (err < 0) return 0;
- return snd_soc_dapm_add_routes(dapm, audio_mapb, - ARRAY_SIZE(audio_mapb)); + err = snd_soc_dapm_add_routes(dapm, audio_mapb, ARRAY_SIZE(audio_mapb)); + if (err < 0) + return 0; + + return snd_soc_dapm_new_widgets(dapm); }
/* Digital audio interface glue - connects codec <--> CPU */ diff --git a/sound/soc/omap/sdp3430.c b/sound/soc/omap/sdp3430.c index 269aded..4bb650f 100644 --- a/sound/soc/omap/sdp3430.c +++ b/sound/soc/omap/sdp3430.c @@ -159,7 +159,7 @@ static int sdp3430_twl4030_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_nc_pin(dapm, "CARKITL"); snd_soc_dapm_nc_pin(dapm, "CARKITR");
- ret = snd_soc_dapm_sync(dapm); + ret = snd_soc_dapm_new_widgets(dapm); if (ret) return ret;
diff --git a/sound/soc/omap/sdp4430.c b/sound/soc/omap/sdp4430.c index 79ec76d..10972fb 100644 --- a/sound/soc/omap/sdp4430.c +++ b/sound/soc/omap/sdp4430.c @@ -140,7 +140,7 @@ static int sdp4430_twl6040_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_enable_pin(dapm, "Headset Mic"); snd_soc_dapm_enable_pin(dapm, "Headset Stereophone");
- ret = snd_soc_dapm_sync(dapm); + ret = snd_soc_dapm_new_widgets(dapm); if (ret) return ret;
diff --git a/sound/soc/omap/zoom2.c b/sound/soc/omap/zoom2.c index 8b1ebbc..9e7d21d 100644 --- a/sound/soc/omap/zoom2.c +++ b/sound/soc/omap/zoom2.c @@ -126,7 +126,7 @@ static int zoom2_twl4030_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_nc_pin(dapm, "CARKITL"); snd_soc_dapm_nc_pin(dapm, "CARKITR");
- ret = snd_soc_dapm_sync(dapm); + ret = snd_soc_dapm_new_widgets(dapm);
return ret; }
On Fri, Oct 07, 2011 at 10:06:46AM +0300, Peter Ujfalusi wrote:
Calling soc_dapm_sync() after adding DAPM widgets/routes will lead to kernel crash caused by unitialized widget->power_check callback (NULL pointer dereference).
Call snd_soc_dapm_new_widgets(dapm); instead of soc_dapm_sync which will initialize the widgets, and will call the dapm_power_widgets (soc_dapm_sync is a wrapper for dapm_power_widgets).
No, there should be no need for either callback in individual drivers unless they're doing something very specialist. This will have been the case for a while now.
On Friday 07 October 2011 11:17:12 Mark Brown wrote:
On Fri, Oct 07, 2011 at 10:06:46AM +0300, Peter Ujfalusi wrote:
Calling soc_dapm_sync() after adding DAPM widgets/routes will lead to kernel crash caused by unitialized widget->power_check callback (NULL pointer dereference).
Call snd_soc_dapm_new_widgets(dapm); instead of soc_dapm_sync which will initialize the widgets, and will call the dapm_power_widgets (soc_dapm_sync is a wrapper for dapm_power_widgets).
No, there should be no need for either callback in individual drivers unless they're doing something very specialist. This will have been the case for a while now.
This might be true for machines, which adds jack functionality. We are now calling snd_soc_dapm_new_widgets before adding jack pins. Also machines passing their DAPM widgets/routes via snd_soc_card are safe from this issue.
For machines, which does not add jacks the snd_soc_dapm_new_widgets will be not called at all, which will eventually leads to a crash.
in soc-core.c: soc_post_component_init() the dai->init called, but there's no additional snd_soc_dapm_new_widgets call to make sure that the new widgets added by the machine driver are instantiated.
-- Péter
On Fri, Oct 07, 2011 at 01:40:21PM +0300, Péter Ujfalusi wrote:
On Friday 07 October 2011 11:17:12 Mark Brown wrote:
No, there should be no need for either callback in individual drivers unless they're doing something very specialist. This will have been the case for a while now.
This might be true for machines, which adds jack functionality. We are now calling snd_soc_dapm_new_widgets before adding jack pins. Also machines passing their DAPM widgets/routes via snd_soc_card are safe from this issue.
No, it should never be needed by anything.
For machines, which does not add jacks the snd_soc_dapm_new_widgets will be not called at all, which will eventually leads to a crash.
in soc-core.c: soc_post_component_init() the dai->init called, but there's no additional snd_soc_dapm_new_widgets call to make sure that the new widgets added by the machine driver are instantiated.
We could either go round every single machine driver in the kernel modifying them or we could make sure it's handled in the core - I know which of those seems better to me!
On Friday 07 October 2011 11:48:01 Mark Brown wrote:
This might be true for machines, which adds jack functionality. We are now calling snd_soc_dapm_new_widgets before adding jack pins. Also machines passing their DAPM widgets/routes via snd_soc_card are safe from this issue.
No, it should never be needed by anything.
Could be, but day before yesterday the sdp4430 was fine. I've pulled yesterday morning, and welcomed me with a kernel crash at boot time.
For machines, which does not add jacks the snd_soc_dapm_new_widgets will be not called at all, which will eventually leads to a crash.
in soc-core.c: soc_post_component_init() the dai->init called, but there's no additional snd_soc_dapm_new_widgets call to make sure that the new widgets added by the machine driver are instantiated.
We could either go round every single machine driver in the kernel modifying them or we could make sure it's handled in the core - I know which of those seems better to me!
For sure fixing this in core is the best place. Try to boot smartq_wm8987, s3c24xx_simtec_tlv320aic23, s3c24xx_simtec_hermes, jive_wm8750, h1940_uda1380, etc. Lots of the machine drivers are calling soc_dapm_sync after adding widgets. They will crash. If we want to fix this in core we need to remove the soc_dapm_sync from _init calls, and add the snd_soc_dapm_new_widgets post dai_link->init call in soc_post_component_init of soc-core.c.
Having the soc_dapm_sync without calling snd_soc_dapm_new_widgets after adding new widgets will trigger this.
As I said: sdp4430 strated to crash yesterday morning - without any change in the sdp4430 driver.
-- Péter
On Friday 07 October 2011 14:12:43 Péter Ujfalusi wrote:
As I said: sdp4430 strated to crash yesterday morning - without any change in the sdp4430 driver.
bisect given me this commit: commit 35c64bcad5c8244d973efbf7e58f6e0e09635504 Author: Mark Brown broonie@opensource.wolfsonmicro.com Date: Wed Sep 28 18:23:53 2011 +0100
ASoC: Ensure all DAPM widgets have a power check callback
Makes the code simpler.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
So this means we had this issue masked by the if (!w->power_check). Since machine drivers rarely add mixers/muxes the issue has not been really seen. We tend to add _SPK, _HP, and stuff like that...
But I have checked the commit before this one. While it is not crashing the kernel it does ignores the machine added DAPM widgets. They did not power on (they used to power on). Based on this commit this is expected behavior.
The bisected commit just masked the underlaying issue.
-- Péter
On Fri, Oct 07, 2011 at 02:12:43PM +0300, Péter Ujfalusi wrote:
On Friday 07 October 2011 11:48:01 Mark Brown wrote:
This might be true for machines, which adds jack functionality. We are now calling snd_soc_dapm_new_widgets before adding jack pins. Also machines passing their DAPM widgets/routes via snd_soc_card are safe from this issue.
No, it should never be needed by anything.
Could be, but day before yesterday the sdp4430 was fine. I've pulled yesterday morning, and welcomed me with a kernel crash at boot time.
You're missing the point here. A crash has been introduced but doing the syncs during startup should have been and should continue to be a waste of time with no useful impact on the behaviour of the system.
in soc-core.c: soc_post_component_init() the dai->init called, but there's no additional snd_soc_dapm_new_widgets call to make sure that the new widgets added by the machine driver are instantiated.
We could either go round every single machine driver in the kernel modifying them or we could make sure it's handled in the core - I know which of those seems better to me!
For sure fixing this in core is the best place. Try to boot smartq_wm8987, s3c24xx_simtec_tlv320aic23, s3c24xx_simtec_hermes, jive_wm8750, h1940_uda1380, etc. Lots of the machine drivers are calling soc_dapm_sync after adding widgets. They will crash. If we want to fix this in core we need to remove the soc_dapm_sync from _init calls, and add the snd_soc_dapm_new_widgets post dai_link->init call in soc_post_component_init of soc-core.c.
Removing the calls is a totally sensible thing to do, like I say they should at best be a waste of time. The problem with your patch was that weren't just removing them, you were replacing them with calls to new_widgets() which should be equally pointless. If we have to do that we're clearly failing at something.
On Friday 07 October 2011 12:47:49 Mark Brown wrote:
Removing the calls is a totally sensible thing to do, like I say they should at best be a waste of time. The problem with your patch was that weren't just removing them, you were replacing them with calls to new_widgets() which should be equally pointless. If we have to do that we're clearly failing at something.
I tend to agree with all of these. I have done some bisecting, and this is my conclusion: We have quite many machine drivers adding their DAPM widgets/routes in their dai_link->init callback. All of these machine driver added DAPM widgets stopped working with:
commit 0b07ab9244d34929b6e07d6a275137a229e9c839 ASoC: Instantiate DAPM widgets before we do the DAI link init
This patch moves the snd_soc_dapm_new_widgets() call pre dai_link->init. Make sense for those machines which passing their DAPM widgets/routes via the snd_soc_card struct. Rest of the machines were ended up their DAPM widgets being not initialized - ever. They had the soc_dapm_sync call which is pointless thing to call anyway. These machines started to crash with commit:
commit 35c64bcad5c8244d973efbf7e58f6e0e09635504 ASoC: Ensure all DAPM widgets have a power check callback
Since the check for w->power_check has been removed from dapm_power_one_widget
What we can do: - Add back the snd_soc_dapm_new_widgets() call post dai_link->init in the soc_post_component_init (while keeping the pre dai_link->init call to this). - Convert all machine drivers which uses the dai_link->init call to just add their DAPM widgets/routes to pass it via snd_soc_card struct. - From the remaining drivers the soc_dapm_sync need to be removed. If they do funky stuff with their widgets we might need to add snd_soc_dapm_new_widgets() for their init call to be sure they are not crashing.
I have converted some of the OMAP machine drivers according to point 2 after this patch. I only changed those which seamed obvious.
This patch was mostly a search'n'replace to make sure they are no longer crashing.
-- Péter
On Fri, Oct 07, 2011 at 04:14:36PM +0300, Péter Ujfalusi wrote:
What we can do:
- Add back the snd_soc_dapm_new_widgets() call post dai_link->init in the
soc_post_component_init (while keeping the pre dai_link->init call to this).
Yes, that's needed.
- Convert all machine drivers which uses the dai_link->init call to just add
their DAPM widgets/routes to pass it via snd_soc_card struct.
Obviously we should be doing this where we can - we won't be able to get all of them as some of them will have conditionals.
- From the remaining drivers the soc_dapm_sync need to be removed. If they do
funky stuff with their widgets we might need to add snd_soc_dapm_new_widgets() for their init call to be sure they are not crashing.
new_widgets() is orthogonal to the sync(), there is now a stronger requirement for it though.
I have converted some of the OMAP machine drivers according to point 2 after this patch. I only changed those which seamed obvious.
Yeah, I didn't apply all those patches as they depended on this one.
On Friday 07 October 2011 14:46:36 Mark Brown wrote:
- From the remaining drivers the soc_dapm_sync need to be removed. If
they do funky stuff with their widgets we might need to add snd_soc_dapm_new_widgets() for their init call to be sure they are not crashing.
new_widgets() is orthogonal to the sync(), there is now a stronger requirement for it though.
Yes, new_widgets() is instantiating the new widgets + sync() IMHO it is going to be rare case when machine drivers would need to do this.
I have converted some of the OMAP machine drivers according to point 2 after this patch. I only changed those which seamed obvious.
Yeah, I didn't apply all those patches as they depended on this one.
I'll rewrite the series to skip the sync -> new_widgets() step.
-- Péter
participants (2)
-
Mark Brown -
Peter Ujfalusi