Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]


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
:-) 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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]