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

[Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>]

Hi Kai

Thank you for feedback

> 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().

Thank you for understanding my headache.
My opinion is that complex duplicated code never bring luck to us.

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

Yeah indeed.
I think this is just one of the BUG.
I guess we can find similar issue everywhere.

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

Yeah, I can understand your concern, but not 100% yet.
In my understanding, counting start vs stop is not enough but not so bad.
If my understanding was correct, your concern is
counting only is not enough, because wrong component-substream pair
can be used, like this ?

	start(substream-A); <=
	start(substream-B);
	start(substream-C);

	stop(substream-Z);  <=
	stop(substream-B);
	stop(substream-C);

But I wonder is it really happen ?
If there is clear such case, yes, we need to consider
component-substream pair list, somehow.

If you are worry about some kind of BUG, I guess we need to solve
is such bug, not error handling method.
But how about step-by-step approach (I like it :) ?
At first, add counting method as 1st step. Indeed it is not enough,
but we can cleanup error handling.
If we found/noticed above not-enough-case,
start to consider about component-substream pair list ?

Thank you for your help !!
Best regards
---
Kuninori Morimoto