[PATCH] usb/fsl_qe_udc: Report disconnect before unbinding

[PATCH] usb/fsl_qe_udc: Report disconnect before unbinding

[Anton Vorontsov]

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

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;
-- 
1.5.6.3

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

[Alan Stern]

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

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

[Anton Vorontsov]

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,

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

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

[Anton Vorontsov]

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)
+		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;

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

[Anton Vorontsov]

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;

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

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

[Alan Stern]

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

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

[Anton Vorontsov]

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

v2:
- Comment update per Alan Stern review.

replaces
usb.current/usb-fsl_qe_udc-report-disconnect-before-unbinding-fixing-oopses.patch

 drivers/usb/gadget/fsl_qe_udc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index 37c8575..3d6c956 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 controller is already quiesced */
+	driver->disconnect(&udc_controller->gadget);
+
 	/* unbind gadget and unhook driver. */
 	driver->unbind(&udc_controller->gadget);
 	udc_controller->gadget.dev.driver = NULL;
-- 
1.5.6.3

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

[David Brownell]

On Thursday 13 November 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().
> 
> 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>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>


> ---
> 
> v2:
> - Comment update per Alan Stern review.
> 
> replaces
> usb.current/usb-fsl_qe_udc-report-disconnect-before-unbinding-fixing-oopses.patch
> 
>  drivers/usb/gadget/fsl_qe_udc.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index 37c8575..3d6c956 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 controller is already quiesced */
> +	driver->disconnect(&udc_controller->gadget);
> +
>  	/* unbind gadget and unhook driver. */
>  	driver->unbind(&udc_controller->gadget);
>  	udc_controller->gadget.dev.driver = NULL;
> -- 
> 1.5.6.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>