| Message ID | 20230321134919.25844-1-peter.ujfalusi@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup() | expand |
On Tue, Mar 21, 2023 at 03:49:19PM +0200, Peter Ujfalusi wrote: > The patch adding the bytes control support moved the error check outside > of the list_for_each_entry() which will cause issues when we will have > support for multiple controls per widgets. Even now it causes an issue. We're exiting the list_for_each_entry() without hitting a break statement so the scontrol points to somewhere in the middle of the sdev instead of to a valid scontrol entry. The scontrol->comp_id will be some garbage value. regards, dan carpenter
On 21/03/2023 16:16, Dan Carpenter wrote: > On Tue, Mar 21, 2023 at 03:49:19PM +0200, Peter Ujfalusi wrote: >> The patch adding the bytes control support moved the error check outside >> of the list_for_each_entry() which will cause issues when we will have >> support for multiple controls per widgets. > > Even now it causes an issue. We're exiting the list_for_each_entry() > without hitting a break statement so the scontrol points to somewhere > in the middle of the sdev instead of to a valid scontrol entry. > > The scontrol->comp_id will be some garbage value. I'm not sure what you see ret = 0; list_for_each_entry(scontrol, &sdev->kcontrol_list, list) { if (scontrol->comp_id == swidget->comp_id) { switch (scontrol->info_type) { ... } if (ret < 0) { /* scontrol is still valid and not changed */ dev_err(); return ret; } } } I think this is correct, I could have the ret check one level up, but no point of doing it if scontrol->comp_id != swidget->comp_id
On Tue, Mar 21, 2023 at 04:40:05PM +0200, Péter Ujfalusi wrote: > > > On 21/03/2023 16:16, Dan Carpenter wrote: > > On Tue, Mar 21, 2023 at 03:49:19PM +0200, Peter Ujfalusi wrote: > >> The patch adding the bytes control support moved the error check outside > >> of the list_for_each_entry() which will cause issues when we will have > >> support for multiple controls per widgets. > > > > Even now it causes an issue. We're exiting the list_for_each_entry() > > without hitting a break statement so the scontrol points to somewhere > > in the middle of the sdev instead of to a valid scontrol entry. > > > > The scontrol->comp_id will be some garbage value. > > I'm not sure what you see No, the patch is correct. My issue is with the commit message because it says "will cause issues when we will have support for multiple controls per widgets." The bug already causes issues now. regards, dan carpenter
On 21/03/2023 16:46, Dan Carpenter wrote: > On Tue, Mar 21, 2023 at 04:40:05PM +0200, Péter Ujfalusi wrote: >> >> >> On 21/03/2023 16:16, Dan Carpenter wrote: >>> On Tue, Mar 21, 2023 at 03:49:19PM +0200, Peter Ujfalusi wrote: >>>> The patch adding the bytes control support moved the error check outside >>>> of the list_for_each_entry() which will cause issues when we will have >>>> support for multiple controls per widgets. >>> >>> Even now it causes an issue. We're exiting the list_for_each_entry() >>> without hitting a break statement so the scontrol points to somewhere >>> in the middle of the sdev instead of to a valid scontrol entry. >>> >>> The scontrol->comp_id will be some garbage value. >> >> I'm not sure what you see > > No, the patch is correct. My issue is with the commit message because > it says "will cause issues when we will have support for multiple > controls per widgets." The bug already causes issues now. Right, I will reword and resend. > > regards, > dan carpenter >
Hi Dan, On 21/03/2023 16:46, Dan Carpenter wrote: > On Tue, Mar 21, 2023 at 04:40:05PM +0200, Péter Ujfalusi wrote: >> >> >> On 21/03/2023 16:16, Dan Carpenter wrote: >>> On Tue, Mar 21, 2023 at 03:49:19PM +0200, Peter Ujfalusi wrote: >>>> The patch adding the bytes control support moved the error check outside >>>> of the list_for_each_entry() which will cause issues when we will have >>>> support for multiple controls per widgets. >>> >>> Even now it causes an issue. We're exiting the list_for_each_entry() >>> without hitting a break statement so the scontrol points to somewhere >>> in the middle of the sdev instead of to a valid scontrol entry. >>> >>> The scontrol->comp_id will be some garbage value. >> >> I'm not sure what you see > > No, the patch is correct. My issue is with the commit message because > it says "will cause issues when we will have support for multiple > controls per widgets." The bug already causes issues now. It bugged me a great deal how could I have missed this initially as I was testing the firmware side and user space, I had not one failure with this until all the pieces found there places... The reason is simple: we have one control per swidget and in the error print: dev_err(sdev->dev, "kcontrol %d set up failed for widget %s\n", scontrol->comp_id, swidget->widget->name); only the widget's name gave usable information for a human. If we would have taken the comp_id from swidget->comp_id then this would not have been discovered. scontrol was not invalid (but ignored), swidget name was correct, one control per swidget, all looked about right for the eye ;) Again, thanks for the report!
On Tue, 21 Mar 2023 15:49:19 +0200, Peter Ujfalusi wrote: > The patch adding the bytes control support moved the error check outside > of the list_for_each_entry() which will cause issues when we will have > support for multiple controls per widgets. > > Restore the original logic and return on the first error with the error > code. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup() commit: 1c12e032cc43256d75fdd22e60a7df85e8df4549 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/sound/soc/sof/ipc4-control.c b/sound/soc/sof/ipc4-control.c index d26ed2a6029f..6f0698be9451 100644 --- a/sound/soc/sof/ipc4-control.c +++ b/sound/soc/sof/ipc4-control.c @@ -429,14 +429,17 @@ static int sof_ipc4_widget_kcontrol_setup(struct snd_sof_dev *sdev, struct snd_s default: break; } + + if (ret < 0) { + dev_err(sdev->dev, + "kcontrol %d set up failed for widget %s\n", + scontrol->comp_id, swidget->widget->name); + return ret; + } } } - if (ret < 0) - dev_err(sdev->dev, "kcontrol %d set up failed for widget %s\n", - scontrol->comp_id, swidget->widget->name); - - return ret; + return 0; } static int
The patch adding the bytes control support moved the error check outside of the list_for_each_entry() which will cause issues when we will have support for multiple controls per widgets. Restore the original logic and return on the first error with the error code. Fixes: a062c8899fed ("ASoC: SOF: ipc4-control: Add support for bytes control get and put") Reported-by: Dan Carpenter <error27@gmail.com> Link: https://lore.kernel.org/alsa-devel/6be945d2-40cb-46fb-67ba-ed3a19cddfa4@linux.intel.com/T/#t Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> --- sound/soc/sof/ipc4-control.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)