Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: vinod.koul@intel.com, alsa-devel@alsa-project.org,
	broonie@kernel.org, lgirdwood@gmail.com,
	dan.carpenter@oracle.com
Subject: Re: [PATCH 1/2] ASoC: compress: Correct handling of compressed ops
Date: Thu, 25 Jan 2018 01:18:37 +0000	[thread overview]
Message-ID: <87wp06lotn.wl%kuninori.morimoto.gx@renesas.com> (raw)
In-Reply-To: <20180124135525.5633-1-ckeepax@opensource.cirrus.com>


Hi Charles

Thank you for your patch.
I'm sorry that my patch breaks your system.

I'm not good at soc_compr_xxx,
thus, basically I have no objection about this patch.
This is just question.

> The soc_compr_copy callback is currently broken. Since the
> changes to move the compr_ops over to the component the return
> value is not correctly proqpagated, always returning zero on
> success rather than the number of bytes copied. This causes
> user-space to stall continuously reading as it does not believe
> it has received any data.
> 
> Furthermore, the changes to move the compr_ops over to the
> component iterate through the list of components and will
> call the compressed operation for any that have compressed
> ops. However, there would be signifcantly more work required
> to support such a system. There isn't currently any sensible
> mechanism to forward the ops to multiple components. For example
> what should be done about merging data from copy?  Or what should
> be returned through the pointer call, each component may have
> different amounts of data available, but user-space expects a
> single answer?
> 
> As such clean up the code a little by factoring out the search
> for the relevant component and bring things back to looking for
> a single component that supports compressed ops on a single
> runtime. These refactorings also put back the original handling
> for the copy callback, correcting the return value.
> 
> Fixes: 9e7e3738ab0e ("ASoC: snd_soc_component_driver has snd_compr_ops")
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
(snip)
> -static int soc_compr_open(struct snd_compr_stream *cstream)
> +static struct snd_soc_component *
> +soc_compr_get_component(struct snd_soc_pcm_runtime *rtd)
>  {
> -	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
>  	struct snd_soc_platform *platform = rtd->platform;
> -	struct snd_soc_component *component;
>  	struct snd_soc_rtdcom_list *rtdcom;
> +
> +	if (platform)
> +		return &platform->component;
> +
> +	for_each_rtdcom(rtd, rtdcom) {
> +		if (rtdcom->component->driver->compr_ops)
> +			return rtdcom->component;
> +	}
> +
> +	return NULL;
> +}
(snip)
> +static int soc_compr_open(struct snd_compr_stream *cstream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_soc_component *component = soc_compr_get_component(rtd);
> +	const struct snd_compr_ops *compr_ops = soc_compr_get_ops(rtd, component);
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> -	int ret = 0, __ret;
> +	int ret = 0;
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> @@ -46,36 +74,15 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>  		}
>  	}
>  
> -	if (platform && platform->driver->compr_ops && platform->driver->compr_ops->open) {
> -		ret = platform->driver->compr_ops->open(cstream);
> +	if (compr_ops && compr_ops->open) {
> +		ret = compr_ops->open(cstream);
>  		if (ret < 0) {
>  			pr_err("compress asoc: can't open platform %s\n",
> -				platform->component.name);
> +				component->name);
>  			goto plat_err;
>  		}
>  	}
>  
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> -
> -		/* ignore duplication for now */
> -		if (platform && (component == &platform->component))
> -			continue;
> -
> -		if (!component->driver->compr_ops ||
> -		    !component->driver->compr_ops->open)
> -			continue;
> -
> -		__ret = component->driver->compr_ops->open(cstream);
> -		if (__ret < 0) {
> -			pr_err("compress asoc: can't open platform %s\n",
> -			       component->name);
> -			ret = __ret;
> -		}
> -	}

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) {
---------------



Best regards
---
Kuninori Morimoto
