These patch series fixes the broken BULK streaming support in
dwc3 gadget driver.
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 | 78 ++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 78 insertions(+), 7 deletions(-)
--
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]>
---
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 3af55f8..1b1bc14 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2290,7 +2290,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
Availability of TRB's are calculated using dwc3_calc_trbs_left(), which
determines available TRB's based on the HWO bit set in a TRB.
__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 have HWO bit already set,
there are chances where dwc3_calc_trbs_left() wrongly calculates this
present working TRB as free(since the HWO bit is not yet set). This could
be a problem. This patch correct this issue by setting HWO bit before
calling dwc3_calc_trbs_left()
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
Changes in v2:
1. Changed the commit message
---
drivers/usb/dwc3/gadget.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 69bf137..f73d219 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -990,6 +990,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
}
+ trb->ctrl |= DWC3_TRB_CTRL_HWO;
+
if ((!no_interrupt && !chain) ||
(dwc3_calc_trbs_left(dep) == 0))
trb->ctrl |= DWC3_TRB_CTRL_IOC;
@@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
- trb->ctrl |= DWC3_TRB_CTRL_HWO;
-
trace_dwc3_prepare_trb(dep, 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]>
---
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 | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 285ce0e..f58640f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -619,6 +619,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
@@ -642,6 +647,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: timer used to aviod deadlock when streams are used
*/
struct dwc3_ep {
struct usb_ep endpoint;
@@ -691,6 +697,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 b3e9e7f..8cef488 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,8 @@ 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;
+ timer_setup(&dep->stream_timeout_timer,
+ stream_timeout_function, 0);
}
if (!usb_endpoint_xfer_control(desc))
@@ -730,6 +733,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 +1263,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 +2415,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) {
@@ -2486,6 +2505,14 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
}
break;
case DWC3_DEPEVT_STREAMEVT:
+ switch (event->status) {
+ case DEPEVT_STREAMEVT_FOUND:
+ del_timer(&dep->stream_timeout_timer);
+ break;
+ case DEPEVT_STREAMEVT_NOTFOUND:
+ default:
+ dev_err(dwc->dev, "unable to find suitable stream");
+ }
case DWC3_DEPEVT_RXTXFIFOEVT:
break;
}
@@ -2587,6 +2614,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
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]>
---
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 260f10f..3af55f8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2424,6 +2424,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
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]>
---
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 1b1bc14..188b043 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2413,7 +2413,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]>
---
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 8cef488..260f10f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -668,7 +668,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
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]>
---
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 f73d219..efc6e13 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]>
---
Changes in v2:
1. None
---
drivers/usb/dwc3/gadget.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index efc6e13..b3e9e7f 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;
}
@@ -999,6 +1000,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,10 @@ 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;
case DWC3_DEPEVT_XFERINPROGRESS:
dwc3_gadget_endpoint_transfer_in_progress(dep, event);
break;
@@ -2472,7 +2486,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
Hi All,
Ping!
Just thought of giving a friendly remainder for this patch series. Does these patches looks okay, can we proceed with them? Please provide me your valuable suggestions.
Thanks,
Anurag Kumar Vulisha
>-----Original Message-----
>From: Anurag Kumar Vulisha [mailto:[email protected]]
>Sent: Friday, August 17, 2018 5:55 PM
>To: [email protected]; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; Anurag Kumar Vulisha <[email protected]>
>Subject: [PATCH v2 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 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 | 78 ++++++++++++++++++++++++++++++++++++++++++-
>----
> 2 files changed, 78 insertions(+), 7 deletions(-)
>
>--
>2.1.1
Hi Anurag,
On 8/17/2018 5:25 AM, Anurag Kumar Vulisha wrote:
> Availability of TRB's are calculated using dwc3_calc_trbs_left(), which
> determines available TRB's based on the HWO bit set in a TRB.
>
> __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 have HWO bit already set,
> there are chances where dwc3_calc_trbs_left() wrongly calculates this
> present working TRB as free(since the HWO bit is not yet set). This could
> be a problem. This patch correct this issue by setting HWO bit before
> calling dwc3_calc_trbs_left()
>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> ---
> Changes in v2:
> 1. Changed the commit message
> ---
> drivers/usb/dwc3/gadget.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 69bf137..f73d219 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -990,6 +990,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
> trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
> }
>
> + trb->ctrl |= DWC3_TRB_CTRL_HWO;
> +
> if ((!no_interrupt && !chain) ||
> (dwc3_calc_trbs_left(dep) == 0))
> trb->ctrl |= DWC3_TRB_CTRL_IOC;
> @@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
> if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
> trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>
> - trb->ctrl |= DWC3_TRB_CTRL_HWO;
> -
> trace_dwc3_prepare_trb(dep, trb);
> }
>
How do you reproduce this issue? We should not set HWO before setting
other trb->ctrl bits. Can you provide a driver tracepoint? If there's an
issue with the check if ((!no_interrupt && !chain) ||
dwc3_calc_trbs_left == 0), then we may need to fix the check there.
BR,
Thinh
On 8/17/2018 5:26 AM, Anurag Kumar Vulisha wrote:
> 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]>
> ---
> Changes in v2:
> 1. None
> ---
> drivers/usb/dwc3/gadget.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index efc6e13..b3e9e7f 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;
> }
>
> @@ -999,6 +1000,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,10 @@ 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;
Add this: /* Falls Through */
> case DWC3_DEPEVT_XFERINPROGRESS:
> dwc3_gadget_endpoint_transfer_in_progress(dep, event);
> break;
> @@ -2472,7 +2486,6 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> }
> break;
> case DWC3_DEPEVT_STREAMEVT:
> - case DWC3_DEPEVT_XFERCOMPLETE:
> case DWC3_DEPEVT_RXTXFIFOEVT:
> break;
> }
Thinh
On 8/17/2018 5:25 AM, Anurag Kumar Vulisha wrote:
> According to dwc3 databook when streams are used, it may be possible
> for the host and device become out of sync, where device may wait for
> host to issue prime transcation and host may wait for device to issue
> erdy. To avoid such deadlock, timeout needs to be implemented. After
> timeout occurs, device will first stop transfer and restart the transfer
> again. This patch does the same.
>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> ---
> 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 | 39 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 285ce0e..f58640f 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -619,6 +619,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
> @@ -642,6 +647,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: timer used to aviod deadlock when streams are used
> */
> struct dwc3_ep {
> struct usb_ep endpoint;
> @@ -691,6 +697,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 b3e9e7f..8cef488 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,8 @@ 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;
> + timer_setup(&dep->stream_timeout_timer,
> + stream_timeout_function, 0);
> }
>
> if (!usb_endpoint_xfer_control(desc))
> @@ -730,6 +733,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 +1263,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 +2415,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) {
> @@ -2486,6 +2505,14 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> }
> break;
> case DWC3_DEPEVT_STREAMEVT:
> + switch (event->status) {
> + case DEPEVT_STREAMEVT_FOUND:
> + del_timer(&dep->stream_timeout_timer);
> + break;
> + case DEPEVT_STREAMEVT_NOTFOUND:
> + default:
> + dev_err(dwc->dev, "unable to find suitable stream");
> + }
Add break after this. Also, it's probably easier to read if we use
if-else case here.
> case DWC3_DEPEVT_RXTXFIFOEVT:
> break;
> }
> @@ -2587,6 +2614,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;
Thinh
Hi Thinh,
Thanks for spending your time in reviewing this code, please find my comments inline
>-----Original Message-----
>From: Thinh Nguyen [mailto:[email protected]]
>Sent: Wednesday, September 05, 2018 11:04 AM
>To: Anurag Kumar Vulisha <[email protected]>; [email protected];
>[email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]
>Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in
>__dwc3_prepare_one_trb()
>
>Hi Anurag,
>
>On 8/17/2018 5:25 AM, Anurag Kumar Vulisha wrote:
>> Availability of TRB's are calculated using dwc3_calc_trbs_left(), which
>> determines available TRB's based on the HWO bit set in a TRB.
>>
>> __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 have HWO bit already set,
>> there are chances where dwc3_calc_trbs_left() wrongly calculates this
>> present working TRB as free(since the HWO bit is not yet set). This could
>> be a problem. This patch correct this issue by setting HWO bit before
>> calling dwc3_calc_trbs_left()
>>
>> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>> ---
>> Changes in v2:
>> 1. Changed the commit message
>> ---
>> drivers/usb/dwc3/gadget.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 69bf137..f73d219 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -990,6 +990,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep,
>struct dwc3_trb *trb,
>> trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
>> }
>>
>> + trb->ctrl |= DWC3_TRB_CTRL_HWO;
>> +
>> if ((!no_interrupt && !chain) ||
>> (dwc3_calc_trbs_left(dep) == 0))
>> trb->ctrl |= DWC3_TRB_CTRL_IOC;
>> @@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep
>*dep, struct dwc3_trb *trb,
>> if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
>> trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>>
>> - trb->ctrl |= DWC3_TRB_CTRL_HWO;
>> -
>> trace_dwc3_prepare_trb(dep, trb);
>> }
>>
>How do you reproduce this issue? We should not set HWO before setting
>other trb->ctrl bits. Can you provide a driver tracepoint? If there's an
>issue with the check if ((!no_interrupt && !chain) ||
>dwc3_calc_trbs_left == 0), then we may need to fix the check there.
>
This issue gets triggered very rarely on long runs when dep->trb_enqueue == dep->trb_dequeue.
In __dwc3_prepare_one_trb() , IOC bit is set when no TRBs are available, so that a complete
event can be generated and TRBs can be cleaned after complete . Dwc3_calc_trbs_left() is called
to determine the available TRBs, which depends on the previous TRB's HWO bit set when
dep->trb_enqueue == dep->trb_dequeue. There are chances where the dwc3_calc_trbs_left() wrongly
returns DWC3_TRB_NUM -1 instead of 0 , even though there are no available TRBs. Please
consider the below example
1. Consider a TRB passed to __dwc3_prepare_one_trb() is the last available TRB in the pool.
2. __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments the dep->trb_enqueue
before preparing the TRB and since the current TRB is the last available, incrementing
dep->enqueue will make dep->enqueue == dep->dequeue
3. IOC bit is set by __dwc3_prepare_one_trb() only when dwc3_calc_trbs_left() returns 0 (empty TRBs)
4. Since dep->enqueue == dep->dequeue and the previous TRB(the one which we are working)
doesn't yet has the HWO bit set, dwc3_calc_trbs_left() returns DWC3_TRB_NUM -1 instead of
zero (Though there are no available TRBs)
5. Since Dwc3_calc_trbs_left() returns non-zero value, IOC bit is not set in __dwc3_prepare_one_trb()
for the last TRB and no complete event is generated. Because of this no further TRBs are queued.
To avoid the above mentioned issue, I moved the code logic for setting HWO bit before setting IOC bit.
I agree that HWO bit should always be set at the last, but I didn't find any better logic for fixing this.
Please suggest if any better way to handle this situation.
Thanks,
Anurag Kumar Vulisha
Hi Thinh,
>> + * 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,10 @@ 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;
> Add this: /* Falls Through */
Thanks for correcting, will fix it in the next version of patch
Thanks,
Anurag Kumar Vulisha
>> case DWC3_DEPEVT_XFERINPROGRESS:
>> dwc3_gadget_endpoint_transfer_in_progress(dep, event);
>> break;
>> @@ -2472,7 +2486,6 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>> }
>> break;
>> case DWC3_DEPEVT_STREAMEVT:
>> - case DWC3_DEPEVT_XFERCOMPLETE:
>> case DWC3_DEPEVT_RXTXFIFOEVT:
>> break;
>> }
>
>Thinh
Hi Thinh,
>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) {
>> @@ -2486,6 +2505,14 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>> }
>> break;
>> case DWC3_DEPEVT_STREAMEVT:
>> + switch (event->status) {
>> + case DEPEVT_STREAMEVT_FOUND:
>> + del_timer(&dep->stream_timeout_timer);
>> + break;
>> + case DEPEVT_STREAMEVT_NOTFOUND:
>> + default:
>> + dev_err(dwc->dev, "unable to find suitable stream");
>> + }
>Add break after this. Also, it's probably easier to read if we use if-else case here.
>
Thanks for correcting the code, will fix it in next series of patch
Thanks,
Anurag Kumar Vulisha
>> case DWC3_DEPEVT_RXTXFIFOEVT:
>> break;
>> }
>> @@ -2587,6 +2614,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;
>
>Thinh
Hi,
On 9/5/2018 2:19 AM, Anurag Kumar Vulisha wrote:
> Hi Thinh,
>
> Thanks for spending your time in reviewing this code, please find my comments inline
>
>> -----Original Message-----
>> From: Thinh Nguyen [mailto:[email protected]]
>> Sent: Wednesday, September 05, 2018 11:04 AM
>> To: Anurag Kumar Vulisha <[email protected]>; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]
>> Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in
>> __dwc3_prepare_one_trb()
>>
>> Hi Anurag,
>>
>> On 8/17/2018 5:25 AM, Anurag Kumar Vulisha wrote:
>>> Availability of TRB's are calculated using dwc3_calc_trbs_left(), which
>>> determines available TRB's based on the HWO bit set in a TRB.
>>>
>>> __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 have HWO bit already set,
>>> there are chances where dwc3_calc_trbs_left() wrongly calculates this
>>> present working TRB as free(since the HWO bit is not yet set). This could
>>> be a problem. This patch correct this issue by setting HWO bit before
>>> calling dwc3_calc_trbs_left()
>>>
>>> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>>> ---
>>> Changes in v2:
>>> 1. Changed the commit message
>>> ---
>>> drivers/usb/dwc3/gadget.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 69bf137..f73d219 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -990,6 +990,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep,
>> struct dwc3_trb *trb,
>>> trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
>>> }
>>>
>>> + trb->ctrl |= DWC3_TRB_CTRL_HWO;
>>> +
>>> if ((!no_interrupt && !chain) ||
>>> (dwc3_calc_trbs_left(dep) == 0))
>>> trb->ctrl |= DWC3_TRB_CTRL_IOC;
>>> @@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep
>> *dep, struct dwc3_trb *trb,
>>> if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
>>> trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>>>
>>> - trb->ctrl |= DWC3_TRB_CTRL_HWO;
>>> -
>>> trace_dwc3_prepare_trb(dep, trb);
>>> }
>>>
>> How do you reproduce this issue? We should not set HWO before setting
>> other trb->ctrl bits. Can you provide a driver tracepoint? If there's an
>> issue with the check if ((!no_interrupt && !chain) ||
>> dwc3_calc_trbs_left == 0), then we may need to fix the check there.
>>
> This issue gets triggered very rarely on long runs when dep->trb_enqueue == dep->trb_dequeue.
> In __dwc3_prepare_one_trb() , IOC bit is set when no TRBs are available, so that a complete
> event can be generated and TRBs can be cleaned after complete . Dwc3_calc_trbs_left() is called
> to determine the available TRBs, which depends on the previous TRB's HWO bit set when
> dep->trb_enqueue == dep->trb_dequeue. There are chances where the dwc3_calc_trbs_left() wrongly
> returns DWC3_TRB_NUM -1 instead of 0 , even though there are no available TRBs. Please
> consider the below example
>
> 1. Consider a TRB passed to __dwc3_prepare_one_trb() is the last available TRB in the pool.
> 2. __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments the dep->trb_enqueue
> before preparing the TRB and since the current TRB is the last available, incrementing
> dep->enqueue will make dep->enqueue == dep->dequeue
> 3. IOC bit is set by __dwc3_prepare_one_trb() only when dwc3_calc_trbs_left() returns 0 (empty TRBs)
> 4. Since dep->enqueue == dep->dequeue and the previous TRB(the one which we are working)
> doesn't yet has the HWO bit set, dwc3_calc_trbs_left() returns DWC3_TRB_NUM -1 instead of
> zero (Though there are no available TRBs)
> 5. Since Dwc3_calc_trbs_left() returns non-zero value, IOC bit is not set in __dwc3_prepare_one_trb()
> for the last TRB and no complete event is generated. Because of this no further TRBs are queued.
>
> To avoid the above mentioned issue, I moved the code logic for setting HWO bit before setting IOC bit.
> I agree that HWO bit should always be set at the last, but I didn't find any better logic for fixing this.
> Please suggest if any better way to handle this situation.
>
I haven't tested it, but you can try this:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d7a327eaee12..37171d46390b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -924,8 +924,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);
@@ -1004,7 +1002,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)
@@ -1013,6 +1011,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep
*dep, struct dwc3_trb *trb,
if (usb_endpoint_xfer_bulk(dep->endpoint.desc) &&
dep->stream_capable)
trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
+ dwc3_ep_inc_enq(dep);
+
trb->ctrl |= DWC3_TRB_CTRL_HWO;
trace_dwc3_prepare_trb(dep, trb);
BR,
Thinh
Hi Thinh,
>-----Original Message-----
>From: Thinh Nguyen [mailto:[email protected]]
>Sent: Thursday, September 06, 2018 7:28 AM
>To: Anurag Kumar Vulisha <[email protected]>; Thinh Nguyen
><[email protected]>; [email protected]; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]
>Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in
>__dwc3_prepare_one_trb()
>
>Hi,
>
>On 9/5/2018 2:19 AM, Anurag Kumar Vulisha wrote:
>> Hi Thinh,
>>
>> Thanks for spending your time in reviewing this code, please find my
>> comments inline
>>
>>> -----Original Message-----
>>> From: Thinh Nguyen [mailto:[email protected]]
>>> Sent: Wednesday, September 05, 2018 11:04 AM
>>> To: Anurag Kumar Vulisha <[email protected]>; [email protected];
>>> [email protected]
>>> Cc: [email protected]; [email protected]; linux-
>>> [email protected]
>>> Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking
>>> TRB full in
>>> __dwc3_prepare_one_trb()
>>>
>>> Hi Anurag,
>>>
>>>> + trb->ctrl |= DWC3_TRB_CTRL_HWO;
>>>> +
>>>> if ((!no_interrupt && !chain) ||
>>>> (dwc3_calc_trbs_left(dep) == 0))
>>>> trb->ctrl |= DWC3_TRB_CTRL_IOC;
>>>> @@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct
>>>> dwc3_ep
>>> *dep, struct dwc3_trb *trb,
>>>> if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
>>>> trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>>>>
>>>> - trb->ctrl |= DWC3_TRB_CTRL_HWO;
>>>> -
>>>> trace_dwc3_prepare_trb(dep, trb);
>>>> }
>>>>
>>> How do you reproduce this issue? We should not set HWO before setting
>>> other trb->ctrl bits. Can you provide a driver tracepoint? If there's
>>> an issue with the check if ((!no_interrupt && !chain) ||
>>> dwc3_calc_trbs_left == 0), then we may need to fix the check there.
>>>
>> This issue gets triggered very rarely on long runs when dep->trb_enqueue == dep-
>>trb_dequeue.
>> In __dwc3_prepare_one_trb() , IOC bit is set when no TRBs are
>> available, so that a complete event can be generated and TRBs can be
>> cleaned after complete . Dwc3_calc_trbs_left() is called to determine
>> the available TRBs, which depends on the previous TRB's HWO bit set
>> when
>> dep->trb_enqueue == dep->trb_dequeue. There are chances where the
>> dep->dwc3_calc_trbs_left() wrongly
>> returns DWC3_TRB_NUM -1 instead of 0 , even though there are no
>> available TRBs. Please consider the below example
>>
>> 1. Consider a TRB passed to __dwc3_prepare_one_trb() is the last available TRB in
>the pool.
>> 2. __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments the dep-
>>trb_enqueue
>> before preparing the TRB and since the current TRB is the last available,
>incrementing
>> dep->enqueue will make dep->enqueue == dep->dequeue 3. IOC bit is
>> set by __dwc3_prepare_one_trb() only when dwc3_calc_trbs_left()
>> returns 0 (empty TRBs) 4. Since dep->enqueue == dep->dequeue and the previous
>TRB(the one which we are working)
>> doesn't yet has the HWO bit set, dwc3_calc_trbs_left() returns DWC3_TRB_NUM
>-1 instead of
>> zero (Though there are no available TRBs) 5. Since
>> Dwc3_calc_trbs_left() returns non-zero value, IOC bit is not set in
>__dwc3_prepare_one_trb()
>> for the last TRB and no complete event is generated. Because of this no further
>TRBs are queued.
>>
>> To avoid the above mentioned issue, I moved the code logic for setting HWO bit
>before setting IOC bit.
>> I agree that HWO bit should always be set at the last, but I didn't find any better
>logic for fixing this.
>> Please suggest if any better way to handle this situation.
>>
>
>I haven't tested it, but you can try this:
>
>diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index
>d7a327eaee12..37171d46390b 100644
>--- a/drivers/usb/dwc3/gadget.c
>+++ b/drivers/usb/dwc3/gadget.c
>@@ -924,8 +924,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);
>@@ -1004,7 +1002,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)
>@@ -1013,6 +1011,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep,
>struct dwc3_trb *trb,
> if (usb_endpoint_xfer_bulk(dep->endpoint.desc) &&
>dep->stream_capable)
> trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>
>+ dwc3_ep_inc_enq(dep);
>+
> trb->ctrl |= DWC3_TRB_CTRL_HWO;
>
> trace_dwc3_prepare_trb(dep, trb);
>
Thanks for pointing out a solution , the fix looks good. I will test with this fix and resend the patches
Best Regards,
Anurag Kumar Vulisha
Hi Anurag,
On 9/6/2018 8:12 AM, Anurag Kumar Vulisha wrote:
> Hi Thinh,
>
>> -----Original Message-----
>> From: Thinh Nguyen [mailto:[email protected]]
>> Sent: Thursday, September 06, 2018 7:28 AM
>> To: Anurag Kumar Vulisha <[email protected]>; Thinh Nguyen
>> <[email protected]>; [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]
>> Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in
>> __dwc3_prepare_one_trb()
>>
>> Hi,
>>
>> On 9/5/2018 2:19 AM, Anurag Kumar Vulisha wrote:
>>> Hi Thinh,
>>>
>>> Thanks for spending your time in reviewing this code, please find my
>>> comments inline
>>>
>>>> -----Original Message-----
>>>> From: Thinh Nguyen [mailto:[email protected]]
>>>> Sent: Wednesday, September 05, 2018 11:04 AM
>>>> To: Anurag Kumar Vulisha <[email protected]>; [email protected];
>>>> [email protected]
>>>> Cc: [email protected]; [email protected]; linux-
>>>> [email protected]
>>>> Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking
>>>> TRB full in
>>>> __dwc3_prepare_one_trb()
>>>>
>>>> Hi Anurag,
>>>>
>>>>> + trb->ctrl |= DWC3_TRB_CTRL_HWO;
>>>>> +
>>>>> if ((!no_interrupt && !chain) ||
>>>>> (dwc3_calc_trbs_left(dep) == 0))
>>>>> trb->ctrl |= DWC3_TRB_CTRL_IOC;
>>>>> @@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct
>>>>> dwc3_ep
>>>> *dep, struct dwc3_trb *trb,
>>>>> if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
>>>>> trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>>>>>
>>>>> - trb->ctrl |= DWC3_TRB_CTRL_HWO;
>>>>> -
>>>>> trace_dwc3_prepare_trb(dep, trb);
>>>>> }
>>>>>
>>>> How do you reproduce this issue? We should not set HWO before setting
>>>> other trb->ctrl bits. Can you provide a driver tracepoint? If there's
>>>> an issue with the check if ((!no_interrupt && !chain) ||
>>>> dwc3_calc_trbs_left == 0), then we may need to fix the check there.
>>>>
>>> This issue gets triggered very rarely on long runs when dep->trb_enqueue == dep-
>>> trb_dequeue.
>>> In __dwc3_prepare_one_trb() , IOC bit is set when no TRBs are
>>> available, so that a complete event can be generated and TRBs can be
>>> cleaned after complete . Dwc3_calc_trbs_left() is called to determine
>>> the available TRBs, which depends on the previous TRB's HWO bit set
>>> when
>>> dep->trb_enqueue == dep->trb_dequeue. There are chances where the
>>> dep->dwc3_calc_trbs_left() wrongly
>>> returns DWC3_TRB_NUM -1 instead of 0 , even though there are no
>>> available TRBs. Please consider the below example
>>>
>>> 1. Consider a TRB passed to __dwc3_prepare_one_trb() is the last available TRB in
>> the pool.
>>> 2. __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments the dep-
>>> trb_enqueue
>>> before preparing the TRB and since the current TRB is the last available,
>> incrementing
>>> dep->enqueue will make dep->enqueue == dep->dequeue 3. IOC bit is
>>> set by __dwc3_prepare_one_trb() only when dwc3_calc_trbs_left()
>>> returns 0 (empty TRBs) 4. Since dep->enqueue == dep->dequeue and the previous
>> TRB(the one which we are working)
>>> doesn't yet has the HWO bit set, dwc3_calc_trbs_left() returns DWC3_TRB_NUM
>> -1 instead of
>>> zero (Though there are no available TRBs) 5. Since
>>> Dwc3_calc_trbs_left() returns non-zero value, IOC bit is not set in
>> __dwc3_prepare_one_trb()
>>> for the last TRB and no complete event is generated. Because of this no further
>> TRBs are queued.
>>> To avoid the above mentioned issue, I moved the code logic for setting HWO bit
>> before setting IOC bit.
>>> I agree that HWO bit should always be set at the last, but I didn't find any better
>> logic for fixing this.
>>> Please suggest if any better way to handle this situation.
>>>
>> I haven't tested it, but you can try this:
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index
>> d7a327eaee12..37171d46390b 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -924,8 +924,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);
>> @@ -1004,7 +1002,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)
>> @@ -1013,6 +1011,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep,
>> struct dwc3_trb *trb,
>> if (usb_endpoint_xfer_bulk(dep->endpoint.desc) &&
>> dep->stream_capable)
>> trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>>
>> + dwc3_ep_inc_enq(dep);
>> +
>> trb->ctrl |= DWC3_TRB_CTRL_HWO;
>>
>> trace_dwc3_prepare_trb(dep, trb);
>>
> Thanks for pointing out a solution , the fix looks good. I will test with this fix and resend the patches
>
> Best Regards,
> Anurag Kumar Vulisha
>
>
The rest of the patch series looks fine to me.
Reviewed-by: Thinh Nguyen <[email protected]>
BR,
Thinh