LinuxLists.cc - [PATCH v3 0/3] usb: dwc3: dual-role support

by Greg KH

[permalink] [raw]

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

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

2017-04-12 06:02:14

Hi,

Greg KH <[email protected]> writes:
>> 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.

okay, this goes all the way back to when Dave B wrote the API, it has
always had empty ->release() functions (not all UDCs, though). I'll make
sure to review all of this for v4.13.

--
balbi

Attachments:

signature.asc (832.00 B)

2017-04-12 14:30:06

by Alan Stern

[permalink] [raw]

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

On Wed, 12 Apr 2017, Felipe Balbi wrote:

> >> 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;
> }

Allow me to point out that the struct device is embedded inside the
struct gadget (actually struct usb_gadget) embedded inside the struct
my_udc, which _is_ dynamically allocated. Therefore the struct device
is located in dynamically allocated memory.

> 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?

It would, and it would be equally wrong.

Alan Stern