This patch series fixes the broken BULK streaming support in
dwc3 gadget driver and also adds timer into udc/core.c to
avoid deadlock for the endpoints which are bulk stream capable.
Changes in v6:
1. Added timer into udc/core.c for stream capable endpoint
as suggested by "Felipe Balbi"
Changes in v5:
1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen"
Changes in v4:
1. Corrected the commit message and stream timeout description
as suggested by "Thinh Nguyen"
Changes in v3:
1. Added the changes suggested by "Thinh Nguyen"
Changes in v2:
1. Added "usb: dwc3:" in subject heading
Anurag Kumar Vulisha (10):
usb: gadget: udc: Add timer for stream capable endpoints
usb: dwc3: gadget: Add stream timeout handler for avoiding deadlock
usb: dwc3: gadget: Remove references to dep->stream_capable
usb: dwc3: update stream id in depcmd
usb: dwc3: make controller clear transfer resources after complete
usb: dwc3: don't issue no-op trb for stream capable endpoints
usb: dwc3: check for requests in started list for stream capable
endpoints
usb: dwc3: Correct the logic for checking TRB full in
__dwc3_prepare_one_trb()
usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl
fields
usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints
drivers/usb/dwc3/core.h | 2 --
drivers/usb/dwc3/gadget.c | 67 +++++++++++++++++++++++++++++++++-------
drivers/usb/gadget/udc/core.c | 71 ++++++++++++++++++++++++++++++++++++++++++-
include/linux/usb/gadget.h | 12 ++++++++
4 files changed, 138 insertions(+), 14 deletions(-)
--
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]>
Tested-By: Tejas Joglekar <[email protected]>
---
Chnages in v6:
1. None
Changes in v5:
1. None
Changes in v4:
1. None
Changes in v3:
1. None
Changes in v2:
1. None
---
drivers/usb/dwc3/gadget.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f54de80..5bc37dd 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2416,7 +2416,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
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 v6:
1. Replaced dep->stream_capable with dep->endpoint.stream_caapable
Changes in v5:
1. None
Changes in v4:
1. None
Changes in v3:
1. None
Changes in v2:
1. None
---
drivers/usb/dwc3/gadget.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 89df030..9bf1688 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2420,6 +2420,9 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
+ if (dep->endpoint.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
According to dwc3 databook when streams are enabled, it may be possible
for the host and gadget to become out of sync, where gadget may wait for
host to issue prime transcation and host may wait for gadget to issue
ERDY. To avoid such potential deadlock conditions, a timer is implemented
in udc/core.c and which is started after queuing a valid request in
usb_ep_queue(). This timer would be stopped by the gadget driver when
a valid stream event is found, otherwise the timer gets expired after
STREAM_TIMEOUT_MS value and stream_timeout_function() which is registered
as a callback function to usb_ep->ops->stream_timeout is called by udc
core.
As a part of recovery mechanism, the stream_timeout_function() stops
the active transfer on the endpoint and starts the transfer again on
the endpoint. Doing so, will reset the stream into ready state and ERDY
is sent to the host, thus the deadlock is avoided.
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
Changes in v6:
1. This patch is newly added in this series
---
drivers/usb/dwc3/gadget.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 032ea7d..aab2970 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -573,6 +573,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE
| DWC3_DEPCFG_STREAM_EVENT_EN;
dep->stream_capable = true;
+ dep->endpoint.stream_capable = true;
}
if (!usb_endpoint_xfer_control(desc))
@@ -1535,6 +1536,18 @@ static int dwc3_gadget_ep_set_wedge(struct usb_ep *ep)
return ret;
}
+static void stream_timeout_function(struct usb_ep *ep)
+{
+ struct dwc3_ep *dep = to_dwc3_ep(ep);
+ 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 struct usb_endpoint_descriptor dwc3_gadget_ep0_desc = {
@@ -1563,6 +1576,7 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
.dequeue = dwc3_gadget_ep_dequeue,
.set_halt = dwc3_gadget_ep_set_halt,
.set_wedge = dwc3_gadget_ep_set_wedge,
+ .stream_timeout = stream_timeout_function,
};
/* -------------------------------------------------------------------------- */
@@ -2469,6 +2483,10 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
}
break;
case DWC3_DEPEVT_STREAMEVT:
+ if ((event->status == DEPEVT_STREAMEVT_FOUND) &&
+ timer_pending(&dep->endpoint.stream_timeout_timer))
+ del_timer(&dep->endpoint.stream_timeout_timer);
+
case DWC3_DEPEVT_XFERCOMPLETE:
case DWC3_DEPEVT_RXTXFIFOEVT:
break;
--
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]>
Tested-By: Tejas Joglekar <[email protected]>
---
Changes in v6:
1. None
Changes in v5:
1. None
Changes in v4:
1. None
Changes in v3:
1. None
Changes in v2:
1. None
---
drivers/usb/dwc3/gadget.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2d4b184..f54de80 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2293,7 +2293,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
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 v6:
1. Used dep->endpoint.stream_capable instead of dep->stream_capable
flag
Changes in v5:
1. None
Changes in v4:
1. None
Changes in v3:
1. Added the changes suggested by "Thinh Nguyen"
Changes in v2:
1. None
---
drivers/usb/dwc3/gadget.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 862ec5a..b58cd69 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -571,7 +571,9 @@ 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->endpoint.stream_capable = true;
}
@@ -997,6 +999,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->endpoint.stream_capable)
+ trb->ctrl |= DWC3_TRB_CTRL_LST;
+
if (usb_endpoint_xfer_bulk(dep->endpoint.desc) &&
dep->endpoint.stream_capable)
trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
@@ -2282,7 +2293,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;
@@ -2471,6 +2482,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
}
switch (event->endpoint_event) {
+ case DWC3_DEPEVT_XFERCOMPLETE:
+ if (!dep->endpoint.stream_capable)
+ break;
+ dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+ /* Fall Through */
case DWC3_DEPEVT_XFERINPROGRESS:
dwc3_gadget_endpoint_transfer_in_progress(dep, event);
break;
@@ -2490,7 +2506,6 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
timer_pending(&dep->endpoint.stream_timeout_timer))
del_timer(&dep->endpoint.stream_timeout_timer);
- case DWC3_DEPEVT_XFERCOMPLETE:
case DWC3_DEPEVT_RXTXFIFOEVT:
break;
}
--
2.1.1
When bulk streams are enabled for an endpoint, there can
be a condition where the gadget controller waits for the
host to issue prime transaction and the host controller
waits for the gadget to issue ERDY. This condition could
create a deadlock. To avoid such potential deadlocks, a
timer is started after queuing any request for the stream
capable endpoint in usb_ep_queue(). The gadget driver is
expected to stop the timer if a valid stream event is found.
If no stream event is found, the timer expires after the
STREAM_TIMEOUT_MS value and a callback function registered
by gadget driver to endpoint ops->stream_timeout API would be
called, so that the gadget driver can handle this situation.
This kind of behaviour is observed in dwc3 controller and
expected to be generic issue with other controllers supporting
bulk streams also.
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
Changes in v6:
1. This patch is newly added in this series to add timer into udc/core.c
---
drivers/usb/gadget/udc/core.c | 71 ++++++++++++++++++++++++++++++++++++++++++-
include/linux/usb/gadget.h | 12 ++++++++
2 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48..41cc23b 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -52,6 +52,24 @@ static int udc_bind_to_driver(struct usb_udc *udc,
/* ------------------------------------------------------------------------- */
/**
+ * usb_ep_stream_timeout - callback function for endpoint stream timeout timer
+ * @arg: pointer to struct timer_list
+ *
+ * This function gets called only when bulk streams are enabled in the endpoint
+ * and only after ep->stream_timeout_timer has expired. The timer gets expired
+ * only when the gadget controller failed to find a valid stream event for this
+ * endpoint. On timer expiry, this function calls the endpoint-specific timeout
+ * handler registered to endpoint ops->stream_timeout API.
+ */
+static void usb_ep_stream_timeout(struct timer_list *arg)
+{
+ struct usb_ep *ep = from_timer(ep, arg, stream_timeout_timer);
+
+ if (ep->stream_capable && ep->ops->stream_timeout)
+ ep->ops->stream_timeout(ep);
+}
+
+/**
* usb_ep_set_maxpacket_limit - set maximum packet size limit for endpoint
* @ep:the endpoint being configured
* @maxpacket_limit:value of maximum packet size limit
@@ -87,6 +105,18 @@ EXPORT_SYMBOL_GPL(usb_ep_set_maxpacket_limit);
* configurable, with more generic names like "ep-a". (remember that for
* USB, "in" means "towards the USB master".)
*
+ * When bulk streams are enabled (stream_capable == true), a timer is setup
+ * by this function, which would be started at the time of queuing the request
+ * in usb_ep_queue(). This is because, when streams are enabled the host and
+ * gadget can go out sync, the gadget may wait until the host issues a prime
+ * transaction and the host may wait until gadget issues a ERDY. This behaviour
+ * may create a deadlock. To avoid such a deadlock, the timer is started after
+ * submitting the request in usb_ep_queue(). If a valid stream event is
+ * generated, the gadget driver stops the timer. If no valid stream event is
+ * found, the timer gets expired and usb_ep_stream_timeout() function which is
+ * registered as a callback to stream_timeout_timer is called. This callback
+ * function handles the deadlock.
+ *
* This routine must be called in process context.
*
* returns zero, or a negative error code.
@@ -102,6 +132,10 @@ int usb_ep_enable(struct usb_ep *ep)
if (ret)
goto out;
+ if (ep->stream_capable)
+ timer_setup(&ep->stream_timeout_timer,
+ usb_ep_stream_timeout, 0);
+
ep->enabled = true;
out:
@@ -121,6 +155,9 @@ EXPORT_SYMBOL_GPL(usb_ep_enable);
* gadget drivers must call usb_ep_enable() again before queueing
* requests to the endpoint.
*
+ * For stream capable endpoints, the timer which was started in usb_ep_enable()
+ * would be removed.
+ *
* This routine must be called in process context.
*
* returns zero, or a negative error code.
@@ -132,6 +169,9 @@ int usb_ep_disable(struct usb_ep *ep)
if (!ep->enabled)
goto out;
+ if (ep->stream_capable && timer_pending(&ep->stream_timeout_timer))
+ del_timer(&ep->stream_timeout_timer);
+
ret = ep->ops->disable(ep);
if (ret)
goto out;
@@ -245,6 +285,18 @@ EXPORT_SYMBOL_GPL(usb_ep_free_request);
* Note that @req's ->complete() callback must never be called from
* within usb_ep_queue() as that can create deadlock situations.
*
+ * For stream capable endpoints (stream_capable == true), a timer which was
+ * setup by usb_ep_enable() is started in this function to avoid deadlock.
+ * When streams are enabled the host and gadget can go out sync, the gadget
+ * may wait until the host issues prime transaction and the host may wait
+ * until gadget issues a ERDY. This behaviour may create a deadlock. To avoid
+ * such a deadlock, when endpoint is bulk stream capable, the timer is started
+ * after submitting the request. If a valid stream event is generated by the
+ * gadget controller, the gadget driver stops the timer. If no valid stream
+ * event is found, the timer keeps running until expired after STREAM_TIMEOUT_MS
+ * value and the stream timeout function - usb_ep_stream_timeout() gets
+ * called, which takes necessary action to avoid the deadlock condition.
+ *
* This routine may be called in interrupt context.
*
* Returns zero, or a negative error code. Endpoints that are not enabled
@@ -269,6 +321,13 @@ int usb_ep_queue(struct usb_ep *ep,
ret = ep->ops->queue(ep, req, gfp_flags);
+ if (ep->stream_capable) {
+ ep->stream_timeout_timer.expires = jiffies +
+ msecs_to_jiffies(STREAM_TIMEOUT_MS);
+ mod_timer(&ep->stream_timeout_timer,
+ ep->stream_timeout_timer.expires);
+ }
+
out:
trace_usb_ep_queue(ep, req, ret);
@@ -291,6 +350,9 @@ EXPORT_SYMBOL_GPL(usb_ep_queue);
* restrictions prevent drivers from supporting configuration changes,
* even to configuration zero (a "chapter 9" requirement).
*
+ * For stream capable endpoints, the timer which was started in usb_ep_queue()
+ * would be removed.
+ *
* This routine may be called in interrupt context.
*/
int usb_ep_dequeue(struct usb_ep *ep, struct usb_request *req)
@@ -300,6 +362,9 @@ int usb_ep_dequeue(struct usb_ep *ep, struct usb_request *req)
ret = ep->ops->dequeue(ep, req);
trace_usb_ep_dequeue(ep, req, ret);
+ if (ep->stream_capable && timer_pending(&ep->stream_timeout_timer))
+ del_timer(&ep->stream_timeout_timer);
+
return ret;
}
EXPORT_SYMBOL_GPL(usb_ep_dequeue);
@@ -878,7 +943,8 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
* Context: in_interrupt()
*
* This is called by device controller drivers in order to return the
- * completed request back to the gadget layer.
+ * completed request back to the gadget layer. For stream capable endpoints,
+ * the timer which was started in usb_ep_queue() would be removed.
*/
void usb_gadget_giveback_request(struct usb_ep *ep,
struct usb_request *req)
@@ -886,6 +952,9 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
if (likely(req->status == 0))
usb_led_activity(USB_LED_EVENT_GADGET);
+ if (ep->stream_capable && timer_pending(&ep->stream_timeout_timer))
+ del_timer(&ep->stream_timeout_timer);
+
trace_usb_gadget_giveback_request(ep, req, 0);
req->complete(ep, req);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a..2ebaef0 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -144,6 +144,7 @@ struct usb_ep_ops {
int (*fifo_status) (struct usb_ep *ep);
void (*fifo_flush) (struct usb_ep *ep);
+ void (*stream_timeout) (struct usb_ep *ep);
};
/**
@@ -184,6 +185,11 @@ struct usb_ep_caps {
.dir_out = !!(_dir & USB_EP_CAPS_DIR_OUT), \
}
+/*
+ * Timeout value in msecs used by stream_timeout_timer when streams are enabled
+ */
+#define STREAM_TIMEOUT_MS 50
+
/**
* struct usb_ep - device side representation of USB endpoint
* @name:identifier for the endpoint, such as "ep-a" or "ep9in-bulk"
@@ -191,6 +197,7 @@ struct usb_ep_caps {
* @ep_list:the gadget's ep_list holds all of its endpoints
* @caps:The structure describing types and directions supported by endoint.
* @enabled: The current endpoint enabled/disabled state.
+ * @stream_capable: Set to true when ep supports bulk streams.
* @claimed: True if this endpoint is claimed by a function.
* @maxpacket:The maximum packet size used on this endpoint. The initial
* value can sometimes be reduced (hardware allowing), according to
@@ -209,6 +216,9 @@ struct usb_ep_caps {
* enabled and remains valid until the endpoint is disabled.
* @comp_desc: In case of SuperSpeed support, this is the endpoint companion
* descriptor that is used to configure the endpoint
+ * @stream_timeout_timer: timeout timer used by bulk streams to avoid deadlock
+ * where host waits for the gadget to issue ERDY and gadget waits for host
+ * to issue prime transaction.
*
* the bus controller driver lists all the general purpose endpoints in
* gadget->ep_list. the control endpoint (gadget->ep0) is not in that list,
@@ -224,12 +234,14 @@ struct usb_ep {
struct usb_ep_caps caps;
bool claimed;
bool enabled;
+ bool stream_capable;
unsigned maxpacket:16;
unsigned maxpacket_limit:16;
unsigned max_streams:16;
unsigned mult:2;
unsigned maxburst:5;
u8 address;
+ struct timer_list stream_timeout_timer;
const struct usb_endpoint_descriptor *desc;
const struct usb_ss_ep_comp_descriptor *comp_desc;
};
--
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 v6:
1. Used dep->endpoint.stream_capable instead of dep->stream_capable
flag
Changes in v5:
1. None
Changes in v4:
1. None
Changes in v3:
1. None
Changes in v2:
1. None
---
drivers/usb/dwc3/gadget.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ac752d4..862ec5a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1225,6 +1225,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->endpoint.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
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]>
Tested-By: Tejas Joglekar <[email protected]>
---
Changes in v6:
1. None
Changes in v5:
1. None
Changes in v4:
1. Corrected the commit message as suggested by "Thinh Nguyen"
Changes in v3:
1. Corrected the logic for setting HWO bit as suggested by "Thinh Nguyen"
Changes in v2:
1. Changed the commit message
---
drivers/usb/dwc3/gadget.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9bf1688..2d4b184 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -913,8 +913,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);
@@ -993,7 +991,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)
@@ -1014,6 +1012,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
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]>
---
Chnages in v6:
1. Replaced dep->stream_capable with dep->endpoint.stream_capable
Changes in v5:
1. None
Changes in v4:
1. None
Changes in v3:
1. None
Changes in v2:
1. None
---
drivers/usb/dwc3/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index b58cd69..89df030 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -666,7 +666,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->endpoint.stream_capable) ||
usb_endpoint_xfer_int(desc)) {
struct dwc3_gadget_ep_cmd_params params;
struct dwc3_trb *trb;
--
2.1.1
As a part of adding stream timeout timer for stream capable endpoints
stream_capable flag is added into struct usb_ep. Replace the usage of
dep->stream_capable in with usb_ep->stream_capable.
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
Changes in v6:
1. This patch is newly added in this series
---
drivers/usb/dwc3/core.h | 2 --
drivers/usb/dwc3/gadget.c | 6 +++---
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5bfb625..89a2ee6 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -655,7 +655,6 @@ struct dwc3_event_buffer {
* @interval: the interval on which the ISOC transfer is started
* @name: a human readable name e.g. ep1out-bulk
* @direction: true for TX, false for RX
- * @stream_capable: true when streams are enabled
*/
struct dwc3_ep {
struct usb_ep endpoint;
@@ -704,7 +703,6 @@ struct dwc3_ep {
char name[20];
unsigned direction:1;
- unsigned stream_capable:1;
};
enum dwc3_phy {
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index aab2970..ac752d4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -572,7 +572,6 @@ 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;
- dep->stream_capable = true;
dep->endpoint.stream_capable = true;
}
@@ -740,7 +739,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
reg &= ~DWC3_DALEPENA_EP(dep->number);
dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
- dep->stream_capable = false;
+ dep->endpoint.stream_capable = false;
dep->type = 0;
dep->flags &= DWC3_EP_END_TRANSFER_PENDING;
@@ -998,7 +997,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
if (chain)
trb->ctrl |= DWC3_TRB_CTRL_CHN;
- if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
+ if (usb_endpoint_xfer_bulk(dep->endpoint.desc) &&
+ dep->endpoint.stream_capable)
trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
trb->ctrl |= DWC3_TRB_CTRL_HWO;
--
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]>
Tested-By: Tejas Joglekar <[email protected]>
---
Changes in v6:
1. None
Changes in v5:
1. None
Changes in v4:
1. Corrected the commit message as suggested by "Thinh Nguyen"
Changes in v3:
1. Corrected the logic for setting HWO bit as suggested by "Thinh Nguyen"
Changes in v2:
1. Changed the commit message
---
drivers/usb/dwc3/gadget.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9bf1688..2d4b184 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -913,8 +913,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);
@@ -993,7 +991,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)
@@ -1014,6 +1012,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
Hi Felipe,
Please let me know if you have any comments on this patch series.
If you think patches are good, can we proceed with them ?
Thanks,
Anurag Kumar Vulisha
>-----Original Message-----
>From: Anurag Kumar Vulisha [mailto:[email protected]]
>Sent: Saturday, October 13, 2018 6:45 PM
>To: Felipe Balbi <[email protected]>; Greg Kroah-Hartman
><[email protected]>; Alan Stern <[email protected]>; Johan
>Hovold <[email protected]>; Jaejoong Kim <[email protected]>; Benjamin
>Herrenschmidt <[email protected]>; Roger Quadros <[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; Thinh Nguyen <[email protected]>; Tejas Joglekar
><[email protected]>; Ajay Yugalkishore Pandey <[email protected]>;
>Anurag Kumar Vulisha <[email protected]>
>Subject: [PATCH V6 00/10] usb: dwc3: Fix broken BULK stream support to dwc3
>gadget driver
>
>This patch series fixes the broken BULK streaming support in
>dwc3 gadget driver and also adds timer into udc/core.c to avoid deadlock for the
>endpoints which are bulk stream capable.
>
>Changes in v6:
> 1. Added timer into udc/core.c for stream capable endpoint
> as suggested by "Felipe Balbi"
>
>Changes in v5:
> 1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen"
>
>Changes in v4:
> 1. Corrected the commit message and stream timeout description
> as suggested by "Thinh Nguyen"
>
>Changes in v3:
> 1. Added the changes suggested by "Thinh Nguyen"
>
>Changes in v2:
> 1. Added "usb: dwc3:" in subject heading
>
>Anurag Kumar Vulisha (10):
> usb: gadget: udc: Add timer for stream capable endpoints
> usb: dwc3: gadget: Add stream timeout handler for avoiding deadlock
> usb: dwc3: gadget: Remove references to dep->stream_capable
> usb: dwc3: update stream id in depcmd
> usb: dwc3: make controller clear transfer resources after complete
> usb: dwc3: don't issue no-op trb for stream capable endpoints
> usb: dwc3: check for requests in started list for stream capable
> endpoints
> usb: dwc3: Correct the logic for checking TRB full in
> __dwc3_prepare_one_trb()
> usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl
> fields
> usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints
>
> drivers/usb/dwc3/core.h | 2 --
> drivers/usb/dwc3/gadget.c | 67 +++++++++++++++++++++++++++++++++-------
> drivers/usb/gadget/udc/core.c | 71
>++++++++++++++++++++++++++++++++++++++++++-
> include/linux/usb/gadget.h | 12 ++++++++
> 4 files changed, 138 insertions(+), 14 deletions(-)
>
>--
>2.1.1
Hi,
Anurag Kumar Vulisha <[email protected]> writes:
> When bulk streams are enabled for an endpoint, there can
> be a condition where the gadget controller waits for the
> host to issue prime transaction and the host controller
> waits for the gadget to issue ERDY. This condition could
> create a deadlock. To avoid such potential deadlocks, a
> timer is started after queuing any request for the stream
> capable endpoint in usb_ep_queue(). The gadget driver is
> expected to stop the timer if a valid stream event is found.
> If no stream event is found, the timer expires after the
> STREAM_TIMEOUT_MS value and a callback function registered
> by gadget driver to endpoint ops->stream_timeout API would be
> called, so that the gadget driver can handle this situation.
> This kind of behaviour is observed in dwc3 controller and
> expected to be generic issue with other controllers supporting
> bulk streams also.
>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> ---
> Changes in v6:
> 1. This patch is newly added in this series to add timer into udc/core.c
> ---
> drivers/usb/gadget/udc/core.c | 71 ++++++++++++++++++++++++++++++++++++++++++-
> include/linux/usb/gadget.h | 12 ++++++++
> 2 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index af88b48..41cc23b 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -52,6 +52,24 @@ static int udc_bind_to_driver(struct usb_udc *udc,
> /* ------------------------------------------------------------------------- */
>
> /**
> + * usb_ep_stream_timeout - callback function for endpoint stream timeout timer
> + * @arg: pointer to struct timer_list
> + *
> + * This function gets called only when bulk streams are enabled in the endpoint
> + * and only after ep->stream_timeout_timer has expired. The timer gets expired
> + * only when the gadget controller failed to find a valid stream event for this
> + * endpoint. On timer expiry, this function calls the endpoint-specific timeout
> + * handler registered to endpoint ops->stream_timeout API.
> + */
> +static void usb_ep_stream_timeout(struct timer_list *arg)
> +{
> + struct usb_ep *ep = from_timer(ep, arg, stream_timeout_timer);
> +
> + if (ep->stream_capable && ep->ops->stream_timeout)
why is the timer only for stream endpoints? What if we want to run this
on non-stream capable eps?
> + ep->ops->stream_timeout(ep);
you don't ned an extra operation here, ep_dequeue should be enough.
> @@ -102,6 +132,10 @@ int usb_ep_enable(struct usb_ep *ep)
> if (ret)
> goto out;
>
> + if (ep->stream_capable)
> + timer_setup(&ep->stream_timeout_timer,
> + usb_ep_stream_timeout, 0);
the timer should be per-request, not per-endpoint. Otherwise in case of
multiple requests being queued, you can't give them different timeouts
> @@ -269,6 +321,13 @@ int usb_ep_queue(struct usb_ep *ep,
>
> ret = ep->ops->queue(ep, req, gfp_flags);
>
> + if (ep->stream_capable) {
> + ep->stream_timeout_timer.expires = jiffies +
> + msecs_to_jiffies(STREAM_TIMEOUT_MS);
timeout value should be passed by the gadget driver. Add a new
usb_ep_queue_timeout() that takes the new argument. Rename the current
usb_ep_queue() to static int __usb_ep_queue() with an extra argument for
timeout and introduce usb_ep_queue() without the argument, calling
__usb_ep_queue() passing timeout as 0.
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index e5cd84a..2ebaef0 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -144,6 +144,7 @@ struct usb_ep_ops {
>
> int (*fifo_status) (struct usb_ep *ep);
> void (*fifo_flush) (struct usb_ep *ep);
> + void (*stream_timeout) (struct usb_ep *ep);
why?
--
balbi
Hi Felipe,
Thanks a lot for spending your time in reviewing this patch. Please find
my comments inline
>-----Original Message-----
>From: Felipe Balbi [mailto:[email protected]]
>Sent: Wednesday, November 14, 2018 7:28 PM
>To: Anurag Kumar Vulisha <[email protected]>; Greg Kroah-Hartman
><[email protected]>; Alan Stern <[email protected]>; Johan
>Hovold <[email protected]>; Jaejoong Kim <[email protected]>; Benjamin
>Herrenschmidt <[email protected]>; Roger Quadros <[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; Thinh Nguyen <[email protected]>; Tejas Joglekar
><[email protected]>; Ajay Yugalkishore Pandey <[email protected]>;
>Anurag Kumar Vulisha <[email protected]>
>Subject: Re: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable
>endpoints
>
>
>Hi,
>
>Anurag Kumar Vulisha <[email protected]> writes:
>> When bulk streams are enabled for an endpoint, there can
>> be a condition where the gadget controller waits for the
>> host to issue prime transaction and the host controller
>> waits for the gadget to issue ERDY. This condition could
>> create a deadlock. To avoid such potential deadlocks, a
>> timer is started after queuing any request for the stream
>> capable endpoint in usb_ep_queue(). The gadget driver is
>> expected to stop the timer if a valid stream event is found.
>> If no stream event is found, the timer expires after the
>> STREAM_TIMEOUT_MS value and a callback function registered
>> by gadget driver to endpoint ops->stream_timeout API would be
>> called, so that the gadget driver can handle this situation.
>> This kind of behaviour is observed in dwc3 controller and
>> expected to be generic issue with other controllers supporting
>> bulk streams also.
>>
>> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>> ---
>> Changes in v6:
>> 1. This patch is newly added in this series to add timer into udc/core.c
>> ---
>> drivers/usb/gadget/udc/core.c | 71
>++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/usb/gadget.h | 12 ++++++++
>> 2 files changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index af88b48..41cc23b 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -52,6 +52,24 @@ static int udc_bind_to_driver(struct usb_udc *udc,
>> /* ------------------------------------------------------------------------- */
>>
>> /**
>> + * usb_ep_stream_timeout - callback function for endpoint stream timeout timer
>> + * @arg: pointer to struct timer_list
>> + *
>> + * This function gets called only when bulk streams are enabled in the endpoint
>> + * and only after ep->stream_timeout_timer has expired. The timer gets expired
>> + * only when the gadget controller failed to find a valid stream event for this
>> + * endpoint. On timer expiry, this function calls the endpoint-specific timeout
>> + * handler registered to endpoint ops->stream_timeout API.
>> + */
>> +static void usb_ep_stream_timeout(struct timer_list *arg)
>> +{
>> + struct usb_ep *ep = from_timer(ep, arg, stream_timeout_timer);
>> +
>> + if (ep->stream_capable && ep->ops->stream_timeout)
>
>why is the timer only for stream endpoints? What if we want to run this
>on non-stream capable eps?
>
I have seen this issue only with stream capable eps between PRIME & EPRDY. But this issue
can potentially occur with non-stream endpoints also. Will remove this stream capable checks
in next series of patch.
>> + ep->ops->stream_timeout(ep);
>
>you don't ned an extra operation here, ep_dequeue should be enough.
>
I think issuing ep_dequeue() would be more trickier than calling ep->ops->stream_timeout().
This is because, every gadget driver has their own way of handling ep_dequeue. Some
drivers (like dwc3) may sleep for an event (wait_event_lock_irq) in the ep_dequeue routine.
Since we are calling ep_dequeue from timer timeout callback which is in softirq context,
sleeping or waiting for an event would hang the system. Also adding ep->ops->stream_timeout()
would make the gadget drivers handle the issue in better way based on their implementation.
Another advantage is the drivers which doesn't have this timeout issue doesn't have to register the
timeout handler and can avoid the logic of deleting the timer for every request. So, I think
ep->ops->stream_timeout() would be better than having a generic handler which does
ep_dequeue() & ep_queue(). Please provide your suggestion on this implementation.
>> @@ -102,6 +132,10 @@ int usb_ep_enable(struct usb_ep *ep)
>> if (ret)
>> goto out;
>>
>> + if (ep->stream_capable)
>> + timer_setup(&ep->stream_timeout_timer,
>> + usb_ep_stream_timeout, 0);
>
>the timer should be per-request, not per-endpoint. Otherwise in case of
>multiple requests being queued, you can't give them different timeouts
I agree with you. Will correct this in next series of patch.
>
>> @@ -269,6 +321,13 @@ int usb_ep_queue(struct usb_ep *ep,
>>
>> ret = ep->ops->queue(ep, req, gfp_flags);
>>
>> + if (ep->stream_capable) {
>> + ep->stream_timeout_timer.expires = jiffies +
>> + msecs_to_jiffies(STREAM_TIMEOUT_MS);
>
>timeout value should be passed by the gadget driver. Add a new
>usb_ep_queue_timeout() that takes the new argument. Rename the current
>usb_ep_queue() to static int __usb_ep_queue() with an extra argument for
>timeout and introduce usb_ep_queue() without the argument, calling
>__usb_ep_queue() passing timeout as 0.
>
Thanks for correcting and providing the steps for implementing. Will modify as
suggested in next series of patch.
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e5cd84a..2ebaef0 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -144,6 +144,7 @@ struct usb_ep_ops {
>>
>> int (*fifo_status) (struct usb_ep *ep);
>> void (*fifo_flush) (struct usb_ep *ep);
>> + void (*stream_timeout) (struct usb_ep *ep);
>
>why?
>
As discussed above, the common timeout handler with ep_dequeue() and
ep_queue() may not work for all the gadget drivers, adding the stream_timeout()
in ep->ops would enable the drivers to implement their own handler for
handling the stream timeout issue.
Thanks,
Anurag Kumar Vulisha
>--
>balbi
Hi,
Anurag Kumar Vulisha <[email protected]> writes:
>>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>>> index af88b48..41cc23b 100644
>>> --- a/drivers/usb/gadget/udc/core.c
>>> +++ b/drivers/usb/gadget/udc/core.c
>>> @@ -52,6 +52,24 @@ static int udc_bind_to_driver(struct usb_udc *udc,
>>> /* ------------------------------------------------------------------------- */
>>>
>>> /**
>>> + * usb_ep_stream_timeout - callback function for endpoint stream timeout timer
>>> + * @arg: pointer to struct timer_list
>>> + *
>>> + * This function gets called only when bulk streams are enabled in the endpoint
>>> + * and only after ep->stream_timeout_timer has expired. The timer gets expired
>>> + * only when the gadget controller failed to find a valid stream event for this
>>> + * endpoint. On timer expiry, this function calls the endpoint-specific timeout
>>> + * handler registered to endpoint ops->stream_timeout API.
>>> + */
>>> +static void usb_ep_stream_timeout(struct timer_list *arg)
>>> +{
>>> + struct usb_ep *ep = from_timer(ep, arg, stream_timeout_timer);
>>> +
>>> + if (ep->stream_capable && ep->ops->stream_timeout)
>>
>>why is the timer only for stream endpoints? What if we want to run this
>>on non-stream capable eps?
>>
>
> I have seen this issue only with stream capable eps between PRIME &
> EPRDY. But this issue can potentially occur with non-stream endpoints
> also. Will remove this stream capable checks in next series of patch.
you're adding support for transfer timeouts, which some gadgets may want
regardless of endpoint type. One use is to correct cases of streams
going out of sync.
>>> + ep->ops->stream_timeout(ep);
>>
>>you don't ned an extra operation here, ep_dequeue should be enough.
>>
>
> I think issuing ep_dequeue() would be more trickier than calling ep->ops->stream_timeout().
> This is because, every gadget driver has their own way of handling ep_dequeue. Some
> drivers (like dwc3) may sleep for an event (wait_event_lock_irq) in the ep_dequeue routine.
not anymore. There's, now, a requirement that ->dequeue can be called
from any context.
> Since we are calling ep_dequeue from timer timeout callback which is in softirq context,
> sleeping or waiting for an event would hang the system. Also adding ep->ops->stream_timeout()
> would make the gadget drivers handle the issue in better way based on their implementation.
no problems
> Another advantage is the drivers which doesn't have this timeout issue doesn't have to register the
> timeout handler and can avoid the logic of deleting the timer for every request. So, I think
> ep->ops->stream_timeout() would be better than having a generic handler which does
> ep_dequeue() & ep_queue(). Please provide your suggestion on this implementation.
call ep_dequeue()
--
balbi
Hi Felipe,
>-----Original Message-----
>From: Felipe Balbi [mailto:[email protected]]
>Sent: Thursday, November 29, 2018 6:22 PM
>To: Anurag Kumar Vulisha <[email protected]>; Greg Kroah-Hartman
><[email protected]>; Alan Stern <[email protected]>; Johan
>Hovold <[email protected]>; Jaejoong Kim <[email protected]>; Benjamin
>Herrenschmidt <[email protected]>; Roger Quadros <[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; Thinh Nguyen <[email protected]>; Tejas Joglekar
><[email protected]>; Ajay Yugalkishore Pandey <[email protected]>
>Subject: RE: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable
>endpoints
>
>
>Hi,
>
>Anurag Kumar Vulisha <[email protected]> writes:
>>>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>>>> index af88b48..41cc23b 100644
>>>> --- a/drivers/usb/gadget/udc/core.c
>>>> +++ b/drivers/usb/gadget/udc/core.c
>>>> @@ -52,6 +52,24 @@ static int udc_bind_to_driver(struct usb_udc *udc,
>>>> /* ------------------------------------------------------------------------- */
>>>>
>>>> /**
>>>> + * usb_ep_stream_timeout - callback function for endpoint stream timeout
>timer
>>>> + * @arg: pointer to struct timer_list
>>>> + *
>>>> + * This function gets called only when bulk streams are enabled in the endpoint
>>>> + * and only after ep->stream_timeout_timer has expired. The timer gets expired
>>>> + * only when the gadget controller failed to find a valid stream event for this
>>>> + * endpoint. On timer expiry, this function calls the endpoint-specific timeout
>>>> + * handler registered to endpoint ops->stream_timeout API.
>>>> + */
>>>> +static void usb_ep_stream_timeout(struct timer_list *arg)
>>>> +{
>>>> + struct usb_ep *ep = from_timer(ep, arg, stream_timeout_timer);
>>>> +
>>>> + if (ep->stream_capable && ep->ops->stream_timeout)
>>>
>>>why is the timer only for stream endpoints? What if we want to run this
>>>on non-stream capable eps?
>>>
>>
>> I have seen this issue only with stream capable eps between PRIME &
>> EPRDY. But this issue can potentially occur with non-stream endpoints
>> also. Will remove this stream capable checks in next series of patch.
>
>you're adding support for transfer timeouts, which some gadgets may want
>regardless of endpoint type. One use is to correct cases of streams
>going out of sync.
>
Thanks for making me understand, will implement your suggestions and resend
the patches soon.
Best Regards,
Anurag Kumar Vulisha
>>>> + ep->ops->stream_timeout(ep);
>>>
>>>you don't ned an extra operation here, ep_dequeue should be enough.
>>>
>>
>> I think issuing ep_dequeue() would be more trickier than calling ep->ops-
>>stream_timeout().
>> This is because, every gadget driver has their own way of handling ep_dequeue.
>Some
>> drivers (like dwc3) may sleep for an event (wait_event_lock_irq) in the ep_dequeue
>routine.
>
>not anymore. There's, now, a requirement that ->dequeue can be called
>from any context.
>
>> Since we are calling ep_dequeue from timer timeout callback which is in softirq
>context,
>> sleeping or waiting for an event would hang the system. Also adding ep->ops-
>>stream_timeout()
>> would make the gadget drivers handle the issue in better way based on their
>implementation.
>
>no problems
>
>> Another advantage is the drivers which doesn't have this timeout issue doesn't have
>to register the
>> timeout handler and can avoid the logic of deleting the timer for every request. So, I
>think
>> ep->ops->stream_timeout() would be better than having a generic handler which
>does
>> ep_dequeue() & ep_queue(). Please provide your suggestion on this
>implementation.
>
>call ep_dequeue()
>
>--
>balbi