The main thing I'm missing with this is a coherent explanation of the
problem and how the changes proposed fix it.
Just to emphasize: the main concern here is that the issue is understood
and that it's not just going to pop up again as soon as something
changes.
Here are more details Mark.
<1. finish machine driver probe >
[ 115.253970] bdw-rt5677 bdw-rt5677: rt5677-dspbuffer <->
spi-RT5677AA:00 mapping ok
<2. execute BE dailink .init() and request a gpiod from the codec
component device>
[ 115.254387] rt5677 i2c-RT5677CE:00: plb devm_gpiod_get
[ 115.254390] rt5677 i2c-RT5677CE:00: GPIO lookup for consumer
headphone-enable
[ 115.254391] rt5677 i2c-RT5677CE:00: using ACPI for GPIO lookup
[ 115.254395] acpi RT5677CE:00: GPIO: looking up headphone-enable-gpios
[ 115.254399] acpi RT5677CE:00: GPIO: _DSD returned RT5677CE:00 4 0 0
[ 115.254724] rt5677 i2c-RT5677CE:00: GPIO lookup for consumer plug-det
[ 115.254725] rt5677 i2c-RT5677CE:00: using ACPI for GPIO lookup
[ 115.254727] acpi RT5677CE:00: GPIO: looking up plug-det-gpios
[ 115.254729] acpi RT5677CE:00: GPIO: _DSD returned RT5677CE:00 0 0 0
[ 115.255451] rt5677 i2c-RT5677CE:00: GPIO lookup for consumer mic-present
[ 115.255453] rt5677 i2c-RT5677CE:00: using ACPI for GPIO lookup
[ 115.255455] acpi RT5677CE:00: GPIO: looking up mic-present-gpios
[ 115.255458] acpi RT5677CE:00: GPIO: _DSD returned RT5677CE:00 1 0 0
<3. gpiod handling complete>
[ 115.256293] bdw-rt5677 bdw-rt5677: rt5677-aif1 <-> ssp0-port mapping ok
<4. jack handling complete>
[ 115.262040] input: sof-bdw-rt5677 Headphone Jack as
/devices/pci0000:00/INT3438:00/bdw-rt5677/sound/card1/input11
[ 115.262240] input: sof-bdw-rt5677 Mic Jack as
/devices/pci0000:00/INT3438:00/bdw-rt5677/sound/card1/input12
<5. card fully functional>
<6. rmmod snd_sof-acpi>
<7. rmmod machine driver>
<8. rmmod codec driver>
rmmod: ERROR: Module snd_soc_rt5677 is in use
<9. rmmod -f codec driver>
[ 194.118221] gpio gpiochip0: REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED
[ 194.118440] rt5677 i2c-RT5677CE:00: plb devm_gpiod_release
So this is a self-inflicted deadlock - broken by design.
When the machine driver is removed, the gpiod is not freed.
Only removing the codec driver can free the gpiod, but the gpio/module
refcount prevents the codec driver from being removed.
I don't know how to move all the gpio handling in the codec driver,
since there are platform-dependent ACPI mappings.
The only proposal I can make is to avoid using devm_ but we need a hook
to call gpiod_put().
using the add_dai_link()/remove_dai_link as I suggested earlier is not
possible since the runtime is created after this callback is involved.
The proposal suggested by Andy was to have a dual callback to the
init(), or as in my initial version to call gpiod_put() in the machine
driver .remove() function, which isn't very elegant but does work.
I also tested a different solution (attached) based on your input where
the gpiod handing is performed in the machine driver probe, after the
card registration, and the gpiod_put() called from remove. This is
simple enough but there might be some issues left with the jack/input
handling - not sure why the logs for jacks are missing.
Does this clarify the issue and options?
Thanks
-Pierre
>From a4cafa8950b624f894db2a6a88c837728c286f1b Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
Date: Thu, 5 Mar 2020 14:45:27 -0600
Subject: [PATCH] ASoC: Intel: bdw-rt5677: enable module removal
The mainline code currently prevents modules from being removed.
The BE dailink .init() function calls devm_gpiod_get() using the codec
component device as argument. When the machine driver is removed, the
references to the gpiod are not released, and it's not possible to
remove the codec driver module - which is the only entity which could
free the gpiod.
This conceptual deadlock can be avoided by invoking gpiod_get() after
the card registration , and calling gpiod_put() in the platform device
.remove() function.
This solution seems to work fine, with jack detection ok w/
PulseAudio, however the following log is no longer showing in dmesg.
[ 13.790412] input: sof-bdw-rt5677 Headphone Jack as /devices/pci0000:00/INT3438:00/bdw-rt5677/sound/card1/input11
[ 13.790865] input: sof-bdw-rt5677 Mic Jack as /devices/pci0000:00/INT3438:00/bdw-rt5677/sound/card1/input12
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
---
sound/soc/intel/boards/bdw-rt5677.c | 33 +++++++++++++++++++++++++----
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c
index a94f498388e1..0be1dc60863e 100644
--- a/sound/soc/intel/boards/bdw-rt5677.c
+++ b/sound/soc/intel/boards/bdw-rt5677.c
@@ -247,8 +247,8 @@ static int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
RT5677_CLK_SEL_SYS2);
/* Request rt5677 GPIO for headphone amp control */
- bdw_rt5677->gpio_hp_en = devm_gpiod_get(component->dev, "headphone-enable",
- GPIOD_OUT_LOW);
+ bdw_rt5677->gpio_hp_en = gpiod_get(component->dev, "headphone-enable",
+ GPIOD_OUT_LOW);
if (IS_ERR(bdw_rt5677->gpio_hp_en)) {
dev_err(component->dev, "Can't find HP_AMP_SHDN_L gpio\n");
return PTR_ERR(bdw_rt5677->gpio_hp_en);
@@ -349,7 +349,6 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = {
.ops = &bdw_rt5677_ops,
.dpcm_playback = 1,
.dpcm_capture = 1,
- .init = bdw_rt5677_init,
SND_SOC_DAILINK_REG(ssp0_port, be, platform),
},
};
@@ -399,6 +398,7 @@ static int bdw_rt5677_probe(struct platform_device *pdev)
{
struct bdw_rt5677_priv *bdw_rt5677;
struct snd_soc_acpi_mach *mach;
+ struct snd_soc_pcm_runtime *rtd;
int ret;
bdw_rt5677_card.dev = &pdev->dev;
@@ -420,11 +420,36 @@ static int bdw_rt5677_probe(struct platform_device *pdev)
snd_soc_card_set_drvdata(&bdw_rt5677_card, bdw_rt5677);
- return devm_snd_soc_register_card(&pdev->dev, &bdw_rt5677_card);
+ ret = devm_snd_soc_register_card(&pdev->dev, &bdw_rt5677_card);
+ if (ret)
+ return ret;
+
+ for_each_card_rtds(&bdw_rt5677_card, rtd) {
+ if (!strcmp(rtd->dai_link->name, "Codec") &&
+ rtd->dai_link->no_pcm) {
+ ret = bdw_rt5677_init(rtd);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static int bdw_rt5677_remove(struct platform_device *pdev)
+{
+ struct bdw_rt5677_priv *bdw_rt5677;
+
+ bdw_rt5677 = snd_soc_card_get_drvdata(&bdw_rt5677_card);
+
+ if (bdw_rt5677->component)
+ gpiod_put(bdw_rt5677->gpio_hp_en);
+
+ return 0;
}
static struct platform_driver bdw_rt5677_audio = {
.probe = bdw_rt5677_probe,
+ .remove = bdw_rt5677_remove,
.driver = {
.name = "bdw-rt5677",
},
--
2.20.1
