linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
@ 2018-09-15 14:29 Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 1/8] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb() Anurag Kumar Vulisha
                   ` (10 more replies)
  0 siblings, 11 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:29 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	Anurag Kumar Vulisha

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

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

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 | 85 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 84 insertions(+), 8 deletions(-)

-- 
2.1.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v5 1/8] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb()
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
@ 2018-09-15 14:29 ` Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 2/8] usb: dwc3: update stream id in depcmd Anurag Kumar Vulisha
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:29 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	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 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 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 2/8] usb: dwc3: update stream id in depcmd
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 1/8] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb() Anurag Kumar Vulisha
@ 2018-09-15 14:29 ` Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 3/8] usb: dwc3: make controller clear transfer resources after complete Anurag Kumar Vulisha
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:29 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	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 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 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 3/8] usb: dwc3: make controller clear transfer resources after complete
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 1/8] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb() Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 2/8] usb: dwc3: update stream id in depcmd Anurag Kumar Vulisha
@ 2018-09-15 14:29 ` Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 4/8] usb: dwc3: implement stream transfer timeout Anurag Kumar Vulisha
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:29 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	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 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 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 4/8] usb: dwc3: implement stream transfer timeout
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
                   ` (2 preceding siblings ...)
  2018-09-15 14:29 ` [PATCH v5 3/8] usb: dwc3: make controller clear transfer resources after complete Anurag Kumar Vulisha
@ 2018-09-15 14:29 ` Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 5/8] usb: dwc3: don't issue no-op trb for stream capable endpoints Anurag Kumar Vulisha
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:29 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	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>
---
 Changes in v5:
	1. Removed dev_dbg prints as suggested by "Thinh Nguyen"

 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 | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 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..e18f9b9 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,9 @@ 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);
+		break;
 	case DWC3_DEPEVT_RXTXFIFOEVT:
 		break;
 	}
@@ -2588,6 +2619,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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 5/8] usb: dwc3: don't issue no-op trb for stream capable endpoints
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
                   ` (3 preceding siblings ...)
  2018-09-15 14:29 ` [PATCH v5 4/8] usb: dwc3: implement stream transfer timeout Anurag Kumar Vulisha
@ 2018-09-15 14:29 ` Anurag Kumar Vulisha
  2018-09-15 14:29 ` [PATCH v5 6/8] usb: dwc3: check for requests in started list " Anurag Kumar Vulisha
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:29 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	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 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 e18f9b9..b2d68f1 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 6/8] usb: dwc3: check for requests in started list for stream capable endpoints
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
                   ` (4 preceding siblings ...)
  2018-09-15 14:29 ` [PATCH v5 5/8] usb: dwc3: don't issue no-op trb for stream capable endpoints Anurag Kumar Vulisha
@ 2018-09-15 14:29 ` Anurag Kumar Vulisha
  2018-09-15 14:30 ` [PATCH v5 7/8] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields Anurag Kumar Vulisha
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:29 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	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 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 b2d68f1..1cb7df9 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 7/8] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
                   ` (5 preceding siblings ...)
  2018-09-15 14:29 ` [PATCH v5 6/8] usb: dwc3: check for requests in started list " Anurag Kumar Vulisha
@ 2018-09-15 14:30 ` Anurag Kumar Vulisha
  2018-09-15 14:30 ` [PATCH v5 8/8] usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints Anurag Kumar Vulisha
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:30 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	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 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 1cb7df9..872c956 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 8/8] usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
                   ` (6 preceding siblings ...)
  2018-09-15 14:30 ` [PATCH v5 7/8] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields Anurag Kumar Vulisha
@ 2018-09-15 14:30 ` Anurag Kumar Vulisha
  2018-09-21  2:53 ` [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Tejas Joglekar
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-15 14:30 UTC (permalink / raw)
  To: balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	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 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 872c956..0b6d859 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
                   ` (7 preceding siblings ...)
  2018-09-15 14:30 ` [PATCH v5 8/8] usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints Anurag Kumar Vulisha
@ 2018-09-21  2:53 ` Tejas Joglekar
  2018-09-21 13:31 ` Tejas Joglekar
  2018-10-08 15:25 ` Anurag Kumar Vulisha
  10 siblings, 0 replies; 19+ messages in thread
From: Tejas Joglekar @ 2018-09-21  2:53 UTC (permalink / raw)
  To: Anurag Kumar Vulisha, balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey

Hello Anurag,
On 9/15/2018 8:00 PM, Anurag Kumar Vulisha wrote:
> These patch series fixes the broken BULK streaming support in
> dwc3 gadget driver.
>
> Changes in v5:
> 	1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen"
>
> 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 | 85 ++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 84 insertions(+), 8 deletions(-)
>
Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com>

I have tested this patch series except the stream transfer timeout patch on HAPS-DX platform. I am not aware of exact scenarios where I can test the timeout patch, and don't have test to perform timeout testing.

Thanks & Regards,
 Tejas Joglekar

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
                   ` (8 preceding siblings ...)
  2018-09-21  2:53 ` [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Tejas Joglekar
@ 2018-09-21 13:31 ` Tejas Joglekar
  2018-09-21 14:05   ` Anurag Kumar Vulisha
  2018-10-08 15:25 ` Anurag Kumar Vulisha
  10 siblings, 1 reply; 19+ messages in thread
From: Tejas Joglekar @ 2018-09-21 13:31 UTC (permalink / raw)
  To: Anurag Kumar Vulisha, balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen, apandey,
	joglekartejas

Hello Anurag,
On 9/15/2018 8:00 PM, Anurag Kumar Vulisha wrote:
> These patch series fixes the broken BULK streaming support in
> dwc3 gadget driver.
>
> Changes in v5:
> 	1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen"
>
> 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 | 85 ++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 84 insertions(+), 8 deletions(-)
>
Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com>
I have tested this patch series except the stream transfer timeout patch on HAPS-DX platform.  I am not aware of exact scenarios to test the timeout patch and don't have a test for the same.
Thanks,
Tejas Joglekar

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-09-21 13:31 ` Tejas Joglekar
@ 2018-09-21 14:05   ` Anurag Kumar Vulisha
  2018-09-24  4:57     ` Tejas Joglekar
  0 siblings, 1 reply; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-09-21 14:05 UTC (permalink / raw)
  To: Tejas Joglekar, balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen,
	Ajay Yugalkishore Pandey, joglekartejas


Hi Tejas,

>-----Original Message-----
>From: Tejas Joglekar [mailto:Tejas.Joglekar@synopsys.com]
>Sent: Friday, September 21, 2018 7:01 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org;
>gregkh@linuxfoundation.org
>Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>; joglekartejas@gmail.com
>Subject: Re: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3
>gadget driver
>
>Hello Anurag,
>On 9/15/2018 8:00 PM, Anurag Kumar Vulisha wrote:
>> These patch series fixes the broken BULK streaming support in
>> dwc3 gadget driver.
>>
>> Changes in v5:
>> 	1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen"
>>
>> 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 | 85
>++++++++++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 84 insertions(+), 8 deletions(-)
>>
>Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com>
>I have tested this patch series except the stream transfer timeout patch on HAPS-DX
>platform.  I am not aware of exact scenarios to test the timeout patch and don't have
>a test for the same.

Thanks for testing the patches. The issue mentioned in the timeout patch (Patch 4) will
occur very rarely on the long runs and only when tested with stream capable host. This
issue happens only when the host & dwc3 controller go out of sync, where the dwc3
controller may wait for host to issue prime transaction and host may wait for the gadget
to issue ERDY. I used controller version 2.90A  for testing this issue.  This issue is mentioned
in databook section 9.5.2

Thanks,
Anurag Kumar Vulisha


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-09-21 14:05   ` Anurag Kumar Vulisha
@ 2018-09-24  4:57     ` Tejas Joglekar
  0 siblings, 0 replies; 19+ messages in thread
From: Tejas Joglekar @ 2018-09-24  4:57 UTC (permalink / raw)
  To: Anurag Kumar Vulisha, Tejas Joglekar, balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen,
	Ajay Yugalkishore Pandey, joglekartejas

Hello Anurag,
On 9/21/2018 7:36 PM, Anurag Kumar Vulisha wrote:
> Hi Tejas,
>
>> -----Original Message-----
>> From: Tejas Joglekar [mailto:Tejas.Joglekar@synopsys.com]
>> Sent: Friday, September 21, 2018 7:01 PM
>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org;
>> gregkh@linuxfoundation.org
>> Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey
>> <APANDEY@xilinx.com>; joglekartejas@gmail.com
>> Subject: Re: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3
>> gadget driver
>>
>> Hello Anurag,
>> On 9/15/2018 8:00 PM, Anurag Kumar Vulisha wrote:
>>> These patch series fixes the broken BULK streaming support in
>>> dwc3 gadget driver.
>>>
>>> Changes in v5:
>>> 	1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen"
>>>
>>> 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 | 85
>> ++++++++++++++++++++++++++++++++++++++++++-----
>>>  2 files changed, 84 insertions(+), 8 deletions(-)
>>>
>> Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com>
>> I have tested this patch series except the stream transfer timeout patch on HAPS-DX
>> platform.  I am not aware of exact scenarios to test the timeout patch and don't have
>> a test for the same.
> Thanks for testing the patches. The issue mentioned in the timeout patch (Patch 4) will
> occur very rarely on the long runs and only when tested with stream capable host. This
> issue happens only when the host & dwc3 controller go out of sync, where the dwc3
> controller may wait for host to issue prime transaction and host may wait for the gadget
> to issue ERDY. I used controller version 2.90A  for testing this issue.  This issue is mentioned
> in databook section 9.5.2
>
> Thanks,
> Anurag Kumar Vulisha
>
>
I see, as it is rare so maybe it is not seen here. I have used 3.30A for testing. I will keep watch on this issue during my tests.

---
Thanks,
Tejas Joglekar

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
                   ` (9 preceding siblings ...)
  2018-09-21 13:31 ` Tejas Joglekar
@ 2018-10-08 15:25 ` Anurag Kumar Vulisha
  2018-10-09  5:36   ` Felipe Balbi
  10 siblings, 1 reply; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-10-08 15:25 UTC (permalink / raw)
  To: Anurag Kumar Vulisha, balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen,
	Ajay Yugalkishore Pandey


Hi Felipe,

Please let us know if you have any suggestions / comments on this patch series.
If you feel this patch series are okay, can we proceed with them?

Thanks,
Anurag Kumar Vulisha

>-----Original Message-----
>From: Anurag Kumar Vulisha [mailto:anurag.kumar.vulisha@xilinx.com]
>Sent: Saturday, September 15, 2018 8:00 PM
>To: balbi@kernel.org; gregkh@linuxfoundation.org
>Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>; Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget
>driver
>
>These patch series fixes the broken BULK streaming support in
>dwc3 gadget driver.
>
>Changes in v5:
>	1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen"
>
>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 | 85 ++++++++++++++++++++++++++++++++++++++++++-
>----
> 2 files changed, 84 insertions(+), 8 deletions(-)
>
>--
>2.1.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-10-08 15:25 ` Anurag Kumar Vulisha
@ 2018-10-09  5:36   ` Felipe Balbi
  2018-10-09  6:38     ` Anurag Kumar Vulisha
  0 siblings, 1 reply; 19+ messages in thread
From: Felipe Balbi @ 2018-10-09  5:36 UTC (permalink / raw)
  To: Anurag Kumar Vulisha, Anurag Kumar Vulisha, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen,
	Ajay Yugalkishore Pandey

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


(no top-posting, please)

Hi,


Anurag Kumar Vulisha <anuragku@xilinx.com> writes:

> Hi Felipe,
>
> Please let us know if you have any suggestions / comments on this patch series.
> If you feel this patch series are okay, can we proceed with them?

I really don't like this dwc3-specific timer. The best way here would be
to add a timer on udc/core.c which can be reused by any udc. This would
mean, of course, teaching udc/core about streams and lettting it do part
of the handling.

Best

-- 
balbi

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-10-09  5:36   ` Felipe Balbi
@ 2018-10-09  6:38     ` Anurag Kumar Vulisha
  2018-10-09  7:20       ` Felipe Balbi
  0 siblings, 1 reply; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-10-09  6:38 UTC (permalink / raw)
  To: Felipe Balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen,
	Ajay Yugalkishore Pandey

Hi Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Tuesday, October 09, 2018 11:07 AM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Anurag Kumar Vulisha
><anuragku@xilinx.com>; gregkh@linuxfoundation.org
>Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>
>Subject: RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3
>gadget driver
>
>
>(no top-posting, please)
>

Will ensure this won't happen again

>Hi,
>
>
>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>
>> Hi Felipe,
>>
>> Please let us know if you have any suggestions / comments on this patch series.
>> If you feel this patch series are okay, can we proceed with them?
>
>I really don't like this dwc3-specific timer. The best way here would be
>to add a timer on udc/core.c which can be reused by any udc. This would
>mean, of course, teaching udc/core about streams and lettting it do part
>of the handling.
>

Thanks for spending your time in reviewing this patch. The reason for adding the
timer is when streams are enabled there could be chances for the host and gadget
controller to become out of sync, the gadget may wait for the host to issue prime
transaction and the host may wait for the gadget to issue ERDY. To avoid such a
potential deadlock conditions, timeout needs to be implemented in dwc3 driver.
After timeout occurs, gadget will first stop transfer and restart the transfer again.
This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how
other controllers are handling the streams, but since this issue looks more like a
dwc3 specific issue, I think it would be more convincing to add the timer in dwc3
gadget driver rather than adding in udc framework. Also we are stopping the timer
when a valid StreamEvnt is found, which would be difficult to handle if the timer is
moved into udc. Please help me by correcting , if I am missing something or my
understanding is wrong.

Thanks,
Anurag Kumar Vulisha

>Best
>
>--
>balbi

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-10-09  6:38     ` Anurag Kumar Vulisha
@ 2018-10-09  7:20       ` Felipe Balbi
  2018-10-09 13:00         ` Anurag Kumar Vulisha
  0 siblings, 1 reply; 19+ messages in thread
From: Felipe Balbi @ 2018-10-09  7:20 UTC (permalink / raw)
  To: Anurag Kumar Vulisha, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen,
	Ajay Yugalkishore Pandey

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


Hi,

Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>> Please let us know if you have any suggestions / comments on this patch series.
>>> If you feel this patch series are okay, can we proceed with them?
>>
>>I really don't like this dwc3-specific timer. The best way here would be
>>to add a timer on udc/core.c which can be reused by any udc. This would
>>mean, of course, teaching udc/core about streams and lettting it do part
>>of the handling.
>>
>
> Thanks for spending your time in reviewing this patch. The reason for adding the
> timer is when streams are enabled there could be chances for the host and gadget
> controller to become out of sync, the gadget may wait for the host to issue prime
> transaction and the host may wait for the gadget to issue ERDY. To avoid such a
> potential deadlock conditions, timeout needs to be implemented in dwc3 driver.

"in dwc3 driver" is an implementation choice. The situation you describe
could happen with any UDC, right?

> After timeout occurs, gadget will first stop transfer and restart the transfer again.
> This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how
> other controllers are handling the streams, but since this issue looks more like a

We should get in touch with other UDC authors. We have at least Renesas,
net2280, bcd_udc and mtu3 supporting superspeed.

> dwc3 specific issue, I think it would be more convincing to add the timer in dwc3
> gadget driver rather than adding in udc framework. Also we are stopping the timer

why? When the situation you describe is something that can happen with
any udc, why should we reimplement the solution on all UDCs supporting
streams when we can give generic support for handling certain
situations?

> when a valid StreamEvnt is found, which would be difficult to handle if the timer is

Why difficult? udc-core would call:

mod_timer(gadget->stream_timeout_timer, msecs_to_jiffies(50));

Once you receive stream event, dwc3 would call:

if (timer_pending(dwc->gadget.stream_timeout_timer))
	del_timer(dwc->gadget.stream_timeout_timer);

Why is that difficult? You could even avoid anything to be written in
dwc3 and put the del_timer() inside usb_gadget_giveback_request()
itself. That why, dwc3 doesn't even have to know that there's a timer
running. Also, you're timer function, instead of calling dwc3's private
functions, should be relying on the gadget API.

Your timer, apparently, should be fired per-request, then your timer
function would call:

usb_ep_dequeue(request);
usb_ep_queue(request);

If the timer expires. This would work for any UDC, not only dwc3. Then,
this is something we document for all UDCs and they'd have to adhere to
the API.

In summary, not that many changes needed to dwc3. Nothing related to
timers inside dwc3. Almost everythin can, and should, be done
generically.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-10-09  7:20       ` Felipe Balbi
@ 2018-10-09 13:00         ` Anurag Kumar Vulisha
  2018-10-09 13:04           ` Felipe Balbi
  0 siblings, 1 reply; 19+ messages in thread
From: Anurag Kumar Vulisha @ 2018-10-09 13:00 UTC (permalink / raw)
  To: Felipe Balbi, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen,
	Ajay Yugalkishore Pandey


Hi Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Tuesday, October 09, 2018 12:51 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; gregkh@linuxfoundation.org
>Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>
>Subject: RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3
>gadget driver
>
>
>Hi,
>
>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>>> Please let us know if you have any suggestions / comments on this patch series.
>>>> If you feel this patch series are okay, can we proceed with them?
>>>
>>>I really don't like this dwc3-specific timer. The best way here would be
>>>to add a timer on udc/core.c which can be reused by any udc. This would
>>>mean, of course, teaching udc/core about streams and lettting it do part
>>>of the handling.
>>>
>>
>> Thanks for spending your time in reviewing this patch. The reason for adding the
>> timer is when streams are enabled there could be chances for the host and gadget
>> controller to become out of sync, the gadget may wait for the host to issue prime
>> transaction and the host may wait for the gadget to issue ERDY. To avoid such a
>> potential deadlock conditions, timeout needs to be implemented in dwc3 driver.
>
>"in dwc3 driver" is an implementation choice. The situation you describe
>could happen with any UDC, right?
>

Yes this could happen to other UDC drivers also, unless controller is capable of handling

>> After timeout occurs, gadget will first stop transfer and restart the transfer again.
>> This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how
>> other controllers are handling the streams, but since this issue looks more like a
>
>We should get in touch with other UDC authors. We have at least Renesas,
>net2280, bcd_udc and mtu3 supporting superspeed.
>

Thanks for pointing other drivers. Will refer these drivers to see how they are handling streams
 
>> dwc3 specific issue, I think it would be more convincing to add the timer in dwc3
>> gadget driver rather than adding in udc framework. Also we are stopping the timer
>
>why? When the situation you describe is something that can happen with
>any udc, why should we reimplement the solution on all UDCs supporting
>streams when we can give generic support for handling certain
>situations?
>

I agree with you. As you suggested will work on implementing changes in UDC

>> when a valid StreamEvnt is found, which would be difficult to handle if the timer is
>
>Why difficult? udc-core would call:
>
>mod_timer(gadget->stream_timeout_timer, msecs_to_jiffies(50));
>
>Once you receive stream event, dwc3 would call:
>
>if (timer_pending(dwc->gadget.stream_timeout_timer))
>	del_timer(dwc->gadget.stream_timeout_timer);
>
>Why is that difficult? You could even avoid anything to be written in
>dwc3 and put the del_timer() inside usb_gadget_giveback_request()
>itself. That why, dwc3 doesn't even have to know that there's a timer
>running. Also, you're timer function, instead of calling dwc3's private
>functions, should be relying on the gadget API.
>
>Your timer, apparently, should be fired per-request, then your timer
>function would call:
>
>usb_ep_dequeue(request);
>usb_ep_queue(request);
>
>If the timer expires. This would work for any UDC, not only dwc3. Then,
>this is something we document for all UDCs and they'd have to adhere to
>the API.
>
>In summary, not that many changes needed to dwc3. Nothing related to
>timers inside dwc3. Almost everythin can, and should, be done
>generically.

Thanks a lot for giving a detailed explanation. Will implement the timeout
changes into UDC core.

Best Regards,
Anurag Kumar Vulisha


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
  2018-10-09 13:00         ` Anurag Kumar Vulisha
@ 2018-10-09 13:04           ` Felipe Balbi
  0 siblings, 0 replies; 19+ messages in thread
From: Felipe Balbi @ 2018-10-09 13:04 UTC (permalink / raw)
  To: Anurag Kumar Vulisha, gregkh
  Cc: v.anuragkumar, linux-usb, linux-kernel, Thinh.Nguyen,
	Ajay Yugalkishore Pandey

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


Hi,

Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>> Thanks for spending your time in reviewing this patch. The reason for adding the
>>> timer is when streams are enabled there could be chances for the host and gadget
>>> controller to become out of sync, the gadget may wait for the host to issue prime
>>> transaction and the host may wait for the gadget to issue ERDY. To avoid such a
>>> potential deadlock conditions, timeout needs to be implemented in dwc3 driver.
>>
>>"in dwc3 driver" is an implementation choice. The situation you describe
>>could happen with any UDC, right?
>>
>
> Yes this could happen to other UDC drivers also, unless controller is capable of handling
>
>>> After timeout occurs, gadget will first stop transfer and restart the transfer again.
>>> This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how
>>> other controllers are handling the streams, but since this issue looks more like a
>>
>>We should get in touch with other UDC authors. We have at least Renesas,
>>net2280, bcd_udc and mtu3 supporting superspeed.
>>
>
> Thanks for pointing other drivers. Will refer these drivers to see how they are handling streams
>  
>>> dwc3 specific issue, I think it would be more convincing to add the timer in dwc3
>>> gadget driver rather than adding in udc framework. Also we are stopping the timer
>>
>>why? When the situation you describe is something that can happen with
>>any udc, why should we reimplement the solution on all UDCs supporting
>>streams when we can give generic support for handling certain
>>situations?
>>
>
> I agree with you. As you suggested will work on implementing changes in UDC
>
>>> when a valid StreamEvnt is found, which would be difficult to handle if the timer is
>>
>>Why difficult? udc-core would call:
>>
>>mod_timer(gadget->stream_timeout_timer, msecs_to_jiffies(50));
>>
>>Once you receive stream event, dwc3 would call:
>>
>>if (timer_pending(dwc->gadget.stream_timeout_timer))
>>	del_timer(dwc->gadget.stream_timeout_timer);
>>
>>Why is that difficult? You could even avoid anything to be written in
>>dwc3 and put the del_timer() inside usb_gadget_giveback_request()
>>itself. That why, dwc3 doesn't even have to know that there's a timer
>>running. Also, you're timer function, instead of calling dwc3's private
>>functions, should be relying on the gadget API.
>>
>>Your timer, apparently, should be fired per-request, then your timer
>>function would call:
>>
>>usb_ep_dequeue(request);
>>usb_ep_queue(request);
>>
>>If the timer expires. This would work for any UDC, not only dwc3. Then,
>>this is something we document for all UDCs and they'd have to adhere to
>>the API.
>>
>>In summary, not that many changes needed to dwc3. Nothing related to
>>timers inside dwc3. Almost everythin can, and should, be done
>>generically.
>
> Thanks a lot for giving a detailed explanation. Will implement the timeout
> changes into UDC core.

no problem. I just wanna make sure that this work happens in one place
and one place only :)

cheers

-- 
balbi

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2018-10-09 13:11 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-15 14:29 [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
2018-09-15 14:29 ` [PATCH v5 1/8] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb() Anurag Kumar Vulisha
2018-09-15 14:29 ` [PATCH v5 2/8] usb: dwc3: update stream id in depcmd Anurag Kumar Vulisha
2018-09-15 14:29 ` [PATCH v5 3/8] usb: dwc3: make controller clear transfer resources after complete Anurag Kumar Vulisha
2018-09-15 14:29 ` [PATCH v5 4/8] usb: dwc3: implement stream transfer timeout Anurag Kumar Vulisha
2018-09-15 14:29 ` [PATCH v5 5/8] usb: dwc3: don't issue no-op trb for stream capable endpoints Anurag Kumar Vulisha
2018-09-15 14:29 ` [PATCH v5 6/8] usb: dwc3: check for requests in started list " Anurag Kumar Vulisha
2018-09-15 14:30 ` [PATCH v5 7/8] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields Anurag Kumar Vulisha
2018-09-15 14:30 ` [PATCH v5 8/8] usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints Anurag Kumar Vulisha
2018-09-21  2:53 ` [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Tejas Joglekar
2018-09-21 13:31 ` Tejas Joglekar
2018-09-21 14:05   ` Anurag Kumar Vulisha
2018-09-24  4:57     ` Tejas Joglekar
2018-10-08 15:25 ` Anurag Kumar Vulisha
2018-10-09  5:36   ` Felipe Balbi
2018-10-09  6:38     ` Anurag Kumar Vulisha
2018-10-09  7:20       ` Felipe Balbi
2018-10-09 13:00         ` Anurag Kumar Vulisha
2018-10-09 13:04           ` Felipe Balbi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).