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

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

[Cezary Rojewski]

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.

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
 	 */
@@ -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);
 	/* remove and free each DAI */
 	soc_remove_link_dais(card);
 	soc_remove_link_components(card);
-- 
2.25.1

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

[Pierre-Louis Bossart]

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

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

[Cezary Rojewski]

On 2022-06-21 3:31 PM, Pierre-Louis Bossart wrote:
> On 6/21/22 06:57, Cezary Rojewski wrote:

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

The lack of soc_free_pcm_runtime() included here is done on purpose. 
After revisiting probe and remove flows it seems that there are more 
de-synchronization than just link->init() and link->exit(). However, I 
believe providing incremental changes is better than providing one 
single big patch which has much larger impact of the framework. Moving 
just the snd_soc_link_exit() has very limited impact.

In regard to the flow, we have to remember that 'someone' has to be the 
first one, 'someone' has to be the last one too. If the probe flow ends 
with link->init() (for the selected range of functions), then it feels 
natural for remove flow to begin with link->exit().

I've scanned the repo for usages of link->exit() and it seems only Intel 
platforms do so. Given their (functions) current content, status quo is 
achieved. AVS and CATPT drivers reload-tests show no regression too. The 
DAPM-pin warning I'd mentioned earlier in the bdw_rt286 discussion is 
also gone.


Regards,
Czarek

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

[Cezary Rojewski]

On 2022-06-21 4:53 PM, Cezary Rojewski wrote:
> On 2022-06-21 3:31 PM, Pierre-Louis Bossart wrote:
>> On 6/21/22 06:57, Cezary Rojewski wrote:
> 
>>> 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
>>
> 
> The lack of soc_free_pcm_runtime() included here is done on purpose. 
> After revisiting probe and remove flows it seems that there are more 
> de-synchronization than just link->init() and link->exit(). However, I 
> believe providing incremental changes is better than providing one 
> single big patch which has much larger impact of the framework. Moving 
> just the snd_soc_link_exit() has very limited impact.
> 
> In regard to the flow, we have to remember that 'someone' has to be the 
> first one, 'someone' has to be the last one too. If the probe flow ends 
> with link->init() (for the selected range of functions), then it feels 
> natural for remove flow to begin with link->exit().
> 
> I've scanned the repo for usages of link->exit() and it seems only Intel 
> platforms do so. Given their (functions) current content, status quo is 
> achieved. AVS and CATPT drivers reload-tests show no regression too. The 
> DAPM-pin warning I'd mentioned earlier in the bdw_rt286 discussion is 
> also gone.


Hello,

Are there any questions left unanswered here? This patch is a dependency 
for jack handling changes for avs/catpt machine boards.


Regards,
Czarek