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

Commit Message

Pierre-Louis Bossart June 22, 2020, 3:42 p.m. UTC

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 <[e-mail]>
Reviewed-by: Guennadi Liakhovetski <[e-mail]>
Reviewed-by: Curtis Malainey <[e-mail]>
Signed-off-by: Pierre-Louis Bossart <[e-mail]>
---
 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(+)

Comments

Kuninori Morimoto June 22, 2020, 11:54 p.m. UTC | #1

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

Pierre-Louis Bossart June 23, 2020, 2:50 p.m. UTC | #2

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!

Kuninori Morimoto June 24, 2020, 11:25 p.m. UTC | #3

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

Patch

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