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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux