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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>>>       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?