On Tue, 4 Apr 2017, Felipe Balbi wrote:
> Hi,
>
> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
> > On Mon, 3 Apr 2017, Roger Quadros wrote:
> >
> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
> >> repeatedly on the same gadget->dev structure.
> >>
> >> We need to clear the gadget->dev structure so that kobject_init()
> >> doesn't complain about already initialized object.
> >>
> >> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> >> ---
> >> drivers/usb/gadget/udc/core.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> >> index d685d82..efce68e 100644
> >> --- 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?
(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.)
> 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.
Or a pointer to a separate dynamically allocated device (the way struct
usb_hcd contains a pointer to the root hub device)? That would work.
If the UDC driver wanted to re-register the gadget, it would have to
allocate a new device.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
