Hi, Baolin Wang <baolin.w...@linaro.org> writes: >> Alan Stern <st...@rowland.harvard.edu> writes: >>>> Baolin Wang <baolin.w...@linaro.org> writes: >>>> >> Felipe Balbi <felipe.ba...@linux.intel.com> 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.w...@linaro.org> >>>> >>> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com> >>>> >> >>>> >> 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.ba...@linux.intel.com> >>>> >> 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.w...@linaro.org> >>>> >> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com> >>>> > >>>> > 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
signature.asc
Description: PGP signature
