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

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

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@linux.intel.com>
>> ---
>>
>>   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