From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754053AbcIILEV (ORCPT ); Fri, 9 Sep 2016 07:04:21 -0400 Received: from mga07.intel.com ([134.134.136.100]:42660 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753733AbcIILES (ORCPT ); Fri, 9 Sep 2016 07:04:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,304,1470726000"; d="asc'?scan'208";a="758780288" From: Felipe Balbi To: Baolin Wang , gregkh@linuxfoundation.org Cc: broonie@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, baolin.wang@linaro.org Subject: Re: [PATCH v2 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget In-Reply-To: <3a4c91231787d31e82874161aa2ef9351f1c4b73.1473405255.git.baolin.wang@linaro.org> References: <3a4c91231787d31e82874161aa2ef9351f1c4b73.1473405255.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: Fri, 09 Sep 2016 14:03:31 +0300 Message-ID: <871t0tgwsc.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: > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 057739d..22787b6 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -999,6 +999,7 @@ static int dwc3_probe(struct platform_device *pdev) > goto err0; >=20=20 > spin_lock_init(&dwc->lock); > + init_completion(&dwc->ep0_completed); this should be done only when gadget is required; meaning that this should be moved to dwc3_gadget_init() > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index b2317e7..858e661 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h [...] > @@ -843,6 +844,7 @@ struct dwc3 { > dma_addr_t ep0_bounce_addr; > dma_addr_t scratch_addr; > struct dwc3_request ep0_usb_req; > + struct completion ep0_completed; when you call this "ep0_completed" it seems like you're defining a flag, but you're not :) How about "ep0_in_setup" instead? That conveys the idea that we're waiting for ep0 to reach setup phase. > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c > index 632e5a4..baf932d 100644 > --- a/drivers/usb/dwc3/ep0.c > +++ b/drivers/usb/dwc3/ep0.c > @@ -286,6 +286,7 @@ static void dwc3_ep0_stall_and_restart(struct dwc3 *d= wc) >=20=20 > dwc->ep0state =3D EP0_SETUP_PHASE; > dwc3_ep0_out_start(dwc); > + complete(&dwc->ep0_completed); no, this is wrong. I see what you're trying to do here, but we don't want to duplicate this call to complete() right? One thing we can realize is that *always* after STATUS phase or after a STALL, we will go through dwc3_ep0_out_start(), this mean we can call complete() before starting the following SETUP phase. Single place to call complete() ;-) > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 1a33308..c9026ce 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1441,6 +1441,15 @@ 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. > + */ > + if (!is_on && dwc->ep0state !=3D EP0_SETUP_PHASE) { > + reinit_completion(&dwc->ep0_completed); this seems unnecessary to me. Also, why return here so the caller has to wait? You could just have called wait_for_completion() here straight away: if (!is_on && dwc->ep0state !=3D EP0_SETUP_PHASE) { /* should this be interruptible? */ ret =3D wait_for_completion_timeout(&dwc->ep0_in_setup, msecs_to_jiffies(500)); if (ret =3D=3D 0) { dwc3_trace(trace_dwc3_gadget, "RUN/STOP timeout"); return -ETIMEDOUT; } }=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 There's also no need for that "try_again" trickery. We either can halt the controller within 500ms or we cannot. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJX0pcDAAoJEMy+uJnhGpkGFKYP/3OSpQi0gtAK6lpFRqO+DzPf 4ddx1WoKYTNVx2/tTo/xQljpt7bedZNxVJgXaKjfkzGgLtfWWCwq9gCYGp/pokjF sD+M2mILYfGIftr8Rjhg2ABGME4gRzGGWYEy38jXeKydCF26dW679QaCN/fsK8wr d8+UlAHnPV3kbuY+2J/ffNE4+Zfr3On/zcraF1iuYZbIhq6IlEzVmQdvl35vIVfh gtQfCs76KD11xMMYovsCUX9CLTQf9npdd/IQqJfYy/kFGRQ4bTabpbBpLIQWsSia KySw8JzVNmTYqhFF8HAbcLrNgCEj6QsDUvGoC0tVQg/9LiZmz9z8EK0VmL+6R0Hj Gg3jV0CucuLVY2jz/LJ1X0QQIOV2wkIhs1eBBPC7k6rOgHp9LAOY8PWT+qgZ/Sv/ Uj01+/N+kc+GJjOgJltn1/5iTsolkcScAHjCxmPOyI4cxuTJFwRSmLsBzhVGzvyp oy4mesY5i/04eP2a8jwYcHVC53ACU6oDPaWhANidEaUk0zN1bbwDlJWUoEX/4Qd6 9j0/nhIAGquFQdG/qjn7NMknhuZiEm5HQdey0V7aSOfP0+B/R+LBXvEf6JhI1T6g Sor/vh09MbQpuoOo8g6JZ+2mJusXIuMABf/e8mUsBsh2UaMlwaEJ6k1sFTkmQ6K+ iKIQjsfoA6Fw+yfDZJ0k =17zU -----END PGP SIGNATURE----- --=-=-=--