Re: [PATCH] ASoC: core: Exit all links before removing their components

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 6/21/22 06:57, Cezary Rojewski wrote:
> Flows leading to link->init() and link->exit() are not symmetric.
> Currently the relevant part of card probe sequence goes as:
> 
> 	for_each_card_rtds(card, rtd)
> 		for_each_rtd_components(rtd, i, component)
> 			component->probe()
> 	for_each_card_rtds(card, rtd)
> 		for_each_rtd_dais(rtd, i, dai)
> 			dai->probe()
> 	for_each_card_rtds(card, rtd)
> 		rtd->init()
> 
> On the other side, equivalent remove sequence goes as:
> 
> 	for_each_card_rtds(card, rtd)
> 		for_each_rtd_dais(rtd, i, dai)
> 			dai->remove()
> 	for_each_card_rtds(card, rtd)
> 		for_each_rtd_components(rtd, i, component)
> 			component->remove()
> 	for_each_card_rtds(card, rtd)
> 		rtd->exit()
> 
> what can lead to errors as link->exit() may still operate on resources
> owned by its components despite the probability of them being freed
> during the component->remove().
> 
> This change modifies the remove sequence to:
> 
> 	for_each_card_rtds(card, rtd)
> 		rtd->exit()
> 	for_each_card_rtds(card, rtd)
> 		for_each_rtd_dais(rtd, i, dai)
> 			dai->remove()
> 	for_each_card_rtds(card, rtd)
> 		for_each_rtd_components(rtd, i, component)
> 			component->remove()
> 
> so code found in link->exit() is safe to touch any component stuff as
> component->remove() has not been called yet.


This looks harmless as described, but...

> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> ---
>  sound/soc/soc-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 4a3fca50a536..638e781df3b0 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -935,9 +935,6 @@ 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
>  	 */

.... what is not shown here is the 	

	soc_free_pcm_runtime(rtd);

which will call device_unregister(rtd->dev);

....

> @@ -1888,6 +1885,9 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card)
>  
>  	snd_soc_dapm_shutdown(card);
>  
> +	/* release machine specific resources */
> +	for_each_card_rtds(card, rtd)
> +		snd_soc_link_exit(rtd);

... so there's still a risk that the link exit refers to memory that's
been released already.

We would need to make sure rtd->dev is not used in any of the existing
callbacks - or other functions such as e.g. snd_soc_close_delayed_work()
which makes use of rtd->dev

>  	/* remove and free each DAI */
>  	soc_remove_link_dais(card);
>  	soc_remove_link_components(card);