[PATCH v4 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver

[PATCH v4 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver

[Anurag Kumar Vulisha]

These patch series fixes the broken BULK streaming support in
dwc3 gadget driver.

Changes in v4:
	1. Corrected the commit messgae 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 (8):
  usb: dwc3: Correct the logic for checking TRB full in
    __dwc3_prepare_one_trb()
  usb: dwc3: update stream id in depcmd
  usb: dwc3: make controller clear transfer resources after complete
  usb: dwc3: implement stream transfer timeout
  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: 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/core.h   |  7 ++++
 drivers/usb/dwc3/gadget.c | 87 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 86 insertions(+), 8 deletions(-)

-- 
2.1.1

[PATCH v4 1/8] 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>
---
 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 032ea7d..8a1622b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -911,8 +911,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);
@@ -991,7 +989,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)
@@ -1002,6 +1000,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 v4 2/8] 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>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
---
 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 8a1622b..43d63a8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1224,6 +1224,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 v4 3/8] 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>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
---
 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 43d63a8..13ea282 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -571,7 +571,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;
 	}
 
@@ -995,6 +996,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);
 
@@ -2268,7 +2278,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;
@@ -2457,6 +2467,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;
@@ -2472,7 +2487,6 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		}
 		break;
 	case DWC3_DEPEVT_STREAMEVT:
-	case DWC3_DEPEVT_XFERCOMPLETE:
 	case DWC3_DEPEVT_RXTXFIFOEVT:
 		break;
 	}
-- 
2.1.1

[PATCH v4 4/8] usb: dwc3: implement stream transfer timeout

[Anurag Kumar Vulisha]

According to dwc3 databook when streams are used, it may be possible
for the host and device become out of sync, where device may wait for
host to issue prime transcation and host may wait for device to issue
erdy. To avoid such deadlock, timeout needs to be implemented. After
timeout occurs, device will first stop transfer and restart the transfer
again. This patch does the same.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Chnages in v4:
	1. Added description for stream timeout timer as suggested by
	   "Thinh Nguyen"

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

 Changes in v2:
	1. Changed STREAM_TIMEOUT to STREAM_TIMEOUT_MS as suggested by
	   "Andy Shevchenko"
---
 drivers/usb/dwc3/core.h   |  7 +++++++
 drivers/usb/dwc3/gadget.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5bfb625..f62e8c4 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -633,6 +633,11 @@ struct dwc3_event_buffer {
 
 #define DWC3_TRB_NUM		256
 
+/*
+ * Timeout value in msecs used by stream_timeout_timer when streams are enabled
+ */
+#define STREAM_TIMEOUT_MS	50
+
 /**
  * struct dwc3_ep - device side endpoint representation
  * @endpoint: usb endpoint
@@ -656,6 +661,7 @@ struct dwc3_event_buffer {
  * @name: a human readable name e.g. ep1out-bulk
  * @direction: true for TX, false for RX
  * @stream_capable: true when streams are enabled
+ * @stream_timeout_timer: timeout timer used by bulk streams
  */
 struct dwc3_ep {
 	struct usb_ep		endpoint;
@@ -705,6 +711,7 @@ struct dwc3_ep {
 
 	unsigned		direction:1;
 	unsigned		stream_capable:1;
+	struct timer_list	stream_timeout_timer;
 };
 
 enum dwc3_phy {
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 13ea282..306d4c5 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -254,6 +254,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 param)
 }
 
 static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
+static void stream_timeout_function(struct timer_list *arg);
 
 /**
  * dwc3_send_gadget_ep_cmd - issue an endpoint command
@@ -574,6 +575,17 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
 			| DWC3_DEPCFG_STREAM_EVENT_EN
 			| DWC3_DEPCFG_XFER_COMPLETE_EN;
 		dep->stream_capable = true;
+
+		/*
+		 * When BULK streams are enabled it may be possible for the host
+		 * and device become out of sync, where device may wait for host
+		 * to issue prime transcation and host may wait for device to
+		 * issue ERDY. To avoid such deadlock, timeout needs to be
+		 * implemented. After timeout occurs, device will first stop
+		 * transfer and restart the transfer again.
+		 */
+		timer_setup(&dep->stream_timeout_timer,
+			    stream_timeout_function, 0);
 	}
 
 	if (!usb_endpoint_xfer_control(desc))
@@ -730,6 +742,9 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
 
 	trace_dwc3_gadget_ep_disable(dep);
 
+	if (dep->stream_capable)
+		del_timer(&dep->stream_timeout_timer);
+
 	dwc3_remove_requests(dwc, dep);
 
 	/* make sure HW endpoint isn't stalled */
@@ -1257,6 +1272,12 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 		return ret;
 	}
 
+	if (starting && dep->stream_capable) {
+		dep->stream_timeout_timer.expires = jiffies +
+					msecs_to_jiffies(STREAM_TIMEOUT_MS);
+		add_timer(&dep->stream_timeout_timer);
+	}
+
 	return 0;
 }
 
@@ -2403,6 +2424,13 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 			stop = true;
 	}
 
+	/*
+	 * Delete the timer that was started in __dwc3_gadget_kick_transfer()
+	 * for stream capable endpoints.
+	 */
+	if (dep->stream_capable)
+		del_timer(&dep->stream_timeout_timer);
+
 	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
 
 	if (stop) {
@@ -2487,6 +2515,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		}
 		break;
 	case DWC3_DEPEVT_STREAMEVT:
+		if (event->status == DEPEVT_STREAMEVT_FOUND)
+			del_timer(&dep->stream_timeout_timer);
+		else
+			dev_dbg(dwc->dev, "unable to find suitable stream");
+		break;
 	case DWC3_DEPEVT_RXTXFIFOEVT:
 		break;
 	}
@@ -2588,6 +2621,18 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force)
 	}
 }
 
+static void stream_timeout_function(struct timer_list *arg)
+{
+	struct dwc3_ep *dep = from_timer(dep, arg, stream_timeout_timer);
+	struct dwc3		*dwc = dep->dwc;
+	unsigned long		flags;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_stop_active_transfer(dep, true);
+	__dwc3_gadget_kick_transfer(dep);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+}
+
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
 {
 	u32 epnum;
-- 
2.1.1

[PATCH v4 5/8] 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>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
---
 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 306d4c5..97bfdf0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -677,7 +677,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 v4 6/8] 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>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
---
 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 97bfdf0..c50cad8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2433,6 +2433,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 v4 7/8] 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>
---
 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 c50cad8..6b6bdd2 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2299,7 +2299,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 v4 8/8] 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>
---
 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 6b6bdd2..2c9e3ab 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2422,7 +2422,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 v4 6/8] usb: dwc3: check for requests in started list for stream capable endpoints

[Thinh Nguyen]

Hi Anurag,

On 9/8/2018 8:03 AM, Anurag Kumar Vulisha wrote:
> 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.

Whenever a function driver queues a request, then
__dwc3_gadget_kick_transfer() will be called right? What case exactly is
this for? Scatter gathering? If so, then we probably need further
explanation. (e.g. Why wait to call __dwc3_gadget_kick_transfer() on
XferComplete event rather than sending a START_TRANSFER command for
every prepared TRB whenever we do __dwc3_gadget_kick_transfer()?).

Thanks,
Thinh

>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  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 97bfdf0..c50cad8 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2433,6 +2433,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;

Re: [PATCH v4 6/8] usb: dwc3: check for requests in started list for stream capable endpoints

[Thinh Nguyen]

Hi,

On 9/10/2018 1:13 PM, Thinh Nguyen wrote:
> Hi Anurag,
>
> On 9/8/2018 8:03 AM, Anurag Kumar Vulisha wrote:
>> 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.
> Whenever a function driver queues a request, then
> __dwc3_gadget_kick_transfer() will be called right? What case exactly is
> this for? Scatter gathering? If so, then we probably need further
> explanation. (e.g. Why wait to call __dwc3_gadget_kick_transfer() on
> XferComplete event rather than sending a START_TRANSFER command for
> every prepared TRB whenever we do __dwc3_gadget_kick_transfer()?).

Please ignore my question.  We only do 1 transfer per stream id at a
time. That's fine.

Thanks,
Thinh

Re: [PATCH v4 4/8] usb: dwc3: implement stream transfer timeout

[Thinh Nguyen]

Hi Anurag,

On 9/8/2018 8:03 AM, Anurag Kumar Vulisha wrote:
> According to dwc3 databook when streams are used, it may be possible
> for the host and device become out of sync, where device may wait for
> host to issue prime transcation and host may wait for device to issue
> erdy. To avoid such deadlock, timeout needs to be implemented. After
> timeout occurs, device will first stop transfer and restart the transfer
> again. This patch does the same.
>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  Chnages in v4:
> 	1. Added description for stream timeout timer as suggested by
> 	   "Thinh Nguyen"
>
>  Changes in v3:
> 	1. Added the changes suggested by "Thinh Nguyen"
>
>  Changes in v2:
> 	1. Changed STREAM_TIMEOUT to STREAM_TIMEOUT_MS as suggested by
> 	   "Andy Shevchenko"
> ---
>  drivers/usb/dwc3/core.h   |  7 +++++++
>  drivers/usb/dwc3/gadget.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5bfb625..f62e8c4 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -633,6 +633,11 @@ struct dwc3_event_buffer {
>  
>  #define DWC3_TRB_NUM		256
>  
> +/*
> + * Timeout value in msecs used by stream_timeout_timer when streams are enabled
> + */
> +#define STREAM_TIMEOUT_MS	50
> +
>  /**
>   * struct dwc3_ep - device side endpoint representation
>   * @endpoint: usb endpoint
> @@ -656,6 +661,7 @@ struct dwc3_event_buffer {
>   * @name: a human readable name e.g. ep1out-bulk
>   * @direction: true for TX, false for RX
>   * @stream_capable: true when streams are enabled
> + * @stream_timeout_timer: timeout timer used by bulk streams
>   */
>  struct dwc3_ep {
>  	struct usb_ep		endpoint;
> @@ -705,6 +711,7 @@ struct dwc3_ep {
>  
>  	unsigned		direction:1;
>  	unsigned		stream_capable:1;
> +	struct timer_list	stream_timeout_timer;
>  };
>  
>  enum dwc3_phy {
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 13ea282..306d4c5 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -254,6 +254,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 param)
>  }
>  
>  static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
> +static void stream_timeout_function(struct timer_list *arg);
>  
>  /**
>   * dwc3_send_gadget_ep_cmd - issue an endpoint command
> @@ -574,6 +575,17 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
>  			| DWC3_DEPCFG_STREAM_EVENT_EN
>  			| DWC3_DEPCFG_XFER_COMPLETE_EN;
>  		dep->stream_capable = true;
> +
> +		/*
> +		 * When BULK streams are enabled it may be possible for the host
> +		 * and device become out of sync, where device may wait for host
> +		 * to issue prime transcation and host may wait for device to
> +		 * issue ERDY. To avoid such deadlock, timeout needs to be
> +		 * implemented. After timeout occurs, device will first stop
> +		 * transfer and restart the transfer again.
> +		 */
> +		timer_setup(&dep->stream_timeout_timer,
> +			    stream_timeout_function, 0);
>  	}
>  
>  	if (!usb_endpoint_xfer_control(desc))
> @@ -730,6 +742,9 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  
>  	trace_dwc3_gadget_ep_disable(dep);
>  
> +	if (dep->stream_capable)
> +		del_timer(&dep->stream_timeout_timer);
> +
>  	dwc3_remove_requests(dwc, dep);
>  
>  	/* make sure HW endpoint isn't stalled */
> @@ -1257,6 +1272,12 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>  		return ret;
>  	}
>  
> +	if (starting && dep->stream_capable) {
> +		dep->stream_timeout_timer.expires = jiffies +
> +					msecs_to_jiffies(STREAM_TIMEOUT_MS);
> +		add_timer(&dep->stream_timeout_timer);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2403,6 +2424,13 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>  			stop = true;
>  	}
>  
> +	/*
> +	 * Delete the timer that was started in __dwc3_gadget_kick_transfer()
> +	 * for stream capable endpoints.
> +	 */
> +	if (dep->stream_capable)
> +		del_timer(&dep->stream_timeout_timer);
> +
>  	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>  
>  	if (stop) {
> @@ -2487,6 +2515,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>  		}
>  		break;
>  	case DWC3_DEPEVT_STREAMEVT:
> +		if (event->status == DEPEVT_STREAMEVT_FOUND)
> +			del_timer(&dep->stream_timeout_timer);
> +		else
> +			dev_dbg(dwc->dev, "unable to find suitable stream");

This debug message is already printed in the driver tracepoint. There's
no need to print it here. Also, I don't think Felipe will accept adding
new dev_dbg() after removing all of them and converting them to tracepoints.

Thinh

RE: [PATCH v4 4/8] usb: dwc3: implement stream transfer timeout

[Anurag Kumar Vulisha]

Hi Thinh,

>>
>> @@ -2403,6 +2424,13 @@ static void
>dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>  			stop = true;
>>  	}
>>
>> +	/*
>> +	 * Delete the timer that was started in __dwc3_gadget_kick_transfer()
>> +	 * for stream capable endpoints.
>> +	 */
>> +	if (dep->stream_capable)
>> +		del_timer(&dep->stream_timeout_timer);
>> +
>>  	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>>
>>  	if (stop) {
>> @@ -2487,6 +2515,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>>  		}
>>  		break;
>>  	case DWC3_DEPEVT_STREAMEVT:
>> +		if (event->status == DEPEVT_STREAMEVT_FOUND)
>> +			del_timer(&dep->stream_timeout_timer);
>> +		else
>> +			dev_dbg(dwc->dev, "unable to find suitable stream");
>
>This debug message is already printed in the driver tracepoint. There's no need to
>print it here. Also, I don't think Felipe will accept adding new dev_dbg() after removing
>all of them and converting them to tracepoints.
>

I agree with you, thanks for correcting. Will remove the debug prints and resend the patches

Thanks,
Anurag Kumar Vulisha