From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941730AbcIHMB3 (ORCPT ); Thu, 8 Sep 2016 08:01:29 -0400 Received: from mga09.intel.com ([134.134.136.24]:61670 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S941709AbcIHMBZ (ORCPT ); Thu, 8 Sep 2016 08:01:25 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,300,1470726000"; d="asc'?scan'208";a="5990683" From: Felipe Balbi To: Baolin Wang , gregkh@linuxfoundation.org Cc: peter@hurleysoftware.com, broonie@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, baolin.wang@linaro.org Subject: Re: [PATCH] usb: dwc3: gadget: Add disconnect checking when changing function dynamically In-Reply-To: <2cb239ba53679a6fae1f4e1f14968e63fcd59e6b.1473334354.git.baolin.wang@linaro.org> References: <2cb239ba53679a6fae1f4e1f14968e63fcd59e6b.1473334354.git.baolin.wang@linaro.org> User-Agent: Notmuch/0.22.1+63~g994277e (https://notmuchmail.org) Emacs/25.1.3 (x86_64-pc-linux-gnu) Date: Thu, 08 Sep 2016 15:00:50 +0300 Message-ID: <87oa3yha8d.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Baolin Wang writes: > When we change the USB function with configfs dynamically, we possibly me= t 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 fla= g. > > Also adding some disconnect checking to avoid queuing any requests when t= he > 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; >=20=20 > spin_lock_irqsave(&dwc->lock, flags); > + if (dwc->pullups_connected =3D=3D false) { > + dwc3_trace(trace_dwc3_ep0, > + "queuing request %p to %s when gadget is disconnected", > + request, dep->name); > + ret =3D -ESHUTDOWN; > + goto out; > + } this could go on its own patch, however we can use if (!dwc->pullups_connected) instead. > 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 =3D dep->dwc; > int ret; >=20=20 > + if (dwc->pullups_connected =3D=3D false) { > + dwc3_trace(trace_dwc3_gadget, > + "queuing request %p to %s when gadget is disconnected", > + &req->request, dep->endpoint.name); > + return -ESHUTDOWN; > + } ditto > @@ -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; >=20=20 > + /* > + * 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? > + if (!is_on && dwc->ep0state !=3D EP0_SETUP_PHASE) > + return -EBUSY; > + > reg =3D dwc3_readl(dwc->regs, DWC3_DCTL); > if (is_on) { > if (dwc->revision <=3D DWC3_REVISION_187A) { > @@ -1481,12 +1495,20 @@ static int dwc3_gadget_pullup(struct usb_gadget *= g, int is_on) > struct dwc3 *dwc =3D gadget_to_dwc(g); > unsigned long flags; > int ret; > + int timeout =3D 500; >=20=20 > is_on =3D !!is_on; >=20=20 > - spin_lock_irqsave(&dwc->lock, flags); > - ret =3D dwc3_gadget_run_stop(dwc, is_on, false); > - spin_unlock_irqrestore(&dwc->lock, flags); > + do { > + spin_lock_irqsave(&dwc->lock, flags); > + ret =3D dwc3_gadget_run_stop(dwc, is_on, false); > + spin_unlock_irqrestore(&dwc->lock, flags); > + > + if (ret !=3D -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 =3D=3D 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 ;-) > @@ -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 =3D=3D false) is this really necessary? Can we really get pullups_connect set to false with dep->endpoint.desc still valid? > @@ -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 =3D=3D false) ditto =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJX0VLyAAoJEMy+uJnhGpkG/ZUQAN8/lt3gHaGSYnnpE/xFx7MA N7fCRj1infFXvdQ0aMrNeWQA4ypTUrM4GiycVZrVXsEPY3PVagAxMYILAkYB4l3c 3ZhVktVsFXUhtZu9+xFTgQ2cyiyrfYnDHowG98UUEZGRrEb/UKehHW1TaFrhG8FB cAJcMxksv4heSov0GnetJJlXWUtbf2318VMX7a9MCCPzDIu1JJykN/6JetzRkWBv MqwQqWyga8KWfAka6zewyfpAUKGC2JyJSECJmlJielRhmR9usl4c/vVdiViE8yi6 62whmzBFJaNg2Tvk0KjrNIyIfNuRbDW8NUzn5kLgDh+PAJ9ibs9ZBy/TtZmdimaS Ahozkss8UpuTAtO32yD3epaKDBEEg84Ufi9bKj08rXZIV82+K9LooJ9aEu5tVUxV OUYOawnzKcNkIXICmNqAuKl3NvN50JjwEpf6Er/HxwHWk/RdzCX5T1ZHRGWCP9tS nOKx5+Th8tys4FLFsNMId/IiJdwRY5fhRQpH/qC/l3fbV850tqzUEp9gDo7sdbFn ZxRwuhjx8poWR8oAQ+2/I2Zl9Z4RD+AVzPgMmErXh1q9dXacf0VfdK7CjXhvj66h lHgqnRSJc4JM97hdCNE4qMNkz6LL+vETZtoimN5TRl8R+aImc13Loq1LN6eBWNj6 Txrc0Qe6DKChPrkJDY5R =TLuu -----END PGP SIGNATURE----- --=-=-=--