Hi Felipe, Resending...
Since I am waiting on your suggestion, thought of giving remainder. Thanks, Anurag Kumar Vulisha >-----Original Message----- >From: Anurag Kumar Vulisha >Sent: Wednesday, December 12, 2018 8:41 PM >To: 'Alan Stern' <st...@rowland.harvard.edu>; Felipe Balbi <ba...@kernel.org> >Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>; Shuah Khan ><sh...@kernel.org>; Johan Hovold <jo...@kernel.org>; Jaejoong Kim ><climbbb....@gmail.com>; Benjamin Herrenschmidt <b...@kernel.crashing.org>; >Roger Quadros <rog...@ti.com>; Manu Gautam <mgau...@codeaurora.org>; >martin.peter...@oracle.com; Bart Van Assche <bvanass...@acm.org>; Mike >Christie <mchri...@redhat.com>; Matthew Wilcox <wi...@infradead.org>; Colin Ian >King <colin.k...@canonical.com>; linux-...@vger.kernel.org; linux- >ker...@vger.kernel.org; v.anuragku...@gmail.com; Thinh Nguyen ><thi...@synopsys.com>; Tejas Joglekar <tejas.jogle...@synopsys.com>; Ajay >Yugalkishore Pandey <apan...@xilinx.com> >Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb >requests > > >Hi Felipe, > >>-----Original Message----- >>From: Alan Stern [mailto:st...@rowland.harvard.edu] >>Sent: Friday, December 07, 2018 10:40 PM >>To: Felipe Balbi <ba...@kernel.org> >>Cc: Anurag Kumar Vulisha <anura...@xilinx.com>; Greg Kroah-Hartman >><gre...@linuxfoundation.org>; Shuah Khan <sh...@kernel.org>; Johan Hovold >><jo...@kernel.org>; Jaejoong Kim <climbbb....@gmail.com>; Benjamin >>Herrenschmidt <b...@kernel.crashing.org>; Roger Quadros <rog...@ti.com>; >Manu >>Gautam <mgau...@codeaurora.org>; martin.peter...@oracle.com; Bart Van >>Assche <bvanass...@acm.org>; Mike Christie <mchri...@redhat.com>; Matthew >>Wilcox <wi...@infradead.org>; Colin Ian King <colin.k...@canonical.com>; >>linux- >>u...@vger.kernel.org; linux-kernel@vger.kernel.org; v.anuragku...@gmail.com; >>Thinh Nguyen <thi...@synopsys.com>; Tejas Joglekar >><tejas.jogle...@synopsys.com>; Ajay Yugalkishore Pandey <apan...@xilinx.com> >>Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb >>requests >> >>On Fri, 7 Dec 2018, Felipe Balbi wrote: >> >>> >>> hi, >>> >>> Anurag Kumar Vulisha <anura...@xilinx.com> writes: >>> >>Does the data book suggest a value for the timeout? >>> >> >>> > >>> > No, the databook doesn't mention about the timeout value >>> > >>> >>> >At this point, it seems that the generic approach will be messier than >>> >>> >having >>every >>> >>> >controller driver implement its own fix. At least, that's how it >>> >>> >appears to me. >>> >>> Why, if the UDC implementation will, anyway, be a timer? >> >>It's mostly a question of what happens when the timer expires. (After >>all, starting a timer is just as easy to do in a UDC driver as it is in >>the core.) When the timer expires, what can the core do? >> >>Don't say it can cancel the offending request and resubmit it. That >>leads to the sort of trouble we discussed earlier in this thread. In >>particular, we don't want the class driver's completion routine to be >>called when the cancel occurs. Furthermore, this leads to a race: >>Suppose the class driver decides to cancel the request at the same time >>as the core does a cancel and resubmit. Then the class driver's cancel >>could get lost and the request would remain on the UDC's queue. >> >>What you really want to do is issue the appropriate stop and restart >>commands to the hardware, while leaving the request logically active >>the entire time. The UDC core can't do this, but a UDC driver can. >> > >I agree with Alan's comment as it looks like there may be some corner cases >where the issue may occur with dequeue approach. Are you okay if the timeout >handler gets moved to the dwc3 driver (the timers still added into udc layer)? >Please let us know your suggestion on this > >Thanks, >Anurag Kumar Vulisha > >>> >>(Especially if dwc3 is the only driver affected.) >>> > >>> > As discussed above, the issue may happen with other gadgets too. As I got >>> > divide >>opinions >>> > on this implementation and both the implementations looks fine to me, I am >little >>confused >>> > on which should be implemented. >>> > >>> > @Felipe: Do you agree with Alan's implementation? Please let us know your >>suggestion >>> > on this. >>> >>> I still think a generic timer is a better solution since it has other uses. >> >>Putting a struct timer into struct usb_request is okay with me, but I >>wouldn't go any farther than that. >> >>> >>Since the purpose of the timeout is to detect a deadlock caused by a >>> >>hardware bug, I suggest a fixed and relatively short timeout value such >>> >>as one second. Cancelling and requeuing a few requests at 1-second >>> >>intervals shouldn't add very much overhead. >>> >>> I wouldn't call this a HW bug though. This is just how the UDC >>> behaves. There are N streams and host can move data in any stream at any >>> time. This means that host & gadget _can_ disagree on what stream to >>> start next. >> >>But the USB 3 spec says what should happen when the host and gadget >>disagree in this way, doesn't it? And it doesn't say that they should >>deadlock. :-) Or have I misread the spec? >> >>> One way to avoid this would be to never pre-start any streams and always >>> rely on XferNotReady, but that would mean greatly reduced throughput for >>> streams. >> >>It would be good if there was some way to actively detect the problem >>instead of passively waiting for a timer to expire. >> >>Alan Stern
