| Message ID | 20200327204729.397-4-amadeuszx.slawinski@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2ae548f30d7f6973388fc3769bb3c2f6fd13652b |
| Headers | show |
| Series | ASoC: topology: Propagate error appropriately | expand |
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; >
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
>>> 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?
>>> 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.
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.
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;
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(-)