Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753737AbdDKOTp (ORCPT ); Tue, 11 Apr 2017 10:19:45 -0400 Received: from out1-smtp.messagingengine.com ([66.111.4.25]:51770 "EHLO out1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752815AbdDKOTm (ORCPT ); Tue, 11 Apr 2017 10:19:42 -0400 X-ME-Sender: X-Sasl-enc: CpFTIuEQLeqkKxQKls3Pw6RlDki71D//FUh8jJ9HUauT 1491920375 Date: Tue, 11 Apr 2017 16:19:27 +0200 From: Greg KH To: Alan Stern Cc: Felipe Balbi , 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 Message-ID: <20170411141927.GB27233@kroah.com> References: <87r30zcs95.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2090 Lines: 57 On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote: > On Tue, 11 Apr 2017, Felipe Balbi wrote: > > > > Oddly enough, yes. But it doesn't explain why this code doesn't blow > > > up every time it gets called, in its current form. > > > > Well, it does :-) > > > > dev_get_drvdata(_dev) -> NULL -> kfree(NULL) > > > > We're just leaking memory. I guess a patch like below would be best: > > > > 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) > > > > /*-------------------------------------------------------------------------*/ > > > > -static void gadget_release(struct device *_dev) > > -{ > > - struct net2280 *dev = dev_get_drvdata(_dev); > > - > > - kfree(dev); > > -} > > - > > /* tear down the binding between this driver and the pci device */ > > > > 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); > > > > ep_info(dev, "unbind\n"); > > + > > + kfree(dev); > > } > > > > /* wrap this driver around the specified device, but > > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > if (retval) > > goto done; > > > > - retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, > > - gadget_release); > > + retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget); > > if (retval) > > goto done; > > return 0; > > Maybe... But I can't shake the feeling that Greg KH would strongly > disagree. Hasn't he said, many times in the past, that any dynamically > allocated device structure _must_ have a real release routine? > 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 :)