Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753166AbdDJKFx (ORCPT ); Mon, 10 Apr 2017 06:05:53 -0400 Received: from mga05.intel.com ([192.55.52.43]:4006 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751946AbdDJKFw (ORCPT ); Mon, 10 Apr 2017 06:05:52 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,182,1488873600"; d="asc'?scan'208";a="75735503" 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: Mon, 10 Apr 2017 13:05:29 +0300 Message-ID: <87tw5wefx2.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: 4433 Lines: 138 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Alan Stern writes: > On Wed, 5 Apr 2017, Felipe Balbi wrote: > >> >> >> --- 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 *g= adget) >> >> >> 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_unregist= er()=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 wi= ll=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? >>=20 >> nope, not being used. At least not yet. > > I'm not convinced (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.) >>=20 >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_i= d *id) >> { >> struct net2280 *dev; >> unsigned long resource, len; >> void __iomem *base =3D NULL; >> int retval, i; >>=20 >> /* alloc, and start init */ >> dev =3D kzalloc(sizeof(*dev), GFP_KERNEL); >> if (dev =3D=3D NULL) { >> retval =3D -ENOMEM; >> goto done; >> } >>=20 >> pci_set_drvdata(pdev, dev); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > That sets the driver data in the struct pci_dev, not in > dev->gadget.dev. As far as I can see, _nothing_ in the driver sets the=20 > driver data in dev->gadget.dev. hmmm, indeed. The same is happening with other callers of usb_add_gadget_udc_release(). I guess this should be enough? @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev) =20 static void gadget_release(struct device *_dev) { =2D struct net2280 *dev =3D dev_get_drvdata(_dev); + struct net2280 *dev =3D dev_get_drvdata(_dev->parent); =20 kfree(dev); } > (Even after all these years, I still get bothered by the way Dave=20 > Brownell used to call everything "dev"... IIRC, at one time he had a=20 > line of code that went something like: dev->dev.dev =3D &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. >>=20 >> indeed. Would that be a problem? > > Possibly for some userspace tool. yeah, we can do dynamic allocation of the device pointer, no issue. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAljrWOkACgkQzL64meEa mQZVrRAArtgZ4tICyeECqyGr+R+ku2BtxYHXz02G5Q3tRqbgkjkN/yCeaD53dKuM ivBagXmwIXIipRsgQ6Gu9DhF/4e7HJ1R/agGum4YyjC47xzHnJM1KxRUAEOfIN5/ nxtvgxEnaS4r8NHtjAUotA1N86r8qEw2plnioH4dg/GRqfoSNdQc3oh7s/+Nr+Jt QdHSQcT1hQ28Ue2RWRxWcVCvdLxs5hGDcNbkkbSep3FqwESAVkfwQGndlnQHLQvV O8r/J6aPnmK0BXc47GF+R2iNnOaaEt1Qg3DgRDfCEaByhn/6BMTF5c6Xzf8X5XAQ crAgq07cEwIVSWIAzuT269/kYHVDEC1UiLdCDcGPN9kXL/7ZqHybBUfjvKhXuDvz mhINPg2u2/d5ATi7lFz4xCsEb//W3IiETA+VQqLcYhx47y9M98cCTPHMU0my61yA qtAuldgBht0LzmIJ5KXeolfYb/+BnubDfaMK0KDp7/RAROEddX2cXAcleMNCajlI cTNKNX2Ra0kaXXr5yIRMKXXJ22nrizhsk5T2GsfTLaBUpEsOuP0kkyFiHMwZW/Db goEJ0AUslcaAEtW9wUNHByRvE6msFsQ6KOS8dCEXyryCjF4Q9OvxO3p93t6bhrSa jrABQO5HKW79LScgrn6FeGs4QoENqW64Tw1zJvooxusC8jQKGNg= =ak4f -----END PGP SIGNATURE----- --=-=-=--