Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754201AbdDDORs (ORCPT ); Tue, 4 Apr 2017 10:17:48 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:43914 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1753275AbdDDORq (ORCPT ); Tue, 4 Apr 2017 10:17:46 -0400 Date: Tue, 4 Apr 2017 10:17:45 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Felipe Balbi cc: Roger Quadros , , , Subject: Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device In-Reply-To: <87d1csipgl.fsf@linux.intel.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2583 Lines: 72 On Tue, 4 Apr 2017, Felipe Balbi wrote: > Hi, > > Alan Stern 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 > >> --- > >> 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