Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754358AbcJMLSw (ORCPT ); Thu, 13 Oct 2016 07:18:52 -0400 Received: from mail-yb0-f177.google.com ([209.85.213.177]:34539 "EHLO mail-yb0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754033AbcJMLS2 (ORCPT ); Thu, 13 Oct 2016 07:18:28 -0400 MIME-Version: 1.0 In-Reply-To: <877f9cv7no.fsf@linux.intel.com> References: <87r37kviic.fsf@linux.intel.com> <87inswvg80.fsf@linux.intel.com> <87a8e8vaif.fsf@linux.intel.com> <877f9cv7no.fsf@linux.intel.com> From: Baolin Wang Date: Thu, 13 Oct 2016 19:16:44 +0800 Message-ID: Subject: Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq 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: 5903 Lines: 163 On 13 October 2016 at 18:56, Felipe Balbi wrote: > > 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 Yes, maybe we need check DWC3_EP_END_TRANSFER_PENDING flag when queuing one request. > 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? Sure. I will test it and send out the result tomorrow. > > 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 > #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,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(¶ms, 0, sizeof(params)); > ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > 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 -- Baolin.wang Best Regards