[alsa-devel] [RFC PATCH 1/2] ASoC: core: don't increase component module refcount unconditionally

[Vinod Koul]

On 01-02-19, 11:22, Pierre-Louis Bossart wrote:
> The ASoC core has for the longest time increased the module reference
> counts, even before the transition to the component model. This is
> probably fine on most platforms, but it introduces a deadlock case on
> Intel devices with the Skylake and SOF drivers which cannot be removed
> due to their reference counts being modified by the core.
> 
> In these 2 cases, the PCI or ACPI driver .probe creates a platform
> device to let the machine driver .probe register the audio
> card. Conversely the PCI or ACPI driver .remove will unregister the
> platform device which results in the card being removed by the machine
> driver .remove.
> 
> With ascii art, this can be represented as
> 
> modprobe
> snd_soc_skl/
> soc-pci-dev/sof-acpci-dev  ----------> pci/acpi probe
>        ^                                    |
>        |                     ---------------|
>        |                    |               |
>        |                    V               V
>     increase            register        register machine
>     refcount            component       platform_device
>        ^                                    |
>        |                                    |
>        |                                    V
>     component <----   register card  <---- probe
>     probe
> 
> The issue is that by playing with the component's module reference
> counts during the card registration, it's no longer possible to remove
> the module which controls the component. This can be shown, e.g. with
> the following error:
> 
> root at plb-XPS-13-9350:~# lsmod | grep snd_soc_skl
> snd_soc_skl           110592  1
> 
> root at plb-XPS-13-9350:~# rmmod snd_soc_skl
> rmmod: ERROR: Module snd_soc_skl is in use

Yup, that would be correct, the inuse is due to the fact the sound card
is up and someone needs to unload the sound card to remove the
reference.

That can be done by doing the rmmod of machine driver first and IIRC
that would remove the sound card and drop the reference and then
snd_soc_skl can be unloaded.

Now one can argue that is not required, but I feel that is correct for
core to hold references of modules the card is tied up to :)

> Increasing the reference count during the component probe is not
> useful. If the PCI/ACPI module is removed, the card will be removed
> anyway.
> 
> To avoid breaking existing platforms and allowing Intel platforms to
> safely deal with module load/unload cases, this patch introduces a
> flag which needs to be set during the component initialization. This
> is a strictly opt-in capability that should only be used when the
> handling of the component module does not require a reference count
> increase to prevent removal during use.
> 
> Note that this solution is not directly applicable to the legacy
> Atom/SST driver, which uses a different device hierarchy. There are
> however additional refcount issues which prevent the ACPI driver from
> being removed. This is a different issue which would need a different
> patch.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> ---
>  include/sound/soc.h  | 3 +++
>  sound/soc/soc-core.c | 6 ++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 95689680336b..eb7db605955b 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -802,6 +802,9 @@ 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;
> +
>  	/* bits */
>  	unsigned int idle_bias_on:1;
>  	unsigned int suspend_bias_off:1;
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 9dad2b1498c1..80ab81f1df3d 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -947,7 +947,8 @@ 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;
> -	module_put(component->dev->driver->owner);
> +	if (!component->driver->ignore_module_refcount)
> +		module_put(component->dev->driver->owner);
>  }
>  
>  static void soc_remove_component(struct snd_soc_component *component)
> @@ -1362,7 +1363,8 @@ static int soc_probe_component(struct snd_soc_card *card,
>  		return 0;
>  	}
>  
> -	if (!try_module_get(component->dev->driver->owner))
> +	if (!component->driver->ignore_module_refcount &&
> +	    !try_module_get(component->dev->driver->owner))
>  		return -ENODEV;
>  
>  	component->card = card;
> -- 
> 2.17.1

-- 
~Vinod