Message-ID: <1543662811-5194-2-git-send-email-anurag.kumar.vulisha@xilinx.com>
Date:   Sat, 1 Dec 2018 16:43:22 +0530
From:   Anurag Kumar Vulisha <anurag.kumar.vulisha@...inx.com>
To:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Shuah Khan <shuah@...nel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        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>
CC:     <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>,
        Anurag Kumar Vulisha <anurag.kumar.vulisha@...inx.com>
Subject: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests

In some corner cases the gadget controller may get out of sync
with host and may get into hang state, thus creating a dealock.
For example when bulk streams are enabled for an endpoint, there
can be a condition where the gadget controller waits for the host
to issue prime transaction and the host controller waits for the
gadget to issue ERDY. This condition could create a deadlock.

To avoid such potential deadlocks, a timer is started after queuing
any request for the endpoint in usb_ep_queue(). The gadget driver
is expected to stop the timer if a valid event is found (ex: stream
event for stream capable endpoints). If no valid event is found, the
timer expires after the programmed timeout value and a timeout
callback function registered would be called. This callback function
dequeues the request and re-queues it again, doing so makes the
controller restart the transfer, thus avoiding deadlocks.

This kind of behaviour is observed in dwc3 controller and expected
to be generic issue with other controllers supporting bulk streams.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@...inx.com>
---
 Changes in v7:
	1. Added usb_ep_dequeue() & usb_ep_queue() logic into the timeout
	   handler as suggested by "Felipe Balbi"
	2. Created a usb_ep_queue_timeout() & __usb_ep_queue() functions

 Changes in v6:
	1. This patch is newly added in this series to add timer into udc/core.c
---
 drivers/usb/gadget/udc/core.c | 119 +++++++++++++++++++++++++++++++++++++-----
 include/linux/usb/gadget.h    |  16 ++++++
 2 files changed, 121 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 87d6b12..daeb9bf 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -52,6 +52,25 @@ static int udc_bind_to_driver(struct usb_udc *udc,
 /* ------------------------------------------------------------------------- */
 
 /**
+ * usb_request_timeout - callback function for endpoint stream timeout timer
+ * @arg: pointer to struct timer_list
+ *
+ * This function gets called only when bulk streams are enabled in the endpoint
+ * and only after req->req_timeout_timer has expired. This timer gets expired
+ * only when the gadget controller failed to find a valid stream event for this
+ * request. On timer expiry, this function dequeues the request and requeues it
+ * again to restart the transfer.
+ */
+static void usb_request_timeout(struct timer_list *arg)
+{
+	struct usb_request *req = from_timer(req, arg, req_timeout_timer);
+	struct usb_ep *ep = req->ep;
+
+	usb_ep_dequeue(ep, req);
+	usb_ep_queue_timeout(ep, req, GFP_ATOMIC, req->timeout_ms);
+}
+
+/**
  * usb_ep_set_maxpacket_limit - set maximum packet size limit for endpoint
  * @ep:the endpoint being configured
  * @maxpacket_limit:value of maximum packet size limit
@@ -190,6 +209,47 @@ void usb_ep_free_request(struct usb_ep *ep,
 EXPORT_SYMBOL_GPL(usb_ep_free_request);
 
 /**
+ * __usb_ep_queue - queues (submits) an I/O request to an endpoint.
+ * @ep:the endpoint associated with the request
+ * @req:the request being submitted
+ * @gfp_flags: GFP_* flags to use in case the lower level driver couldn't
+ *	pre-allocate all necessary memory with the request.
+ * @timeout: The timeout value in msecs used by the usb_request timer.
+ *
+ * This should only be called from usb_ep_queue() or usb_ep_queue_timeout().
+ * This function queues the requests to the controller driver and starts the
+ * timer if the timeout value is not zero.
+ */
+static int __usb_ep_queue(struct usb_ep *ep, struct usb_request *req,
+			  gfp_t gfp_flags, const unsigned int timeout)
+{
+	int ret = 0;
+
+	if (WARN_ON_ONCE(!ep->enabled && ep->address)) {
+		ret = -ESHUTDOWN;
+		goto out;
+	}
+
+	ret = ep->ops->queue(ep, req, gfp_flags);
+
+	if (timeout != 0) {
+		timer_setup(&req->req_timeout_timer, usb_request_timeout, 0);
+		req->req_timeout_timer.expires = jiffies +
+				msecs_to_jiffies(timeout);
+		mod_timer(&req->req_timeout_timer,
+			  req->req_timeout_timer.expires);
+		req->timeout_ms = timeout;
+	}
+
+	/* Assign the ep to req for future usage */
+	req->ep = ep;
+out:
+	trace_usb_ep_queue(ep, req, ret);
+
+	return ret;
+}
+
+/**
  * usb_ep_queue - queues (submits) an I/O request to an endpoint.
  * @ep:the endpoint associated with the request
  * @req:the request being submitted
@@ -260,23 +320,45 @@ EXPORT_SYMBOL_GPL(usb_ep_free_request);
 int usb_ep_queue(struct usb_ep *ep,
 			       struct usb_request *req, gfp_t gfp_flags)
 {
-	int ret = 0;
-
-	if (WARN_ON_ONCE(!ep->enabled && ep->address)) {
-		ret = -ESHUTDOWN;
-		goto out;
-	}
-
-	ret = ep->ops->queue(ep, req, gfp_flags);
-
-out:
-	trace_usb_ep_queue(ep, req, ret);
-
-	return ret;
+	return __usb_ep_queue(ep, req, gfp_flags, 0);
 }
 EXPORT_SYMBOL_GPL(usb_ep_queue);
 
 /**
+ * usb_ep_queue_timeout - queues (submits) an I/O request to an endpoint.
+ * @ep:the endpoint associated with the request
+ * @req:the request being submitted
+ * @gfp_flags: GFP_* flags to use in case the lower level driver couldn't
+ *	pre-allocate all necessary memory with the request.
+ * @timeout: The timeout value used by the timer present in usb_request.
+ *
+ * This functions starts the timer for the requests queued to the controller.
+ * This routine does the same as usb_ep_queue() but takes an extra timeout
+ * argument which is used for setting the timeout value for the timer. There
+ * can be some corner case where the endpoint may go out of sync with the host
+ * and enter into deadlock situation. To avoid such potential deadlocks a timer
+ * is started at the time of queuing the request. This timer should be stopped
+ * by the controller driver on valid conditions otherwise the timer gets
+ * timedout and the handler is called which handles the deadlock.
+ *
+ * For example, when streams are enabled the host and gadget can go out sync,the
+ * gadget may wait until the host issues prime transaction and the host may wait
+ * until gadget issues a ERDY. This behaviour may create a deadlock situation.
+ * To avoid such a deadlock, when request is queued to an endpoint, the timer
+ * present in usb_request is started. If a valid stream event is found the
+ * gadget driver stops the timer. If no valid stream event is found, the timer
+ * keeps running until expired and the timeout handler registered to the timer
+ * usb_request_timeout() gets called, which dequeues the request and requeues
+ * the request to avoid the deadlock condition.
+ */
+int usb_ep_queue_timeout(struct usb_ep *ep, struct usb_request *req,
+			 gfp_t gfp_flags, const unsigned int timeout)
+{
+	return __usb_ep_queue(ep, req, gfp_flags, timeout);
+}
+EXPORT_SYMBOL_GPL(usb_ep_queue_timeout);
+
+/**
  * usb_ep_dequeue - dequeues (cancels, unlinks) an I/O request from an endpoint
  * @ep:the endpoint associated with the request
  * @req:the request being canceled
@@ -291,6 +373,8 @@ EXPORT_SYMBOL_GPL(usb_ep_queue);
  * restrictions prevent drivers from supporting configuration changes,
  * even to configuration zero (a "chapter 9" requirement).
  *
+ * If a timer is started in usb_ep_queue(), it would be removed.
+ *
  * This routine may be called in interrupt context.
  */
 int usb_ep_dequeue(struct usb_ep *ep, struct usb_request *req)
@@ -300,6 +384,9 @@ int usb_ep_dequeue(struct usb_ep *ep, struct usb_request *req)
 	ret = ep->ops->dequeue(ep, req);
 	trace_usb_ep_dequeue(ep, req, ret);
 
+	if (timer_pending(&req->req_timeout_timer))
+		del_timer(&req->req_timeout_timer);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_ep_dequeue);
@@ -883,7 +970,8 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
  * Context: in_interrupt()
  *
  * This is called by device controller drivers in order to return the
- * completed request back to the gadget layer.
+ * completed request back to the gadget layer. If a timer is started
+ * in usb_ep_queue(), it would be removed.
  */
 void usb_gadget_giveback_request(struct usb_ep *ep,
 		struct usb_request *req)
@@ -891,6 +979,9 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
 	if (likely(req->status == 0))
 		usb_led_activity(USB_LED_EVENT_GADGET);
 
+	if (timer_pending(&req->req_timeout_timer))
+		del_timer(&req->req_timeout_timer);
+
 	trace_usb_gadget_giveback_request(ep, req, 0);
 
 	req->complete(ep, req);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a..5b2516b 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -61,6 +61,13 @@ struct usb_ep;
  *	invalidated by the error may first be dequeued.
  * @context: For use by the completion callback
  * @list: For use by the gadget driver.
+ * @req_timeout_timer: Some endpoints may go out of sync with host and
+ *	enter into deadlock. For example, stream capable endpoints may enter
+ *	into deadlock where the host waits on gadget to issue ERDY and gadget
+ *	waits for host to issue prime transaction. To avoid such deadlock this
+ *	timer is used.
+ * @timeout_ms: timeout value in msecs used by the req_timeout_timer.
+ * @ep: pointer to the endpoint for which this request is queued.
  * @status: Reports completion code, zero or a negative errno.
  *	Normally, faults block the transfer queue from advancing until
  *	the completion callback returns.
@@ -111,6 +118,9 @@ struct usb_request {
 					struct usb_request *req);
 	void			*context;
 	struct list_head	list;
+	unsigned		timeout_ms;
+	struct timer_list	req_timeout_timer;
+	struct usb_ep		*ep;
 
 	int			status;
 	unsigned		actual;
@@ -243,6 +253,8 @@ int usb_ep_disable(struct usb_ep *ep);
 struct usb_request *usb_ep_alloc_request(struct usb_ep *ep, gfp_t gfp_flags);
 void usb_ep_free_request(struct usb_ep *ep, struct usb_request *req);
 int usb_ep_queue(struct usb_ep *ep, struct usb_request *req, gfp_t gfp_flags);
+int usb_ep_queue_timeout(struct usb_ep *ep, struct usb_request *req,
+			 gfp_t gfp_flags, const unsigned int timeout);
 int usb_ep_dequeue(struct usb_ep *ep, struct usb_request *req);
 int usb_ep_set_halt(struct usb_ep *ep);
 int usb_ep_clear_halt(struct usb_ep *ep);
@@ -266,6 +278,10 @@ static inline void usb_ep_free_request(struct usb_ep *ep,
 static inline int usb_ep_queue(struct usb_ep *ep, struct usb_request *req,
 		gfp_t gfp_flags)
 { return 0; }
+static inline int usb_ep_queue_timeout(struct usb_ep *ep,
+				       struct usb_request *req, gfp_t gfp_flags,
+				       const unsigned int timeout)
+{ return 0; }
 static inline int usb_ep_dequeue(struct usb_ep *ep, struct usb_request *req)
 { return 0; }
 static inline int usb_ep_set_halt(struct usb_ep *ep)
-- 
2.1.1

Powered by blists - more mailing lists