Re: [PATCH v2 3/6] ASoC: topology: Check return value of soc_tplg_*_create

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

> > >       if (tplg->pass != SOC_TPLG_PASS_MIXER) {
> > > @@ -1152,25 +1153,30 @@ static int 
> > > soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg,
> > >           case SND_SOC_TPLG_CTL_RANGE:
> > >           case SND_SOC_TPLG_DAPM_CTL_VOLSW:
> > >           case SND_SOC_TPLG_DAPM_CTL_PIN:
> > > -            soc_tplg_dmixer_create(tplg, 1,
> > > -                           le32_to_cpu(hdr->payload_size));
> > > +            ret = soc_tplg_dmixer_create(tplg, 1,
> > > +                    le32_to_cpu(hdr->payload_size));
> > >               break;
> > >           case SND_SOC_TPLG_CTL_ENUM:
> > >           case SND_SOC_TPLG_CTL_ENUM_VALUE:
> > >           case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE:
> > >           case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT:
> > >           case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE:
> > > -            soc_tplg_denum_create(tplg, 1,
> > > -                          le32_to_cpu(hdr->payload_size));
> > > +            ret = soc_tplg_denum_create(tplg, 1,
> > > +                    le32_to_cpu(hdr->payload_size));
> > >               break;
> > >           case SND_SOC_TPLG_CTL_BYTES:
> > > -            soc_tplg_dbytes_create(tplg, 1,
> > > -                           le32_to_cpu(hdr->payload_size));
> > > +            ret = soc_tplg_dbytes_create(tplg, 1,
> > > +                    le32_to_cpu(hdr->payload_size));
> > >               break;
> > >           default:
> > >               soc_bind_err(tplg, control_hdr, i);
> > >               return -EINVAL;
> > >           }
> > > +        if (ret < 0) {
> > > +            dev_err(tplg->dev, "ASoC: invalid control\n");
> > > +            return ret;
> > > +        }
> >
> > Sounds good, but this happens in a loop, so would all the memory 
> > previously allocated by denum/dbytes/dmixer_create leak, or is it 
> > freed automatically somewhere else?
>
> Well, now that error is propagated, snd_soc_tplg_component_remove() 
> should be called by snd_soc_tplg_component_load() in case of errors 
> while parsing. From quick look it seems like it should be able to free 
> it up correctly by calling remove_enum/bytes/mixer.

I am not sure what you meant by 'should be called', if it's a 
recommendation for a future change or a description of the existing 
behavior.
Just to be clear, are you saying the existing code will take care of 
this error flow or that a new patch is needed?