[PATCH 0/5] ASoC: add dailink .exit() callback
While looking at reboot issues and module load/unload tests, I found out some resources allocated in the dailink .init() callback are not properly released - there is no existing mechanism in the soc-core to do so.
The addition of a dailink .exit() callback seems to be the simplest solution overall. It can be argued that the existing machine platform device .remove() callback can also perform the necessary cleanups, however as shown in the last two examples this might require a loop to identify components whereas the dailink .exit() already has all the necessary information to revert the actions done in the .init() step.
Changes since RFC: Better commit messages and explanations rt5682 cases with snd_soc_component_set_jack() called in the .exit()
Fred Oh (2): ASoC: intel: sof_rt5682: move disabling jack to dai link's exit() ASoC: intel: cml_rt1011_rt5682: disable jack in dailink .exit()
Pierre-Louis Bossart (3): ASoC: soc-link: introduce exit() callback ASoC: Intel: bdw-rt5677: fix module load/unload issues ASoC: Intel: kbl-rt5660: use .exit() dailink callback to release gpiod
include/sound/soc-link.h | 1 + include/sound/soc.h | 3 +++ sound/soc/intel/boards/bdw-rt5677.c | 18 ++++++++++++++-- sound/soc/intel/boards/cml_rt1011_rt5682.c | 8 ++++++++ sound/soc/intel/boards/kbl_rt5660.c | 17 +++++++++++++-- sound/soc/intel/boards/sof_rt5682.c | 24 ++++++++-------------- sound/soc/soc-core.c | 3 +++ sound/soc/soc-link.c | 6 ++++++ 8 files changed, 60 insertions(+), 20 deletions(-)
base-commit: 39853b1438bf9b07349c8c44b48f6c2eda6f8840
Some machine drivers allocate or request resources with snd_soc_link_init() phase of the card probe. These resources need to be properly released when removing a card, and this patch suggests a dual exit() callback.
The exit() is invoked in soc_remove_pcm_runtime(), which is not completely symmetric with the init() invoked in soc_init_pcm_runtime().
Alternate solutions were considered, e.g. adding a .remove() callback for the platform driver, but that's not symmetrical at all and would be difficult to handle if there are more than one dailink implementing an .init(). We looked also into using .remove_dai_link() callback, but that would also be imbalanced.
Note that because of the error handling in snd_soc_bind_card(), which jumps to probe_end, there is no way to guarantee the exit() is invoked with resources allocated in the init(). Prior to releasing those resources, implementations of the exit() callback shall check the resources are valid.
Suggested-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Curtis Malainey curtis@malainey.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/sound/soc-link.h | 1 + include/sound/soc.h | 3 +++ sound/soc/soc-core.c | 3 +++ sound/soc/soc-link.c | 6 ++++++ 4 files changed, 13 insertions(+)
diff --git a/include/sound/soc-link.h b/include/sound/soc-link.h index 3dd6e33e94ec..337ac5666757 100644 --- a/include/sound/soc-link.h +++ b/include/sound/soc-link.h @@ -9,6 +9,7 @@ #define __SOC_LINK_H
int snd_soc_link_init(struct snd_soc_pcm_runtime *rtd); +void snd_soc_link_exit(struct snd_soc_pcm_runtime *rtd); int snd_soc_link_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_hw_params *params);
diff --git a/include/sound/soc.h b/include/sound/soc.h index 2756f9bcac3e..33aceadebd03 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -799,6 +799,9 @@ struct snd_soc_dai_link { /* codec/machine specific init - e.g. add machine controls */ int (*init)(struct snd_soc_pcm_runtime *rtd);
+ /* codec/machine specific exit - dual of init() */ + void (*exit)(struct snd_soc_pcm_runtime *rtd); + /* optional hw_params re-writing for BE and FE sync */ int (*be_hw_params_fixup)(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_hw_params *params); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 62c0c9482018..adedadcb0efb 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -945,6 +945,9 @@ void snd_soc_remove_pcm_runtime(struct snd_soc_card *card, { lockdep_assert_held(&client_mutex);
+ /* release machine specific resources */ + snd_soc_link_exit(rtd); + /* * Notify the machine driver for extra destruction */ diff --git a/sound/soc/soc-link.c b/sound/soc/soc-link.c index f849278beba0..1c3bf2118718 100644 --- a/sound/soc/soc-link.c +++ b/sound/soc/soc-link.c @@ -40,6 +40,12 @@ int snd_soc_link_init(struct snd_soc_pcm_runtime *rtd) return soc_link_ret(rtd, ret); }
+void snd_soc_link_exit(struct snd_soc_pcm_runtime *rtd) +{ + if (rtd->dai_link->exit) + rtd->dai_link->exit(rtd); +} + int snd_soc_link_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_hw_params *params) {
Hi Pierre-Louis
The exit() is invoked in soc_remove_pcm_runtime(), which is not completely symmetric with the init() invoked in soc_init_pcm_runtime().
(snip)
@@ -945,6 +945,9 @@ void snd_soc_remove_pcm_runtime(struct snd_soc_card *card, { lockdep_assert_held(&client_mutex);
- /* release machine specific resources */
- snd_soc_link_exit(rtd);
I can understand that 100% symmetric calling init()/exit() is difficult. So we can't help it this time. But if so, it is easy to notice that this .exit() is the one of non-symmetric if it has such comment.
Thank you for your help !!
Best regards --- Kuninori Morimoto
On 6/22/20 6:54 PM, Kuninori Morimoto wrote:
Hi Pierre-Louis
The exit() is invoked in soc_remove_pcm_runtime(), which is not completely symmetric with the init() invoked in soc_init_pcm_runtime().
(snip)
@@ -945,6 +945,9 @@ void snd_soc_remove_pcm_runtime(struct snd_soc_card *card, { lockdep_assert_held(&client_mutex);
- /* release machine specific resources */
- snd_soc_link_exit(rtd);
I can understand that 100% symmetric calling init()/exit() is difficult. So we can't help it this time. But if so, it is easy to notice that this .exit() is the one of non-symmetric if it has such comment.
Sorry Morimoto-san, I don't understand your last sentence. Were you suggesting to reword the "release machine specific resources" comment above, or add a comment in the snd_soc_link_exit() definition, or something else altogether? I don't mind sending an update if you feel that makes the core more readable. Thanks!
Hi Pierre-Louis
- /* release machine specific resources */
- snd_soc_link_exit(rtd);
I can understand that 100% symmetric calling init()/exit() is difficult. So we can't help it this time. But if so, it is easy to notice that this .exit() is the one of non-symmetric if it has such comment.
Sorry Morimoto-san, I don't understand your last sentence. Were you suggesting to reword the "release machine specific resources" comment above, or add a comment in the snd_soc_link_exit() definition, or something else altogether? I don't mind sending an update if you feel that makes the core more
Sorry for my unclear comment. I want to have is something like this
/* * release machine specific resources * * Note * This is pair of snd_soc_link_init() which is called at ... * Because ... these are not symmetric called ... */ snd_soc_link_exit(rtd);
Thank you for your help !!
Best regards --- Kuninori Morimoto
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() in the .init() callback, and calling gpiod_put() in the exit() callback.
Tested on SAMUS Chromebook with SOF driver.
Suggested-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Curtis Malainey curtis@malainey.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/boards/bdw-rt5677.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c index 34a3abb5991f..c9da91147770 100644 --- a/sound/soc/intel/boards/bdw-rt5677.c +++ b/sound/soc/intel/boards/bdw-rt5677.c @@ -272,8 +272,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); @@ -307,6 +307,19 @@ static int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd) return 0; }
+static void bdw_rt5677_exit(struct snd_soc_pcm_runtime *rtd) +{ + struct bdw_rt5677_priv *bdw_rt5677 = + snd_soc_card_get_drvdata(rtd->card); + + /* + * The .exit() can be reached without going through the .init() + * so explicitly test if the gpiod is valid + */ + if (!IS_ERR_OR_NULL(bdw_rt5677->gpio_hp_en)) + gpiod_put(bdw_rt5677->gpio_hp_en); +} + /* broadwell digital audio interface glue - connects codec <--> CPU */ SND_SOC_DAILINK_DEF(dummy, DAILINK_COMP_ARRAY(COMP_DUMMY())); @@ -372,6 +385,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = { .dpcm_playback = 1, .dpcm_capture = 1, .init = bdw_rt5677_init, + .exit = bdw_rt5677_exit, #if !IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL) SND_SOC_DAILINK_REG(dummy, be, dummy), #else
On Mon, Jun 22, 2020 at 10:42:38AM -0500, Pierre-Louis Bossart wrote:
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() in the .init() callback, and calling gpiod_put() in the exit() callback.
Tested on SAMUS Chromebook with SOF driver.
+static void bdw_rt5677_exit(struct snd_soc_pcm_runtime *rtd) +{
- struct bdw_rt5677_priv *bdw_rt5677 =
snd_soc_card_get_drvdata(rtd->card);- /*
* The .exit() can be reached without going through the .init()* so explicitly test if the gpiod is valid*/
- if (!IS_ERR_OR_NULL(bdw_rt5677->gpio_hp_en))
_OR_NULL is redundant. gpiod_put() is explicitly NULL-aware.
IS_ERR() I suppose can be avoided by using gpiod_get_optional(), if it suits the case. Otherwise it would be questionable why we got error pointer value on ->exit().
gpiod_put(bdw_rt5677->gpio_hp_en);+}
On 6/22/20 1:18 PM, Andy Shevchenko wrote:
On Mon, Jun 22, 2020 at 10:42:38AM -0500, Pierre-Louis Bossart wrote:
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() in the .init() callback, and calling gpiod_put() in the exit() callback.
Tested on SAMUS Chromebook with SOF driver.
+static void bdw_rt5677_exit(struct snd_soc_pcm_runtime *rtd) +{
- struct bdw_rt5677_priv *bdw_rt5677 =
snd_soc_card_get_drvdata(rtd->card);- /*
* The .exit() can be reached without going through the .init()* so explicitly test if the gpiod is valid*/
- if (!IS_ERR_OR_NULL(bdw_rt5677->gpio_hp_en))
_OR_NULL is redundant. gpiod_put() is explicitly NULL-aware.
IS_ERR() I suppose can be avoided by using gpiod_get_optional(), if it suits the case. Otherwise it would be questionable why we got error pointer value on ->exit().
As I explained in the cover letter we can reach this function even if the init was not called or was not successful. There are tons of cases which reach the same probe_end label in the ASoC core.
So I explicitly added a test for all possible cases. I don't mind removing the _OR_NULL if indeed it's safe, but showing this redundancy also shows an intent to deal with such cases.
gpiod_put(bdw_rt5677->gpio_hp_en);+}
On Mon, Jun 22, 2020 at 01:23:04PM -0500, Pierre-Louis Bossart wrote:
So I explicitly added a test for all possible cases. I don't mind removing the _OR_NULL if indeed it's safe, but showing this redundancy also shows an intent to deal with such cases.
Yeah, I think that's fine - it's perhaps redundant but we're not in a hot path and as well as the intentionality it saves the reader from having to know if gpiod_put() copes with NULL or not.
On Mon, Jun 22, 2020 at 07:26:51PM +0100, Mark Brown wrote:
On Mon, Jun 22, 2020 at 01:23:04PM -0500, Pierre-Louis Bossart wrote:
So I explicitly added a test for all possible cases. I don't mind removing the _OR_NULL if indeed it's safe, but showing this redundancy also shows an intent to deal with such cases.
Yeah, I think that's fine - it's perhaps redundant but we're not in a hot path and as well as the intentionality it saves the reader from having to know if gpiod_put() copes with NULL or not.
What the point? We can document this instead of being a dead code, right?
IS_ERR() may happen there only if we assign such pointer to be error. Is it possible case here?
On Tue, Jun 23, 2020 at 11:40:11AM +0300, Andy Shevchenko wrote:
On Mon, Jun 22, 2020 at 07:26:51PM +0100, Mark Brown wrote:
On Mon, Jun 22, 2020 at 01:23:04PM -0500, Pierre-Louis Bossart wrote:
So I explicitly added a test for all possible cases. I don't mind removing the _OR_NULL if indeed it's safe, but showing this redundancy also shows an intent to deal with such cases.
Yeah, I think that's fine - it's perhaps redundant but we're not in a hot path and as well as the intentionality it saves the reader from having to know if gpiod_put() copes with NULL or not.
What the point? We can document this instead of being a dead code, right?
To a certain extent the _OR_NULL is documentation - the effect is the same with or without it.
IS_ERR() may happen there only if we assign such pointer to be error. Is it possible case here?
That one is a bit more real.
On Mon, Jun 22, 2020 at 01:23:04PM -0500, Pierre-Louis Bossart wrote:
On 6/22/20 1:18 PM, Andy Shevchenko wrote:
On Mon, Jun 22, 2020 at 10:42:38AM -0500, Pierre-Louis Bossart wrote:
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() in the .init() callback, and calling gpiod_put() in the exit() callback.
Tested on SAMUS Chromebook with SOF driver.
+static void bdw_rt5677_exit(struct snd_soc_pcm_runtime *rtd) +{
- struct bdw_rt5677_priv *bdw_rt5677 =
snd_soc_card_get_drvdata(rtd->card);- /*
* The .exit() can be reached without going through the .init()* so explicitly test if the gpiod is valid*/
- if (!IS_ERR_OR_NULL(bdw_rt5677->gpio_hp_en))
_OR_NULL is redundant. gpiod_put() is explicitly NULL-aware.
IS_ERR() I suppose can be avoided by using gpiod_get_optional(), if it suits the case. Otherwise it would be questionable why we got error pointer value on ->exit().
As I explained in the cover letter we can reach this function even if the init was not called or was not successful. There are tons of cases which reach the same probe_end label in the ASoC core.
So I explicitly added a test for all possible cases. I don't mind removing the _OR_NULL if indeed it's safe, but showing this redundancy also shows an intent to deal with such cases.
Thanks for an explanation. I think it's an established practice to have NULL-aware resource release-like functions.
Do you put always something like below in your code? No.
if (foo) kfree(foo);
On 2020-06-22 5:42 PM, Pierre-Louis Bossart wrote:
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() in the .init() callback, and calling gpiod_put() in the exit() callback.
Tested on SAMUS Chromebook with SOF driver.
As /intel/haswell is the go-to driver for BDW platforms, please test and confirm with legacy driver first. SOF is optional and thus non-blocking.
Czarek
On 6/24/20 2:14 PM, Cezary Rojewski wrote:
On 2020-06-22 5:42 PM, Pierre-Louis Bossart wrote:
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() in the .init() callback, and calling gpiod_put() in the exit() callback.
Tested on SAMUS Chromebook with SOF driver.
As /intel/haswell is the go-to driver for BDW platforms, please test and confirm with legacy driver first. SOF is optional and thus non-blocking.
I'll retest when you've fixed the go-to legacy driver, I am not even going to try module load/unload tests when the platform code has known issues requiring reverts.
On 2020-06-24 10:06 PM, Pierre-Louis Bossart wrote:
On 6/24/20 2:14 PM, Cezary Rojewski wrote:
On 2020-06-22 5:42 PM, Pierre-Louis Bossart wrote:
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() in the .init() callback, and calling gpiod_put() in the exit() callback.
Tested on SAMUS Chromebook with SOF driver.
As /intel/haswell is the go-to driver for BDW platforms, please test and confirm with legacy driver first. SOF is optional and thus non-blocking.
I'll retest when you've fixed the go-to legacy driver, I am not even going to try module load/unload tests when the platform code has known issues requiring reverts.
??
Judging by recent comments from the revert thread, you already had build with patch-reverted ready. Shouldn't be a problem to retest with that one.
Power transition update is unavoidable if that's what you're asking. Without it, hardware *may* achieve a power state, but that's certainly not a D3 one.
Czarek
The gpiod handling is inspired from the bdw-rt5677 code. Apply same fix to avoid reference count issue while removing modules for consistency.
Suggested-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Curtis Malainey curtis@malainey.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/boards/kbl_rt5660.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_rt5660.c b/sound/soc/intel/boards/kbl_rt5660.c index d2a078454784..f4c0b983c990 100644 --- a/sound/soc/intel/boards/kbl_rt5660.c +++ b/sound/soc/intel/boards/kbl_rt5660.c @@ -165,8 +165,8 @@ static int kabylake_rt5660_codec_init(struct snd_soc_pcm_runtime *rtd) dev_warn(component->dev, "Failed to add driver gpios\n");
/* Request rt5660 GPIO for lineout mute control, return if fails */ - ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute", - GPIOD_OUT_HIGH); + ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute", + GPIOD_OUT_HIGH); if (IS_ERR(ctx->gpio_lo_mute)) { dev_err(component->dev, "Can't find GPIO_MUTE# gpio\n"); return PTR_ERR(ctx->gpio_lo_mute); @@ -207,6 +207,18 @@ static int kabylake_rt5660_codec_init(struct snd_soc_pcm_runtime *rtd) return 0; }
+static void kabylake_rt5660_codec_exit(struct snd_soc_pcm_runtime *rtd) +{ + struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(rtd->card); + + /* + * The .exit() can be reached without going through the .init() + * so explicitly test if the gpiod is valid + */ + if (!IS_ERR_OR_NULL(ctx->gpio_lo_mute)) + gpiod_put(ctx->gpio_lo_mute); +} + static int kabylake_hdmi_init(struct snd_soc_pcm_runtime *rtd, int device) { struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(rtd->card); @@ -421,6 +433,7 @@ static struct snd_soc_dai_link kabylake_rt5660_dais[] = { .id = 0, .no_pcm = 1, .init = kabylake_rt5660_codec_init, + .exit = kabylake_rt5660_codec_exit, .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS,
On Mon, Jun 22, 2020 at 10:42:39AM -0500, Pierre-Louis Bossart wrote:
The gpiod handling is inspired from the bdw-rt5677 code. Apply same fix to avoid reference count issue while removing modules for consistency.
...
- ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute",
GPIOD_OUT_HIGH);
- ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute",
GPIOD_OUT_HIGH);
Is it fatal? Then IS_ERR() is not needed below. For NULL I already told.
- /*
* The .exit() can be reached without going through the .init()* so explicitly test if the gpiod is valid*/
This comment should be amended after fixing the code.
- if (!IS_ERR_OR_NULL(ctx->gpio_lo_mute))
gpiod_put(ctx->gpio_lo_mute);+}
On 6/22/20 1:20 PM, Andy Shevchenko wrote:
On Mon, Jun 22, 2020 at 10:42:39AM -0500, Pierre-Louis Bossart wrote:
The gpiod handling is inspired from the bdw-rt5677 code. Apply same fix to avoid reference count issue while removing modules for consistency.
...
- ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute",
GPIOD_OUT_HIGH);
- ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute",
GPIOD_OUT_HIGH);Is it fatal? Then IS_ERR() is not needed below. For NULL I already told.
this patch only fixes a deadlock, whether or not this is fatal or not is a different question. I would assert if if you can't mute your audio is broken.
- /*
* The .exit() can be reached without going through the .init()* so explicitly test if the gpiod is valid*/This comment should be amended after fixing the code.
- if (!IS_ERR_OR_NULL(ctx->gpio_lo_mute))
gpiod_put(ctx->gpio_lo_mute);+}
On Mon, Jun 22, 2020 at 01:26:15PM -0500, Pierre-Louis Bossart wrote:
On 6/22/20 1:20 PM, Andy Shevchenko wrote:
On Mon, Jun 22, 2020 at 10:42:39AM -0500, Pierre-Louis Bossart wrote:
The gpiod handling is inspired from the bdw-rt5677 code. Apply same fix to avoid reference count issue while removing modules for consistency.
...
- ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute",
GPIOD_OUT_HIGH);
- ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute",
GPIOD_OUT_HIGH);Is it fatal? Then IS_ERR() is not needed below. For NULL I already told.
this patch only fixes a deadlock, whether or not this is fatal or not is a different question. I would assert if if you can't mute your audio is broken.
Ah, sorry, I mean IS_ERR() is not needed for an optional case. Since it is fatal, it's fine.
- /*
* The .exit() can be reached without going through the .init()* so explicitly test if the gpiod is valid*/This comment should be amended after fixing the code.
- if (!IS_ERR_OR_NULL(ctx->gpio_lo_mute))
gpiod_put(ctx->gpio_lo_mute);+}
From: Fred Oh fred.oh@linux.intel.com
Move disabling jack from platform driver's remove() to dai link's exit(). This is symmetrical change as jack is enabled in init().
Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/boards/sof_rt5682.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c index 13a48b0c35ae..f80ed62025f3 100644 --- a/sound/soc/intel/boards/sof_rt5682.c +++ b/sound/soc/intel/boards/sof_rt5682.c @@ -206,6 +206,13 @@ static int sof_rt5682_codec_init(struct snd_soc_pcm_runtime *rtd) return ret; };
+static void sof_rt5682_codec_exit(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_component *component = asoc_rtd_to_codec(rtd, 0)->component; + + snd_soc_component_set_jack(component, NULL, NULL); +} + static int sof_rt5682_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { @@ -525,6 +532,7 @@ static struct snd_soc_dai_link *sof_card_dai_links_create(struct device *dev, links[id].platforms = platform_component; links[id].num_platforms = ARRAY_SIZE(platform_component); links[id].init = sof_rt5682_codec_init; + links[id].exit = sof_rt5682_codec_exit; links[id].ops = &sof_rt5682_ops; links[id].nonatomic = true; links[id].dpcm_playback = 1; @@ -786,21 +794,6 @@ static int sof_audio_probe(struct platform_device *pdev) &sof_audio_card_rt5682); }
-static int sof_rt5682_remove(struct platform_device *pdev) -{ - struct snd_soc_card *card = platform_get_drvdata(pdev); - struct snd_soc_component *component = NULL; - - for_each_card_components(card, component) { - if (!strcmp(component->name, rt5682_component[0].name)) { - snd_soc_component_set_jack(component, NULL, NULL); - break; - } - } - - return 0; -} - static const struct platform_device_id board_ids[] = { { .name = "sof_rt5682", @@ -836,7 +829,6 @@ static const struct platform_device_id board_ids[] = {
static struct platform_driver sof_audio = { .probe = sof_audio_probe, - .remove = sof_rt5682_remove, .driver = { .name = "sof_rt5682", .pm = &snd_soc_pm_ops,
From: Fred Oh fred.oh@linux.intel.com
When removing the machine driver, the rt5682 jack handler will oops if jack detection is not disabled. The jack can be disabled in the dai link's exit().
This is symmetrical change as jack is enabled in init().
Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/boards/cml_rt1011_rt5682.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/soc/intel/boards/cml_rt1011_rt5682.c b/sound/soc/intel/boards/cml_rt1011_rt5682.c index 68eff29daf8f..189c908c4aa8 100644 --- a/sound/soc/intel/boards/cml_rt1011_rt5682.c +++ b/sound/soc/intel/boards/cml_rt1011_rt5682.c @@ -161,6 +161,13 @@ static int cml_rt5682_codec_init(struct snd_soc_pcm_runtime *rtd) return ret; };
+static void cml_rt5682_codec_exit(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_component *component = asoc_rtd_to_codec(rtd, 0)->component; + + snd_soc_component_set_jack(component, NULL, NULL); +} + static int cml_rt1011_spk_init(struct snd_soc_pcm_runtime *rtd) { int ret = 0; @@ -415,6 +422,7 @@ static struct snd_soc_dai_link cml_rt1011_rt5682_dailink[] = { .name = "SSP0-Codec", .id = 0, .init = cml_rt5682_codec_init, + .exit = cml_rt5682_codec_exit, .ignore_pmdown_time = 1, .ops = &cml_rt5682_ops, .dpcm_playback = 1,
On Mon, 22 Jun 2020 10:42:36 -0500, Pierre-Louis Bossart wrote:
While looking at reboot issues and module load/unload tests, I found out some resources allocated in the dailink .init() callback are not properly released - there is no existing mechanism in the soc-core to do so.
The addition of a dailink .exit() callback seems to be the simplest solution overall. It can be argued that the existing machine platform device .remove() callback can also perform the necessary cleanups, however as shown in the last two examples this might require a loop to identify components whereas the dailink .exit() already has all the necessary information to revert the actions done in the .init() step.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/5] ASoC: soc-link: introduce exit() callback commit: 21a00fb33790f828a34b9ce50ab9f9130bc1ffb4 [2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues commit: bcb43fdae1c0d08b772b792cf46f323ad0d17968 [3/5] ASoC: Intel: kbl-rt5660: use .exit() dailink callback to release gpiod commit: e56054e75325c347f09c1be2f6400ef67bb9662d [4/5] ASoC: intel: sof_rt5682: move disabling jack to dai link's exit() commit: b0c96fc1ab2947e331f817cecc5ca733eaf5619b [5/5] ASoC: intel: cml_rt1011_rt5682: disable jack in dailink .exit() commit: 4fcc922cb31179f9bc1e99cd707f2b36138fbcf8
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
participants (5)
-
Andy Shevchenko -
Cezary Rojewski -
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart