[PATCH v2 1/2] ASoC: core: conditionally increase module refcount on component open

[PATCH v2 1/2] ASoC: core: conditionally increase module refcount on component open

[Ranjani Sridharan]

Recently, for Intel platforms the "ignore_module_refcount" field
was introduced for the component driver. In order to avoid a
deadlock preventing the PCI modules from being removed
even when the card was idle, the refcounts were not incremented
for the device driver module during component probe.

However, this change introduced a nasty side effect:
the device driver module can be unloaded while a pcm stream is open.

This patch proposes to change the field to be renamed as
"module_get_upon_open". When this field is set, the module
refcount should be incremented on pcm open amd decremented
upon pcm close. This will enable modules to be removed
when no PCM playback/capture happens and prevent removal
when the component is actually in use.

Also, align with the skylake component driver with the new name.

Fixes: b450b878('ASoC: core: don't increase component module refcount
                 unconditionally'

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 include/sound/soc.h               | 9 +++++++--
 sound/soc/intel/skylake/skl-pcm.c | 2 +-
 sound/soc/soc-core.c              | 4 ++--
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 1e2be35ed36f..482b4ea87c3c 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -802,8 +802,13 @@ struct snd_soc_component_driver {
 	int probe_order;
 	int remove_order;
 
-	/* signal if the module handling the component cannot be removed */
-	unsigned int ignore_module_refcount:1;
+	/*
+	 * signal if the module handling the component should not be removed
+	 * if a pcm is open. Setting this would prevent the module
+	 * refcount being incremented in probe() but allow it be incremented
+	 * when a pcm is opened and decremented when it is closed.
+	 */
+	unsigned int module_get_upon_open:1;
 
 	/* bits */
 	unsigned int idle_bias_on:1;
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 56099db8f86d..ee53d047c3bd 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -1468,7 +1468,7 @@ static const struct snd_soc_component_driver skl_component  = {
 	.ops		= &skl_platform_ops,
 	.pcm_new	= skl_pcm_new,
 	.pcm_free	= skl_pcm_free,
-	.ignore_module_refcount = 1, /* do not increase the refcount in core */
+	.module_get_upon_open = 1, /* increment refcount when a pcm is opened */
 };
 
 int skl_platform_register(struct device *dev)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 6f4842977b8d..e78534bbd092 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -947,7 +947,7 @@ static void soc_cleanup_component(struct snd_soc_component *component)
 	snd_soc_dapm_free(snd_soc_component_get_dapm(component));
 	soc_cleanup_component_debugfs(component);
 	component->card = NULL;
-	if (!component->driver->ignore_module_refcount)
+	if (!component->driver->module_get_upon_open)
 		module_put(component->dev->driver->owner);
 }
 
@@ -1381,7 +1381,7 @@ static int soc_probe_component(struct snd_soc_card *card,
 		return 0;
 	}
 
-	if (!component->driver->ignore_module_refcount &&
+	if (!component->driver->module_get_upon_open &&
 	    !try_module_get(component->dev->driver->owner))
 		return -ENODEV;
 
-- 
2.17.1

[PATCH v2 2/2] ASoC: pcm: update module refcount if module_get_upon_open is set

[Ranjani Sridharan]

Setting the module_get_upon_open field for component driver
prevents the module refcount from being incremented during
component probe(). This could lead to the module being
allowed to be unloaded when a pcm stream is open. So,
if this field is set, the module's refcount should be
incremented during pcm open to prevent module removal
when the component is in use. And, the refcount should
be decremented upon pcm close.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/soc-pcm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 7fe5321000e8..4192f6900d4d 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
 #include <linux/export.h>
@@ -463,6 +464,9 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream,
 			continue;
 
 		component->driver->ops->close(substream);
+
+		if (component->driver->module_get_upon_open)
+			module_put(component->dev->driver->owner);
 	}
 
 	return 0;
@@ -513,6 +517,10 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		    !component->driver->ops->open)
 			continue;
 
+		if (component->driver->module_get_upon_open &&
+		    !try_module_get(component->dev->driver->owner))
+			return -ENODEV;
+
 		ret = component->driver->ops->open(substream);
 		if (ret < 0) {
 			dev_err(component->dev,
-- 
2.17.1

Re: [PATCH v2 1/2] ASoC: core: conditionally increase module refcount on component open

[Pierre-Louis Bossart]

On 4/5/19 11:57 AM, Ranjani Sridharan wrote:
> Recently, for Intel platforms the "ignore_module_refcount" field
> was introduced for the component driver. In order to avoid a
> deadlock preventing the PCI modules from being removed
> even when the card was idle, the refcounts were not incremented
> for the device driver module during component probe.
> 
> However, this change introduced a nasty side effect:
> the device driver module can be unloaded while a pcm stream is open.
> 
> This patch proposes to change the field to be renamed as
> "module_get_upon_open". When this field is set, the module
> refcount should be incremented on pcm open amd decremented
> upon pcm close. This will enable modules to be removed
> when no PCM playback/capture happens and prevent removal
> when the component is actually in use.
> 
> Also, align with the skylake component driver with the new name.
> 
> Fixes: b450b878('ASoC: core: don't increase component module refcount
>                   unconditionally'
> 
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Both patches 1..2

Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Thanks for improving and fixing this Ranjani!

> ---
>   include/sound/soc.h               | 9 +++++++--
>   sound/soc/intel/skylake/skl-pcm.c | 2 +-
>   sound/soc/soc-core.c              | 4 ++--
>   3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 1e2be35ed36f..482b4ea87c3c 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -802,8 +802,13 @@ struct snd_soc_component_driver {
>   	int probe_order;
>   	int remove_order;
>   
> -	/* signal if the module handling the component cannot be removed */
> -	unsigned int ignore_module_refcount:1;
> +	/*
> +	 * signal if the module handling the component should not be removed
> +	 * if a pcm is open. Setting this would prevent the module
> +	 * refcount being incremented in probe() but allow it be incremented
> +	 * when a pcm is opened and decremented when it is closed.
> +	 */
> +	unsigned int module_get_upon_open:1;
>   
>   	/* bits */
>   	unsigned int idle_bias_on:1;
> diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
> index 56099db8f86d..ee53d047c3bd 100644
> --- a/sound/soc/intel/skylake/skl-pcm.c
> +++ b/sound/soc/intel/skylake/skl-pcm.c
> @@ -1468,7 +1468,7 @@ static const struct snd_soc_component_driver skl_component  = {
>   	.ops		= &skl_platform_ops,
>   	.pcm_new	= skl_pcm_new,
>   	.pcm_free	= skl_pcm_free,
> -	.ignore_module_refcount = 1, /* do not increase the refcount in core */
> +	.module_get_upon_open = 1, /* increment refcount when a pcm is opened */
>   };
>   
>   int skl_platform_register(struct device *dev)
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 6f4842977b8d..e78534bbd092 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -947,7 +947,7 @@ static void soc_cleanup_component(struct snd_soc_component *component)
>   	snd_soc_dapm_free(snd_soc_component_get_dapm(component));
>   	soc_cleanup_component_debugfs(component);
>   	component->card = NULL;
> -	if (!component->driver->ignore_module_refcount)
> +	if (!component->driver->module_get_upon_open)
>   		module_put(component->dev->driver->owner);
>   }
>   
> @@ -1381,7 +1381,7 @@ static int soc_probe_component(struct snd_soc_card *card,
>   		return 0;
>   	}
>   
> -	if (!component->driver->ignore_module_refcount &&
> +	if (!component->driver->module_get_upon_open &&
>   	    !try_module_get(component->dev->driver->owner))
>   		return -ENODEV;
>   
> 

Applied "ASoC: pcm: update module refcount if module_get_upon_open is set" to the asoc tree

[Mark Brown]

The patch

   ASoC: pcm: update module refcount if module_get_upon_open is set

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 52034add758e268c39110f33d46e2a9492e82aef Mon Sep 17 00:00:00 2001
From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Date: Fri, 5 Apr 2019 09:57:09 -0700
Subject: [PATCH] ASoC: pcm: update module refcount if module_get_upon_open is
 set

Setting the module_get_upon_open field for component driver
prevents the module refcount from being incremented during
component probe(). This could lead to the module being
allowed to be unloaded when a pcm stream is open. So,
if this field is set, the module's refcount should be
incremented during pcm open to prevent module removal
when the component is in use. And, the refcount should
be decremented upon pcm close.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-pcm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 2d5d5cac4ba6..d21247546f7f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
 #include <linux/export.h>
@@ -463,6 +464,9 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream,
 			continue;
 
 		component->driver->ops->close(substream);
+
+		if (component->driver->module_get_upon_open)
+			module_put(component->dev->driver->owner);
 	}
 
 	return 0;
@@ -513,6 +517,10 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		    !component->driver->ops->open)
 			continue;
 
+		if (component->driver->module_get_upon_open &&
+		    !try_module_get(component->dev->driver->owner))
+			return -ENODEV;
+
 		ret = component->driver->ops->open(substream);
 		if (ret < 0) {
 			dev_err(component->dev,
-- 
2.20.1