Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753575AbdHOKDe (ORCPT ); Tue, 15 Aug 2017 06:03:34 -0400 Received: from mga02.intel.com ([134.134.136.20]:49720 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753358AbdHOKDd (ORCPT ); Tue, 15 Aug 2017 06:03:33 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,377,1498546800"; d="asc'?scan'208";a="123723713" From: Felipe Balbi To: Danilo Krummrich , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, gregkh@linuxfoundation.org, k.opasiak@samsung.com Cc: Danilo Krummrich Subject: Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind In-Reply-To: <20170814163652.29108-1-danilokrummrich@dk-develop.de> References: <20170814163652.29108-1-danilokrummrich@dk-develop.de> Date: Tue, 15 Aug 2017 13:03:20 +0300 Message-ID: <87efsd6tcn.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-Length: 3486 Lines: 89 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Danilo Krummrich writes: > udc_stop needs to be called before gadget driver unbind. Otherwise it > might happen that udc drivers still call into the gadget driver (e.g. > to reset gadget after OTG event). If this happens it is likely to get > panics from gadget driver dereferencing NULL ptr, as gadget's drvdata > is set to NULL on unbind. seems like the problem here is with the OTG layer, not UDC core. > Signed-off-by: Danilo Krummrich > --- > Actually there could still be a race: > (CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as exsample) > > CPU0 CPU1 > ---- ---- > usb_gadget_disconnect(udc->gadget); > udc->driver->disconnect(udc->gadget); > if (dwc->gadget_driver && dwc->gadget_driver->disconnect) > usb_gadget_udc_stop(udc); > udc->driver->unbind(udc->gadget); > dwc->gadget_driver->disconnect(&dwc->gadget); > > UDC drivers typically set their gadget driver pointer to NULL in udc_stop > and check for it before calling into the gadget driver. To fix the issue > above every udc driver could apply a lock around this. > > If you see the need for having this or another solutions I can provide > further patches. This patch could also just serve as a base for discussion > if someone knows a smarter solution. > > I saw this problem causing a panic on hikey960 board and provided a quick > workaround for the same problem here: > https://android-review.googlesource.com/#/c/kernel/common/+/457476/ > (panic log in the commit message of the linked patch) > --- > drivers/usb/gadget/udc/core.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index efce68e9a8e0..8155468afc0d 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct usb_ud= c *udc) >=20=20 > usb_gadget_disconnect(udc->gadget); > udc->driver->disconnect(udc->gadget); > - udc->driver->unbind(udc->gadget); > + /* udc_stop needs to be called before gadget driver unbind to prevent > + * udc driver calls into gadget driver after unbind which could cause > + * a nullptr exception. > + */ > usb_gadget_udc_stop(udc); > + udc->driver->unbind(udc->gadget); This patch is incorrect, it will prevent us from giving back requests to gadget driver properly. ->unbind() has to happen before ->udc_stop(). =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlmSxugACgkQzL64meEa mQawwg//Xz+0wZDJi/m2OR3RE8dWxIBZ0HC4yfWw5i5FjHt6mvnMkmaDmea7XKuu Itp9DUxyVth9purJSzYxdKfSKtGrgpq//UkUTwtf6v/v9DgTmEPaHyTFM9A2m9cd k1BaNu54TTPXeDiiCFxcMNfc8bqmeOtEuX8qgwbvqg7hUWXV6hWNqLb/tr4jZxpF sWKBaNHMnmWqPQJSh9uHHUUzzc5kFrFks6GVuvuGpWML5DZ+mraBOYxM+QIeOXsx kXi41EWvJAAh0R2lMSs6I0ZKX1wKYKidFHLAL2tbWVsoQXm/Zg8txvFKjKvyjtlX RB4Q4kO2z8d+3yeOwUZRwqnGoA1+o+7ZqW8rVredfCMRSdUeAJ7c8eginZ9g0kDF EQ7Pt59mVb5/3QcP9zbNR4Yr+rgdDyrN2PyuaP9Qy1OH0djsdTCoXz7fBjCgqoP0 PZI5EMoujPuGmr6qy+/J8oU2tY3nJQrTu5coMP194bSRuQeKDolQcZlAbXsoK44I R1f7JPIn1jJ0HQ0IK3L3rPakaswpnhxkc4IzS/kDcyYaGV+OrRPPGVDQcF02ynND PpXVQTXLbwvK4GlhBsBsrm0Wfiw2fRPxcxaHp4z0whrQ9S7Y/0u/jm9M5uY9Dd50 ToHbOHFVR3o5e90CONaHS2yAwMkhR2uY+4OQj8h8GP7nFA3/6FY= =FrXv -----END PGP SIGNATURE----- --=-=-=--