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

[Keyon Jie <yang.jie@xxxxxxxxxxxxxxx>]

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.

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@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 */