[v2,3/6] ASoC: topology: Check return value of soc_tplg_*_create
diff mbox series

Message ID 20200327204729.397-4-amadeuszx.slawinski@linux.intel.com
State Accepted
Commit 2ae548f30d7f6973388fc3769bb3c2f6fd13652b
Headers show
Series
  • ASoC: topology: Propagate error appropriately
Related show

Commit Message

Amadeusz Sławiński March 27, 2020, 8:47 p.m. UTC
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(-)

Comments

Pierre-Louis Bossart March 27, 2020, 6:56 p.m. UTC | #1
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?

> +
>   	}
>   
>   	return 0;
>
Amadeusz Sławiński March 30, 2020, 8:12 a.m. UTC | #2
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
Pierre-Louis Bossart March 30, 2020, 3:51 p.m. UTC | #3
>>>       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?
Amadeusz Sławiński March 30, 2020, 4:10 p.m. UTC | #4
>>> 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?

Existing code should handle this properly.
No new code is needed.
Pierre-Louis Bossart March 30, 2020, 4:36 p.m. UTC | #5
On 3/30/20 11:10 AM, Amadeusz Sławiński wrote:
> 
>>>> 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?
> 
> Existing code should handle this properly.
> No new code is needed.

Sounds good, thanks.

Patch
diff mbox series

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;
+		}
+
 	}
 
 	return 0;