Re: [PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass

[Keyon Jie <yang.jie@linux.intel.com>]

On 5/26/20 8:38 PM, Cezary Rojewski wrote:
> On 2020-05-21 9:48 AM, Keyon Jie wrote:
>> The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is
>> comparing two different enums, refine the element loading functions, and
>> log the information when the header is being parsed in the corresponding
>> parsing pass.
>>
>> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
>> ---
>>   sound/soc/soc-topology.c | 53 +++++++++++++++++++++++++++++++---------
>>   1 file changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>> index 49875978a1ce..70c0ff7ce13f 100644
>> --- a/sound/soc/soc-topology.c
>> +++ b/sound/soc/soc-topology.c
>> @@ -2636,6 +2636,17 @@ static int soc_tplg_manifest_load(struct 
>> soc_tplg *tplg,
>>       return ret;
>>   }
>> +static int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr 
>> *) = {
>> +    [SOC_TPLG_PASS_MANIFEST]    = soc_tplg_manifest_load,
>> +    [SOC_TPLG_PASS_VENDOR]        = soc_tplg_vendor_load,
>> +    [SOC_TPLG_PASS_MIXER]        = soc_tplg_kcontrol_elems_load,
>> +    [SOC_TPLG_PASS_WIDGET]        = soc_tplg_dapm_widget_elems_load,
>> +    [SOC_TPLG_PASS_PCM_DAI]        = soc_tplg_pcm_elems_load,
>> +    [SOC_TPLG_PASS_GRAPH]        = soc_tplg_dapm_graph_elems_load,
>> +    [SOC_TPLG_PASS_BE_DAI]        = soc_tplg_dai_elems_load,
>> +    [SOC_TPLG_PASS_LINK]        = soc_tplg_link_elems_load,
>> +};
>> +
> 
> Do we really need private array for this? There is a separate 
> soc_tplg_load_header function already. By separate, I mean an isolate 
> scope for single task only - validating tplg headers. While I agree with 
> the idea behind second patch of the series - removal of individual 
> checks in favor of generic one - I believe said checks could be 
> "inlined". See below.
> 
>>   /* validate header magic, size and type */
>>   static int soc_valid_header(struct soc_tplg *tplg,
>>       struct snd_soc_tplg_hdr *hdr)
>> @@ -2685,19 +2696,31 @@ static int soc_valid_header(struct soc_tplg 
>> *tplg,
>>           return -EINVAL;
>>       }
>> -    if (tplg->pass == le32_to_cpu(hdr->type))
>> +    return 1;
>> +}
>> +
>> +/* check and load the element for the current pass */
>> +static int soc_pass_load(struct soc_tplg *tplg,
>> +             struct snd_soc_tplg_hdr *hdr,
>> +             unsigned int hdr_pass)
>> +{
>> +    if (tplg->pass == hdr_pass) {
>>           dev_dbg(tplg->dev,
>>               "ASoC: Got 0x%x bytes of type %d version %d vendor %d at 
>> pass %d\n",
>>               hdr->payload_size, hdr->type, hdr->version,
>>               hdr->vendor_type, tplg->pass);
>> +        return elem_load[hdr_pass](tplg, hdr);
>> +    }
>> -    return 1;
>> +    return 0;
>>   }
> 
> Remove the function and merge its body into soc_tplg_load_header.
> 
>>   /* check header type and call appropriate handler */we
>>   static int soc_tplg_load_header(struct soc_tplg *tplg,
>>       struct snd_soc_tplg_hdr *hdr)
>>   {
>> +    unsigned int hdr_pass;
>> +
>>       tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
>>       /* check for matching ID */
>> @@ -2711,27 +2734,35 @@ static int soc_tplg_load_header(struct 
>> soc_tplg *tplg,
>>       case SND_SOC_TPLG_TYPE_MIXER:
>>       case SND_SOC_TPLG_TYPE_ENUM:
>>       case SND_SOC_TPLG_TYPE_BYTES:
>> -        return soc_tplg_kcontrol_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_MIXER;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
>> -        return soc_tplg_dapm_graph_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_GRAPH;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
>> -        return soc_tplg_dapm_widget_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_WIDGET;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_PCM:
>> -        return soc_tplg_pcm_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_PCM_DAI;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_DAI:
>> -        return soc_tplg_dai_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_BE_DAI;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_DAI_LINK:
>>       case SND_SOC_TPLG_TYPE_BACKEND_LINK:
>>           /* physical link configurations */
>> -        return soc_tplg_link_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_LINK;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_MANIFEST:
>> -        return soc_tplg_manifest_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_MANIFEST;
>> +        break;
>>       default:
>>           /* bespoke vendor data object */
>> -        return soc_tplg_vendor_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_VENDOR;
>> +        break;
>>       }
>> -    return 0;
>> +    return soc_pass_load(tplg, hdr, hdr_pass);
> 
> By having "log" code here we have one place for hdr validation, rather 
> than two (the second being just an "if" to be fair..) and private array 
> is no longer necessary. Local func ptr variable would take care of 
> storing adequate function to call.

Hi Cezary, so what you suggested above is changing the 
soc_tplg_load_header() to be something like this, right?


static int soc_tplg_load_header(struct soc_tplg *tplg,
	struct snd_soc_tplg_hdr *hdr)
{
	unsigned int hdr_pass;
	int (*elem_load)(struct soc_tplg *, struct snd_soc_tplg_hdr *);

	tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);

	/* check for matching ID */
	if (le32_to_cpu(hdr->index) != tplg->req_index &&
		tplg->req_index != SND_SOC_TPLG_INDEX_ALL)
		return 0;

	tplg->index = le32_to_cpu(hdr->index);

	switch (le32_to_cpu(hdr->type)) {
	case SND_SOC_TPLG_TYPE_MIXER:
	case SND_SOC_TPLG_TYPE_ENUM:
	case SND_SOC_TPLG_TYPE_BYTES:
		hdr_pass = SOC_TPLG_PASS_MIXER;
		elem_load = soc_tplg_kcontrol_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
		hdr_pass = SOC_TPLG_PASS_GRAPH;
		elem_load = soc_tplg_dapm_graph_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
		hdr_pass = SOC_TPLG_PASS_WIDGET;
		elem_load = soc_tplg_dapm_widget_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_PCM:
		hdr_pass = SOC_TPLG_PASS_PCM_DAI;
		elem_load = soc_tplg_pcm_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_DAI:
		hdr_pass = SOC_TPLG_PASS_BE_DAI;
		elem_load = soc_tplg_dai_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_DAI_LINK:
	case SND_SOC_TPLG_TYPE_BACKEND_LINK:
		/* physical link configurations */
		hdr_pass = SOC_TPLG_PASS_LINK;
		elem_load = soc_tplg_link_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_MANIFEST:
		hdr_pass = SOC_TPLG_PASS_MANIFEST;
		elem_load = soc_tplg_manifest_load;
		break;
	default:
		/* bespoke vendor data object */
		hdr_pass = SOC_TPLG_PASS_VENDOR;
		elem_load = soc_tplg_vendor_load;
		break;
	}

	if (tplg->pass == hdr_pass) {
		dev_dbg(tplg->dev,
			"ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n",
			hdr->payload_size, hdr->type, hdr->version,
			hdr->vendor_type, tplg->pass);
		return elem_load(tplg, hdr);
	}

	return 0;
}


I am also fine with this, though I thought my previous version looks 
more organized and not so error-prone as we need 8 more assignation here.

Mark, Pierre, preference about this?

Thanks,
~Keyon

> 
>>   }
>>   /* process the topology file headers */
>>