Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752729AbcJNJuz (ORCPT ); Fri, 14 Oct 2016 05:50:55 -0400 Received: from mga04.intel.com ([192.55.52.120]:24350 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750882AbcJNJur (ORCPT ); Fri, 14 Oct 2016 05:50:47 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,344,1473145200"; d="asc'?scan'208";a="1070343189" From: Felipe Balbi To: Baolin Wang Cc: Alan Stern , Greg KH , Mark Brown , USB , LKML Subject: Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq In-Reply-To: References: <87r37kviic.fsf@linux.intel.com> <87inswvg80.fsf@linux.intel.com> <87a8e8vaif.fsf@linux.intel.com> <877f9cv7no.fsf@linux.intel.com> <87y41struu.fsf@linux.intel.com> <87shs0tltb.fsf@linux.intel.com> <87oa2ntlty.fsf@linux.intel.com> Date: Fri, 14 Oct 2016 12:50:04 +0300 Message-ID: <87lgxrtg37.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: 9986 Lines: 261 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Baolin Wang writes: >> Baolin Wang writes: >>>>>>>>>>> I see what the problem is. Databook tells us we *must* set CMDI= OC when >>>>>>>>>>> issuing EndTransfer command and we should always wait for Comma= nd >>>>>>>>>>> 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 chang= ing >>>>>>>>>> function with configfs frequently, we will met problems. >>>>>>>>> >>>>>>>>> heh, 100us *is* enough. However we still get an IRQ because we re= quested >>>>>>>>> for it. If you want to fix this, then you need to find a way to g= et 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 >>>>>>> >>>>>>> Yes, maybe we need check DWC3_EP_END_TRANSFER_PENDING flag when >>>>>>> queuing one request. >>>>>> >>>>>> Yeah, I'll add that check later. I still need to make sure we would >>>>>> still try to kick any transfers that might have been queued while >>>>>> waiting for End Transfer Command Complete IRQ. >>>>> >>>>> OK. But I still worried if there are other races in some places which >>>> >>>> There are no other places where this needs to be sorted out. >>>> >>>>> is not easy to find out by testing. No introducing race condition >>>>> would be one better solution, anyway I would like to test the patch >>>>> you send out firstly. >>>> >>>> Right, but your patch was working around another problem, rather then >>>> fixing it. >>>> >>>> Here's another version which should make sure everything remains worki= ng >>>> as it should. >>>> >>>> 8<--------------------------------------------------------------------= ---- >>>> From 1f922a902a21d5cee32082be18bbfbf1d46137e4 Mon Sep 17 00:00:00 2001 >>>> From: Felipe Balbi >>>> Date: Thu, 13 Oct 2016 14:09:47 +0300 >>>> Subject: [PATCH] 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 >>>> Signed-off-by: Felipe Balbi >>>> --- >>>> drivers/usb/dwc3/core.h | 10 +++++++++- >>>> drivers/usb/dwc3/gadget.c | 31 ++++++++++++++++++++++++++++--- >>>> 2 files changed, 37 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>>> index e878366ead00..cf495d932252 100644 >>>> --- a/drivers/usb/dwc3/core.h >>>> +++ b/drivers/usb/dwc3/core.h >>>> @@ -26,6 +26,7 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> >>>> #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 = 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,8 @@ 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) >>>> +#define DWC3_EP_KICK_POST_END_TRANSFER (1 << 8) >>>> /* This last one is specific to EP0 */ >>>> #define DWC3_EP0_DIR_IN (1 << 31) >>>> >>>> @@ -1047,6 +1052,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..a3f81b5ba901 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 |=3D 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; >>>> >>>> @@ -1151,6 +1153,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_e= p *dep, struct dwc3_request *req) >>>> if (!dwc3_calc_trbs_left(dep)) >>>> return 0; >>>> >>>> + if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) { >>>> + dep->flags &=3D DWC3_EP_KICK_POST_END_TRANSFER; >>>> + return 0; >>>> + } >>>> + >>>> ret =3D __dwc3_gadget_kick_transfer(dep, 0); >>>> if (ret && ret !=3D -EBUSY) >>>> dwc3_trace(trace_dwc3_gadget, >>>> @@ -2156,6 +2163,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 = *dwc, >>>> { >>>> struct dwc3_ep *dep; >>>> u8 epnum =3D event->endpoint_number; >>>> + u8 cmd; >>>> >>>> dep =3D dwc->eps[epnum]; >>>> >>>> @@ -2200,8 +2208,13 @@ static void dwc3_endpoint_interrupt(struct dwc3= *dwc, >>>> return; >>>> } >>>> break; >>>> - 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; >>> >>> I think you missed wake_up(&dep->wait_end_transfer) function here. >>> >>>> + break; >>>> + case DWC3_DEPEVT_RXTXFIFOEVT: >>>> break; >>>> } >>>> } >>>> @@ -2246,6 +2259,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc) >>>> } >>>> >>>> static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bo= ol force) >>>> +__releases(&dwc->lock) __acquires(&dwc->lock) >>>> { >>>> struct dwc3_ep *dep; >>>> struct dwc3_gadget_ep_cmd_params params; >>>> @@ -2295,11 +2309,22 @@ static void dwc3_stop_active_transfer(struct d= wc3 *dwc, u32 epnum, bool force) >>>> memset(¶ms, 0, sizeof(params)); >>>> ret =3D dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); >>>> WARN_ON_ONCE(ret); >>>> + >>>> + if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) { >>>> + dep->flags |=3D DWC3_EP_END_TRANSFER_PENDING; >>>> + wait_event_lock_irq(dep->wait_end_transfer, >>>> + !(dep->flags & DWC3_EP_END_TRANSFER_PE= NDING), >>>> + dwc->lock); >>>> + } >>> >>> I've tested your patch, unfortunately it can not work on my platform. >>> Since we can not issue wait_event_lock_irq() function in atomic >>> context, we will hold another 'cdev->lock' in composite_disconnect() >>> function. The dump stack as below: >> >> argh, we have nested spinlocks :-( Well, we shouldn't call >> usb_ep_disable() with locks held AFAICR. So the following should be >> enough: >> >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composi= te.c >> index 919d7d1b611c..2e9359c58eb9 100644 >> --- a/drivers/usb/gadget/composite.c >> +++ b/drivers/usb/gadget/composite.c >> @@ -732,8 +732,10 @@ static void reset_config(struct usb_composite_dev *= cdev) >> DBG(cdev, "reset config\n"); >> >> list_for_each_entry(f, &cdev->config->functions, list) { >> + spin_unlock_irq(&cdev->lock); >> if (f->disable) >> f->disable(f); >> + spin_lock_irq(&cdev->lock); >> >> bitmap_zero(f->endpoints, 32); >> } > > After applying above modification, there are no obvious errors, but I > still met problem: I can not change usb function with configfs. I need > some time to find out why. I'm also looking into this and testing it out here on SKL. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYAKpMAAoJEMy+uJnhGpkGGtIQAK8IiwQIOt2uTN12MJu3JQfo Nk9gKt6ZTb5IK4G5vFMcR9hWj4GM6WLeVxSoM02A+o/V8qm5yE+1NKqFfarvaHCv ZcFBFNw5d7OLHSAEbLgzUcD+6VrOD1443fX20LQHK71FO5W+gwX0RjXvkmgxuXXS 3LwQQBGCqUVLudF+A3k7ypjz3u8trf2gWA6vkoboE3u2w6RFC5LWZFzXLkjZdnz8 3Bnn4xeIRo1oGJAwbtS16EaYGMh6CH42wb3e3P3eoFDubCjVeO1wETQBCLwoMP9C Fh+ILyu8j+alWRY2Tk35nJOg2ItfodgyuMXXc4z7TRFAkk8XvUvuD2OKdqUxzDjy IdWuf4NFvmwBXcRlwi5VkzbsOMgm6HAUMJ/VN1FGlcfZcxuhEeWqa5I4QcLxfOKp RXOX94+WclR6+L9CVyV5dlfsD4QKpyyqDvfoCQJKA3zcVq8l1lYgij4KmAvkuxfr HQNfiMBitIaBaoFRaIvceFhXMG2CIoEG5cliwRljO+wFRomnumcARmCMDoGo/xCz Uuue2jdL4YCu4LIf3GiEV8cCBr9GQv5RymQ+JstZ3GOs2Da/fDbeL3HzHoh+pTov ZqWwABWzScby6sd1pUj2kiO85oiA6pDzzf6K6Q5BVAESm7MrhMl/07EU9+ZirM3H CuU1OBvHMcZc2YHNb6qr =bIe3 -----END PGP SIGNATURE----- --=-=-=--