[PATCH] usb/fsl_qe_udc: Report disconnect before unbinding

Subsystems: the rest, usb gadget/peripheral subsystem, usb subsystem · Maintainers: Linus Torvalds, Neill Kapron, Greg Kroah-Hartman

STALE6554d

8 messages, 3 authors, 2008-11-18 · open the first message on its own page

[PATCH] usb/fsl_qe_udc: Report disconnect before unbinding

From: Anton Vorontsov <hidden>
Date: 2008-11-12 18:56:30

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

From: Alan Stern <stern@rowland.harvard.edu>
Date: 2008-11-12 19:38:16

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

quoted hunk

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

From: Anton Vorontsov <hidden>
Date: 2008-11-12 19:59:19

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

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

From: Anton Vorontsov <hidden>
Date: 2008-11-12 20:12:19

On Wed, Nov 12, 2008 at 10:59:16PM +0300, Anton Vorontsov wrote:
[...]

quoted

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

From: Anton Vorontsov <hidden>
Date: 2008-11-12 20:20:19

On Wed, Nov 12, 2008 at 11:12:18PM +0300, Anton Vorontsov wrote:

quoted hunk

On Wed, Nov 12, 2008 at 10:59:16PM +0300, Anton Vorontsov wrote:
[...]

quoted

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

quoted hunk

+		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

From: Alan Stern <stern@rowland.harvard.edu>
Date: 2008-11-12 21:12:37

On Wed, 12 Nov 2008, Anton Vorontsov wrote:

On Wed, Nov 12, 2008 at 11:12:18PM +0300, Anton Vorontsov wrote:

quoted

On Wed, Nov 12, 2008 at 10:59:16PM +0300, Anton Vorontsov wrote:
[...]

quoted

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.

quoted

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

quoted

+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

From: Anton Vorontsov <hidden>
Date: 2008-11-13 11:57:22

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

v2:
- Comment update per Alan Stern review.

replaces

5 more lines

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

From: David Brownell <hidden>
Date: 2008-11-18 01:48:27

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

Acked-by: David Brownell <redacted>

quoted hunk

---

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help