Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Felipe Balbi <balbi@kernel.org>
To:     Wesley Cheng <wcheng@codeaurora.org>, gregkh@linuxfoundation.org
Cc:     linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        jackp@codeaurora.org, Wesley Cheng <wcheng@codeaurora.org>
Subject: Re: [PATCH] usb: dwc3: Stop active transfers before halting the
 controller
In-Reply-To: <20200819051739.22123-1-wcheng@codeaurora.org>
References: <20200819051739.22123-1-wcheng@codeaurora.org>
Date:   Wed, 19 Aug 2020 14:37:11 +0300
Message-ID: <87zh6qyihk.fsf@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


Hi,

Wesley Cheng <wcheng@codeaurora.org> writes:
> In the DWC3 databook, for a device initiated disconnect, the driver is
> required to send dependxfer commands for any pending transfers.
> In addition, before the controller can move to the halted state, the SW
> needs to acknowledge any pending events.  If the controller is not halted
> properly, there is a chance the controller will continue accessing stale =
or
> freed TRBs and buffers.
>
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>
> ---
> Verified fix by adding a check for ETIMEDOUT during the run stop call.
> Shell script writing to the configfs UDC file to trigger disconnect and
> connect.  Batch script to have PC execute data transfers over adb (ie adb
> push)  After a few iterations, we'd run into a scenario where the
> controller wasn't halted.  With the following change, no failed halts aft=
er
> many iterations.
> ---
>  drivers/usb/dwc3/ep0.c    |  2 +-
>  drivers/usb/dwc3/gadget.c | 59 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 59f2e8c31bd1..456aa87e8778 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct u=
sb_request *request,
>  	int				ret;
>=20=20
>  	spin_lock_irqsave(&dwc->lock, flags);
> -	if (!dep->endpoint.desc) {
> +	if (!dep->endpoint.desc || !dwc->pullups_connected) {

these two should be the same. If pullups are not connected, there's no
way we can have an endpoint descriptor. Did you find a race condition here?

> @@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct usb_=
gadget *g,
>  	return 0;
>  }
>=20=20
> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
> +{
> +	u32 epnum;
> +
> +	for (epnum =3D 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> +		struct dwc3_ep *dep;
> +
> +		dep =3D dwc->eps[epnum];
> +		if (!dep)
> +			continue;
> +
> +		if (!(dep->flags & DWC3_EP_ENABLED))
> +			continue;
> +
> +		dwc3_remove_requests(dwc, dep);
> +	}
> +}
> +
>  static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  {
>  	u32			reg;
> @@ -1950,16 +1968,37 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc,=
 int is_on, int suspend)
>=20=20
>  		dwc->pullups_connected =3D true;
>  	} else {
> +		dwc->pullups_connected =3D false;
> +
> +		__dwc3_gadget_ep_disable(dwc->eps[0]);
> +		__dwc3_gadget_ep_disable(dwc->eps[1]);
> +
> +		/*
> +		 * The databook explicitly mentions for a device-initiated
> +		 * disconnect sequence, the SW needs to ensure that it ends any
> +		 * active transfers.
> +		 */
> +		dwc3_stop_active_transfers(dwc);

IIRC, gadget driver is required to dequeue transfers before
disconnecting. My memory is a bit fuzzy in that area, but anyway, how
did you trigger this problem?

> @@ -1994,9 +2033,15 @@ static int dwc3_gadget_pullup(struct usb_gadget *g=
, int is_on)
>  		}
>  	}
>=20=20
> +	/*
> +	 * Synchronize and disable any further event handling while controller
> +	 * is being enabled/disabled.
> +	 */
> +	disable_irq(dwc->irq_gadget);

looks like a call to synchronize_irq() would be enough here.

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQJFBAEBCAAvFiEElLzh7wn96CXwjh2IzL64meEamQYFAl89DucRHGJhbGJpQGtl
cm5lbC5vcmcACgkQzL64meEamQbK0g/+PGZ3r/FwZvIA5KkkqUjbQpd2wXWBXLrz
Vv0ZPdNu/VsVIulBDYtGYghXEaGkvstsCDwvmO0WPk2jZgxQ8qdhsS34m5G+G/AT
QmCtVKaKi5voX5u66cOf4NmiRph5In5R42gybkgFVzMmgje4YlcQS3c6cMe3Yzdl
z9wQX36dvZ/4gA2BmYNIACp/fmFACICG4H3N5VRCETDo1Ok/JfBHEKtlJTFIIw5Z
BpAoKlloPKN8O7Ge7bcrr3MFECohJguw2J6q59hCEJthMLJu5Ru1himq19whGdbn
ii6hLwWvLW4/hxpw1MHNNWobpDFl37B/dcfUgSh0doHsN/DbDvT28fl9cJb7/DZV
VrX8eFdbZ0bsruTmdHKT/R+lWB5r9icAf0dO3v9m0O/LYKcRAT0k3E3fnOvnuxWG
Qtgdh8DOJm0swhgThNsCTX2Q2ikHLV21ts6tdz6h2WJCLSLkUGku07MMP33mHz2S
PvUF/gFJjxB7UwAPKrUAVWyp/8pB+tY3CpxYwXJsDVTzfMlzzI1eztbKLR8uyY0e
HoG+J+LAcfecBZu6jLmFWtVwkQEG5AoOxhFmzW6Nfeo9hkr0UxuIxbvDK+jvHo35
dQMzd3hq4414CEtzjle5ySYwDE6IkufT6DGn9fyOmGpkKbgK/13lbwA1u0LOUuEJ
xz36oc6xhZ4=
=qJ1X
-----END PGP SIGNATURE-----
--=-=-=--