[RFC PATCH 0/3] Fixes for component unregister and remove

[RFC PATCH 0/3] Fixes for component unregister and remove

[Ranjani Sridharan]

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(-)

-- 
2.17.1

[RFC PATCH 1/3] ASoC: core: do not unload topology in unregister_component()

[Ranjani Sridharan]

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;
-- 
2.17.1

[RFC PATCH 2/3] ASoC: intel: skylake: add remove() callback for component driver

[Ranjani Sridharan]

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,
-- 
2.17.1

[RFC PATCH 3/3] ASoC: core: remove link components before cleaning up card resources

[Ranjani Sridharan]

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);
-- 
2.17.1

Re: [RFC PATCH 1/3] ASoC: core: do not unload topology in unregister_component()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 740 bytes --]

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);
>  		snd_soc_component_del_unlocked(component);
>  		found = 1;

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 1/3] ASoC: core: do not unload topology in unregister_component()

[Ranjani Sridharan]

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);
> >  		snd_soc_component_del_unlocked(component);
> >  		found = 1;
> 
> 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

Re: [RFC PATCH 1/3] ASoC: core: do not unload topology in unregister_component()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 476 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 1/3] ASoC: core: do not unload topology in unregister_component()

[Ranjani Sridharan]

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

Re: [RFC PATCH 1/3] ASoC: core: do not unload topology in unregister_component()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 370 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Applied "ASoC: core: remove link components before cleaning up card resources" to the asoc tree

[Mark Brown]

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);
-- 
2.20.1

Applied "ASoC: intel: skylake: add remove() callback for component driver" to the asoc tree

[Mark Brown]

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,
-- 
2.20.1