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

[Felipe Balbi <balbi@kernel.org>]

[-- 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 --]