[alsa-devel] [PATCH 1/2] ASoC: compress: Correct handling of compressed ops

[Charles Keepax]

On Thu, Jan 25, 2018 at 01:18:37AM +0000, Kuninori Morimoto wrote:
> soc_compr_get_component() searches 1st component which have compr_ops,
> but it doesn't check compr_ops->xxx, In above case, compr_ops->open.
> Is it OK ?
> 
> For me, it looks like we can solve it by using "break" and "goto".
> "goto" is quick hack for now.
> For example like below (not tested).
> If we can use rtd->platform, use it, and skip rtdcom loop by using "goto".
> If we use rtdcom, call "break" after calling 1st callback.
> soc_rtdcom_xxx() in soc-pcm.c is doing similar things.
> I don't know this is good idea, or not.
> 
> ---------------
> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
> index 81232f4..a147a03 100644
> --- a/sound/soc/soc-compress.c
> +++ b/sound/soc/soc-compress.c
> @@ -53,6 +53,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  				platform->component.name);
>  			goto plat_err;
>  		}
> +		goto rtdcom_skip:
>  	}
>  
>  	for_each_rtdcom(rtd, rtdcom) {
> @@ -72,10 +73,13 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  			       component->name);
>  			ret = __ret;
>  		}
> +		break;
>  	}
>  	if (ret < 0)
>  		goto machine_err;
>  
> +rtdcom_skip:
> +
>  	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->startup) {
>  		ret = rtd->dai_link->compr_ops->startup(cstream);
>  		if (ret < 0) {
> ---------------

Yes I had considered adding in breaks as an initial fix, however,
two things drove me in this direction.

Firstly, it didn't feel like it made sense to be calling
different compr_ops on different components. Not sure if anyone
feels there might be use-cases for that?

And what should happen if two components had the same
callback? Which do we call, should we call both as the current
code does, but then what to do with the results? In short I
felt there is significantly more work to be done to support
systems where we have multiple components providing compressed
functionality on a single runtime, so better to not look like we
support it at the moment.

Secondly it feels like a lot of code duplication having those
almost identical loops in every callback and this solution keeps
things much neater.

I will have a look through the soc-pcm stuff as well and have a
think if there are any issues there as well, although that does
all seem to work on my system.

Thanks,
Charles