Hi Felipe, >-----Original Message----- >From: Felipe Balbi [mailto:balbi@xxxxxxxxxx] >Sent: Tuesday, October 09, 2018 12:51 PM >To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx >Cc: v.anuragkumar@xxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux- >kernel@xxxxxxxxxxxxxxx; Thinh.Nguyen@xxxxxxxxxxxx; Ajay Yugalkishore Pandey ><APANDEY@xxxxxxxxxx> >Subject: RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 >gadget driver > > >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? > Yes this could happen to other UDC drivers also, unless controller is capable of handling >> 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. > Thanks for pointing other drivers. Will refer these drivers to see how they are handling streams >> 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? > I agree with you. As you suggested will work on implementing changes in UDC >> 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. Thanks a lot for giving a detailed explanation. Will implement the timeout changes into UDC core. Best Regards, Anurag Kumar Vulisha
