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

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

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