Re: [PATCH] usb/fsl_qe_udc: Report disconnect before unbinding

On Wed, Nov 12, 2008 at 02:38:02PM -0500, Alan Stern wrote:

On Wed, 12 Nov 2008, Anton Vorontsov wrote:

quoted

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().

quoted

--- 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,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2