[PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[Pawel Laszczak]

From: Pawel Laszczak <pawell@cadence.com>

Patch adds disabling endpoint before enabling it during changing
alternate setting. Lack of this functionality causes that in some
cases uac2 queue the same request multiple time.
Such situation can occur when host send set interface with
alternate setting 1 twice.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>

---
Changelog:
v2:
- moved disabling endpoint into u_audio_start_playback

 drivers/usb/gadget/function/u_audio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 265c4d805f81..c4bbc9decaba 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -401,6 +401,10 @@ int u_audio_start_playback(struct g_audio *audio_dev)
 
 	ep = audio_dev->in_ep;
 	prm = &uac->p_prm;
+
+	if (prm->ep_enabled)
+		u_audio_stop_capture(audio_dev);
+
 	config_ep_by_speed(gadget, &audio_dev->func, ep);
 
 	ep_desc = ep->desc;
-- 
2.25.1

Re: [PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]


Hi,

Pawel Laszczak <pawell@cadence.com> writes:
> From: Pawel Laszczak <pawell@cadence.com>
>
> Patch adds disabling endpoint before enabling it during changing
> alternate setting. Lack of this functionality causes that in some
> cases uac2 queue the same request multiple time.
> Such situation can occur when host send set interface with
> alternate setting 1 twice.

Which host is doing that?

> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>
> ---
> Changelog:
> v2:
> - moved disabling endpoint into u_audio_start_playback
>
>  drivers/usb/gadget/function/u_audio.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> index 265c4d805f81..c4bbc9decaba 100644
> --- a/drivers/usb/gadget/function/u_audio.c
> +++ b/drivers/usb/gadget/function/u_audio.c
> @@ -401,6 +401,10 @@ int u_audio_start_playback(struct g_audio *audio_dev)
>  
>  	ep = audio_dev->in_ep;
>  	prm = &uac->p_prm;
> +
> +	if (prm->ep_enabled)
> +		u_audio_stop_capture(audio_dev);

this looks to be a bug in f_uac2.c::afunc_set_alt(), actually. Note how
e.g. f_obex.c::obex_set_alt() is implemented:

> } else if (intf == obex->data_id) {
> 	if (alt > 1)
> 		goto fail;
>
> 	if (obex->port.in->enabled) {

if interface is already enabled...

> 		dev_dbg(&cdev->gadget->dev,
> 			"reset obex ttyGS%d\n", obex->port_num);
> 		gserial_disconnect(&obex->port);

...disable it first...

> 	}
>
> 	if (!obex->port.in->desc || !obex->port.out->desc) {
> 		dev_dbg(&cdev->gadget->dev,
> 			"init obex ttyGS%d\n", obex->port_num);
> 		if (config_ep_by_speed(cdev->gadget, f,
> 				       obex->port.in) ||
> 		    config_ep_by_speed(cdev->gadget, f,
> 				       obex->port.out)) {

...before configuring endpoints again

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

RE: [PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[Pawel Laszczak]

Hi Felipe,

>
>Pawel Laszczak <pawell@cadence.com> writes:
>> From: Pawel Laszczak <pawell@cadence.com>
>>
>> Patch adds disabling endpoint before enabling it during changing
>> alternate setting. Lack of this functionality causes that in some
>> cases uac2 queue the same request multiple time.
>> Such situation can occur when host send set interface with
>> alternate setting 1 twice.
>
>Which host is doing that?

I've found out this issue on different scenario, but we can imaging the case with double alternate setting.

My case looks like:
  - host send Set Alternate Interface (1)
  - device controller doesn't send ACK for SETUP packet 
  - device delegates request to class and class starts processing it (enable endpoints, etc.)
  - in the meantime  host re-send the same SETUP packet
  - device controller driver detects this packet and try to finish previous one (according to USB spec), but it  cannot disable enabled endpoints. 
  - device  delegate the new SETUP packet to uac2 class
  - uac2 again enables endpoint and try to queues again queued requests 
  - system crash 
 
The similar solution exist in f_uvc.c:
https://elixir.bootlin.com/linux/v5.12-rc8/source/drivers/usb/gadget/function/f_uvc.c#L290

I didn't check the other drivers. Maybe such fix should be added somewhere else.

Such issue can be very rare and very hard to debug.
 
>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>>
>> ---
>> Changelog:
>> v2:
>> - moved disabling endpoint into u_audio_start_playback
>>
>>  drivers/usb/gadget/function/u_audio.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
>> index 265c4d805f81..c4bbc9decaba 100644
>> --- a/drivers/usb/gadget/function/u_audio.c
>> +++ b/drivers/usb/gadget/function/u_audio.c
>> @@ -401,6 +401,10 @@ int u_audio_start_playback(struct g_audio *audio_dev)
>>
>>  	ep = audio_dev->in_ep;
>>  	prm = &uac->p_prm;
>> +
>> +	if (prm->ep_enabled)
>> +		u_audio_stop_capture(audio_dev);
>
>this looks to be a bug in f_uac2.c::afunc_set_alt(), actually. Note how
>e.g. f_obex.c::obex_set_alt() is implemented:
>
>> } else if (intf == obex->data_id) {
>> 	if (alt > 1)
>> 		goto fail;
>>
>> 	if (obex->port.in->enabled) {
>
>if interface is already enabled...
>
>> 		dev_dbg(&cdev->gadget->dev,
>> 			"reset obex ttyGS%d\n", obex->port_num);
>> 		gserial_disconnect(&obex->port);
>
>...disable it first...
>
>> 	}
>>
>> 	if (!obex->port.in->desc || !obex->port.out->desc) {
>> 		dev_dbg(&cdev->gadget->dev,
>> 			"init obex ttyGS%d\n", obex->port_num);
>> 		if (config_ep_by_speed(cdev->gadget, f,
>> 				       obex->port.in) ||
>> 		    config_ep_by_speed(cdev->gadget, f,
>> 				       obex->port.out)) {
>
>...before configuring endpoints again
>


--

Regards
Pawel Laszczak

RE: [PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]


Hi Pawel,

Pawel Laszczak <pawell@cadence.com> writes:
>>Pawel Laszczak <pawell@cadence.com> writes:
>>> From: Pawel Laszczak <pawell@cadence.com>
>>>
>>> Patch adds disabling endpoint before enabling it during changing
>>> alternate setting. Lack of this functionality causes that in some
>>> cases uac2 queue the same request multiple time.
>>> Such situation can occur when host send set interface with
>>> alternate setting 1 twice.
>>
>>Which host is doing that?
>
> I've found out this issue on different scenario, but we can imaging the case with double alternate setting.
>
> My case looks like:
>   - host send Set Alternate Interface (1)
>   - device controller doesn't send ACK for SETUP packet 
>   - device delegates request to class and class starts processing it (enable endpoints, etc.)
>   - in the meantime  host re-send the same SETUP packet
>   - device controller driver detects this packet and try to finish previous one (according to USB spec), but it  cannot disable enabled endpoints. 
>   - device  delegate the new SETUP packet to uac2 class
>   - uac2 again enables endpoint and try to queues again queued requests 
>   - system crash 
>  
> The similar solution exist in f_uvc.c:
> https://elixir.bootlin.com/linux/v5.12-rc8/source/drivers/usb/gadget/function/f_uvc.c#L290
>
> I didn't check the other drivers. Maybe such fix should be added somewhere else.
>
> Such issue can be very rare and very hard to debug.

yeah, this is a requirement by the spec, IIRC. A SetAlt to the same
interface/altSetting means the host wants to reset that altSetting. From
the peripheral point of view that means disabling the endpoints and
reenabling them.

I'm just not entirely sure if we should do this in u_audio or
f_uac[12].c. Arguably, composite.c could detect this and disable the
altSetting, but that would require a huge refactor on the framework.

My gut feeling is that for the minimal bug fix, we should patch
f_uac[12].c, but all audio function drivers have the same exact bug, so
I don't know.

If we follow the "standard" of patching the relevant set_alt functions
in the function drivers, the moment we decide to go for a refactoring,
it'll be easier to see common constructs.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[Alan Stern]

On Mon, Apr 26, 2021 at 03:52:46PM +0300, Felipe Balbi wrote:
> yeah, this is a requirement by the spec, IIRC. A SetAlt to the same
> interface/altSetting means the host wants to reset that altSetting. From
> the peripheral point of view that means disabling the endpoints and
> reenabling them.
> 
> I'm just not entirely sure if we should do this in u_audio or
> f_uac[12].c. Arguably, composite.c could detect this and disable the
> altSetting, but that would require a huge refactor on the framework.
> 
> My gut feeling is that for the minimal bug fix, we should patch
> f_uac[12].c, but all audio function drivers have the same exact bug, so
> I don't know.
> 
> If we follow the "standard" of patching the relevant set_alt functions
> in the function drivers, the moment we decide to go for a refactoring,
> it'll be easier to see common constructs.

FWIW, f_mass_storage.c handles this in its do_set_interface() routine.  
That routine first deallocates any related request buffers and disables 
any enabled endpoints in the interface.  It then goes on to enable 
endpoints for the new alternate setting and allocate request buffers.

The audio function drivers could follow this approach.

Alan Stern

Re: [PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, Apr 26, 2021 at 03:52:46PM +0300, Felipe Balbi wrote:
>> yeah, this is a requirement by the spec, IIRC. A SetAlt to the same
>> interface/altSetting means the host wants to reset that altSetting. From
>> the peripheral point of view that means disabling the endpoints and
>> reenabling them.
>> 
>> I'm just not entirely sure if we should do this in u_audio or
>> f_uac[12].c. Arguably, composite.c could detect this and disable the
>> altSetting, but that would require a huge refactor on the framework.
>> 
>> My gut feeling is that for the minimal bug fix, we should patch
>> f_uac[12].c, but all audio function drivers have the same exact bug, so
>> I don't know.
>> 
>> If we follow the "standard" of patching the relevant set_alt functions
>> in the function drivers, the moment we decide to go for a refactoring,
>> it'll be easier to see common constructs.
>
> FWIW, f_mass_storage.c handles this in its do_set_interface() routine.  
> That routine first deallocates any related request buffers and disables 
> any enabled endpoints in the interface.  It then goes on to enable 
> endpoints for the new alternate setting and allocate request buffers.
>
> The audio function drivers could follow this approach.

right, that's what all other drivers do, in fact. Only audio seems to be
different. The question here is whether to patch every f_uac*.c (there
are three of them) or patch it in the generic u_audio.c

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

RE: [PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[Pawel Laszczak]

Hi,

>
>Alan Stern <stern@rowland.harvard.edu> writes:
>> On Mon, Apr 26, 2021 at 03:52:46PM +0300, Felipe Balbi wrote:
>>> yeah, this is a requirement by the spec, IIRC. A SetAlt to the same
>>> interface/altSetting means the host wants to reset that altSetting. From
>>> the peripheral point of view that means disabling the endpoints and
>>> reenabling them.
>>>
>>> I'm just not entirely sure if we should do this in u_audio or
>>> f_uac[12].c. Arguably, composite.c could detect this and disable the
>>> altSetting, but that would require a huge refactor on the framework.
>>>
>>> My gut feeling is that for the minimal bug fix, we should patch
>>> f_uac[12].c, but all audio function drivers have the same exact bug, so
>>> I don't know.
>>>
>>> If we follow the "standard" of patching the relevant set_alt functions
>>> in the function drivers, the moment we decide to go for a refactoring,
>>> it'll be easier to see common constructs.
>>
>> FWIW, f_mass_storage.c handles this in its do_set_interface() routine.
>> That routine first deallocates any related request buffers and disables
>> any enabled endpoints in the interface.  It then goes on to enable
>> endpoints for the new alternate setting and allocate request buffers.
>>
>> The audio function drivers could follow this approach.
>
>right, that's what all other drivers do, in fact. Only audio seems to be
>different. The question here is whether to patch every f_uac*.c (there
>are three of them) or patch it in the generic u_audio.c
>

I agree that the best solution is to create common implementation in
composite.c. Maybe usb_function->get_alt and usb-function->set_alt will
be enougt to detect such case from composite.c. The problem can be
with testing such change with all functions.

For fast fix bug I don't have opinion which place is better u_audio.c or
f_uac*.c. 

First version of this patch makes change only in f_uac2.c and the second
Version moved fix to u_audio.c (as suggested Peter).

Change in u_audio is simpler, and as wrote Felipe is common for all
UAC drivers. Maybe for fast fix it's better.  

If you want, you can feel free to change and modify this patch. 
It is important for me to fix this issue because it was hard to debug.
 
--

Regards,
Pawel Laszczak

RE: [PATCH v2 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]


Hi,

Pawel Laszczak <pawell@cadence.com> writes:
>>Alan Stern <stern@rowland.harvard.edu> writes:
>>> On Mon, Apr 26, 2021 at 03:52:46PM +0300, Felipe Balbi wrote:
>>>> yeah, this is a requirement by the spec, IIRC. A SetAlt to the same
>>>> interface/altSetting means the host wants to reset that altSetting. From
>>>> the peripheral point of view that means disabling the endpoints and
>>>> reenabling them.
>>>>
>>>> I'm just not entirely sure if we should do this in u_audio or
>>>> f_uac[12].c. Arguably, composite.c could detect this and disable the
>>>> altSetting, but that would require a huge refactor on the framework.
>>>>
>>>> My gut feeling is that for the minimal bug fix, we should patch
>>>> f_uac[12].c, but all audio function drivers have the same exact bug, so
>>>> I don't know.
>>>>
>>>> If we follow the "standard" of patching the relevant set_alt functions
>>>> in the function drivers, the moment we decide to go for a refactoring,
>>>> it'll be easier to see common constructs.
>>>
>>> FWIW, f_mass_storage.c handles this in its do_set_interface() routine.
>>> That routine first deallocates any related request buffers and disables
>>> any enabled endpoints in the interface.  It then goes on to enable
>>> endpoints for the new alternate setting and allocate request buffers.
>>>
>>> The audio function drivers could follow this approach.
>>
>>right, that's what all other drivers do, in fact. Only audio seems to be
>>different. The question here is whether to patch every f_uac*.c (there
>>are three of them) or patch it in the generic u_audio.c
>>
>
> I agree that the best solution is to create common implementation in
> composite.c. Maybe usb_function->get_alt and usb-function->set_alt will
> be enougt to detect such case from composite.c. The problem can be
> with testing such change with all functions.
>
> For fast fix bug I don't have opinion which place is better u_audio.c or
> f_uac*.c. 
>
> First version of this patch makes change only in f_uac2.c and the second
> Version moved fix to u_audio.c (as suggested Peter).

okay, I missed that Peter had already asked you to move to u_audio.c. I
guess we should go with your patch, but it would be nice to get some
Tested-bys.

Peter, would you be willing to provide some testing for this patch?

> Change in u_audio is simpler, and as wrote Felipe is common for all
> UAC drivers. Maybe for fast fix it's better.  
>
> If you want, you can feel free to change and modify this patch. 

heh, that's not how things work around here :-)

> It is important for me to fix this issue because it was hard to debug.

yup, no question.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]