Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1168907AbcKALQh (ORCPT ); Tue, 1 Nov 2016 07:16:37 -0400 Received: from mail-yw0-f175.google.com ([209.85.161.175]:34422 "EHLO mail-yw0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1168761AbcKALQf (ORCPT ); Tue, 1 Nov 2016 07:16:35 -0400 MIME-Version: 1.0 In-Reply-To: <87twbrpipj.fsf@linux.intel.com> References: <87twbrpipj.fsf@linux.intel.com> From: Baolin Wang Date: Tue, 1 Nov 2016 19:16:33 +0800 Message-ID: Subject: Re: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete To: Felipe Balbi Cc: Greg KH , Mark Brown , USB , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8028 Lines: 238 Hi, On 1 November 2016 at 19:01, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >> Changes since v1: >> - Move the suspend checking to right place to avoid checking twice. > > there is still one problem > >> @@ -1736,12 +1739,38 @@ 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); > > this tries to access registers. If __dwc3_gadget_stop() runs while > clocks are gated, we will get Data Abort exception. We have suspend checking in __dwc3_gadget_stop(), thus it will not access registers if clocks are disabled. > > How about this version, instead? I think it is OK except we missed set 'dwc->gadget_driver = NULL'. > > 8<------------------------------------------------------------------------------ > From e21260f349271ff1d12e919be20a9ee47e29e4b4 Mon Sep 17 00:00:00 2001 > From: Baolin Wang > Date: Mon, 31 Oct 2016 19:38:36 +0800 > 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 ] > > Signed-off-by: Baolin Wang > Signed-off-by: Felipe Balbi > --- > drivers/usb/dwc3/core.h | 8 ++++++++ > drivers/usb/dwc3/gadget.c | 49 +++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 49 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 5fc437021ac7..c2b86856e85d 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 4743e53cc295..a375fd28ed96 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -593,11 +593,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; > > @@ -699,7 +702,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; > } > @@ -1783,9 +1786,6 @@ static int dwc3_gadget_start(struct usb_gadget *g, > > static void __dwc3_gadget_stop(struct dwc3 *dwc) > { > - 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]); > @@ -1795,10 +1795,31 @@ 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); > + > + if (pm_runtime_suspended(dwc->dev)) > + goto out; > + > __dwc3_gadget_stop(dwc); > dwc->gadget_driver = NULL; > + > + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) { > + struct dwc3_ep *dep = dwc->eps[epnum]; > + > + 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); > + } > + > +out: > spin_unlock_irqrestore(&dwc->lock, flags); > > free_irq(dwc->irq_gadget, dwc->ev_buf); > @@ -2171,10 +2192,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) { > @@ -2215,8 +2238,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 +2299,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 +2344,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) > -- > 2.10.1 > > > > -- > balbi -- Baolin.wang Best Regards