Hi,
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
> On Wed, 5 Apr 2017, Felipe Balbi wrote:
>
>> >> >> --- 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);
}
> (Even after all these years, I still get bothered by the way Dave
> Brownell used to call everything "dev"... IIRC, at one time he had a
> line of code that went something like: dev->dev.dev = &pdev->dev !)
:-)
>> >> I'm actually thinking that struct usb_gadget shouldn't have a struct
>> >> device at all. Just a pointer to a device, that would solve all these
>> >> issues.
>> >
>> > A pointer to which device? The UDC? That would change the directory
>> > layout in sysfs.
>>
>> indeed. Would that be a problem?
>
> Possibly for some userspace tool.
yeah, we can do dynamic allocation of the device pointer, no issue.
--
balbi
Attachment:
signature.asc
Description: PGP signature
