| Message ID | 20081112185627.GA17699@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Kumar Gala |
| Headers | show |
On Wed, 12 Nov 2008, Anton Vorontsov wrote: > Gadgets disable endpoints in their disconnect callbacks, so > we must call disconnect before unbinding. This also fixes > muram memory leak, since we free muram in the qe_ep_disable(). > --- a/drivers/usb/gadget/fsl_qe_udc.c > +++ b/drivers/usb/gadget/fsl_qe_udc.c > @@ -2382,6 +2382,9 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) > nuke(loop_ep, -ESHUTDOWN); > spin_unlock_irqrestore(&udc_controller->lock, flags); > > + /* report disconnect; the driver is already quiesced */ > + driver->disconnect(&udc_controller->gadget); > + > /* unbind gadget and unhook driver. */ > driver->unbind(&udc_controller->gadget); > udc_controller->gadget.dev.driver = NULL; Wouldn't it be better to do this before nuking the existing requests? The comment is wrong; the gadget driver is _not_ quiesced at this point. In fact the disconnect call is what quiesces the driver! And wouldn't it be better to _skip_ doing this if the gadget wasn't connected before? Alan Stern
On Wed, Nov 12, 2008 at 02:38:02PM -0500, Alan Stern wrote: > On Wed, 12 Nov 2008, Anton Vorontsov wrote: > > > Gadgets disable endpoints in their disconnect callbacks, so > > we must call disconnect before unbinding. This also fixes > > muram memory leak, since we free muram in the qe_ep_disable(). > > > --- a/drivers/usb/gadget/fsl_qe_udc.c > > +++ b/drivers/usb/gadget/fsl_qe_udc.c > > @@ -2382,6 +2382,9 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) > > nuke(loop_ep, -ESHUTDOWN); > > spin_unlock_irqrestore(&udc_controller->lock, flags); > > > > + /* report disconnect; the driver is already quiesced */ > > + driver->disconnect(&udc_controller->gadget); > > + > > /* unbind gadget and unhook driver. */ > > driver->unbind(&udc_controller->gadget); > > udc_controller->gadget.dev.driver = NULL; > > Wouldn't it be better to do this before nuking the existing requests? > The comment is wrong; the gadget driver is _not_ quiesced at this > point. In fact the disconnect call is what quiesces the driver! composite_unbind() says: /* composite_disconnect() must already have been called * by the underlying peripheral controller driver! * so there's no i/o concurrency that could affect the * state protected by cdev->lock. */ Which I read as "at disconnect time the controller should already be disabled, no further i/o can happen". Which means that we should nuke all pending requests and stop the controller. In this comment: "/* report disconnect; the driver is already quiesced */" "the driver" means "the udc driver", not the gadget driver. FWIW, the PXA27x UDC controller also stops all activity and completely disables the controller before calling the disconnect(). > And wouldn't it be better to _skip_ doing this if the gadget wasn't > connected before? Composite framework handles this. If there were no connections, then the disconnect() is a nop (except the spin lock/unlock pair). I'm not sure how the controller driver could tell if there was a connection or not: it doesn't operate these terms. What the udc controller knows is: how to report bus reset and how to receive or transmit the data... Thanks,
diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c index 37c8575..c7de671 100644 --- a/drivers/usb/gadget/fsl_qe_udc.c +++ b/drivers/usb/gadget/fsl_qe_udc.c @@ -2382,6 +2382,9 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) nuke(loop_ep, -ESHUTDOWN); spin_unlock_irqrestore(&udc_controller->lock, flags); + /* report disconnect; the driver is already quiesced */ + driver->disconnect(&udc_controller->gadget); + /* unbind gadget and unhook driver. */ driver->unbind(&udc_controller->gadget); udc_controller->gadget.dev.driver = NULL;
Gadgets disable endpoints in their disconnect callbacks, so we must call disconnect before unbinding. This also fixes muram memory leak, since we free muram in the qe_ep_disable(). But mainly the patch fixes following badness: root@b1:~# insmod fsl_qe_udc.ko fsl_qe_udc: Freescale QE/CPM USB Device Controller driver, 1.0 fsl_qe_udc e01006c0.usb: QE USB controller initialized as device root@b1:~# insmod g_ether.ko g_ether gadget: using random self ethernet address g_ether gadget: using random host ethernet address usb0: MAC be:2d:3c:fa:be:f0 usb0: HOST MAC 62:b8:6a:df:38:66 g_ether gadget: Ethernet Gadget, version: Memorial Day 2008 g_ether gadget: g_ether ready fsl_qe_udc e01006c0.usb: fsl_qe_udc bind to driver g_ether g_ether gadget: high speed config #1: CDC Ethernet (ECM) root@b1:~# rmmod g_ether.ko ------------[ cut here ]------------ Badness at drivers/usb/gadget/composite.c:871 [...] NIP [d10c1374] composite_unbind+0x24/0x15c [g_ether] LR [d10a82f4] usb_gadget_unregister_driver+0x128/0x168 [fsl_qe_udc] Call Trace: [cfb93e80] [cfb1f3a0] 0xcfb1f3a0 (unreliable) [cfb93eb0] [d10a82f4] usb_gadget_unregister_driver+0x128/0x168 [fsl_qe_udc] [cfb93ed0] [d10c2a3c] usb_composite_unregister+0x3c/0x4c [g_ether] [cfb93ee0] [c006bde0] sys_delete_module+0x130/0x19c [cfb93f40] [c00142d8] ret_from_syscall+0x0/0x38 [...] fsl_qe_udc e01006c0.usb: unregistered gadget driver 'g_ether' Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> --- drivers/usb/gadget/fsl_qe_udc.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)