Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754115AbcJMK5w (ORCPT ); Thu, 13 Oct 2016 06:57:52 -0400 Received: from mga05.intel.com ([192.55.52.43]:50792 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753784AbcJMK5o (ORCPT ); Thu, 13 Oct 2016 06:57:44 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,339,1473145200"; d="asc'?scan'208";a="1064256761" From: Felipe Balbi To: Baolin Wang Cc: Greg KH , Mark Brown , USB , LKML Subject: Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq In-Reply-To: <87a8e8vaif.fsf@linux.intel.com> References: <87r37kviic.fsf@linux.intel.com> <87inswvg80.fsf@linux.intel.com> <87a8e8vaif.fsf@linux.intel.com> Date: Thu, 13 Oct 2016 13:56:59 +0300 Message-ID: <877f9cv7no.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5943 Lines: 177 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Felipe Balbi writes: > Hi, > > Baolin Wang 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 =2D-- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -26,6 +26,7 @@ #include #include #include +#include =20 #include #include @@ -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 compl= ete * @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; =20 + wait_queue_head_t wait_end_transfer; + spinlock_t lock; void __iomem *regs; =20 @@ -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) =2D +#define DWC3_EP_END_TRANSFER_PENDING (1 << 7) /* This last one is specific to EP0 */ #define DWC3_EP0_DIR_IN (1 << 31) =20 @@ -1047,6 +1051,9 @@ struct dwc3_event_depevt { #define DEPEVT_TRANSFER_BUS_EXPIRY 2 =20 u32 parameters:16; + +/* For Command Complete Events */ +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 24)) >> 24) } __packed; =20 /** diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 3ba05b12e49a..8037bff43485 100644 =2D-- 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 |=3D DWC3_DALEPENA_EP(dep->number); dwc3_writel(dwc->regs, DWC3_DALEPENA, reg); =20 + init_waitqueue_head(&dep->wait_end_transfer); + if (usb_endpoint_xfer_control(desc)) return 0; =20 @@ -2156,6 +2158,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, { struct dwc3_ep *dep; u8 epnum =3D event->endpoint_number; + u8 cmd; =20 dep =3D dwc->eps[epnum]; =20 @@ -2200,8 +2203,13 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, return; } break; =2D case DWC3_DEPEVT_RXTXFIFOEVT: case DWC3_DEPEVT_EPCMDCMPLT: + cmd =3D DEPEVT_PARAMETER_CMD(event->parameters); + + if (cmd =3D=3D DWC3_DEPCMD_ENDTRANSFER) + dep->flags &=3D ~DWC3_EP_END_TRANSFER_PENDING; + break; + case DWC3_DEPEVT_RXTXFIFOEVT: break; } } @@ -2246,6 +2254,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc) } =20 static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool fo= rce) +__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 *d= wc, u32 epnum, bool force) memset(¶ms, 0, sizeof(params)); ret =3D dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); WARN_ON_ONCE(ret); + + dep->flags |=3D 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 =3D 0; dep->flags &=3D ~DWC3_EP_BUSY; =20 =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJX/2h7AAoJEMy+uJnhGpkGaqQP/03y7yw1hq8hMtCIUrz2h0GR g7zhSeSv1zqAUqppgh9G2l0/kIC/rlMO9zlofz7IqS/PAB7LE/nBOVIM2oSiqhBv xmApZRuakt13OW6Yrol+MyRYb0/9zCSgIoTSIf+u6/cpYHkMVW0RktrjfauV4TBH f8TeYzlz4rsivutaWh9/CgVrmm7erOvklmmbcLapTPYc9jHJgOrdGjZMo9BY9tRy 7bqVhCn0Q4frWIcaAu4uD8ShBBlojew67/m7dNArEr5O2z5D0/SMxvD13E5qSrhO foiJ1QLlM6eIjRXGoIGS84qPXcTirXrzgaJm0S6yG+4ZpQC/+HT9gU/cnM5kdwU2 0zYcEcrnLawe9ae7uRAZ7AsT32lp1Ksgcbyp2lqwTNG++k85VSE8QXCSUSQR5QjE q5r2ZOG/Vr3IIMPxOX/si/06QhpW4zm5fo6RRFNNR55qAIJe+Old9BICp3J7PEzF RqRCY9bLn/AQthA7MMclJ2ktK5mvaxKq/4NJ5a4v0wsCAhN6p/yp0MbCN5D9xJfK 03MwWa6pvHR6tqC89irTWbq/H4kvtta5a8vbHHvdx2rk50F23gfDnVgwu88OcSWL NJ/njp0kZIG/Q/iaHN4DuTADRy47rM8kr/hs394wkYnux4sQc/saUDN/bU4pXhrZ V7W+xOpHY/ayA4i/qyBH =sEyJ -----END PGP SIGNATURE----- --=-=-=--