Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755435AbdDEOOB (ORCPT ); Wed, 5 Apr 2017 10:14:01 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:55792 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1755411AbdDEOMf (ORCPT ); Wed, 5 Apr 2017 10:12:35 -0400 Date: Wed, 5 Apr 2017 10:12:28 -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: <87o9wbgslz.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: 3119 Lines: 90 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. (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. > > 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. > > That could be done as well, if maintaining the directory structure is a > must. Alan Stern