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