Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932416AbdDEIgV (ORCPT ); Wed, 5 Apr 2017 04:36:21 -0400 Received: from mga03.intel.com ([134.134.136.65]:19093 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932150AbdDEIfk (ORCPT ); Wed, 5 Apr 2017 04:35:40 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,277,1486454400"; d="asc'?scan'208";a="69600629" From: Felipe Balbi To: Alan Stern Cc: Roger Quadros , vivek.gautam@codeaurora.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device In-Reply-To: References: Date: Wed, 05 Apr 2017 11:34:48 +0300 Message-ID: <87o9wbgslz.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: 4142 Lines: 125 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Alan Stern writes: >> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called >> >> repeatedly on the same gadget->dev structure. >> >>=20 >> >> We need to clear the gadget->dev structure so that kobject_init() >> >> doesn't complain about already initialized object. >> >>=20 >> >> Signed-off-by: Roger Quadros >> >> --- >> >> drivers/usb/gadget/udc/core.c | 1 + >> >> 1 file changed, 1 insertion(+) >> >>=20 >> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/c= ore.c >> >> index d685d82..efce68e 100644 >> >> --- a/drivers/usb/gadget/udc/core.c >> >> +++ b/drivers/usb/gadget/udc/core.c >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadg= et) >> >> flush_work(&gadget->work); >> >> device_unregister(&udc->dev); >> >> device_unregister(&gadget->dev); >> >> + memset(&gadget->dev, 0x00, sizeof(gadget->dev)); >> >> } >> >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); >> > >> > Isn't this dangerous? It's quite possible that the device_unregister(= )=20 >>=20 >> not on the gadget API, no. >>=20 >> > call on the previous line invokes the gadget->dev.release callback,=20 >> > which might deallocate gadget. If that happens, your new memset will= =20 >> > oops. >>=20 >> that won't happen. struct usb_gadget is a member of the UDC's private >> structure, like this: >>=20 >> struct dwc3 { >> [...] >> struct usb_gadget gadget; >> struct usb_gadget_driver *gadget_driver; >> [...] >> }; > > Yes. So what? Can't the UDC driver use the refcount inside struct=20 > usb_gadget to control the lifetime of its private structure? nope, not being used. At least not yet. > (By the way, can you tell what's going on in net2280.c? I must be > missing something; it looks like gadget_release() would quickly run > into problems because it calls dev_get_drvdata() for &gadget->dev, but > net2280_probe() never calls dev_set_drvdata() for that device.=20=20 > Furthermore, net2280_remove() continues to reference the net2280 struct > after calling usb_del_gadget_udc(), and it never does seem to do a > final put.) static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *= id) { struct net2280 *dev; unsigned long resource, len; void __iomem *base =3D NULL; int retval, i; /* alloc, and start init */ dev =3D kzalloc(sizeof(*dev), GFP_KERNEL); if (dev =3D=3D NULL) { retval =3D -ENOMEM; goto done; } pci_set_drvdata(pdev, dev); ^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> I'm actually thinking that struct usb_gadget shouldn't have a struct >> device at all. Just a pointer to a device, that would solve all these >> issues. > > A pointer to which device? The UDC? That would change the directory=20 > layout in sysfs. indeed. Would that be a problem? > Or a pointer to a separate dynamically allocated device (the way struct=20 > usb_hcd contains a pointer to the root hub device)? That would work.=20= =20 > If the UDC driver wanted to re-register the gadget, it would have to=20 > allocate a new device. That could be done as well, if maintaining the directory structure is a must. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAljkrCgACgkQzL64meEa mQZt3A/8CKsuc2DmhXkC8xZShs196Wi4e4dNcsJXU1PPtoFm+3Qf3NK9GH0ExDDp Grd6H3B2vz/P/sSUySTbalYPD5VA21g9F9uj6naTq5nCfp/njep1acvft2vgUTAo Ow9uDYGAvWjouIfdlkxWjYZMqAezX3eAoM7NpyD6hT1ik9aWeUTuHHdIl6Y6MlHI GxKUjCGLl2bouxjPV9sr9OKqAHv/NJVNuFOG0rMzHcWQjPMy8VbPCS2KBvhl5WPq pfQdaEIpjdZggLVCcbzAZVaJwJAoqNwQdcBJShtHDzSC/dVqibc7WqBjuQnGBXNe Bc8oH1EzCDCYGkWzYbObYmbtU3HT/OmlIIUgFzn+4rZ+7uElpkL3qD9YtKNS/c7N IINyBsBJcjl9hWjvMIzAafTqOIXm6/H9l3AYpFCmfoYPpAE60EEfyVq28WPe16ut r0a5mwoDmbnWEEWWGdTIHH4t/kWc+VhIch94suMt8Yx/RG9eyJ5jnRVqkVsd9haM u+humqq7W2P60rlxRP39rEAReOEzRFdxa6fFkYIgq8ps532EGIJzLUfCyTXfE+pt hzpgdstG2kaOAvuWMfGQXa+trjOTTU3sZgYUwAAkf1KgOKqaUDdiV1L9ekvqUWS7 ktKseWYq3lmYoMXwrW1jqw1ApUUylWsuml6qan0pFjWbXt3F1VI= =k8so -----END PGP SIGNATURE----- --=-=-=--