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
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 <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
---
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
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 <[email protected]>
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
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 <[email protected]>
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
Hi Tejas,
>-----Original Message-----
>From: Tejas Joglekar [mailto:[email protected]]
>Sent: Friday, September 21, 2018 7:01 PM
>To: Anurag Kumar Vulisha <[email protected]>; [email protected];
>[email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; Ajay Yugalkishore Pandey
><[email protected]>; [email protected]
>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 <[email protected]>
>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
Hello Anurag,
On 9/21/2018 7:36 PM, Anurag Kumar Vulisha wrote:
> Hi Tejas,
>
>> -----Original Message-----
>> From: Tejas Joglekar [mailto:[email protected]]
>> Sent: Friday, September 21, 2018 7:01 PM
>> To: Anurag Kumar Vulisha <[email protected]>; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; [email protected]; Ajay Yugalkishore Pandey
>> <[email protected]>; [email protected]
>> 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 <[email protected]>
>> 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
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:[email protected]]
>Sent: Saturday, September 15, 2018 8:00 PM
>To: [email protected]; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; Ajay Yugalkishore Pandey
><[email protected]>; Anurag Kumar Vulisha <[email protected]>
>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
(no top-posting, please)
Hi,
Anurag Kumar Vulisha <[email protected]> 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
Hi Felipe,
>-----Original Message-----
>From: Felipe Balbi [mailto:[email protected]]
>Sent: Tuesday, October 09, 2018 11:07 AM
>To: Anurag Kumar Vulisha <[email protected]>; Anurag Kumar Vulisha
><[email protected]>; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; Ajay Yugalkishore Pandey
><[email protected]>
>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 <[email protected]> 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
Hi,
Anurag Kumar Vulisha <[email protected]> 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
Hi Felipe,
>-----Original Message-----
>From: Felipe Balbi [mailto:[email protected]]
>Sent: Tuesday, October 09, 2018 12:51 PM
>To: Anurag Kumar Vulisha <[email protected]>; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; Ajay Yugalkishore Pandey
><[email protected]>
>Subject: RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3
>gadget driver
>
>
>Hi,
>
>Anurag Kumar Vulisha <[email protected]> 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
Hi,
Anurag Kumar Vulisha <[email protected]> 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