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

[Anurag Kumar Vulisha]

Hi Felipe,

>-----Original Message-----
>From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx]
>Sent: Friday, December 07, 2018 10:40 PM
>To: Felipe Balbi <balbi@xxxxxxxxxx>
>Cc: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Greg Kroah-Hartman
><gregkh@xxxxxxxxxxxxxxxxxxx>; Shuah Khan <shuah@xxxxxxxxxx>; Johan Hovold
><johan@xxxxxxxxxx>; Jaejoong Kim <climbbb.kim@xxxxxxxxx>; Benjamin
>Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>; Roger Quadros <rogerq@xxxxxx>; Manu
>Gautam <mgautam@xxxxxxxxxxxxxx>; martin.petersen@xxxxxxxxxx; Bart Van
>Assche <bvanassche@xxxxxxx>; Mike Christie <mchristi@xxxxxxxxxx>; Matthew
>Wilcox <willy@xxxxxxxxxxxxx>; Colin Ian King <colin.king@xxxxxxxxxxxxx>; linux-
>usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; v.anuragkumar@xxxxxxxxx;
>Thinh Nguyen <thinhn@xxxxxxxxxxxx>; Tejas Joglekar
><tejas.joglekar@xxxxxxxxxxxx>; Ajay Yugalkishore Pandey <APANDEY@xxxxxxxxxx>
>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 <anuragku@xxxxxxxxxx> 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