Re: [alsa-devel] [PATCH v3 06/19] ASoC: soc-core: add soc_unbind_dai_link() - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
	Mark Brown <broonie@kernel.org>
Cc: Linux-ALSA <alsa-devel@alsa-project.org>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH v3 06/19] ASoC: soc-core: add soc_unbind_dai_link()
Date: Mon, 11 Nov 2019 23:21:14 -0600	[thread overview]
Message-ID: <8ed58ca1-0f9d-63e8-ba5d-28ee5209aee5@linux.intel.com> (raw)
In-Reply-To: <877e4e3jni.wl-kuninori.morimoto.gx@renesas.com>

On 11/5/19 12:46 AM, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> It is easy to read code if it is cleanly using paired function/naming,
> like start <-> stop, register <-> unregister, etc, etc.
> But, current ALSA SoC code is very random, unbalance, not paired, etc.
> It is easy to create bug at the such code, and it will be difficult to
> debug.
> 
> ALSA SoC has soc_bind_dai_link(), but its paired soc_unbind_dai_link()
> is not implemented.
> More confusable is that soc_remove_pcm_runtimes() which should be
> soc_unbind_dai_link() is implemented without synchronised
> to soc_bind_dai_link().
> 
> This patch cleanup this unbalance.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Morimoto-san, this patch seems to introduce a regression in our SOF 
module removal tests. Without a couple of load/unload modules cycles, we 
hit a kernel oops while freeing the card.

see https://github.com/thesofproject/linux/issues/1466 for some logs.

This issue did not happen with our November 6 rebase on Mark's tree, and 
showed up today. I couldn't really bisect the whole tree due to other 
issues, so manually applied your patches on top of this 11/06 tree and 
bisected from there.

I will need to confirm this finding (it's quite late for me) but looking 
at the code I wonder if the move of pcm_runtime deletion is correct?

> ---
> v2 -> v3
> 	- no change
> 	- add Reviewed-by
> 
>   sound/soc/soc-core.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index e8ff6f2..1e8dd19 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -470,14 +470,6 @@ static struct snd_soc_pcm_runtime *soc_new_pcm_runtime(
>   	return NULL;
>   }
>   
> -static void soc_remove_pcm_runtimes(struct snd_soc_card *card)
> -{
> -	struct snd_soc_pcm_runtime *rtd, *_rtd;
> -
> -	for_each_card_rtds_safe(card, rtd, _rtd)
> -		soc_free_pcm_runtime(rtd);
> -}
> -
>   struct snd_soc_pcm_runtime *snd_soc_get_pcm_runtime(struct snd_soc_card *card,
>   		const char *dai_link)
>   {
> @@ -1037,6 +1029,16 @@ static int soc_dai_link_sanity_check(struct snd_soc_card *card,
>   	return 0;
>   }
>   
> +static void soc_unbind_dai_link(struct snd_soc_card *card,
> +				struct snd_soc_dai_link *dai_link)
> +{
> +	struct snd_soc_pcm_runtime *rtd;
> +
> +	rtd = snd_soc_get_pcm_runtime(card, dai_link->name);
> +	if (rtd)
> +		soc_free_pcm_runtime(rtd);
> +}
> +
>   static int soc_bind_dai_link(struct snd_soc_card *card,
>   	struct snd_soc_dai_link *dai_link)
>   {
> @@ -1466,6 +1468,8 @@ void snd_soc_remove_dai_link(struct snd_soc_card *card,
>   		card->remove_dai_link(card, dai_link);
>   
>   	list_del(&dai_link->list);
> +
> +	soc_unbind_dai_link(card, dai_link);
>   }
>   EXPORT_SYMBOL_GPL(snd_soc_remove_dai_link);
>   
> @@ -1974,8 +1978,6 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card)
>   	for_each_card_links_safe(card, link, _link)
>   		snd_soc_remove_dai_link(card, link);
>   
> -	soc_remove_pcm_runtimes(card);
> -
>   	/* remove auxiliary devices */
>   	soc_remove_aux_devices(card);
>   	soc_unbind_aux_dev(card);
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel