Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen <kai.vehmanen@linux.intel.com>]

Hello Morimoto-san,

On Wed, 26 Feb 2020, Kuninori Morimoto wrote:

> Maybe this is too late, but I want to tell
> the reason why I wanted to add flag on each component.
[...]
> If each component / rtd / dai have "done" flag or count,
> soc_pcm_open() can call soc_pcm_close() directly
> without thinking about "until", because each flag can handle/indicate it.

thanks for the longer explanation. It's true we have a lot of code with 
for_each_xxx() loops, and loop logic where errors can occur. It seems the 
most common approach is to store the index and use for_each_xxx_rollback() 
macros to clean up in case error happens. This does lead to the problem of 
essentially duplicated logic e.g. for soc_pcm_close() and error handling 
of snd_pcm_open(). And yeah, it's also a bit error prone as the logic is 
not exactly the same. E.g. I'm not convinced this is quite right in 
soc_pcm_open():

»       for_each_rtd_codec_dai(rtd, i, codec_dai) {                                                                                                                                                          
»       »       ret = snd_soc_dai_startup(codec_dai, substream);                                                                                                                                             
»       »       if (ret < 0) {                                                                                                                                                                               
»       »       »       dev_err(codec_dai->dev,                                                                                                                                                              
»       »       »       »       "ASoC: can't open codec %s: %d\n",                                                                                                                                           
»       »       »       »       codec_dai->name, ret);                                                                                                                                                       
»       »       »       goto config_err;                                                                                                                                                                     
»       »       }                                             
...
config_err:                                                                                                                                                                                                  
»       for_each_rtd_codec_dai(rtd, i, codec_dai)                                                                                                                                                            
»       »       snd_soc_dai_shutdown(codec_dai, substream);                                                                                                                                                  
»       i = rtd->num_cpus;
 
... i.e. we should use _rollback() macro here, but we don't so shutdown 
is called on all codec dais if I read this right.

Having a single cleanup path would be easier, but I think in the end it 
comes down to how cleanly you can track the opened state. It seems biggest 
issue is how to cleanly track the component-substream pairs. Ideally we'd 
have a dedicated state object to represent an opened component-substream 
pair, but that's not how the APIs are defined now. But something to that
effect is needed.

Br, Kai