Date: Wed, 12 Apr 2017 08:45:50 +0200
From: Greg KH <greg@kroah.com>
To: Felipe Balbi <balbi@kernel.org>
Cc: Alan Stern <stern@rowland.harvard.edu>, Roger Quadros <rogerq@ti.com>,
        vivek.gautam@codeaurora.org, USB list <linux-usb@vger.kernel.org>,
        Kernel development list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same
 gadget device
Message-ID: <20170412064550.GA6143@kroah.com>
References: <87r30zcs95.fsf@linux.intel.com>
 <Pine.LNX.4.44L0.1704110955370.1707-100000@iolanthe.rowland.org>
 <20170411141927.GB27233@kroah.com>
 <87wpaqb1vb.fsf@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87wpaqb1vb.fsf@linux.intel.com>
User-Agent: Mutt/1.8.0 (2017-02-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3372
Lines: 101

On Wed, Apr 12, 2017 at 09:01:44AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Greg KH <greg@kroah.com> writes:
> > 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 :)
> 
> heh, except that we're not dynamically allocating struct device at all
> :-)

Please don't say that, that's even worse :(

> Here's what we have for most UDCs (net2280.c included):
> 
> 	struct my_udc {
>         	struct gadget gadget;
>                 [...]
> 	};
> 
> 	probe()
>         {
>         	struct my_udc *u;
> 
> 		u = 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
> ->release() implementations.
> 
> As it is, it brings nothing to the table, IMO.

You always have to have a release function for a kobject, no matter
where it is, as it is being reference counted.  To not do so, is a huge
indication of a problem in the design.

thanks,

greg k-h