linux-kernel - Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 13 Oct 2016 13:56:59 +0300
From:   Felipe Balbi <balbi@...nel.org>
To:     Baolin Wang <baolin.wang@...aro.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Mark Brown <broonie@...nel.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq


Hi,

Felipe Balbi <balbi@...nel.org> writes:
> Hi,
>
> Baolin Wang <baolin.wang@...aro.org> writes:
>>>>> I'm thinking this is a bug in configfs interface of Gadget API, not
>>>>> dwc3. The only reason for this to happen would be if we get to
>>>>> ->udc_stop() with endpoints still enabled.
>>>>>
>>>>> Can you check if that's the case? i.e. can you check if any endpoints
>>>>> are still enabled when we get here?
>>>>
>>>> No, it is not any endpoints are still enabled. Like I said in commit
>>>> message, we will start end transfer command when disable the endpoint,
>>>> if the end transfer command complete event comes after we have freed
>>>> the gadget irq, it will trigger the interrupt line all the time to
>>>> crash the system.
>>>
>>> I see what the problem is. Databook tells us we *must* set CMDIOC when
>>> issuing EndTransfer command and we should always wait for Command
>>> Complete IRQ. However, we took a shortcut and just delayed for 100us
>>> after issuing End Transfer.
>>
>> Yes, but 100us delay is not enough in some scenarios, like changing
>> function with configfs frequently, we will met problems.
>
> heh, 100us *is* enough. However we still get an IRQ because we requested
> for it. If you want to fix this, then you need to find a way to get rid
> of that 100us udelay() and add a proper wait_for_completion() to delay
> execution until command complete IRQ fires up.

I haven't tested this yet, but it shows the idea (I think we might still
have a race with ep_queue if we're still waiting for End Transfer, but
that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
before calling kick_transfer). Could you have a look and, perhaps, run a
test?

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index e878366ead00..24a77e9f9bba 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>
@@ -504,6 +505,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
@@ -529,6 +531,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;
 
@@ -545,7 +549,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)
 
@@ -1047,6 +1051,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 << 24)) >> 24)
 } __packed;
 
 /**
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3ba05b12e49a..8037bff43485 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;
 
@@ -2156,6 +2158,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 {
 	struct dwc3_ep		*dep;
 	u8			epnum = event->endpoint_number;
+	u8			cmd;
 
 	dep = dwc->eps[epnum];
 
@@ -2200,8 +2203,13 @@ 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;
+		break;
+	case DWC3_DEPEVT_RXTXFIFOEVT:
 		break;
 	}
 }
@@ -2246,6 +2254,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
 }
 
 static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
+__releases(&dwc->lock) __acquires(&dwc->lock)
 {
 	struct dwc3_ep *dep;
 	struct dwc3_gadget_ep_cmd_params params;
@@ -2295,6 +2304,12 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
 	memset(&params, 0, sizeof(params));
 	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
 	WARN_ON_ONCE(ret);
+
+	dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+	wait_event_cmd(dep->wait_end_transfer,
+			!(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
+			spin_unlock_irq(&dwc->lock), spin_lock_irq(&dwc->lock));
+
 	dep->resource_index = 0;
 	dep->flags &= ~DWC3_EP_BUSY;
 



-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (801 bytes)