Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752974AbdDLGCO (ORCPT ); Wed, 12 Apr 2017 02:02:14 -0400 Received: from mga04.intel.com ([192.55.52.120]:23729 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752192AbdDLGCK (ORCPT ); Wed, 12 Apr 2017 02:02:10 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,189,1488873600"; d="asc'?scan'208";a="955064191" From: Felipe Balbi To: Greg KH , Alan Stern Cc: Roger Quadros , vivek.gautam@codeaurora.org, USB list , Kernel development list Subject: Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device In-Reply-To: <20170411141927.GB27233@kroah.com> References: <87r30zcs95.fsf@linux.intel.com> <20170411141927.GB27233@kroah.com> Date: Wed, 12 Apr 2017 09:01:44 +0300 Message-ID: <87wpaqb1vb.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: 3963 Lines: 122 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Greg KH writes: > On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote: >> On Tue, 11 Apr 2017, Felipe Balbi wrote: >>=20 >> > > Oddly enough, yes. But it doesn't explain why this code doesn't blo= w=20 >> > > up every time it gets called, in its current form. >> >=20 >> > Well, it does :-) >> >=20 >> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL) >> >=20 >> > We're just leaking memory. I guess a patch like below would be best: >> >=20 >> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc= /net2280.c >> > index 3828c2ec8623..4dc04253da61 100644 >> > --- a/drivers/usb/gadget/udc/net2280.c >> > +++ b/drivers/usb/gadget/udc/net2280.c >> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_= dev) >> >=20=20 >> > /*-------------------------------------------------------------------= ------*/ >> >=20=20 >> > -static void gadget_release(struct device *_dev) >> > -{ >> > - struct net2280 *dev =3D dev_get_drvdata(_dev); >> > - >> > - kfree(dev); >> > -} >> > - >> > /* tear down the binding between this driver and the pci device */ >> >=20=20 >> > static void net2280_remove(struct pci_dev *pdev) >> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev) >> > device_remove_file(&pdev->dev, &dev_attr_registers); >> >=20=20 >> > ep_info(dev, "unbind\n"); >> > + >> > + kfree(dev); >> > } >> >=20=20 >> > /* wrap this driver around the specified device, but >> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, c= onst struct pci_device_id *id) >> > if (retval) >> > goto done; >> >=20=20 >> > - retval =3D usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, >> > - gadget_release); >> > + retval =3D usb_add_gadget_udc(&pdev->dev, &dev->gadget); >> > if (retval) >> > goto done; >> > return 0; >>=20 >> Maybe... But I can't shake the feeling that Greg KH would strongly=20 >> disagree. Hasn't he said, many times in the past, that any dynamically= =20 >> allocated device structure _must_ have a real release routine?=20=20 >> usb_udc_nop_release() doesn't qualify. > > Aw, I wanted to publically yell at someone like the kernel documentation > says I am allowed to do so if anyone does such a foolish thing :) heh, except that we're not dynamically allocating struct device at all :-) Here's what we have for most UDCs (net2280.c included): struct my_udc { struct gadget gadget; [...] }; probe() { struct my_udc *u; u =3D kzalloc(sizeof(*u), GFP_KERNEL); [...] return 0; } Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't this result on a functionally equivalent execution to the patch I proposed above? Iff we change struct gadget to contain a struct device *dev instead of a struct device dev, then sure, we will need to cope with proper =2D>release() implementations. As it is, it brings nothing to the table, IMO. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAljtwsgACgkQzL64meEa mQYXjhAA0nIduhEFbIMu0SFWyDh10yGuvanL5/GSf7QmRN05hG7Cs5XXv+FTZKI7 jGpr78274d6zaONaPtttA651cqLZWHz6sKCbY9sH5v0xA0vJlE50WVJ47CPvY/m+ nMERkZ0x6fWyyLp3n+bJxkT5av0vjpOLp4sk4Kj+yJSgNmF/eIiCSSOhM0HGqHSN oA7V8ulQC4QcHFPwZnfsd2AUB8gT/fAwVZsAooebqPHWSi7nrDnlTIz4vcPCDkTd RH68kh830ppK9lsTl5zlMDHDk99wItax4LaIEHbXhn/9s2E9PLHrjKP4JvsC6QSm zTtWPmmK4UN6CnYIjeFWbWqBbFZpcRb95ZKhEukffNSpd+RHOQaVXT+o0nSD0KQO CaCrYV1JCYLtEp85mZ5vxDKYVPC8wd6KRQg3+Y2yXS/D0agx5hDnEqkw+cGPNOm/ g+VGVoT0l4MSIsCXnt5F5Ism2XV16HQ6Cs7gZXVB5O+qNfzWV3ZQaBH8iTJCDY6J D1V2gNKNyUIBdorez3MVKciF2sGI91b/ileWQXaOr0XFRar6E2jpz6GY7Lp5bCuE QPrx1ovhDgxEDu/nVI2Pnxy0AKdcTElooi2hDMNc5PSGx26Zs+2w42od47u1pCaJ Knqq3pcYrGovVnM7p5iNl38H4LjZTy5zx7KxbcpHT407M/8ozF0= =RWBr -----END PGP SIGNATURE----- --=-=-=--