From patchwork Thu Oct 20 07:44:30 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "\(Exiting\) Baolin Wang" X-Patchwork-Id: 78430 Delivered-To: patch@linaro.org Received: by 10.140.97.247 with SMTP id m110csp645583qge; Thu, 20 Oct 2016 00:44:36 -0700 (PDT) X-Received: by 10.98.70.208 with SMTP id o77mr18787120pfi.177.1476949476240; Thu, 20 Oct 2016 00:44:36 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id o30si21911511pgd.34.2016.10.20.00.44.36; Thu, 20 Oct 2016 00:44:36 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-usb-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-usb-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-usb-owner@vger.kernel.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752401AbcJTHoe (ORCPT + 4 others); Thu, 20 Oct 2016 03:44:34 -0400 Received: from mail-yb0-f174.google.com ([209.85.213.174]:35448 "EHLO mail-yb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750793AbcJTHod (ORCPT ); Thu, 20 Oct 2016 03:44:33 -0400 Received: by mail-yb0-f174.google.com with SMTP id 184so18504895yby.2 for ; Thu, 20 Oct 2016 00:44:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=qPzQqP3/NO0azSLaXcYwIMfffFyeKmyRvZDqKaO8jKY=; b=U4ZbCbU/DuPb2lhbfMz9M4FnuujKR2tkag08BQCtKH4pR6ztMpmsq9Fhhc+ayEWAN4 2uBrtrxRxzI87PZjANlhz/Q62gGbavMy5PFRM8xZb+SxDeIKcg5oHEpAViqOhzy04sDV KNOxkEmocjY26j8puRF1c1zQRn/ldGfK1f9xM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=qPzQqP3/NO0azSLaXcYwIMfffFyeKmyRvZDqKaO8jKY=; b=YcCOI0ZUEyynjI5FV3lTfUYiBofkEKpWriC6LfnemwvwVmj5W7o70/jv5CukbYh9sZ LK1b814jPaqnGSxnL5Mk6x0iu0gQHehs/aFq96dOKFuXiUHSZ93w/wAUYkq2fmxfL5g0 mpIqZdUzDNwuwa5yI0ux83/Ij+shF1+8VD1qvbgJhwT0OfpU8PnyjpdwqL1pPA2Ng4Nj SyaepcJMVB88L5ARkStXkF9nTNPMdzh28TfRb3uL6B9JzcVIV42VYTFcvpK/b8/oLysP UtG9B952nJtvxZSssqULWUvoV1KETxmmZpgD+9JOOBqUY9Y/ooqlL9sls/0XAoCYDiWR rPKg== X-Gm-Message-State: AA6/9RmaDSfBBTTkEEosQGkJkrFq0cePrjdeu5LaPpRuOTeanPhl3+vnUdelLY6YLCn6F/rSSFeZw3fzYzUiyuKP X-Received: by 10.37.65.21 with SMTP id o21mr10307424yba.65.1476949471297; Thu, 20 Oct 2016 00:44:31 -0700 (PDT) MIME-Version: 1.0 Received: by 10.129.145.78 with HTTP; Thu, 20 Oct 2016 00:44:30 -0700 (PDT) In-Reply-To: <87a8e0ps4k.fsf@linux.intel.com> References: <87y41mqg39.fsf@linux.intel.com> <87vawqqd8q.fsf@linux.intel.com> <87a8e0ps4k.fsf@linux.intel.com> From: Baolin Wang Date: Thu, 20 Oct 2016 15:44:30 +0800 Message-ID: Subject: Re: usb_ep_{dis, en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete) To: Felipe Balbi Cc: Alan Stern , Linux USB , Greg Kroah-Hartman , John Youn , Peter Chen Sender: linux-usb-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org Hi, On 19 October 2016 at 18:09, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >> >> We should not check the DWC3_EP_ENABLED flag, since we will clear all >> flags in __dwc3_gadget_ep_disable() after setting >> DWC3_EP_END_TRANSFER_PENDING flags in dwc3_stop_active_transfer(). > > good catch. > >> >>> + >>> + if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) >>> + continue; >> >> Ditto. > > yeah, END_TRANSFER_PENDING should only be cleared by command completion, > so ep_disable needs to be patched. > >>> + >>> + 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; >> >> Here we should add below modification, or the event can not be handled. >> >> - if (!(dep->flags & DWC3_EP_ENABLED)) >> + if (!(dep->flags & DWC3_EP_ENABLED) && >> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) >> return; > > this seems unnecessary to me. The only interrupt with a disabled > endpoint should be due to END_TRANSFER cmd completion. In fact, I'm now > thinking that maybe usb_ep_disable() should also block until EndTransfer > completes. > > This would means replacing wake_up() with wake_up_all() > >>> I'll run some tests with this in a couple hours. >> >> I would like to send out new version based on my original patch >> according to your suggestion, or you can send out new version I can >> help to test. Thanks. > > From ce24ab50d57a18287a99ea776e9bdc7d5cfd282c Mon Sep 17 00:00:00 2001 > From: Baolin Wang > 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. > > [ felipe.balbi@linux.intel.com: minor improvements ] > > NYET-Signed-off-by: Baolin Wang > Signed-off-by: Felipe Balbi > --- > drivers/usb/dwc3/core.h | 8 ++++++++ > drivers/usb/dwc3/gadget.c | 40 +++++++++++++++++++++++++++++++++++++--- > 2 files changed, 45 insertions(+), 3 deletions(-) > > 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 > #include > #include > +#include > > #include > #include > @@ -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..bc985f8571c6 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; > > @@ -772,6 +774,10 @@ static int dwc3_gadget_ep_disable(struct usb_ep *ep) > dep->name)) > return 0; > > + if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) > + wait_event(dep->wait_end_transfer, > + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)); > + We will set DWC3_EP_END_TRANSFER_PENDING flag in dwc3_stop_active_transfer() function (function stack: dwc3_gadget_ep_disable()--->__dwc3_gadget_ep_disable()--->dwc3_remove_requests()---->dwc3_stop_active_transfer()), but it will cleane all flags in __dwc3_gadget_ep_disable() function. So when issuing __dwc3_gadget_stop(), we can not check DWC3_EP_END_TRANSFER_PENDING flag to wait for command complete event. > spin_lock_irqsave(&dwc->lock, flags); > ret = __dwc3_gadget_ep_disable(dep); > spin_unlock_irqrestore(&dwc->lock, flags); > @@ -1782,13 +1788,30 @@ static int dwc3_gadget_start(struct usb_gadget *g, > } > > static void __dwc3_gadget_stop(struct dwc3 *dwc) > +__releases(&dwc->lock) __acquires(&dwc->lock) > { > + 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); > + } When gadget suspend to stop gadget, we will never get the command complete event, since we will exit the dwc3 core. Thus we can move this checking into dwc3_gadget_stop() and we will clean the DWC3_EP_END_TRANSFER_PENDING flag when re-enable the endpoint. I did one patch based on your patch which did some modification, and I tested the usb function changing and suspend/resume, it can work well. Could you check it if it is OK? Thanks. >From 6fdb5cd4216755f07bb1a1e5d081e269046c60c6 Mon Sep 17 00:00:00 2001 Message-Id: <6fdb5cd4216755f07bb1a1e5d081e269046c60c6.1476949020.git.baolin.wang@linaro.org> From: Baolin Wang Date: Thu, 20 Oct 2016 15:35:54 +0800 Subject: [PATCH v1] 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. [ felipe.balbi@linux.intel.com: minor improvements ] Signed-off-by: Baolin Wang --- drivers/usb/dwc3/core.h | 8 ++++++++ drivers/usb/dwc3/gadget.c | 47 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 51 insertions(+), 4 deletions(-) /* @@ -2277,8 +2314,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_stop_active_transfers(struct dwc3 *dwc) -- 1.7.9.5 -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 23765a1..c5fd862 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -496,6 +497,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 @@ -521,6 +523,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; @@ -537,6 +541,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) @@ -1044,6 +1049,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 3722c90..018611d 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -570,11 +570,14 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, dep->comp_desc = comp_desc; dep->type = usb_endpoint_type(desc); dep->flags |= DWC3_EP_ENABLED; + dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING; reg = dwc3_readl(dwc->regs, DWC3_DALEPENA); 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; @@ -647,7 +650,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) dep->endpoint.desc = NULL; dep->comp_desc = NULL; dep->type = 0; - dep->flags = 0; + dep->flags &= DWC3_EP_END_TRANSFER_PENDING; return 0; } @@ -1736,10 +1739,34 @@ static int dwc3_gadget_stop(struct usb_gadget *g) { struct dwc3 *dwc = gadget_to_dwc(g); unsigned long flags; + int epnum; spin_lock_irqsave(&dwc->lock, flags); __dwc3_gadget_stop(dwc); dwc->gadget_driver = NULL; + + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) { + struct dwc3_ep *dep = dwc->eps[epnum]; + + /* + * If the dwc3 core has been in suspend state, we will never get + * the command complete event, thus ignore. + */ + if (pm_runtime_suspended(dwc->dev)) + break; + + if (!dep) + 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); + } + spin_unlock_irqrestore(&dwc->lock, flags); free_irq(dwc->irq_gadget, dwc->ev_buf); @@ -2105,10 +2132,12 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, { struct dwc3_ep *dep; u8 epnum = event->endpoint_number; + u8 cmd; dep = dwc->eps[epnum]; - if (!(dep->flags & DWC3_EP_ENABLED)) + if (!(dep->flags & DWC3_EP_ENABLED) && + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) return; if (epnum == 0 || epnum == 1) { @@ -2180,6 +2209,13 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, dwc3_trace(trace_dwc3_gadget, "%s FIFO Overrun", dep->name); break; 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); + } + dwc3_trace(trace_dwc3_gadget, "Endpoint Command Complete"); break; } @@ -2233,7 +2269,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;