RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests

[Alan Stern <stern@rowland.harvard.edu>]

On Fri, 7 Dec 2018, Felipe Balbi wrote:

> 
> hi,
> 
> Anurag Kumar Vulisha <anuragku@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.

> >>(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