[PATCH] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup()

[PATCH] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup()

[Peter Ujfalusi]

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(-)

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

Re: [PATCH] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup()

[Dan Carpenter]

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

Re: [PATCH] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup()

[Péter Ujfalusi]

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

-- 
Péter

Re: [PATCH] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup()

[Dan Carpenter]

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

Re: [PATCH] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup()

[Péter Ujfalusi]

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
> 

-- 
Péter

Re: [PATCH] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup()

[Péter Ujfalusi]

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!

-- 
Péter

Re: [PATCH] ASoC: SOF: ipc4-control: Return on error in sof_ipc4_widget_kcontrol_setup()

[Mark Brown]

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