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