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: 3471 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]