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

[Ranjani Sridharan <ranjani.sridharan@linux.intel.com>]

On Fri, 2019-02-01 at 23:42 +0530, Vinod Koul wrote:
> 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@plb-XPS-13-9350:~# lsmod | grep snd_soc_skl
> > snd_soc_skl           110592  1
> > 
> > root@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.

This doesnt seem to be the case though. The machine driver module
cannot be removed either because its refcnt is also > 0.

Thanks,
Ranjani

> 
> 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@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
> 
>