[PATCH 0/5] ASoC: add dailink .exit() callback

[PATCH 0/5] ASoC: add dailink .exit() callback

[Pierre-Louis Bossart]

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

[PATCH 1/5] ASoC: soc-link: introduce exit() callback

[Pierre-Louis Bossart]

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)
 {
-- 
2.20.1

[PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Pierre-Louis Bossart]

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

[PATCH 3/5] ASoC: Intel: kbl-rt5660: use .exit() dailink callback to release gpiod

[Pierre-Louis Bossart]

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,
-- 
2.20.1

[PATCH 4/5] ASoC: intel: sof_rt5682: move disabling jack to dai link's exit()

[Pierre-Louis Bossart]

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,
-- 
2.20.1

[PATCH 5/5] ASoC: intel: cml_rt1011_rt5682: disable jack in dailink .exit()

[Pierre-Louis Bossart]

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,
-- 
2.20.1

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Andy Shevchenko]

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);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] ASoC: Intel: kbl-rt5660: use .exit() dailink callback to release gpiod

[Andy Shevchenko]

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);
>  	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);

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);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Pierre-Louis Bossart]

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);
>> +}
> 

Re: [PATCH 3/5] ASoC: Intel: kbl-rt5660: use .exit() dailink callback to release gpiod

[Pierre-Louis Bossart]

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);
>>   	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);
> 
> 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);
>> +}
> 

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] ASoC: soc-link: introduce exit() callback

[Kuninori Morimoto]

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

Re: [PATCH 3/5] ASoC: Intel: kbl-rt5660: use .exit() dailink callback to release gpiod

[Andy Shevchenko]

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);
> > >   	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);
> > 
> > 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);
> > > +}
> > 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Andy Shevchenko]

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?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Andy Shevchenko]

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);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] ASoC: add dailink .exit() callback

[Mark Brown]

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

Re: [PATCH 1/5] ASoC: soc-link: introduce exit() callback

[Pierre-Louis Bossart]

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!

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Cezary Rojewski]

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

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Pierre-Louis Bossart]

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.

Re: [PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Cezary Rojewski]

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

Re: [PATCH 1/5] ASoC: soc-link: introduce exit() callback

[Kuninori Morimoto]

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