[PATCH v7 00/10] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver

[PATCH v7 00/10] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver

[Anurag Kumar Vulisha]

This patch series fixes the broken BULK streaming support in
dwc3 gadget driver and also adds timers in udc/core.c  for
the endpoints which may go out of sync with host and enter into
deadlock. 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.


Changes in v7:
	1. Added timer timeout handler into udc/core.c
	2. Started timer per request instead of per endpoint as suggested by
	   "Felipe Balbi"
	3. Added usb_ep_dequeue() & usb_ep_queue() logic into timeout handler
	   as suggested by "Felipe Balbi"

Changes in v6:
	1. Added timer into udc/core.c for stream capable endpoint
	   as suggested by "Felipe Balbi"

Changes in v5:
	1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen"

Changes in v4:
	1. Corrected the commit message and stream timeout description
	   as suggested by "Thinh Nguyen"

Changes in v3:
	1. Added the changes suggested by "Thinh Nguyen"

Changes in v2:
	1. Added "usb: dwc3:" in subject heading

Anurag Kumar Vulisha (10):
  usb: gadget: udc: Add timer support for usb requests
  usb: gadget: function: tcm: Add timeout for stream capable endpoints
  usb: dwc3: gadget: handle stream events
  usb: dwc3: update stream id in depcmd
  usb: dwc3: make controller clear transfer resources after complete
  usb: dwc3: don't issue no-op trb for stream capable endpoints
  usb: dwc3: check for requests in started list for stream capable
    endpoints
  usb: dwc3: Correct the logic for checking TRB full in
    __dwc3_prepare_one_trb()
  usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl
    fields
  usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints

 drivers/usb/dwc3/gadget.c           |  67 +++++++++++++++++---
 drivers/usb/gadget/function/f_tcm.c |  25 +++++---
 drivers/usb/gadget/udc/core.c       | 119 +++++++++++++++++++++++++++++++-----
 include/linux/usb/gadget.h          |  15 +++++
 4 files changed, 197 insertions(+), 29 deletions(-)

-- 
2.1.1

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

[Anurag Kumar Vulisha]

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

[PATCH v7 02/10] usb: gadget: function: tcm: Add timeout for stream capable endpoints

[Anurag Kumar Vulisha]

When stream transfers 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, use usb_ep_queue_timeout()
instead of usb_ep_queue() for stream capable endpoints. The
usb_ep_queue_timeout(), after queuing any request starts the timer
with STREAM_TIMEOUT_MS timeout value for the stream capable
endpoints. The gadget controller driver is expected to stop the
timer for every request if a valid stream event is found. If no
stream event is found, the timer expires after the STREAM_TIMEOUT_MS
value and a callback function registered by udc/core.c is called,
which handles the deadlock situation by dequeuing and requeuing the
request. 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@xilinx.com>
---
 Changes in v7:
	1. This patch is newly added in this series
---
 drivers/usb/gadget/function/f_tcm.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 106988a..6eaee04 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -27,6 +27,12 @@
 
 #define TPG_INSTANCES		1
 
+/*
+ * Timeout value in msecs passed as an argument to usb_ep_queue_timeout() for
+ * stream capable endpoints
+ */
+#define STREAM_TIMEOUT_MS	50
+
 struct tpg_instance {
 	struct usb_function_instance	*func_inst;
 	struct usbg_tpg			*tpg;
@@ -575,7 +581,8 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 		ret = uasp_prepare_r_request(cmd);
 		if (ret)
 			goto cleanup;
-		ret = usb_ep_queue(fu->ep_in, stream->req_in, GFP_ATOMIC);
+		ret = usb_ep_queue_timeout(fu->ep_in, stream->req_in,
+					   GFP_ATOMIC, STREAM_TIMEOUT_MS);
 		if (ret)
 			pr_err("%s(%d) => %d\n", __func__, __LINE__, ret);
 		break;
@@ -584,15 +591,16 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 		ret = usbg_prepare_w_request(cmd, stream->req_out);
 		if (ret)
 			goto cleanup;
-		ret = usb_ep_queue(fu->ep_out, stream->req_out, GFP_ATOMIC);
+		ret = usb_ep_queue_timeout(fu->ep_out, stream->req_out,
+					   GFP_ATOMIC, STREAM_TIMEOUT_MS);
 		if (ret)
 			pr_err("%s(%d) => %d\n", __func__, __LINE__, ret);
 		break;
 
 	case UASP_SEND_STATUS:
 		uasp_prepare_status(cmd);
-		ret = usb_ep_queue(fu->ep_status, stream->req_status,
-				GFP_ATOMIC);
+		ret = usb_ep_queue_timeout(fu->ep_status, stream->req_status,
+					   GFP_ATOMIC, STREAM_TIMEOUT_MS);
 		if (ret)
 			pr_err("%s(%d) => %d\n", __func__, __LINE__, ret);
 		break;
@@ -622,7 +630,8 @@ static int uasp_send_status_response(struct usbg_cmd *cmd)
 	stream->req_status->context = cmd;
 	cmd->fu = fu;
 	uasp_prepare_status(cmd);
-	return usb_ep_queue(fu->ep_status, stream->req_status, GFP_ATOMIC);
+	return usb_ep_queue_timeout(fu->ep_status, stream->req_status,
+				    GFP_ATOMIC, STREAM_TIMEOUT_MS);
 }
 
 static int uasp_send_read_response(struct usbg_cmd *cmd)
@@ -640,7 +649,8 @@ static int uasp_send_read_response(struct usbg_cmd *cmd)
 		ret = uasp_prepare_r_request(cmd);
 		if (ret)
 			goto out;
-		ret = usb_ep_queue(fu->ep_in, stream->req_in, GFP_ATOMIC);
+		ret = usb_ep_queue_timeout(fu->ep_in, stream->req_in,
+					   GFP_ATOMIC, STREAM_TIMEOUT_MS);
 		if (ret) {
 			pr_err("%s(%d) => %d\n", __func__, __LINE__, ret);
 			kfree(cmd->data_buf);
@@ -686,7 +696,8 @@ static int uasp_send_write_request(struct usbg_cmd *cmd)
 		ret = usbg_prepare_w_request(cmd, stream->req_out);
 		if (ret)
 			goto cleanup;
-		ret = usb_ep_queue(fu->ep_out, stream->req_out, GFP_ATOMIC);
+		ret = usb_ep_queue_timeout(fu->ep_out, stream->req_out,
+					   GFP_ATOMIC, STREAM_TIMEOUT_MS);
 		if (ret)
 			pr_err("%s(%d)\n", __func__, __LINE__);
 
-- 
2.1.1

[PATCH v7 03/10] usb: dwc3: gadget: handle stream events

[Anurag Kumar Vulisha]

When bulk streams are enabled, the udc/core.c starts
the timer for every request queued. This timer needs
to be deleted by the gadget driver when a valid stream
event is found. This is done to avoid the deadlock
situation which occurs when streams are enabled. This
patch modifies the code to delete the pending timer for
the request matching the streamid found in the event.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
---
 Changes in v7:
	1. This patch is newly added into this series
---
 drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9faad89..319a3ed 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2438,6 +2438,28 @@ static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,
 	__dwc3_gadget_start_isoc(dep);
 }
 
+static void dwc3_endpoint_stream_event(struct dwc3 *dwc,
+				       const struct dwc3_event_depevt *event)
+{
+	struct dwc3_ep		*dep;
+	struct dwc3_request	*req;
+	u8			epnum = event->endpoint_number;
+	u8			stream_id;
+
+	dep = dwc->eps[epnum];
+
+	stream_id = event->parameters;
+
+	/* Check for request matching the streamid and delete the timer */
+	list_for_each_entry(req, &dep->started_list, list) {
+		if (req->request.stream_id == stream_id) {
+			if (timer_pending(&req->request.req_timeout_timer))
+				del_timer(&req->request.req_timeout_timer);
+			break;
+		}
+	}
+}
+
 static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		const struct dwc3_event_depevt *event)
 {
@@ -2477,6 +2499,9 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		}
 		break;
 	case DWC3_DEPEVT_STREAMEVT:
+		if (event->status == DEPEVT_STREAMEVT_FOUND)
+			dwc3_endpoint_stream_event(dwc, event);
+
 	case DWC3_DEPEVT_XFERCOMPLETE:
 	case DWC3_DEPEVT_RXTXFIFOEVT:
 		break;
-- 
2.1.1

[PATCH v7 04/10] usb: dwc3: update stream id in depcmd

[Anurag Kumar Vulisha]

For stream capable endpoints, stream id related information
needs to be updated into DEPCMD while issuing START TRANSFER.
This patch does the same.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
---
 Changes in v7:
	1. Reverted to dep->stream_capable from dep->endpoint.stream_capable

 Changes in v6:
	1. Used dep->endpoint.stream_capable instead of dep->stream_capable
	   flag

 Changes in v5:
	1. None

 Changes in v4:
	1. None

 Changes in v3:
	1. None

 Changes in v2:
	1. None
---
 drivers/usb/dwc3/gadget.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 319a3ed..3171c24 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1232,6 +1232,9 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 		params.param1 = lower_32_bits(req->trb_dma);
 		cmd = DWC3_DEPCMD_STARTTRANSFER;
 
+		if (dep->stream_capable)
+			cmd |= DWC3_DEPCMD_PARAM(req->request.stream_id);
+
 		if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
 			cmd |= DWC3_DEPCMD_PARAM(dep->frame_number);
 	} else {
-- 
2.1.1

[PATCH v7 05/10] usb: dwc3: make controller clear transfer resources after complete

[Anurag Kumar Vulisha]

To start transfer with another stream id, controller needs to free
previously allocated transfer resource. This will be automatically
done by the controller at the time of XferComplete Event. This
patch updates the code to issue XferComplete event once all transfers
are done by setting LST bit in the ctrl field of the last TRB.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
---
 Changes in v7:
	1. Reverted to dep->stream_capable from dep->endpoint.stream_capable

 Changes in v6:
	1. Used dep->endpoint.stream_capable instead of dep->stream_capable
	   flag

 Changes in v5:
	1. None

 Changes in v4:
	1. None

 Changes in v3:
	1. Added the changes suggested by "Thinh Nguyen"

 Changes in v2:
	1. None
---
 drivers/usb/dwc3/gadget.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3171c24..3edfc0b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -579,7 +579,8 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
 
 	if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) {
 		params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE
-			| DWC3_DEPCFG_STREAM_EVENT_EN;
+			| DWC3_DEPCFG_STREAM_EVENT_EN
+			| DWC3_DEPCFG_XFER_COMPLETE_EN;
 		dep->stream_capable = true;
 	}
 
@@ -1005,6 +1006,15 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 	if (chain)
 		trb->ctrl |= DWC3_TRB_CTRL_CHN;
 
+	/*
+	 * To issue start transfer on another stream, controller need to free
+	 * previously acquired transfer resource. Setting the LST bit in
+	 * last TRB makes the controller clear transfer resource for that
+	 * endpoint, allowing to start another stream on that endpoint.
+	 */
+	else if (dep->stream_capable)
+		trb->ctrl |= DWC3_TRB_CTRL_LST;
+
 	if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
 		trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
 
@@ -2276,7 +2286,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_SHORT && !chain)
 		return 1;
 
-	if (event->status & DEPEVT_STATUS_IOC)
+	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
 		return 1;
 
 	return 0;
@@ -2487,6 +2497,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 	}
 
 	switch (event->endpoint_event) {
+	case DWC3_DEPEVT_XFERCOMPLETE:
+		if (!dep->stream_capable)
+			break;
+		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+		/* Fall Through */
 	case DWC3_DEPEVT_XFERINPROGRESS:
 		dwc3_gadget_endpoint_transfer_in_progress(dep, event);
 		break;
@@ -2505,7 +2520,6 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		if (event->status == DEPEVT_STREAMEVT_FOUND)
 			dwc3_endpoint_stream_event(dwc, event);
 
-	case DWC3_DEPEVT_XFERCOMPLETE:
 	case DWC3_DEPEVT_RXTXFIFOEVT:
 		break;
 	}
-- 
2.1.1

[PATCH v7 06/10] usb: dwc3: don't issue no-op trb for stream capable endpoints

[Anurag Kumar Vulisha]

The stream capable endpoints require stream id to be given
when issuing START TRANSFER. While issuing no-op trb the
stream id is not yet known, so don't issue no-op trb's on
stream capable endpoints.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
---
 Changes in v7:
	1. Reverted to dep->stream_capable from dep->endpoint.stream_capable

 Changes in v6:
	1. Replaced dep->stream_capable with dep->endpoint.stream_capable

 Changes in v5:
	1. None

 Changes in v4:
	1. None

 Changes in v3:
	1. None

 Changes in v2:
	1. None
---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3edfc0b..78c9bc6 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -673,7 +673,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 	 * Issue StartTransfer here with no-op TRB so we can always rely on No
 	 * Response Update Transfer command.
 	 */
-	if (usb_endpoint_xfer_bulk(desc) ||
+	if ((usb_endpoint_xfer_bulk(desc) && !dep->stream_capable) ||
 			usb_endpoint_xfer_int(desc)) {
 		struct dwc3_gadget_ep_cmd_params params;
 		struct dwc3_trb	*trb;
-- 
2.1.1

[PATCH v7 07/10] usb: dwc3: check for requests in started list for stream capable endpoints

[Anurag Kumar Vulisha]

For stream capable endpoints, uas layer can queue mulpile requests on
single ep with different stream ids. So, there can be multiple pending
requests waiting to be transferred. This patch changes the code to check
for any pending requests waiting to be transferred on ep started_list and
calls __dwc3_gadget_kick_transfer() if any.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
---
 Changes in v7:
	1. Reverted to dep->stream_capable from dep->endpoint.stream_caapable

 Changes in v6:
	1. Replaced dep->stream_capable with dep->endpoint.stream_caapable

 Changes in v5:
	1. None

 Changes in v4:
	1. None

 Changes in v3:
	1. None

 Changes in v2:
	1. None
---
 drivers/usb/dwc3/gadget.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 78c9bc6..d0de7dc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2413,6 +2413,9 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 
 	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
 
+	if (dep->stream_capable && !list_empty(&dep->started_list))
+		__dwc3_gadget_kick_transfer(dep);
+
 	if (stop) {
 		dwc3_stop_active_transfer(dep, true);
 		dep->flags = DWC3_EP_ENABLED;
-- 
2.1.1

[PATCH v7 08/10] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb()

[Anurag Kumar Vulisha]

Availability of TRB's is calculated using dwc3_calc_trbs_left(), which
determines total available TRB's based on the HWO bit set in a TRB.

In the present code, __dwc3_prepare_one_trb() is called with a TRB which
needs to be prepared for transfer. This __dwc3_prepare_one_trb() calls
dwc3_calc_trbs_left() to determine total available TRBs and set IOC bit
if the total available TRBs are zero. Since the present working TRB (which
is passed as an argument to __dwc3_prepare_one_trb() )  doesn't yet have
the HWO bit set before calling dwc3_calc_trbs_left(), there are chances
that dwc3_calc_trbs_left() wrongly calculates this present working TRB
as free(since the HWO bit is not yet set) and returns the total available
TRBs as greater than zero (including the present working TRB). This could
be a problem.

This patch corrects the above mentioned problem in __dwc3_prepare_one_trb()
by increementing the dep->trb_enqueue at the last (after preparing the TRB)
instead of increementing at the start and setting the IOC bit only if the
total available TRBs returned by dwc3_calc_trbs_left() is 1 . Since we are
increementing the dep->trb_enqueue at the last, the present working TRB is
also considered as available by dwc3_calc_trbs_left() and non zero value is
returned . So, according to the modified logic, when the total available
TRBs is equal to 1 that means the total available TRBs in the pool are 0.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
Tested-by: Tejas Joglekar <tejas.joglekar@synopsys.com>
---
 Changes in v7:
	1. None

 Changes in v6:
	1. None

 Changes in v5:
	1. None

 Changes in v4:
	1. Corrected the commit message as suggested by "Thinh Nguyen"

 Changes in v3:
	1. Corrected the logic for setting HWO bit as suggested by "Thinh Nguyen"

 Changes in v2:
	1. Changed the commit message
---
 drivers/usb/dwc3/gadget.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d0de7dc..9ddc9fd 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -920,8 +920,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 	struct usb_gadget	*gadget = &dwc->gadget;
 	enum usb_device_speed	speed = gadget->speed;
 
-	dwc3_ep_inc_enq(dep);
-
 	trb->size = DWC3_TRB_SIZE_LENGTH(length);
 	trb->bpl = lower_32_bits(dma);
 	trb->bph = upper_32_bits(dma);
@@ -1000,7 +998,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 	}
 
 	if ((!no_interrupt && !chain) ||
-			(dwc3_calc_trbs_left(dep) == 0))
+			(dwc3_calc_trbs_left(dep) == 1))
 		trb->ctrl |= DWC3_TRB_CTRL_IOC;
 
 	if (chain)
@@ -1020,6 +1018,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 
 	trb->ctrl |= DWC3_TRB_CTRL_HWO;
 
+	dwc3_ep_inc_enq(dep);
+
 	trace_dwc3_prepare_trb(dep, trb);
 }
 
-- 
2.1.1

[PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Anurag Kumar Vulisha]

The present code in dwc3_gadget_ep_reclaim_completed_trb() will check
for IOC/LST bit in the event->status and returns if IOC/LST bit is
set. This logic doesn't work if multiple TRBs are queued per
request and the IOC/LST bit is set on the last TRB of that request.
Consider an example where a queued request has multiple queued TRBs
and IOC/LST bit is set only for the last TRB. In this case, the Core
generates XferComplete/XferInProgress events only for the last TRB
(since IOC/LST are set only for the last TRB). As per the logic in
dwc3_gadget_ep_reclaim_completed_trb() event->status is checked for
IOC/LST bit and returns on the first TRB. This makes the remaining
TRBs left unhandled.
To aviod this, changed the code to check for IOC/LST bits in both
event->status & TRB->ctrl. This patch does the same.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
Tested-by: Tejas Joglekar <tejas.joglekar@synopsys.com>
---
 Changes in v7:
	1. None

 Changes in v6:
	1. None

 Changes in v5:
	1. None

 Changes in v4:
	1. None

 Changes in v3:
	1. None

 Changes in v2:
	1. None
---
 drivers/usb/dwc3/gadget.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9ddc9fd..216179e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2286,7 +2286,12 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_SHORT && !chain)
 		return 1;
 
-	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
+	if ((event->status & DEPEVT_STATUS_IOC) &&
+	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
+		return 1;
+
+	if ((event->status & DEPEVT_STATUS_LST) &&
+	    (trb->ctrl & DWC3_TRB_CTRL_LST))
 		return 1;
 
 	return 0;
-- 
2.1.1

[PATCH v7 10/10] usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints

[Anurag Kumar Vulisha]

When streaming is enabled on BULK endpoints and LST bit is set
observed MISSED ISOC bit set in event->status for BULK ep. Since
this bit is only valid for isocronous endpoints, changed the code
to check for isocrnous endpoints when MISSED ISOC bit is set.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
Tested-by: Tejas Joglekar <tejas.joglekar@synopsys.com>
---
 Changes in v7:
	1. None

 Changes in v6:
	1. None

 Changes in v5:
	1. None

 Changes in v4:
	1. None

 Changes in v3:
	1. None

 Changes in v2:
	1. None
---
 drivers/usb/dwc3/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 216179e..5d5c572 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2409,7 +2409,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_BUSERR)
 		status = -ECONNRESET;
 
-	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
+	if ((event->status & DEPEVT_STATUS_MISSED_ISOC) &&
+	    usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
 		status = -EXDEV;
 
 		if (list_empty(&dep->started_list))
-- 
2.1.1

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

[Alan Stern]

On Sat, 1 Dec 2018, Anurag Kumar Vulisha wrote:

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

I find this whole approach rather dubious.

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.

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.

Alan Stern

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

[Anurag Kumar Vulisha]

Hi Alan,

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>Sent: Sunday, December 02, 2018 10:06 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>; Manu
>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>; linux-
>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.com>
>Subject: Re: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests
>
>On Sat, 1 Dec 2018, Anurag Kumar Vulisha wrote:
>
>> 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.
>
>I find this whole approach rather dubious.
>
>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 where
the gadget controller is waiting on Host controller to issue PRIME transaction
and  Host controller is waiting on gadget to issue ERDY transaction. Since
the stream protocol is gadget driven, the host may not proceed further until it
receives a valid Start Stream (ERDY) transaction from gadget. Since the gadget
controller driver is aware that the controller is stuck , makes it responsible
to recover the controller from hang condition by restarting the transfer (which
triggers the controller FSM to issue ERDY to host).

>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 the
resubmission of the request should only be done from the class driver and 
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.

Best Regards,
Anurag Kumar Vulisha

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

[Alan Stern]

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 where
> the gadget controller is waiting on Host controller to issue PRIME transaction
> and  Host controller is waiting on gadget to issue ERDY transaction. Since
> the stream protocol is gadget driven, the host may not proceed further until 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?

> Since the gadget
> controller driver is aware that the controller is stuck , makes it responsible
> 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?

> >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 the
> resubmission of the request should only be done from the class driver and 
> 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.

Alan Stern

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

[Anurag Kumar Vulisha]

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>; Manu
>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>; linux-
>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.com>
>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 where
>> the gadget controller is waiting on Host controller to issue PRIME transaction
>> and  Host controller is waiting on gadget to issue ERDY transaction. Since
>> the stream protocol is gadget driven, the host may not proceed further until 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 endpoints using bulk
streaming protocol and may not occur with normal endpoints. AFAIK bulk streaming is
gadget driven, where the gadget is allowed to select which stream id transfer the host
should work on . Since the host doesn't have control on when the transfer would be
selected by gadget, it may wait for longer timeouts before cancelling the transfer. 

>> Since the gadget
>> controller driver is aware that the controller is stuck , makes it responsible
>> 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, which
cleans all the hardware resource allocated for that endpoint. This also resets the
hardware FSMs for that endpoint . So, when re-queuing of the transfer happens
the controller starts allocating hardware resources again, thus avoiding the probability
of entering into the issue. I am not sure of other controllers, but for dwc3, issuing
the stop transfer is the only way to handle this issue. 

@Felipe:  Can you please provide your suggestion on this.  
  
>> >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 the
>> resubmission of the request should only be done from the class driver and
>> 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 discussed 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 decides
the selection of the stream id on which the host should work , so taking all these into
consideration I kept 50ms timeout for stream transfers, so that the performance may
not get decreased.

Thanks,
Anurag Kumar Vulisha

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

[Alan Stern]

On Mon, 3 Dec 2018, Anurag Kumar Vulisha wrote:

> >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 where
> >> the gadget controller is waiting on Host controller to issue PRIME transaction
> >> and  Host controller is waiting on gadget to issue ERDY transaction. Since
> >> the stream protocol is gadget driven, the host may not proceed further until 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 endpoints using bulk
> streaming protocol and may not occur with normal endpoints. AFAIK bulk streaming is
> gadget driven, where the gadget is allowed to select which stream id transfer the host
> should work on . Since the host doesn't have control on when the transfer would be
> selected by gadget, it may wait for longer timeouts before cancelling the transfer. 

You're missing the point.  Although the device selects which stream ID
gets transferred, the _host_ decides whether a stream transfer should
occur in the first place.  No matter how many ERDY packets the device
controller tries to send, no transfer will occur until the host wants
to do it.

In this sense, stream transfers (like all other USB interactions except
wakeup requests) are host-driven.

> >> Since the gadget
> >> controller driver is aware that the controller is stuck , makes it responsible
> >> 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, which
> cleans all the hardware resource allocated for that endpoint. This also resets the
> hardware FSMs for that endpoint . So, when re-queuing of the transfer happens
> the controller starts allocating hardware resources again, thus avoiding the probability
> of entering into the issue. I am not sure of other controllers, but for dwc3, issuing
> the stop transfer is the only way to handle this issue. 

Again you're missing the point.  Can't the controller driver issue the
Stop Transfer command but still consider the request to be in progress
(i.e., don't dequeue the request) so that the gadget driver's
completion callback isn't invoked and the request does not need to be
explicitly resubmitted?

> @Felipe:  Can you please provide your suggestion on this.  

> >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 discussed above
> this issue was observed in streaming protocol , which  is very much faster than normal
> BOT modes and it works on super speed .

Although USB mass storage is currently the only user of the stream 
protocol, that may not be true in the future.  You should think in more 
general terms.  A timeout which is appropriate for mass storage may not 
be appropriate for some other application.

Alan Stern

>  More over the gadget controller decides
> the selection of the stream id on which the host should work , so taking all these into
> consideration I kept 50ms timeout for stream transfers, so that the performance may
> not get decreased.
> 
> Thanks,
> Anurag Kumar Vulisha

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

[Anurag Kumar Vulisha]

Hi Alan,

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>Sent: Tuesday, December 04, 2018 4:38 AM
>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; 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-kernel@vger.kernel.org; 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
>
>On Mon, 3 Dec 2018, Anurag Kumar Vulisha wrote:
>
>> >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 where
>> >> the gadget controller is waiting on Host controller to issue PRIME transaction
>> >> and  Host controller is waiting on gadget to issue ERDY transaction. Since
>> >> the stream protocol is gadget driven, the host may not proceed further until 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 endpoints using
>bulk
>> streaming protocol and may not occur with normal endpoints. AFAIK bulk streaming
>is
>> gadget driven, where the gadget is allowed to select which stream id transfer the
>host
>> should work on . Since the host doesn't have control on when the transfer would be
>> selected by gadget, it may wait for longer timeouts before cancelling the transfer.
>
>You're missing the point.  Although the device selects which stream ID
>gets transferred, the _host_ decides whether a stream transfer should
>occur in the first place.  No matter how many ERDY packets the device
>controller tries to send, no transfer will occur until the host wants
>to do it.
>
>In this sense, stream transfers (like all other USB interactions except
>wakeup requests) are host-driven.
>

I agree with you that without host willing to transfer, the transfer wouldn't have
started and also agree the host always has the capability of detecting the hang. But
we are not sure on what host platform and operating system the gadget would be
tested and also not sure whether the host software is capable of handling the deadlock.
Even if the host has a timer , it would be very long and waiting for the timer to get
timedout  would reduce the performance. In this patch we are giving the user the
flexibility to opt the timeout value, so that the deadlock can be avoided without
effecting the performance. So I think adding the timer in gadget is more easier solution
than handling in host. I have seen this workaround working for both linux & windows.

>> >> Since the gadget
>> >> controller driver is aware that the controller is stuck , makes it responsible
>> >> 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, which
>> cleans all the hardware resource allocated for that endpoint. This also resets the
>> hardware FSMs for that endpoint . So, when re-queuing of the transfer happens
>> the controller starts allocating hardware resources again, thus avoiding the
>probability
>> of entering into the issue. I am not sure of other controllers, but for dwc3, issuing
>> the stop transfer is the only way to handle this issue.
>
>Again you're missing the point.  Can't the controller driver issue the
>Stop Transfer command but still consider the request to be in progress
>(i.e., don't dequeue the request) so that the gadget driver's
>completion callback isn't invoked and the request does not need to be
>explicitly resubmitted?
>

Yes , what you are saying is correct. My initial patches were following the
same approach that you have suggested. We switched to the dequeue
approach because there can be different gadgets which may enter into
this issue and it would be better to follow a generic approach rather than
having every controller driver implementing their own workaround.
Please find the conservation in this link  https://patchwork.kernel.org/patch/10640145/
   
>> @Felipe:  Can you please provide your suggestion on this.
>
>> >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 discussed above
>> this issue was observed in streaming protocol , which  is very much faster than
>normal
>> BOT modes and it works on super speed .
>
>Although USB mass storage is currently the only user of the stream
>protocol, that may not be true in the future.  You should think in more
>general terms.  A timeout which is appropriate for mass storage may not
>be appropriate for some other application.
>

You are correct , this timeout may not be ideal. Since  I was not able to reproduce this issue
on non-stream capable transfers , at  present the only user of usb_ep_queue_timeout() API
is the streaming gadget.  As we are not aware of the optimal timeout value,  instead of
hardcoding the timeout value we can add module_param() for it.  So that the user can configure
timeout based on their use case. Please let me know your suggestion on this.

Thanks,
Anurag Kumar Vulisha 

>>  More over the gadget controller decides
>> the selection of the stream id on which the host should work , so taking all these
>into
>> consideration I kept 50ms timeout for stream transfers, so that the performance
>may
>> not get decreased.
>>
>> Thanks,
>> Anurag Kumar Vulisha

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

[Alan Stern]

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

> >> Yes the host can cancel the transfer. This issue originated from the endpoints using
> >bulk
> >> streaming protocol and may not occur with normal endpoints. AFAIK bulk streaming
> >is
> >> gadget driven, where the gadget is allowed to select which stream id transfer the
> >host
> >> should work on . Since the host doesn't have control on when the transfer would be
> >> selected by gadget, it may wait for longer timeouts before cancelling the transfer.
> >
> >You're missing the point.  Although the device selects which stream ID
> >gets transferred, the _host_ decides whether a stream transfer should
> >occur in the first place.  No matter how many ERDY packets the device
> >controller tries to send, no transfer will occur until the host wants
> >to do it.
> >
> >In this sense, stream transfers (like all other USB interactions except
> >wakeup requests) are host-driven.
> >
> 
> I agree with you that without host willing to transfer, the transfer wouldn't have
> started and also agree the host always has the capability of detecting the hang. But
> we are not sure on what host platform and operating system the gadget would be
> tested and also not sure whether the host software is capable of handling the deadlock.
> Even if the host has a timer , it would be very long and waiting for the timer to get
> timedout  would reduce the performance. In this patch we are giving the user the
> flexibility to opt the timeout value, so that the deadlock can be avoided without
> effecting the performance. So I think adding the timer in gadget is more easier solution
> than handling in host. I have seen this workaround working for both linux & windows.

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?

> >> >> Since the gadget
> >> >> controller driver is aware that the controller is stuck , makes it responsible
> >> >> 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, which
> >> cleans all the hardware resource allocated for that endpoint. This also resets the
> >> hardware FSMs for that endpoint . So, when re-queuing of the transfer happens
> >> the controller starts allocating hardware resources again, thus avoiding the
> >probability
> >> of entering into the issue. I am not sure of other controllers, but for dwc3, issuing
> >> the stop transfer is the only way to handle this issue.
> >
> >Again you're missing the point.  Can't the controller driver issue the
> >Stop Transfer command but still consider the request to be in progress
> >(i.e., don't dequeue the request) so that the gadget driver's
> >completion callback isn't invoked and the request does not need to be
> >explicitly resubmitted?
> >
> 
> Yes , what you are saying is correct. My initial patches were following the
> same approach that you have suggested. We switched to the dequeue
> approach because there can be different gadgets which may enter into
> this issue and it would be better to follow a generic approach rather than
> having every controller driver implementing their own workaround.
> Please find the conservation in this link  https://patchwork.kernel.org/patch/10640145/

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.

> >> @Felipe:  Can you please provide your suggestion on this.
> >
> >> >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 discussed above
> >> this issue was observed in streaming protocol , which  is very much faster than
> >normal
> >> BOT modes and it works on super speed .
> >
> >Although USB mass storage is currently the only user of the stream
> >protocol, that may not be true in the future.  You should think in more
> >general terms.  A timeout which is appropriate for mass storage may not
> >be appropriate for some other application.
> >
> 
> You are correct , this timeout may not be ideal. Since  I was not able to reproduce this issue
> on non-stream capable transfers , at  present the only user of usb_ep_queue_timeout() API
> is the streaming gadget.  As we are not aware of the optimal timeout value,  instead of
> hardcoding the timeout value we can add module_param() for it.  So that the user can configure
> timeout based on their use case. Please let me know your suggestion on this.

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.

Alan Stern

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

[Anurag Kumar Vulisha]

Hi Alan,

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>Sent: Tuesday, December 04, 2018 10:17 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>; Manu
>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>; linux-
>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.com>
>Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests
>
>On Tue, 4 Dec 2018, Anurag Kumar Vulisha wrote:
>
>> >> Yes the host can cancel the transfer. This issue originated from
>> >> the endpoints using
>> >bulk
>> >> streaming protocol and may not occur with normal endpoints. AFAIK
>> >> bulk streaming
>> >is
>> >> gadget driven, where the gadget is allowed to select which stream
>> >> id transfer the
>> >host
>> >> should work on . Since the host doesn't have control on when the
>> >> transfer would be selected by gadget, it may wait for longer timeouts before
>cancelling the transfer.
>> >
>> >You're missing the point.  Although the device selects which stream
>> >ID gets transferred, the _host_ decides whether a stream transfer
>> >should occur in the first place.  No matter how many ERDY packets the
>> >device controller tries to send, no transfer will occur until the
>> >host wants to do it.
>> >
>> >In this sense, stream transfers (like all other USB interactions
>> >except wakeup requests) are host-driven.
>> >
>>
>> I agree with you that without host willing to transfer, the transfer
>> wouldn't have started and also agree the host always has the
>> capability of detecting the hang. But we are not sure on what host
>> platform and operating system the gadget would be tested and also not sure
>whether the host software is capable of handling the deadlock.
>> Even if the host has a timer , it would be very long and waiting for
>> the timer to get timedout  would reduce the performance. In this patch
>> we are giving the user the flexibility to opt the timeout value, so
>> that the deadlock can be avoided without effecting the performance. So
>> I think adding the timer in gadget is more easier solution than handling in host. I
>have seen this workaround working for both linux & windows.
>
>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.

>> >> >> Since the gadget
>> >> >> controller driver is aware that the controller is stuck , makes
>> >> >> it responsible 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, which cleans all the hardware resource allocated for
>> >> that endpoint. This also resets the hardware FSMs for that endpoint
>> >> . So, when re-queuing of the transfer happens the controller starts
>> >> allocating hardware resources again, thus avoiding the
>> >probability
>> >> of entering into the issue. I am not sure of other controllers, but
>> >> for dwc3, issuing the stop transfer is the only way to handle this issue.
>> >
>> >Again you're missing the point.  Can't the controller driver issue
>> >the Stop Transfer command but still consider the request to be in
>> >progress (i.e., don't dequeue the request) so that the gadget
>> >driver's completion callback isn't invoked and the request does not
>> >need to be explicitly resubmitted?
>> >
>>
>> Yes , what you are saying is correct. My initial patches were
>> following the same approach that you have suggested. We switched to
>> the dequeue approach because there can be different gadgets which may
>> enter into this issue and it would be better to follow a generic
>> approach rather than having every controller driver implementing their own
>workaround.
>> Please find the conservation in this link
>> https://patchwork.kernel.org/patch/10640145/
>
>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.

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.
  
>
>> >> @Felipe:  Can you please provide your suggestion on this.
>> >
>> >> >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 discussed above this issue was observed in
>> >> streaming protocol , which  is very much faster than
>> >normal
>> >> BOT modes and it works on super speed .
>> >
>> >Although USB mass storage is currently the only user of the stream
>> >protocol, that may not be true in the future.  You should think in
>> >more general terms.  A timeout which is appropriate for mass storage
>> >may not be appropriate for some other application.
>> >
>>
>> You are correct , this timeout may not be ideal. Since  I was not able
>> to reproduce this issue on non-stream capable transfers , at  present
>> the only user of usb_ep_queue_timeout() API is the streaming gadget.
>> As we are not aware of the optimal timeout value,  instead of
>> hardcoding the timeout value we can add module_param() for it.  So that the user
>can configure timeout based on their use case. Please let me know your suggestion
>on this.
>
>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. 

Thanks,
Anurag Kumar Vulisha 

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

[Alan Stern]

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

Re: [PATCH v7 05/10] usb: dwc3: make controller clear transfer resources after complete

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]


Hi,

Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> writes:
> @@ -2487,6 +2497,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>  	}
>  
>  	switch (event->endpoint_event) {
> +	case DWC3_DEPEVT_XFERCOMPLETE:
> +		if (!dep->stream_capable)
> +			break;
> +		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> +		/* Fall Through */

instead, let's add a proper handler for this:

dwc3_gadget_endpoint_transfer_complete(dep, event);

That handler should be "self-sufficient". IOW, we shouldn't have this
fall through here. This means that the other patch where you modify
dwc3_gadget_transfer_in_progress() shouldn't be necessary, since that
event shouldn't run for stream capable endpoints.

While rewriting the patches, please rebase on my testing/next as I have
applied a few of the patches in this series.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]


Hi,

Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> writes:
> The present code in dwc3_gadget_ep_reclaim_completed_trb() will check
> for IOC/LST bit in the event->status and returns if IOC/LST bit is
> set. This logic doesn't work if multiple TRBs are queued per
> request and the IOC/LST bit is set on the last TRB of that request.
> Consider an example where a queued request has multiple queued TRBs
> and IOC/LST bit is set only for the last TRB. In this case, the Core
> generates XferComplete/XferInProgress events only for the last TRB
> (since IOC/LST are set only for the last TRB). As per the logic in
> dwc3_gadget_ep_reclaim_completed_trb() event->status is checked for
> IOC/LST bit and returns on the first TRB. This makes the remaining
> TRBs left unhandled.
> To aviod this, changed the code to check for IOC/LST bits in both
> event->status & TRB->ctrl. This patch does the same.
>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
> Tested-by: Tejas Joglekar <tejas.joglekar@synopsys.com>
> ---
>  Changes in v7:
> 	1. None
>
>  Changes in v6:
> 	1. None
>
>  Changes in v5:
> 	1. None
>
>  Changes in v4:
> 	1. None
>
>  Changes in v3:
> 	1. None
>
>  Changes in v2:
> 	1. None
> ---
>  drivers/usb/dwc3/gadget.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 9ddc9fd..216179e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2286,7 +2286,12 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>  		return 1;
>  
> -	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
> +	if ((event->status & DEPEVT_STATUS_IOC) &&
> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
> +		return 1;

this shouldn't be necessary. According to databook, event->status
contains the bits from the completed TRB.  Which means that
event->status & IOC will always be equal to trb->ctrl & IOC.

Can you further describe the situation here? Perhaps share tracepoints
exposing the problem?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Anurag Kumar Vulisha]

Hi Alan,

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>Sent: Wednesday, December 05, 2018 12:59 AM
>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; 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-kernel@vger.kernel.org; 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
>
>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.
>

Both host & gadget are equally responsible for this issue and it may potentially occur
with other controllers also(which are incapable of handling this situation) . Because of
this reason I would not call this issue as a dwc3 alone bug. One thing is that, with dwc3 the
issue is easily reproduced. Because of these reasons I feel that it would be better to have
a generic udc timers rather than having driver specific workaround. We had this same
discussion earlier in this mailing list https://lkml.org/lkml/2018/10/9/638

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

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

I agree with you, but the class drivers have to implement their own logic instead of
having a generic logic to implement the timeouts. 

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

Thanks for suggesting. 1 sec timeout should be fair enough. I will change this
in next series 

Best Regards,
Anurag Kumar Vulisha

RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Anurag Kumar Vulisha]

Hi Felipe,
>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Wednesday, December 05, 2018 2:38 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Shuah Khan <shuah@kernel.org>; Alan Stern
><stern@rowland.harvard.edu>; 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; Bart Van Assche <bvanassche@acm.org>; Mike
>Christie <mchristi@redhat.com>; Matthew Wilcox <willy@infradead.org>; Colin Ian
>King <colin.king@canonical.com>
>Cc: linux-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.com>;
>Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: Re: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>and TRB->ctrl fields
>
>
>Hi,
>
>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> writes:
>> The present code in dwc3_gadget_ep_reclaim_completed_trb() will check
>> for IOC/LST bit in the event->status and returns if IOC/LST bit is
>> set. This logic doesn't work if multiple TRBs are queued per
>> request and the IOC/LST bit is set on the last TRB of that request.
>> Consider an example where a queued request has multiple queued TRBs
>> and IOC/LST bit is set only for the last TRB. In this case, the Core
>> generates XferComplete/XferInProgress events only for the last TRB
>> (since IOC/LST are set only for the last TRB). As per the logic in
>> dwc3_gadget_ep_reclaim_completed_trb() event->status is checked for
>> IOC/LST bit and returns on the first TRB. This makes the remaining
>> TRBs left unhandled.
>> To aviod this, changed the code to check for IOC/LST bits in both
>> event->status & TRB->ctrl. This patch does the same.
>>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>> Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
>> Tested-by: Tejas Joglekar <tejas.joglekar@synopsys.com>
>> ---
>>  Changes in v7:
>> 	1. None
>>
>>  Changes in v6:
>> 	1. None
>>
>>  Changes in v5:
>> 	1. None
>>
>>  Changes in v4:
>> 	1. None
>>
>>  Changes in v3:
>> 	1. None
>>
>>  Changes in v2:
>> 	1. None
>> ---
>>  drivers/usb/dwc3/gadget.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 9ddc9fd..216179e 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2286,7 +2286,12 @@ static int
>dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>  		return 1;
>>
>> -	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>> +		return 1;
>
>this shouldn't be necessary. According to databook, event->status
>contains the bits from the completed TRB.  Which means that
>event->status & IOC will always be equal to trb->ctrl & IOC.
>
Thanks for reviewing this patch. Lets consider an example where a request
has num_sgs > 0 and each sg is mapped to a TRB and the last TRB has the
IOC bit set. Once the controller is done with the transfer, it  generates 
XferInProgress for the last TRB (since IOC bit is set). As a part of trb reclaim
process  dwc3_gadget_ep_reclaim_trb_sg() calls
dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
the event already has the IOC bit set, the loop is exited from the loop at the
very first TRB and the remaining TRBs (mapped to the sglist) are left unhandled.
To avoid this we modified the code to exit only if both TRB & event has the IOC
bit set.
 
>Can you further describe the situation here? Perhaps share tracepoints
>exposing the problem?

Sure, will capture the traces and reply back

Thanks,
Anurag Kumar Vulisha

RE: [PATCH v7 05/10] usb: dwc3: make controller clear transfer resources after complete

[Anurag Kumar Vulisha]

HI Felipe,
>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Wednesday, December 05, 2018 2:32 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Shuah Khan <shuah@kernel.org>; Alan Stern
><stern@rowland.harvard.edu>; 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; Bart Van Assche <bvanassche@acm.org>; Mike
>Christie <mchristi@redhat.com>; Matthew Wilcox <willy@infradead.org>; Colin Ian
>King <colin.king@canonical.com>
>Cc: linux-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.com>;
>Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: Re: [PATCH v7 05/10] usb: dwc3: make controller clear transfer resources
>after complete
>
>
>Hi,
>
>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> writes:
>> @@ -2487,6 +2497,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>>  	}
>>
>>  	switch (event->endpoint_event) {
>> +	case DWC3_DEPEVT_XFERCOMPLETE:
>> +		if (!dep->stream_capable)
>> +			break;
>> +		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>> +		/* Fall Through */
>
>instead, let's add a proper handler for this:
>
>dwc3_gadget_endpoint_transfer_complete(dep, event);
>
>That handler should be "self-sufficient". IOW, we shouldn't have this
>fall through here. This means that the other patch where you modify
>dwc3_gadget_transfer_in_progress() shouldn't be necessary, since that
>event shouldn't run for stream capable endpoints.
>

Thanks for  your suggestion, will implement the changes as said and send
the patches

>While rewriting the patches, please rebase on my testing/next as I have
>applied a few of the patches in this series. 

Okay , will rebase on top of testing/next and send the patches

Best Regards,
Anurag Kumar Vulisha

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

[Felipe Balbi]

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

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

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

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.

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.

-- 
balbi

RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Felipe Balbi]

Hi,

Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>> @@ -2286,7 +2286,12 @@ static int
>>dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>>  		return 1;
>>>
>>> -	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>> +		return 1;
>>
>>this shouldn't be necessary. According to databook, event->status
>>contains the bits from the completed TRB.  Which means that
>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>
> Thanks for reviewing this patch. Lets consider an example where a request
> has num_sgs > 0 and each sg is mapped to a TRB and the last TRB has the
> IOC bit set. Once the controller is done with the transfer, it  generates 
> XferInProgress for the last TRB (since IOC bit is set). As a part of trb reclaim
> process  dwc3_gadget_ep_reclaim_trb_sg() calls
> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
> the event already has the IOC bit set, the loop is exited from the loop at the
> very first TRB and the remaining TRBs (mapped to the sglist) are left unhandled.
> To avoid this we modified the code to exit only if both TRB & event has the IOC
> bit set.

Seems like IOC case should just test for chain flag as well:

modified   drivers/usb/dwc3/gadget.c
@@ -2372,7 +2372,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_SHORT && !chain)
 		return 1;
 
-	if (event->status & DEPEVT_STATUS_IOC)
+	if (event->status & DEPEVT_STATUS_IOC && !chain)
 		return 1;
 
 	return 0;

-- 
balbi

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

[Alan Stern]

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

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

RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Anurag Kumar Vulisha]

HI Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Friday, December 07, 2018 11:42 AM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Shuah Khan <shuah@kernel.org>; Alan Stern
><stern@rowland.harvard.edu>; 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; Bart Van Assche <bvanassche@acm.org>; Mike
>Christie <mchristi@redhat.com>; Matthew Wilcox <willy@infradead.org>; Colin Ian
>King <colin.king@canonical.com>
>Cc: linux-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.com>
>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>and TRB->ctrl fields
>
>
>Hi,
>
>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>>> @@ -2286,7 +2286,12 @@ static int
>>>dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>>>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>>>  		return 1;
>>>>
>>>> -	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
>>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>>> +		return 1;
>>>
>>>this shouldn't be necessary. According to databook, event->status
>>>contains the bits from the completed TRB.  Which means that
>>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>>
>> Thanks for reviewing this patch. Lets consider an example where a
>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>> TRB has the IOC bit set. Once the controller is done with the
>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>> is set). As a part of trb reclaim process
>> dwc3_gadget_ep_reclaim_trb_sg() calls
>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>> the event already has the IOC bit set, the loop is exited from the
>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>unhandled.
>> To avoid this we modified the code to exit only if both TRB & event
>> has the IOC bit set.
>
>Seems like IOC case should just test for chain flag as well:
>

Okay. Along with this logic the code for updating chain bit should also be modified I guess.
Since the IOC bit is also set when there are not enough TRBs available, the code should be
modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update below
changes along with your suggestions and resend the patches.

@@ -998,7 +998,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
                        (dwc3_calc_trbs_left(dep) == 1))
                trb->ctrl |= DWC3_TRB_CTRL_IOC;
 
-       if (chain)
+       if (chain && !(trb->ctrl & DWC3_TRB_CTRL_IOC))
                trb->ctrl |= DWC3_TRB_CTRL_CHN;
 
        if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
@@ -2372,7 +2372,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
        if (event->status & DEPEVT_STATUS_SHORT && !chain)
                return 1;
 
-       if (event->status & DEPEVT_STATUS_IOC)
+       if (event->status & DEPEVT_STATUS_IOC && !chain)
                return 1;
 
        return 0;
@@ -2399,7 +2399,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
                req->num_pending_sgs--;
 
                ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
-                               trb, event, status, true);
+                               trb, event, status, (trb & DWC3_TRB_CTRL_CHN));
                if (ret)
                        break;
        }

Thanks,
Anurag Kumar Vulisha

>modified   drivers/usb/dwc3/gadget.c
>@@ -2372,7 +2372,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct
>dwc3_ep *dep,
> 	if (event->status & DEPEVT_STATUS_SHORT && !chain)
> 		return 1;
>
>-	if (event->status & DEPEVT_STATUS_IOC)
>+	if (event->status & DEPEVT_STATUS_IOC && !chain)
> 		return 1;
>
> 	return 0;
>
>--
>balbi

RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3256 bytes --]


Hi,

Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
> HI Felipe,
>
>>-----Original Message-----
>>From: Felipe Balbi [mailto:balbi@kernel.org]
>>Sent: Friday, December 07, 2018 11:42 AM
>>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
>><gregkh@linuxfoundation.org>; Shuah Khan <shuah@kernel.org>; Alan Stern
>><stern@rowland.harvard.edu>; 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; Bart Van Assche <bvanassche@acm.org>; Mike
>>Christie <mchristi@redhat.com>; Matthew Wilcox <willy@infradead.org>; Colin Ian
>>King <colin.king@canonical.com>
>>Cc: linux-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.com>
>>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>>and TRB->ctrl fields
>>
>>
>>Hi,
>>
>>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>>>> @@ -2286,7 +2286,12 @@ static int
>>>>dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>>>>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>>>>  		return 1;
>>>>>
>>>>> -	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
>>>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>>>> +		return 1;
>>>>
>>>>this shouldn't be necessary. According to databook, event->status
>>>>contains the bits from the completed TRB.  Which means that
>>>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>>>
>>> Thanks for reviewing this patch. Lets consider an example where a
>>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>>> TRB has the IOC bit set. Once the controller is done with the
>>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>>> is set). As a part of trb reclaim process
>>> dwc3_gadget_ep_reclaim_trb_sg() calls
>>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>>> the event already has the IOC bit set, the loop is exited from the
>>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>>unhandled.
>>> To avoid this we modified the code to exit only if both TRB & event
>>> has the IOC bit set.
>>
>>Seems like IOC case should just test for chain flag as well:
>>
>
> Okay. Along with this logic the code for updating chain bit should also be modified I guess.

not really

> Since the IOC bit is also set when there are not enough TRBs available, the code should be
> modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update below
> changes along with your suggestions and resend the patches.

no. Actually I don't think we're allowed to split a scatter/gather like
that. I did that quite a while ago, but I don't think we're allowed to
do so. What we should do, in that case, is not even queue that request
until we have enough for all members of the scatter/gather. But that's a
separate patch, anyway.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Anurag Kumar Vulisha]

Hi Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Monday, December 10, 2018 12:24 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Shuah Khan <shuah@kernel.org>; Alan Stern
><stern@rowland.harvard.edu>; 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; Bart Van Assche <bvanassche@acm.org>; Mike
>Christie <mchristi@redhat.com>; Matthew Wilcox <willy@infradead.org>; Colin Ian
>King <colin.king@canonical.com>
>Cc: linux-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.com>
>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>and TRB->ctrl fields
>
>
>Hi,
>
>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>> HI Felipe,
>>
>>>-----Original Message-----
>>>From: Felipe Balbi [mailto:balbi@kernel.org]
>>>Sent: Friday, December 07, 2018 11:42 AM
>>>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
>>><gregkh@linuxfoundation.org>; Shuah Khan <shuah@kernel.org>; Alan Stern
>>><stern@rowland.harvard.edu>; 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; Bart Van Assche <bvanassche@acm.org>; Mike
>>>Christie <mchristi@redhat.com>; Matthew Wilcox <willy@infradead.org>; Colin
>Ian
>>>King <colin.king@canonical.com>
>>>Cc: linux-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.com>
>>>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event-
>>status
>>>and TRB->ctrl fields
>>>
>>>
>>>Hi,
>>>
>>>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>>>>> @@ -2286,7 +2286,12 @@ static int
>>>>>dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>>>>>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>>>>>  		return 1;
>>>>>>
>>>>>> -	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
>>>>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>>>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>>>>> +		return 1;
>>>>>
>>>>>this shouldn't be necessary. According to databook, event->status
>>>>>contains the bits from the completed TRB.  Which means that
>>>>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>>>>
>>>> Thanks for reviewing this patch. Lets consider an example where a
>>>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>>>> TRB has the IOC bit set. Once the controller is done with the
>>>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>>>> is set). As a part of trb reclaim process
>>>> dwc3_gadget_ep_reclaim_trb_sg() calls
>>>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>>>> the event already has the IOC bit set, the loop is exited from the
>>>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>>>unhandled.
>>>> To avoid this we modified the code to exit only if both TRB & event
>>>> has the IOC bit set.
>>>
>>>Seems like IOC case should just test for chain flag as well:
>>>
>>
>> Okay. Along with this logic the code for updating chain bit should also be modified I
>guess.
>
>not really
>
>> Since the IOC bit is also set when there are not enough TRBs available, the code
>should be
>> modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update
>below
>> changes along with your suggestions and resend the patches.
>
>no. Actually I don't think we're allowed to split a scatter/gather like
>that. I did that quite a while ago, but I don't think we're allowed to
>do so. What we should do, in that case, is not even queue that request
>until we have enough for all members of the scatter/gather. But that's a
>separate patch, anyway.
>

Okay. I have a doubt here, not pushing the request until all sgs are mapped to enough TRBs
might remove the driver complexity but reduce the performance (since we are waiting
until enough TRBs are available). Are we okay with that?  

Thanks,
Anurag Kumar Vulisha  

RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2386 bytes --]


Hi,

Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>>>> Thanks for reviewing this patch. Lets consider an example where a
>>>>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>>>>> TRB has the IOC bit set. Once the controller is done with the
>>>>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>>>>> is set). As a part of trb reclaim process
>>>>> dwc3_gadget_ep_reclaim_trb_sg() calls
>>>>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>>>>> the event already has the IOC bit set, the loop is exited from the
>>>>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>>>>unhandled.
>>>>> To avoid this we modified the code to exit only if both TRB & event
>>>>> has the IOC bit set.
>>>>
>>>>Seems like IOC case should just test for chain flag as well:
>>>>
>>>
>>> Okay. Along with this logic the code for updating chain bit should also be modified I
>>guess.
>>
>>not really
>>
>>> Since the IOC bit is also set when there are not enough TRBs available, the code
>>should be
>>> modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update
>>below
>>> changes along with your suggestions and resend the patches.
>>
>>no. Actually I don't think we're allowed to split a scatter/gather like
>>that. I did that quite a while ago, but I don't think we're allowed to
>>do so. What we should do, in that case, is not even queue that request
>>until we have enough for all members of the scatter/gather. But that's a
>>separate patch, anyway.
>>
>
> Okay. I have a doubt here, not pushing the request until all sgs are mapped to enough TRBs
> might remove the driver complexity but reduce the performance (since we are waiting
> until enough TRBs are available). Are we okay with that?  

The only other way would be to copy the buffer over to a contiguous
buffer. That will also reduce performance. I think we need to consider
how frequently this may actually happen. I dare to say we don't have any
usb function in kernel as of today that can, easily and frequently, fall
into such a situation. Besides, the performance loss can be amortized by
a deeper request queue.

IMO, this is a minor problem. But, certainly, if you have the setup,
_do_ run some benchmarking and report your findings :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Anurag Kumar Vulisha]

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>; Manu
>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>; linux-
>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.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@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 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

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

[Anurag Kumar Vulisha]

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@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; 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-
>kernel@vger.kernel.org; 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
>
>
>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>;
>Manu
>>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>; linux-
>>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.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@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 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