Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934156AbcJQLsF (ORCPT ); Mon, 17 Oct 2016 07:48:05 -0400 Received: from mail-yb0-f174.google.com ([209.85.213.174]:34908 "EHLO mail-yb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932794AbcJQLr5 (ORCPT ); Mon, 17 Oct 2016 07:47:57 -0400 MIME-Version: 1.0 In-Reply-To: <87mvi3s2vb.fsf@linux.intel.com> References: <87mvi3s2vb.fsf@linux.intel.com> From: Baolin Wang Date: Mon, 17 Oct 2016 19:47:55 +0800 Message-ID: Subject: Re: [PATCH v4] usb: dwc3: Wait for control tranfer completed when stopping gadget 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: 5184 Lines: 138 Hi, On 17 October 2016 at 18:10, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >> When we change the USB function with configfs dynamically, we possibly met this >> situation: one core is doing the control transfer, another core is trying to >> unregister the USB gadget from userspace, we must wait for completing this >> control tranfer, or it will hang the controller to set the DEVCTRLHLT flag. >> >> Signed-off-by: Baolin Wang > > Can you make sure this still works? With applying this patch, It can work well on my platform, but I have some worries about the risk of accessing 'dwc->ep0state' without lock protection in dwc3_gadget_pullup() function. > > 8<------------------------------------------------------------------------ > From 797c7caab630f650b9ab5e462e8588462e055073 Mon Sep 17 00:00:00 2001 > From: Baolin Wang > Date: Fri, 14 Oct 2016 17:11:33 +0800 > Subject: [PATCH] usb: dwc3: gadget: don't clear RUN/STOP when it's invalid to > do so > > When we change the USB function with configfs dynamically, we possibly > met this situation: one core is doing the control transfer, another core > is trying to unregister the USB gadget from userspace, we must wait for > completing this control tranfer, or it will hang the controller to set > the DEVCTRLHLT flag. > > [ felipe.balbi@linux.intel.com: several fixes to the patch > - call complete() before starting following SETUP transfer > - add a macro for ep0_in_setup's timeout > - change commit subject slightly > - break lines at 72 characters (git adds an 8-character tab) > - avoid changes to dwc3_gadget_run_stop() ] > > Signed-off-by: Baolin Wang > Signed-off-by: Felipe Balbi > --- > drivers/usb/dwc3/core.h | 3 +++ > drivers/usb/dwc3/ep0.c | 2 ++ > drivers/usb/dwc3/gadget.c | 17 +++++++++++++++++ > 3 files changed, 22 insertions(+) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 2f6f7c4bc8d4..80e4e9aa2d33 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -38,6 +38,7 @@ > #define DWC3_MSG_MAX 500 > > /* Global constants */ > +#define DWC3_PULL_UP_TIMEOUT 500 /* ms */ > #define DWC3_ZLP_BUF_SIZE 1024 /* size of a superspeed bulk */ > #define DWC3_EP0_BOUNCE_SIZE 512 > #define DWC3_ENDPOINTS_NUM 32 > @@ -756,6 +757,7 @@ struct dwc3_scratchpad_array { > * @ep0_usb_req: dummy req used while handling STD USB requests > * @ep0_bounce_addr: dma address of ep0_bounce > * @scratch_addr: dma address of scratchbuf > + * @ep0_in_setup: one control transfer is completed and enter setup phase > * @lock: for synchronizing > * @dev: pointer to our struct device > * @xhci: pointer to our xHCI child > @@ -853,6 +855,7 @@ struct dwc3 { > dma_addr_t ep0_bounce_addr; > dma_addr_t scratch_addr; > struct dwc3_request ep0_usb_req; > + struct completion ep0_in_setup; > > /* device lock */ > spinlock_t lock; > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c > index 5e642d65a3b2..417cfd3f04ab 100644 > --- a/drivers/usb/dwc3/ep0.c > +++ b/drivers/usb/dwc3/ep0.c > @@ -284,6 +284,8 @@ void dwc3_ep0_out_start(struct dwc3 *dwc) > { > int ret; > > + complete(&dwc->ep0_in_setup); > + > ret = dwc3_ep0_start_trans(dwc, 0, dwc->ctrl_req_addr, 8, > DWC3_TRBCTL_CONTROL_SETUP, false); > WARN_ON(ret < 0); > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index b53712cbc499..da79716be50d 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1557,6 +1557,21 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) > > is_on = !!is_on; > > + /* > + * Per databook, when we want to stop the gadget, if a control transfer > + * is still in process, complete it and get the core into setup phase. > + */ > + if (!is_on && dwc->ep0state != EP0_SETUP_PHASE) { > + reinit_completion(&dwc->ep0_in_setup); > + > + ret = wait_for_completion_timeout(&dwc->ep0_in_setup, > + msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT)); > + if (ret == 0) { > + dev_err(dwc->dev, "timed out waiting for SETUP phase\n"); > + return -ETIMEDOUT; > + } > + } > + > spin_lock_irqsave(&dwc->lock, flags); > ret = dwc3_gadget_run_stop(dwc, is_on, false); > spin_unlock_irqrestore(&dwc->lock, flags); > @@ -2954,6 +2969,8 @@ int dwc3_gadget_init(struct dwc3 *dwc) > goto err4; > } > > + init_completion(&dwc->ep0_in_setup); > + > dwc->gadget.ops = &dwc3_gadget_ops; > dwc->gadget.speed = USB_SPEED_UNKNOWN; > dwc->gadget.sg_supported = true; > -- > 2.10.0.440.g21f862b > > > > -- > balbi -- Baolin.wang Best Regards