Message-ID: <BYAPR02MB5591FB3CC4F14CAB3CE4204CA78E0@BYAPR02MB5591.namprd02.prod.outlook.com>
Date:   Fri, 4 Jan 2019 14:17:56 +0000
From:   Anurag Kumar Vulisha <anuragku@...inx.com>
To:     Alan Stern <stern@...land.harvard.edu>,
        Felipe Balbi <balbi@...nel.org>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Shuah Khan <shuah@...nel.org>, Johan Hovold <johan@...nel.org>,
        Jaejoong Kim <climbbb.kim@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Roger Quadros <rogerq@...com>,
        Manu Gautam <mgautam@...eaurora.org>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        Bart Van Assche <bvanassche@....org>,
        Mike Christie <mchristi@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Colin Ian King <colin.king@...onical.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "v.anuragkumar@...il.com" <v.anuragkumar@...il.com>,
        Thinh Nguyen <thinhn@...opsys.com>,
        Tejas Joglekar <tejas.joglekar@...opsys.com>,
        Ajay Yugalkishore Pandey <APANDEY@...inx.com>
Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb
 requests

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' <stern@...land.harvard.edu>; Felipe Balbi <balbi@...nel.org>
>Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>; Shuah Khan
><shuah@...nel.org>; Johan Hovold <johan@...nel.org>; Jaejoong Kim
><climbbb.kim@...il.com>; Benjamin Herrenschmidt <benh@...nel.crashing.org>;
>Roger Quadros <rogerq@...com>; Manu Gautam <mgautam@...eaurora.org>;
>martin.petersen@...cle.com; Bart Van Assche <bvanassche@....org>; Mike
>Christie <mchristi@...hat.com>; Matthew Wilcox <willy@...radead.org>; Colin Ian
>King <colin.king@...onical.com>; linux-usb@...r.kernel.org; linux-
>kernel@...r.kernel.org; v.anuragkumar@...il.com; Thinh Nguyen
><thinhn@...opsys.com>; Tejas Joglekar <tejas.joglekar@...opsys.com>; Ajay
>Yugalkishore Pandey <APANDEY@...inx.com>
>Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests
>
>
>Hi Felipe,
>
>>-----Original Message-----
>>From: Alan Stern [mailto:stern@...land.harvard.edu]
>>Sent: Friday, December 07, 2018 10:40 PM
>>To: Felipe Balbi <balbi@...nel.org>
>>Cc: Anurag Kumar Vulisha <anuragku@...inx.com>; Greg Kroah-Hartman
>><gregkh@...uxfoundation.org>; Shuah Khan <shuah@...nel.org>; Johan Hovold
>><johan@...nel.org>; Jaejoong Kim <climbbb.kim@...il.com>; Benjamin
>>Herrenschmidt <benh@...nel.crashing.org>; Roger Quadros <rogerq@...com>;
>Manu
>>Gautam <mgautam@...eaurora.org>; martin.petersen@...cle.com; Bart Van
>>Assche <bvanassche@....org>; Mike Christie <mchristi@...hat.com>; Matthew
>>Wilcox <willy@...radead.org>; Colin Ian King <colin.king@...onical.com>; linux-
>>usb@...r.kernel.org; linux-kernel@...r.kernel.org; v.anuragkumar@...il.com;
>>Thinh Nguyen <thinhn@...opsys.com>; Tejas Joglekar
>><tejas.joglekar@...opsys.com>; Ajay Yugalkishore Pandey <APANDEY@...inx.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 <anuragku@...inx.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

Powered by blists - more mailing lists