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>
CC:     Felipe Balbi <balbi@kernel.org>,
        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+A
Date:   Mon, 3 Dec 2018 16:05:24 +0000
Message-ID: <BL0PR02MB563382F544FBEA926452DCF1A7AE0@BL0PR02MB5633.namprd02.prod.outlook.com>
References: <BL0PR02MB563334585C8E1EEFF91EAA39A7AE0@BL0PR02MB5633.namprd02.prod.outlook.com>
 <Pine.LNX.4.44L0.1812030945440.21114-100000@netrider.rowland.org>
In-Reply-To: <Pine.LNX.4.44L0.1812030945440.21114-100000@netrider.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: e2a7c5fe-b12f-4acd-ec45-08d6593922bc
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Dec 2018 16:05:24.8757
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR02MB4788
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 Alan,

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>Sent: Monday, December 03, 2018 8:21 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>
>Cc: Felipe Balbi <balbi@kernel.org>; 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 Mon, 3 Dec 2018, Anurag Kumar Vulisha wrote:
>
>> >First of all, if some sort of deadlock causes a transfer to fail to
>> >complete, the host is expected to cancel and restart it.  Not the
>> >gadget.
>> >
>>
>> Thanks for spending your time in reviewing this patch.  The deadlock
>> is  a very rare case scenario and is happening because both the gadget
>> controller & host controllers get out of sync and are stuck waiting for =
the
>> relevant event. For example this issue is observed in stream protocol wh=
ere
>> the gadget controller is waiting on Host controller to issue PRIME trans=
action
>> and  Host controller is waiting on gadget to issue ERDY transaction. Sin=
ce
>> the stream protocol is gadget driven, the host may not proceed further u=
ntil it
>> receives a valid Start Stream (ERDY) transaction from gadget.
>
>That's not entirely true.  Can't the host cancel the transfer and then
>restart it?
>

Yes the host can cancel the transfer. This issue originated from the endpoi=
nts using bulk
streaming protocol and may not occur with normal endpoints. AFAIK bulk stre=
aming is
gadget driven, where the gadget is allowed to select which stream id transf=
er the host
should work on . Since the host doesn't have control on when the transfer w=
ould be
selected by gadget, it may wait for longer timeouts before cancelling the t=
ransfer.=20

>> Since the gadget
>> controller driver is aware that the controller is stuck , makes it respo=
nsible
>> to recover the controller from hang condition by restarting the transfer=
 (which
>> triggers the controller FSM to issue ERDY to host).
>
>Isn't there a cleaner way to recover than by cancelling the request and
>resubmitting it?
>

dequeuing the request issues the stop transfer command to the controller, w=
hich
cleans all the hardware resource allocated for that endpoint. This also res=
ets the
hardware FSMs for that endpoint . So, when re-queuing of the transfer happe=
ns
the controller starts allocating hardware resources again, thus avoiding th=
e probability
of entering into the issue. I am not sure of other controllers, but for dwc=
3, issuing
the stop transfer is the only way to handle this issue.=20

@Felipe:  Can you please provide your suggestion on this. =20
 =20
>> >Second, if a request timer expires and the request is cancelled, the
>> >gadget driver's completion handler will be called.  This is not what
>> >you want if the UDC core is going to resubmit the request
>> >automatically.
>> >
>> >Third, if a request timer expires and the timer handler calls
>> >usb_ep_dequeue() followed immediately by usb_ep_queue_timeout(), the
>> >resubmit will probably fail because the dequeue won't have completed
>> >yet.
>> >
>> >Fourth, the patch contains a race between the timer expiring and the
>> >request completing.
>>
>> Thanks for correcting, I agree with you on all the above 3 cases that th=
e
>> resubmission of the request should only be done from the class driver an=
d
>> the udc core should simply dequeue the request on timeout. I am not sure
>> why I haven't seen any issue while testing on this patch series. I will =
modify
>> the code to handle the resubmitting of requests properly.
>
>How can the gadget driver know what timeout to use?  The host is
>allowed to be as slow as it wants; the gadget driver doesn't have any
>way to tell when the host wants to start the transfer.

Yes , I agree with you that the timeout may vary from usage to usage. This =
timeout
should be decided by the class driver which queues the request. As discusse=
d above
this issue was observed in streaming protocol , which  is very much faster =
than normal
BOT modes and it works on super speed .  More over the gadget controller de=
cides
the selection of the stream id on which the host should work , so taking al=
l these into
consideration I kept 50ms timeout for stream transfers, so that the perform=
ance may
not get decreased.

Thanks,
Anurag Kumar Vulisha