From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751671AbcIICHT (ORCPT ); Thu, 8 Sep 2016 22:07:19 -0400 Received: from mail-yb0-f176.google.com ([209.85.213.176]:34190 "EHLO mail-yb0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750975AbcIICHQ (ORCPT ); Thu, 8 Sep 2016 22:07:16 -0400 MIME-Version: 1.0 In-Reply-To: <87oa3yha8d.fsf@linux.intel.com> References: <2cb239ba53679a6fae1f4e1f14968e63fcd59e6b.1473334354.git.baolin.wang@linaro.org> <87oa3yha8d.fsf@linux.intel.com> From: Baolin Wang Date: Fri, 9 Sep 2016 10:07:14 +0800 Message-ID: Subject: Re: [PATCH] usb: dwc3: gadget: Add disconnect checking when changing function dynamically To: Felipe Balbi Cc: Greg KH , Peter Hurley , 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 Hi Felipe, On 8 September 2016 at 20:00, 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. >> >> Also adding some disconnect checking to avoid queuing any requests when the >> gadget is stopped. >> >> Signed-off-by: Baolin Wang >> --- >> drivers/usb/dwc3/ep0.c | 8 ++++++++ >> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++++----- >> 2 files changed, 35 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >> index fe79d77..11519d7 100644 >> --- a/drivers/usb/dwc3/ep0.c >> +++ b/drivers/usb/dwc3/ep0.c >> @@ -228,6 +228,14 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request, >> int ret; >> >> spin_lock_irqsave(&dwc->lock, flags); >> + if (dwc->pullups_connected == false) { >> + dwc3_trace(trace_dwc3_ep0, >> + "queuing request %p to %s when gadget is disconnected", >> + request, dep->name); >> + ret = -ESHUTDOWN; >> + goto out; >> + } > > this could go on its own patch, however we can use if > (!dwc->pullups_connected) instead. Indeed. I will split it into one separate patch. > >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 1783406..bbac8f5 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1040,6 +1040,13 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >> struct dwc3 *dwc = dep->dwc; >> int ret; >> >> + if (dwc->pullups_connected == false) { >> + dwc3_trace(trace_dwc3_gadget, >> + "queuing request %p to %s when gadget is disconnected", >> + &req->request, dep->endpoint.name); >> + return -ESHUTDOWN; >> + } > > ditto OK. > >> @@ -1434,6 +1441,13 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) >> if (pm_runtime_suspended(dwc->dev)) >> return 0; >> >> + /* >> + * 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. >> + */ > > Do you have the section reference for this? Which databook version are > you reading? The databook version is 2.80a and you can find this description in section 8.1.8 "Device-Initiated Disconnect". > >> + if (!is_on && dwc->ep0state != EP0_SETUP_PHASE) >> + return -EBUSY; >> + >> reg = dwc3_readl(dwc->regs, DWC3_DCTL); >> if (is_on) { >> if (dwc->revision <= DWC3_REVISION_187A) { >> @@ -1481,12 +1495,20 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) >> struct dwc3 *dwc = gadget_to_dwc(g); >> unsigned long flags; >> int ret; >> + int timeout = 500; >> >> is_on = !!is_on; >> >> - spin_lock_irqsave(&dwc->lock, flags); >> - ret = dwc3_gadget_run_stop(dwc, is_on, false); >> - spin_unlock_irqrestore(&dwc->lock, flags); >> + do { >> + spin_lock_irqsave(&dwc->lock, flags); >> + ret = dwc3_gadget_run_stop(dwc, is_on, false); >> + spin_unlock_irqrestore(&dwc->lock, flags); >> + >> + if (ret != -EBUSY) >> + break; >> + >> + udelay(10); >> + } while (--timeout); > > no, this is not a good idea at all. I'd rather see: > > wait_event_timeout(dwc->wq, dwc->ep0state == EP0_SETUP_PHASE, > jiffies + msecs_to_jiffies(500)); > > or, perhaps, a wait_for_completion_timeout(). Then, ep0.c needs to call > complete() everytime Status Phase completes. That's probably a better > idea ;-) Yes, you are right. I will fix that with your suggestion. Thanks. > >> @@ -1990,7 +2012,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep, >> * dwc3_gadget_giveback(). If that happens, we're just gonna return 1 >> * early on so DWC3_EP_BUSY flag gets cleared >> */ >> - if (!dep->endpoint.desc) >> + if (!dep->endpoint.desc || dwc->pullups_connected == false) > > is this really necessary? Can we really get pullups_connect set to false > with dep->endpoint.desc still valid? Yes, when we start to unregister gadget by usb_gadget_unregister_driver(), the first thing is disconnect the gadget by usb_gadget_disconnect() to set pullups_connect as false, then will disable the endpoints by composite_disconnect() to set dep->endpoint.desc as NULL. When 'dwc->pullups_connected' is set false, we also need to avoiding transfer. > >> @@ -2064,7 +2086,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc, >> * dwc3_gadget_giveback(). If that happens, we're just gonna return 1 >> * early on so DWC3_EP_BUSY flag gets cleared >> */ >> - if (!dep->endpoint.desc) >> + if (!dep->endpoint.desc || dwc->pullups_connected == false) > > ditto > > -- > balbi -- Baolin.wang Best Regards