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

[Pawel Laszczak <pawell@cadence.com>]

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