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

From: Alan Stern <stern@rowland.harvard.edu>
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>,
	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
Date: Tue, 4 Dec 2018 14:28:50 -0500 (EST)	[thread overview]
Message-ID: <Pine.LNX.4.44L0.1812041420200.1537-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <SN6PR02MB56484B8D1F891926DA7D7900A7AF0@SN6PR02MB5648.namprd02.prod.outlook.com>

On Tue, 4 Dec 2018, Anurag Kumar Vulisha wrote:

> >Is there any way for the device controller driver to detect the problem without relying
> >on a timeout?
> >
> >In fact, is this problem a known bug in the existing device controller hardware?  Have
> >the controller manufacturers published a suggested scheme for working around it?
> >
> 
> Yes, this problem is mentioned in dwc3 controller data book and the workaround
> mentioned  is the same that we are doing , to implement timeout and if no valid
> stream event is found , after timeout issue end transfer followed by start transfer.

Okay.  But this implies that the problem is confined to DWC3 and
doesn't affect other types of controllers, which would mean modifying 
the UDC core would be inappropriate.

Does the data book suggest a value for the timeout?

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

(Especially if dwc3 is the only driver affected.)

> With the dequeue approach there is an advantage(not related to this issue) that the
> class driver can have control of the timed out transfer whether to requeue it or consider
> it as an error (based on implementation). This approach gives more flexibility to the class
> drivers. This may be out of context of this issue but wanted to point out that we may lose
> this advantage on switching to older implementation.

Class drivers can cancel and requeue requests at any time.  There's no 
extra flexibility.

> >Ideally it would not be necessary to rely on a timeout at all.
> >
> >Also, maintainers dislike module parameters.  It would be better not to add one.
> 
> Okay. I would be happy if any alternative for this issue is present but unfortunately
> I am not able to figure out any alternative other than timers. If not module_params()
> we can add an configfs entry in stream gadget to update the timeout. Please provide
> your opinion on this approach. 

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.

Alan Stern