[alsa-devel] [RFC PATCH 0/3] Fixes for component unregister and remove
This set of patches aims to fix the problems seen during module unloading with the skylake driver and also with SOF. The sequence seen during snd_soc_skl module unload is that the machine driver in unregistered first which frees the snd_card. When the component driver is unregistered following this and the topology is unloaded, a null pointer dereference is encountered while removing the kcontrols because the snd_card has already been freed.
The first 2 patches fix this by removing the topology unloading from component_unregister() and moving it to the component remove() callback. This also makes the register_component() and unregister_component() calls balanced.
But this still doesnt solve the problem entirely because the component driver's remove() callback is invoked after soc_cleanup_card_resources() which frees the snd_card. And we encounter the same null pointer dereference seen while remving the kcontrols added to the snd_card.
In order to avoid this problem, the third patch proposes to remove the link components in snd_soc_unbind_card() before soc_cleanup_card_resources(). This way, the snd_card() will remain valid when the topology is unloaded.
Ranjani Sridharan (3): ASoC: core: do not unload topology in unregister_component() ASoC: intel: skylake: add remove() callback for component driver ASoC: core: remove link components before cleaning up card resources
sound/soc/intel/skylake/skl-pcm.c | 7 +++++++ sound/soc/soc-core.c | 13 +++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
Typically, topology is loaded when the card is registered by the machine driver and the link components are probed. Therefore, it should be unloaded when the link components are removed. This will make the register/unregister component methods balanced.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/soc-core.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 6f4842977b8d..ffd4db97a2ee 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3327,8 +3327,6 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue;
- snd_soc_tplg_component_remove(component, - SND_SOC_TPLG_INDEX_ALL); snd_soc_component_del_unlocked(component); found = 1; break;
On Thu, Apr 04, 2019 at 05:30:38PM -0700, Ranjani Sridharan wrote:
Typically, topology is loaded when the card is registered by the machine driver and the link components are probed. Therefore, it should be unloaded when the link components are removed. This will make the register/unregister component methods balanced.
continue;
snd_soc_tplg_component_remove(component,SND_SOC_TPLG_INDEX_ALL);
Isn't this a robustness fix? It's just freeing anything that's left over, it doesn't free specific stuff and shouldn't stop anything else freeing that before so if we made a mistake earlier on it'll clean up after you.
On Fri, 2019-04-05 at 09:17 +0700, Mark Brown wrote:
On Thu, Apr 04, 2019 at 05:30:38PM -0700, Ranjani Sridharan wrote:
Typically, topology is loaded when the card is registered by the machine driver and the link components are probed. Therefore, it should be unloaded when the link components are removed. This will make the register/unregister component methods balanced. continue;
snd_soc_tplg_component_remove(component,SND_SOC_TPLG_INDEX_ALL);Isn't this a robustness fix? It's just freeing anything that's left over, it doesn't free specific stuff and shouldn't stop anything else freeing that before so if we made a mistake earlier on it'll clean up after you.
Hi Mark,
Agree that this might make the clean up robust. But today we call this only when the component is unregistered and that's causing a null pointer dereference. So what we really need to do is to remove the topology in remove() first.
Thanks, Ranjani
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Apr 04, 2019 at 07:23:54PM -0700, Ranjani Sridharan wrote:
Agree that this might make the clean up robust. But today we call this only when the component is unregistered and that's causing a null pointer dereference. So what we really need to do is to remove the topology in remove() first.
If that's what this is doing then the changelog should reflect that it's fixing a null pointer dereference rather than talking about these separate issues.
On Fri, 2019-04-05 at 09:28 +0700, Mark Brown wrote:
On Thu, Apr 04, 2019 at 07:23:54PM -0700, Ranjani Sridharan wrote:
Agree that this might make the clean up robust. But today we call this only when the component is unregistered and that's causing a null pointer dereference. So what we really need to do is to remove the topology in remove() first.
If that's what this is doing then the changelog should reflect that it's fixing a null pointer dereference rather than talking about these separate issues.
Sure, I think I will drop the first patch as like you said it is good for robustness. Adding the remove() callback for skl and sof will fix the null pointer dereference issue.
But I really need some feedback from you on the third patch. I think that's the most invasive and I'm not sure about how to go about removing the topology before the snd_card is freed without removing the link components while unbinding the card.
Thanks, Ranjani
On Thu, Apr 04, 2019 at 07:33:33PM -0700, Ranjani Sridharan wrote:
But I really need some feedback from you on the third patch. I think that's the most invasive and I'm not sure about how to go about removing the topology before the snd_card is freed without removing the link components while unbinding the card.
It seemed reasonable enough, I've applied it.
Topology is not unloaded in the core during unregister_component() anymore. So, add the remove() callback that will unload the topology.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/intel/skylake/skl-pcm.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 56099db8f86d..57031b6d4d45 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1462,9 +1462,16 @@ static int skl_platform_soc_probe(struct snd_soc_component *component) return 0; }
+static void skl_pcm_remove(struct snd_soc_component *component) +{ + /* remove topology */ + snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL); +} + static const struct snd_soc_component_driver skl_component = { .name = "pcm", .probe = skl_platform_soc_probe, + .remove = skl_pcm_remove, .ops = &skl_platform_ops, .pcm_new = skl_pcm_new, .pcm_free = skl_pcm_free,
The patch
ASoC: intel: skylake: add remove() callback for component driver
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 2e05ddd2c9f8000751d52fcf35b8318da46026bc Mon Sep 17 00:00:00 2001
From: Ranjani Sridharan ranjani.sridharan@linux.intel.com Date: Thu, 4 Apr 2019 17:30:39 -0700 Subject: [PATCH] ASoC: intel: skylake: add remove() callback for component driver
Topology is not unloaded in the core during unregister_component() anymore. So, add the remove() callback that will unload the topology.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl-pcm.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 56099db8f86d..57031b6d4d45 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1462,9 +1462,16 @@ static int skl_platform_soc_probe(struct snd_soc_component *component) return 0; }
+static void skl_pcm_remove(struct snd_soc_component *component) +{ + /* remove topology */ + snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL); +} + static const struct snd_soc_component_driver skl_component = { .name = "pcm", .probe = skl_platform_soc_probe, + .remove = skl_pcm_remove, .ops = &skl_platform_ops, .pcm_new = skl_pcm_new, .pcm_free = skl_pcm_free,
When the card is registered by the machine driver, dai link components are probed after the snd_card is created. This is done in snd_soc_bind_card() which calls snd_soc_instantiate_card() to first create the snd_card and then probes the link components by calling soc_probe_link_components(). The snd_card is used by the component driver to add the kcontrols associated with dapm widgets to the card.
When the machine driver is unregistered, the snd_card is freed when the card resources are cleaned up. But the snd_card needs to be valid while unloading the topology dapm widgets in order to remove the kcontrols from the card.
Since, unloading topology is done when the component driver is removed, the link components should be removed in snd_soc_unbind_card(). This will ensure that the kcontrols are removed before the card resources are cleaned up and the snd_card itself is freed.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/soc-core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index ffd4db97a2ee..6277b2da68b7 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2831,10 +2831,21 @@ EXPORT_SYMBOL_GPL(snd_soc_register_card);
static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister) { + struct snd_soc_pcm_runtime *rtd; + int order; + if (card->instantiated) { card->instantiated = false; snd_soc_dapm_shutdown(card); snd_soc_flush_all_delayed_work(card); + + /* remove all components used by DAI links on this card */ + for_each_comp_order(order) { + for_each_card_rtds(card, rtd) { + soc_remove_link_components(card, rtd, order); + } + } + soc_cleanup_card_resources(card); if (!unregister) list_add(&card->list, &unbind_card_list);
The patch
ASoC: core: remove link components before cleaning up card resources
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From f96fb7d198ca624fe33c4145a004eb5a3d0eddec Mon Sep 17 00:00:00 2001
From: Ranjani Sridharan ranjani.sridharan@linux.intel.com Date: Thu, 4 Apr 2019 17:30:40 -0700 Subject: [PATCH] ASoC: core: remove link components before cleaning up card resources
When the card is registered by the machine driver, dai link components are probed after the snd_card is created. This is done in snd_soc_bind_card() which calls snd_soc_instantiate_card() to first create the snd_card and then probes the link components by calling soc_probe_link_components(). The snd_card is used by the component driver to add the kcontrols associated with dapm widgets to the card.
When the machine driver is unregistered, the snd_card is freed when the card resources are cleaned up. But the snd_card needs to be valid while unloading the topology dapm widgets in order to remove the kcontrols from the card.
Since, unloading topology is done when the component driver is removed, the link components should be removed in snd_soc_unbind_card(). This will ensure that the kcontrols are removed before the card resources are cleaned up and the snd_card itself is freed.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 6f4842977b8d..75f6a8085a76 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2831,10 +2831,21 @@ EXPORT_SYMBOL_GPL(snd_soc_register_card);
static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister) { + struct snd_soc_pcm_runtime *rtd; + int order; + if (card->instantiated) { card->instantiated = false; snd_soc_dapm_shutdown(card); snd_soc_flush_all_delayed_work(card); + + /* remove all components used by DAI links on this card */ + for_each_comp_order(order) { + for_each_card_rtds(card, rtd) { + soc_remove_link_components(card, rtd, order); + } + } + soc_cleanup_card_resources(card); if (!unregister) list_add(&card->list, &unbind_card_list);
participants (2)
-
Mark Brown -
Ranjani Sridharan