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

[Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>]

On 3/27/2020 7:56 PM, Pierre-Louis Bossart wrote:
> On 3/27/20 3:47 PM, Amadeusz Sławiński wrote:
> > Functions soc_tplg_denum_create, soc_tplg_dmixer_create,
> > soc_tplg_dbytes_create can fail, so their return values should be
> > checked and error should be propagated.
> >
> > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> > ---
> >
> >   v2:
> >    Added this patch
> >
> >   sound/soc/soc-topology.c | 18 ++++++++++++------
> >   1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> > index c3811dd66b68..860bced933d6 100644
> > --- a/sound/soc/soc-topology.c
> > +++ b/sound/soc/soc-topology.c
> > @@ -1124,6 +1124,7 @@ static int soc_tplg_kcontrol_elems_load(struct 
> > soc_tplg *tplg,
> >       struct snd_soc_tplg_hdr *hdr)
> >   {
> >       struct snd_soc_tplg_ctl_hdr *control_hdr;
> > +    int ret;
> >       int i;
> >       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.

Thanks,
Amadeusz