Hi,
Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
>>>> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>>>> >> Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
>>>> >>> Instead of just delaying for 100us, we should
>>>> >>> actually wait for End Transfer Command Complete
>>>> >>> interrupt before moving on. Note that this should
>>>> >>> only be done if we're dealing with one of the core
>>>> >>> revisions that actually require the interrupt before
>>>> >>> moving on.
>>>> >>>
>>>> >>> Reported-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>>> >>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>>> >>
>>>> >> I've updated this one in order to fix the comment we had about delaying
>>>> >> 100us. No further changes were made, only the comment. Here it is:
>>>> >>
>>>> >> 8<------------------------------------------------------------------------
>>>> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
>>>> >> From: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>>> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
>>>> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
>>>> >>
>>>> >> Instead of just delaying for 100us, we should
>>>> >> actually wait for End Transfer Command Complete
>>>> >> interrupt before moving on. Note that this should
>>>> >> only be done if we're dealing with one of the core
>>>> >> revisions that actually require the interrupt before
>>>> >> moving on.
>>>> >>
>>>> >> Reported-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>>> >> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>>> >
>>>> > From my testing, there are still some problems caused by the nested
>>>> > lock, at lease I found 2 functions will issue the usb_ep_disable()
>>>> > function with spinlock:
>>>>
>>>> Thanks for testing. Seems like I really need to revisit locking in the
>>>> entire gadget API. In either case, we need to have a larger discussion
>>>> about this situation.
>>>>
>>>> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
>>>> process context, but that has never been a requirement before. Still,
>>>> it's not far-fetched to assume that a controller (such as dwc3, but
>>>> probably others) might sleep while cancelling a transfer because they
>>>> need to wait for an Interrupt.
>>>>
>>>> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
>>>
>>> It's not clear that this should be the deciding factor. That is, does
>>> usb_ep_disable() need to wait until all the endpoint's outstanding
>>> transfers have been completed before it returns? I don't think it
>>> does.
>>
>> not all, no. And, frankly, this is really only a problem if we're
>> unregistering a gadget while there are still pending transfers.
>>
>> The original problem statement is that we unregister a gadget, issue End
>> Transfer, but CmdCompletion for the EndTransfer comes way too
>> late. Baolin, can you clarify again what happens when the IRQ comes
>> late?
>
> Sure. The problem I met was, when we change the USB function with
> configfs frequently, sometimes it will hang the system to crash. The
> reason is, we will start end transfer command when disable the
> endpoint, but sometimes the end transfer command complete event comes
> after we have freed the gadget irq (since the xHCI will share the same
> interrupt number with gadget, thus when free the gadget irq, it will
> not shutdown this gadget irq line), it will trigger the interrupt line
> all the time.
okay, thanks for describing it again. Another option would be to only
wait_for_completion() before calling free_irq() is any endpoint has
END_TRANSFER_PENDING flag set. Something like below:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5fc437021ac7..0cb3b78d28b7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -26,6 +26,7 @@
#include <linux/dma-mapping.h>
#include <linux/mm.h>
#include <linux/debugfs.h>
+#include <linux/wait.h>
#include <linux/usb/ch9.h>
#include <linux/usb/gadget.h>
@@ -505,6 +506,7 @@ struct dwc3_event_buffer {
* @endpoint: usb endpoint
* @pending_list: list of pending requests for this endpoint
* @started_list: list of started requests on this endpoint
+ * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
* @lock: spinlock for endpoint request queue traversal
* @regs: pointer to first endpoint register
* @trb_pool: array of transaction buffers
@@ -530,6 +532,8 @@ struct dwc3_ep {
struct list_head pending_list;
struct list_head started_list;
+ wait_queue_head_t wait_end_transfer;
+
spinlock_t lock;
void __iomem *regs;
@@ -546,6 +550,7 @@ struct dwc3_ep {
#define DWC3_EP_BUSY (1 << 4)
#define DWC3_EP_PENDING_REQUEST (1 << 5)
#define DWC3_EP_MISSED_ISOC (1 << 6)
+#define DWC3_EP_END_TRANSFER_PENDING (1 << 7)
/* This last one is specific to EP0 */
#define DWC3_EP0_DIR_IN (1 << 31)
@@ -1050,6 +1055,9 @@ struct dwc3_event_depevt {
#define DEPEVT_TRANSFER_BUS_EXPIRY 2
u32 parameters:16;
+
+/* For Command Complete Events */
+#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 8)) >> 8)
} __packed;
/**
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3b53a5714df4..5929a75b3455 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -598,6 +598,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
reg |= DWC3_DALEPENA_EP(dep->number);
dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
+ init_waitqueue_head(&dep->wait_end_transfer);
+
if (usb_endpoint_xfer_control(desc))
return 0;
@@ -1783,12 +1785,28 @@ static int dwc3_gadget_start(struct usb_gadget *g,
static void __dwc3_gadget_stop(struct dwc3 *dwc)
{
+ int epnum;
+
if (pm_runtime_suspended(dwc->dev))
return;
dwc3_gadget_disable_irq(dwc);
__dwc3_gadget_ep_disable(dwc->eps[0]);
__dwc3_gadget_ep_disable(dwc->eps[1]);
+
+ for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
+ struct dwc3_ep *dep = dwc->eps[epnum];
+
+ if (!(dep->flags & DWC3_EP_ENABLED))
+ continue;
+
+ if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
+ continue;
+
+ wait_event_lock_irq(dep->wait_end_transfer,
+ !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
+ dwc->lock);
+ }
}
static int dwc3_gadget_stop(struct usb_gadget *g)
@@ -2171,6 +2189,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
{
struct dwc3_ep *dep;
u8 epnum = event->endpoint_number;
+ u8 cmd;
dep = dwc->eps[epnum];
@@ -2215,8 +2234,15 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
return;
}
break;
- case DWC3_DEPEVT_RXTXFIFOEVT:
case DWC3_DEPEVT_EPCMDCMPLT:
+ cmd = DEPEVT_PARAMETER_CMD(event->parameters);
+
+ if (cmd == DWC3_DEPCMD_ENDTRANSFER) {
+ dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
+ wake_up(&dep->wait_end_transfer);
+ }
+ break;
+ case DWC3_DEPEVT_RXTXFIFOEVT:
break;
}
}
@@ -2269,7 +2295,8 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
dep = dwc->eps[epnum];
- if (!dep->resource_index)
+ if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
+ !dep->resource_index)
return;
/*
@@ -2313,8 +2340,10 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
dep->resource_index = 0;
dep->flags &= ~DWC3_EP_BUSY;
- if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A)
+ if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) {
+ dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
udelay(100);
+ }
}
static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
I'll run some tests with this in a couple hours.
--
balbi
Attachment:
signature.asc
Description: PGP signature
