RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver

[Felipe Balbi]

Hi,

Anurag Kumar Vulisha <anuragku@xxxxxxxxxx> writes:
>>> Please let us know if you have any suggestions / comments on this patch series.
>>> If you feel this patch series are okay, can we proceed with them?
>>
>>I really don't like this dwc3-specific timer. The best way here would be
>>to add a timer on udc/core.c which can be reused by any udc. This would
>>mean, of course, teaching udc/core about streams and lettting it do part
>>of the handling.
>>
>
> Thanks for spending your time in reviewing this patch. The reason for adding the
> timer is when streams are enabled there could be chances for the host and gadget
> controller to become out of sync, the gadget may wait for the host to issue prime
> transaction and the host may wait for the gadget to issue ERDY. To avoid such a
> potential deadlock conditions, timeout needs to be implemented in dwc3 driver.

"in dwc3 driver" is an implementation choice. The situation you describe
could happen with any UDC, right?

> After timeout occurs, gadget will first stop transfer and restart the transfer again.
> This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how
> other controllers are handling the streams, but since this issue looks more like a

We should get in touch with other UDC authors. We have at least Renesas,
net2280, bcd_udc and mtu3 supporting superspeed.

> dwc3 specific issue, I think it would be more convincing to add the timer in dwc3
> gadget driver rather than adding in udc framework. Also we are stopping the timer

why? When the situation you describe is something that can happen with
any udc, why should we reimplement the solution on all UDCs supporting
streams when we can give generic support for handling certain
situations?

> when a valid StreamEvnt is found, which would be difficult to handle if the timer is

Why difficult? udc-core would call:

mod_timer(gadget->stream_timeout_timer, msecs_to_jiffies(50));

Once you receive stream event, dwc3 would call:

if (timer_pending(dwc->gadget.stream_timeout_timer))
	del_timer(dwc->gadget.stream_timeout_timer);

Why is that difficult? You could even avoid anything to be written in
dwc3 and put the del_timer() inside usb_gadget_giveback_request()
itself. That why, dwc3 doesn't even have to know that there's a timer
running. Also, you're timer function, instead of calling dwc3's private
functions, should be relying on the gadget API.

Your timer, apparently, should be fired per-request, then your timer
function would call:

usb_ep_dequeue(request);
usb_ep_queue(request);

If the timer expires. This would work for any UDC, not only dwc3. Then,
this is something we document for all UDCs and they'd have to adhere to
the API.

In summary, not that many changes needed to dwc3. Nothing related to
timers inside dwc3. Almost everythin can, and should, be done
generically.

-- 
balbi

Attachment: signature.asc
Description: PGP signature