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

[Cezary Rojewski <cezary.rojewski@intel.com>]

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