Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Anurag Kumar Vulisha <anuragku@xilinx.com>
To:     Alan Stern <stern@rowland.harvard.edu>,
        Felipe Balbi <balbi@kernel.org>
CC:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Shuah Khan <shuah@kernel.org>, Johan Hovold <johan@kernel.org>,
        Jaejoong Kim <climbbb.kim@gmail.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Roger Quadros <rogerq@ti.com>,
        Manu Gautam <mgautam@codeaurora.org>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Mike Christie <mchristi@redhat.com>,
        Matthew Wilcox <willy@infradead.org>,
        Colin Ian King <colin.king@canonical.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "v.anuragkumar@gmail.com" <v.anuragkumar@gmail.com>,
        Thinh Nguyen <thinhn@synopsys.com>,
        Tejas Joglekar <tejas.joglekar@synopsys.com>,
        Ajay Yugalkishore Pandey <APANDEY@xilinx.com>
Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb
 requests
Thread-Topic: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb
 requests
Thread-Index: AQHUiWbrxVzoMt2EkU67+RgGKmmQY6Vrp4EAgAEXBVCAAF3jgIAAAp+AgACIUoCAAM7bwIAAWNWAgAAcRaCAABENAIABKdpQgAKsvYCAALl8AIAHtE1w
Date:   Wed, 12 Dec 2018 15:11:24 +0000
Message-ID: <BL0PR02MB5633B3B0A2BEC1272C0C8705A7A70@BL0PR02MB5633.namprd02.prod.outlook.com>
References: <877eglx45o.fsf@linux.intel.com>
 <Pine.LNX.4.44L0.1812071158570.1560-100000@iolanthe.rowland.org>
In-Reply-To: <Pine.LNX.4.44L0.1812071158570.1560-100000@iolanthe.rowland.org>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 23316872-1979-4e36-c090-08d660441549
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Dec 2018 15:11:24.8079
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR02MB3828
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Hi Felipe,

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>Sent: Friday, December 07, 2018 10:40 PM
>To: Felipe Balbi <balbi@kernel.org>
>Cc: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Shuah Khan <shuah@kernel.org>; Johan Hovold
><johan@kernel.org>; Jaejoong Kim <climbbb.kim@gmail.com>; Benjamin
>Herrenschmidt <benh@kernel.crashing.org>; Roger Quadros <rogerq@ti.com>; M=
anu
>Gautam <mgautam@codeaurora.org>; martin.petersen@oracle.com; Bart Van
>Assche <bvanassche@acm.org>; Mike Christie <mchristi@redhat.com>; Matthew
>Wilcox <willy@infradead.org>; Colin Ian King <colin.king@canonical.com>; l=
inux-
>usb@vger.kernel.org; linux-kernel@vger.kernel.org; v.anuragkumar@gmail.com=
;
>Thinh Nguyen <thinhn@synopsys.com>; Tejas Joglekar
><tejas.joglekar@synopsys.com>; Ajay Yugalkishore Pandey <APANDEY@xilinx.co=
m>
>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@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 t=
han having
>every
>> >>> >controller driver implement its own fix.  At least, that's how it a=
ppears 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 timeou=
t
handler gets moved to the dwc3 driver (the timers still added into udc laye=
r)?=20
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 y=
our
>suggestion
>> > on this.
>>
>> I still think a generic timer is a better solution since it has other us=
es.
>
>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 suc=
h
>> >>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