Re: [PATCH 2/2] ASoC: topology: remove the redundant pass checks

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

On 5/12/20 1:23 PM, Keyon Jie wrote:
> As we have check the 'pass' in the soc_elem_pass_load(), so no need to
> check it again in each specific elem_load function, at the same time,
> the tplg->pos will be reset to the next header base when the pass is
> mismatched, so the increasing of the tplg->pos in these cases made no
> sense. Here remove all of them.

Sorry Keyon, I am not comfortable with the changes. This started as a 
removal of a debug log and we ended-up changing the execution flow in 
one of the more complex parts of the ASoC framework.
I spent 30mn looking at the code and I don't have a feeling all corner 
cases are covered. The bar for core changes is higher, it'd help to have 
additional explanations or evidence that your proposal was tested 
extensively. You may be right but we should be careful with topology 
changes.

> Signed-off-by: Keyon Jie <yang.jie@xxxxxxxxxxxxxxx>
> ---
>   sound/soc/soc-topology.c | 46 ++--------------------------------------
>   1 file changed, 2 insertions(+), 44 deletions(-)
>
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index b3dae319c108..682ffaa85e9e 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -246,8 +246,8 @@ static inline void soc_control_err(struct soc_tplg *tplg,
>   }
>   
>   /* pass vendor data to component driver for processing */
> -static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
> -	struct snd_soc_tplg_hdr *hdr)
> +static int soc_tplg_vendor_load(struct soc_tplg *tplg,
> +				struct snd_soc_tplg_hdr *hdr)
>   {
>   	int ret = 0;
>   
> @@ -268,16 +268,6 @@ static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
>   	return ret;
>   }
>   
> -/* pass vendor data to component driver for processing */
> -static int soc_tplg_vendor_load(struct soc_tplg *tplg,
> -	struct snd_soc_tplg_hdr *hdr)
> -{
> -	if (tplg->pass != SOC_TPLG_PASS_VENDOR)
> -		return 0;
> -
> -	return soc_tplg_vendor_load_(tplg, hdr);
> -}
> -
>   /* optionally pass new dynamic widget to component driver. This is mainly for
>    * external widgets where we can assign private data/ops */
>   static int soc_tplg_widget_load(struct soc_tplg *tplg,
> @@ -1127,12 +1117,6 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg,
>   	int ret;
>   	int i;
>   
> -	if (tplg->pass != SOC_TPLG_PASS_MIXER) {
> -		tplg->pos += le32_to_cpu(hdr->size) +
> -			le32_to_cpu(hdr->payload_size);
> -		return 0;
> -	}
> -
>   	dev_dbg(tplg->dev, "ASoC: adding %d kcontrols at 0x%lx\n", hdr->count,
>   		soc_tplg_get_offset(tplg));
>   
> @@ -1204,14 +1188,6 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_GRAPH) {
> -		tplg->pos +=
> -			le32_to_cpu(hdr->size) +
> -			le32_to_cpu(hdr->payload_size);
> -
> -		return 0;
> -	}
> -
>   	if (soc_tplg_check_elem_count(tplg,
>   		sizeof(struct snd_soc_tplg_dapm_graph_elem),
>   		count, le32_to_cpu(hdr->payload_size), "graph")) {
> @@ -1741,9 +1717,6 @@ static int soc_tplg_dapm_widget_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_WIDGET)
> -		return 0;
> -
>   	dev_dbg(tplg->dev, "ASoC: adding %d DAPM widgets\n", count);
>   
>   	for (i = 0; i < count; i++) {
> @@ -2101,9 +2074,6 @@ static int soc_tplg_pcm_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_PCM_DAI)
> -		return 0;
> -
>   	/* check the element size and count */
>   	pcm = (struct snd_soc_tplg_pcm *)tplg->pos;
>   	size = le32_to_cpu(pcm->size);
> @@ -2386,12 +2356,6 @@ static int soc_tplg_link_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_LINK) {
> -		tplg->pos += le32_to_cpu(hdr->size) +
> -			le32_to_cpu(hdr->payload_size);
> -		return 0;
> -	}
> -
>   	/* check the element size and count */
>   	link = (struct snd_soc_tplg_link_config *)tplg->pos;
>   	size = le32_to_cpu(link->size);
> @@ -2528,9 +2492,6 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_BE_DAI)
> -		return 0;
> -
>   	/* config the existing BE DAIs */
>   	for (i = 0; i < count; i++) {
>   		dai = (struct snd_soc_tplg_dai *)tplg->pos;
> @@ -2610,9 +2571,6 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
>   	bool abi_match;
>   	int ret = 0;
>   
> -	if (tplg->pass != SOC_TPLG_PASS_MANIFEST)
> -		return 0;
> -
>   	manifest = (struct snd_soc_tplg_manifest *)tplg->pos;
>   
>   	/* check ABI version by size, create a new manifest if abi not match */