Hi, Alan Stern <[email protected]> writes: >> >> >> >> --- a/drivers/usb/gadget/udc/core.c >> >> >> >> +++ b/drivers/usb/gadget/udc/core.c >> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget >> >> >> >> *gadget) >> >> >> >> flush_work(&gadget->work); >> >> >> >> device_unregister(&udc->dev); >> >> >> >> device_unregister(&gadget->dev); >> >> >> >> + memset(&gadget->dev, 0x00, sizeof(gadget->dev)); >> >> >> >> } >> >> >> >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc); >> >> >> > >> >> >> > Isn't this dangerous? It's quite possible that the >> >> >> > device_unregister() >> >> >> >> >> >> not on the gadget API, no. >> >> >> >> >> >> > call on the previous line invokes the gadget->dev.release callback, >> >> >> > which might deallocate gadget. If that happens, your new memset >> >> >> > will >> >> >> > oops. >> >> >> >> >> >> that won't happen. struct usb_gadget is a member of the UDC's private >> >> >> structure, like this: >> >> >> >> >> >> struct dwc3 { >> >> >> [...] >> >> >> struct usb_gadget gadget; >> >> >> struct usb_gadget_driver *gadget_driver; >> >> >> [...] >> >> >> }; >> >> > >> >> > Yes. So what? Can't the UDC driver use the refcount inside struct >> >> > usb_gadget to control the lifetime of its private structure? >> >> >> >> nope, not being used. At least not yet. >> > >> > I'm not convinced (yet)... >> > >> >> > (By the way, can you tell what's going on in net2280.c? I must be >> >> > missing something; it looks like gadget_release() would quickly run >> >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but >> >> > net2280_probe() never calls dev_set_drvdata() for that device. >> >> > Furthermore, net2280_remove() continues to reference the net2280 struct >> >> > after calling usb_del_gadget_udc(), and it never does seem to do a >> >> > final put.) >> >> >> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id >> >> *id) >> >> { >> >> struct net2280 *dev; >> >> unsigned long resource, len; >> >> void __iomem *base = NULL; >> >> int retval, i; >> >> >> >> /* alloc, and start init */ >> >> dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> >> if (dev == NULL) { >> >> retval = -ENOMEM; >> >> goto done; >> >> } >> >> >> >> pci_set_drvdata(pdev, dev); >> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> > >> > That sets the driver data in the struct pci_dev, not in >> > dev->gadget.dev. As far as I can see, _nothing_ in the driver sets the >> > driver data in dev->gadget.dev. >> >> hmmm, indeed. The same is happening with other callers of >> usb_add_gadget_udc_release(). >> >> I guess this should be enough? >> >> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev) >> >> static void gadget_release(struct device *_dev) >> { >> - struct net2280 *dev = dev_get_drvdata(_dev); >> + struct net2280 *dev = dev_get_drvdata(_dev->parent); >> >> kfree(dev); >> } > > 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;
> And it doesn't help with the fact that net2280_remove() continues to
> access the private data structure after calling usb_del_gadget_udc().
> Strictly speaking, that routine should do
>
> get_device(&dev->gadget.dev);
>
> at the start, with a corresponding put_device() at the end.
>
> There's another problem. Suppose a call to
> usb_add_gadget_udc_release() fails. At the end of that routine, the
> error pathway does put_device(&gadget->dev). This will invoke the
> release callback, deallocating the private data structure without
> giving the caller (i.e., the UDC driver) a chance to clean up.
it won't deallocate anything :-) dev_set_drvdata() was never called,
we will endup with kfree(NULL) which is safe and just silently returns.
--
balbi
signature.asc
Description: PGP signature
