Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux