usb/fsl_qe_udc: Report disconnect before unbinding

On Wed, Nov 12, 2008 at 10:59:16PM +0300, Anton Vorontsov wrote:
[...]
> > 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...

It seems I lied. Did you mean something like this patch?

On Wed, Nov 12, 2008 at 11:12:18PM +0300, Anton Vorontsov wrote:
> On Wed, Nov 12, 2008 at 10:59:16PM +0300, Anton Vorontsov wrote:
> [...]
> > > 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...
> 
> It seems I lied. Did you mean something like this patch?
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index c7de671..fd44cf4 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -2368,6 +2368,9 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  	/* stop usb controller, disable intr */
>  	qe_usb_disable();
>  
> +	if (udc->usb_state == USB_STATE_DEFAULT)

Should be also || usb_state == USB_STATE_ATTACHED, since
this is the initial value after the usb_gadget_register_driver().

And the _DEFAULT state is "we're just after the bus reset" (also
means that we already called the disconnect()).

> +		goto skip_quiesce;
> +
>  	/* in fact, no needed */
>  	udc_controller->usb_state = USB_STATE_ATTACHED;
>  	udc_controller->ep0_state = WAIT_FOR_SETUP;
> @@ -2385,6 +2388,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  	/* report disconnect; the driver is already quiesced */
>  	driver->disconnect(&udc_controller->gadget);
>  
> +skip_quiesce:
>  	/* unbind gadget and unhook driver. */
>  	driver->unbind(&udc_controller->gadget);
>  	udc_controller->gadget.dev.driver = NULL;

On Wed, 12 Nov 2008, Anton Vorontsov wrote:

> On Wed, Nov 12, 2008 at 11:12:18PM +0300, Anton Vorontsov wrote:
> > On Wed, Nov 12, 2008 at 10:59:16PM +0300, Anton Vorontsov wrote:
> > [...]
> > > > 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...
> > 
> > It seems I lied. Did you mean something like this patch?
> > 
> > diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> > index c7de671..fd44cf4 100644
> > --- a/drivers/usb/gadget/fsl_qe_udc.c
> > +++ b/drivers/usb/gadget/fsl_qe_udc.c
> > @@ -2368,6 +2368,9 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
> >  	/* stop usb controller, disable intr */
> >  	qe_usb_disable();
> >  
> > +	if (udc->usb_state == USB_STATE_DEFAULT)
> 
> Should be also || usb_state == USB_STATE_ATTACHED, since
> this is the initial value after the usb_gadget_register_driver().
> 
> And the _DEFAULT state is "we're just after the bus reset" (also
> means that we already called the disconnect()).

That sounds right.  Although come to think of it, I guess there really 
is no harm in calling the disconnect method twice in a row.  But it's 
better to do what the other UDC drivers do.

> > +		goto skip_quiesce;
> > +
> >  	/* in fact, no needed */
> >  	udc_controller->usb_state = USB_STATE_ATTACHED;
> >  	udc_controller->ep0_state = WAIT_FOR_SETUP;
> > @@ -2385,6 +2388,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
> >  	/* report disconnect; the driver is already quiesced */
> >  	driver->disconnect(&udc_controller->gadget);

I would update this last comment slightly.  The word "driver" is
ambiguous, and since this function is named
usb_gadget_unregister_driver(), it looks like you're talking about the
gadget driver.

> > +skip_quiesce:
> >  	/* unbind gadget and unhook driver. */
> >  	driver->unbind(&udc_controller->gadget);
> >  	udc_controller->gadget.dev.driver = NULL;

Alan Stern