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

[Keyon Jie]

On 5/13/20 10:36 AM, Keyon Jie wrote:
> 
> 
> On 5/13/20 3:40 AM, Pierre-Louis Bossart wrote:
>>
>>
>> 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.
> 
> Maybe I should send these 2 patches separately, this 2nd one is actually 
> fix for a long existed issue and it is exposed to me when writing the 
> 1st commit:
> 1. the check of "if (tplg->pass != SOC_TPLG_PASS_xxx" was good and it 
> acted as the correct filter of the pass, but
> 2. the increment "tplg->pos += le32_to_cpu(hdr->size) + 
> le32_to_cpu(hdr->payload_size);" was actually wrong when it the pass is 
> unmatched in #1, it should be "tplg->pos += 
> le32_to_cpu(hdr->payload_size);" as the "hdr->size" was already 
> calculated in the upper layer in soc_tplg_load_header():
> "tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);".
> 3. the exited mistake described in #2 didn't triggered any real problems 
> as the "tplg->pos" will be reset to new value in soc_tplg_load_header() 
> when the next header comes, so increasing the "tplg->pos" here at the 
> end of the current header check actually make no sense, we can just 
> remove them.
> 
> The code changes here might make you nervous but the logic here is not 
> that complicated, if you rule out the issue I mentioned above in #2 and 
> #3, the remained changes here is only removing the checks "if 
> (tplg->pass != SOC_TPLG_PASS_xxx" as they are already covered by the 1st 
> commit.
> 
> I have tested these changes with 3 typical SOF topology files (nocodec, 
> tgl i2s + dsm, hda-generic) and it works all fine there and the correct 
> header information are logged with the changes, agree we should test 
> more extensively and widely, so tests on SST driver are very welcomed.

+ Cezary from the SST driver side.

Hi Cezary, can you help to check this from the SST driver side?

Thanks,
~Keyon

> 
> So in short, I did these changes together since I am already here, but 
> if you and Mark preferred, I can split out the fix for the "tplg->pos" 
> increment and send it ahead.
> 
> Thanks,
> ~Keyon
> 
>>
>>>
>>> Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
>>> ---
>>>   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 */
>>>