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@xxxxxxxxxxxxxxx>
> > ---
> > 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
>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
