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
