Skip to contentSkip to conversation

usb/fsl_qe_udc: Report disconnect before unbinding

PATCH In review Anton Vorontsov 3 participants 8 messages Nov 12, 2008 – Nov 18, 2008 USB

lore
Share & tools
lore

Anton Vorontsov

author
#0 lore raw

PATCH 1/1 usb/fsl_qe_udc: Report disconnect before unbinding

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 <[email protected]>
---
 drivers/usb/gadget/fsl_qe_udc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/usb/gadget/fsl_qe_udc.c +3 -0
index 37c8575..c7de671 100644
@@ -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;
Signature · 1 line
1.5.6.3
#1 lore raw
Quoted 7 lines · 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().

drivers/usb/gadget/fsl_qe_udc.c int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) · line 2390

Quoted 9 lines
drivers/usb/gadget/fsl_qe_udc.c +3 -0
@@ -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

author
#2 lore raw
Quoted 12 lines · Alan Stern
Quoted 10 lines · 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().

drivers/usb/gadget/fsl_qe_udc.c +0 -0
@@ -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);
Quoted 10 lines

drivers/usb/gadget/fsl_qe_udc.c int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) · line 2390

Quoted 6 lines
drivers/usb/gadget/fsl_qe_udc.c +3 -0
+	/* 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().

Quoted 2 lines

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,

Signature · 3 lines
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
author
#3 lore raw

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

Quoted 10 lines
Quoted 2 lines

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?

drivers/usb/gadget/fsl_qe_udc.c +4 -0
index c7de671..fd44cf4 100644
@@ -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;
author
#4 lore raw
Quoted 20 lines · Anton Vorontsov

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

Quoted 10 lines
Quoted 2 lines

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?

drivers/usb/gadget/fsl_qe_udc.c +0 -0
index c7de671..fd44cf4 100644

drivers/usb/gadget/fsl_qe_udc.c int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) · line 2371

Quoted 4 lines
drivers/usb/gadget/fsl_qe_udc.c +1 -0
@@ -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 13 lines
drivers/usb/gadget/fsl_qe_udc.c +3 -0
+		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;
Signature · 3 lines
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
#5 lore raw
Quoted 24 lines · Anton Vorontsov
Quoted 23 lines · Anton Vorontsov

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

Quoted 10 lines
Quoted 2 lines

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?

drivers/usb/gadget/fsl_qe_udc.c +0 -0
index c7de671..fd44cf4 100644
@@ -2368,6 +2368,9 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	/* stop usb controller, disable intr */
 	qe_usb_disable();
Quoted 7 lines

drivers/usb/gadget/fsl_qe_udc.c int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) · line 2371

Quoted 1 line
drivers/usb/gadget/fsl_qe_udc.c +1 -0
+	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 8 lines

drivers/usb/gadget/fsl_qe_udc.c int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) · line 2389

Quoted 8 lines
drivers/usb/gadget/fsl_qe_udc.c +2 -0
+		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 4 lines

drivers/usb/gadget/fsl_qe_udc.c int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) · line 2394

Quoted 4 lines
drivers/usb/gadget/fsl_qe_udc.c +1 -0
+skip_quiesce:
 	/* unbind gadget and unhook driver. */
 	driver->unbind(&udc_controller->gadget);
 	udc_controller->gadget.dev.driver = NULL;

Alan Stern

author
#6 lore raw

PATCH v2 1/1 usb/fsl_qe_udc: Report disconnect before unbinding

In reply to 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 <[email protected]>

---

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(-)
drivers/usb/gadget/fsl_qe_udc.c +3 -0
index 37c8575..3d6c956 100644
@@ -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;
Signature · 1 line
1.5.6.3
Acked-by on v2 1/1
#7 lore raw
Quoted 33 lines · 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'
Quoted 1 line
Signed-off-by Anton Vorontsov <[email protected]>
Acked-by David Brownell <[email protected]>
Quoted 33 lines

---

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(-)
drivers/usb/gadget/fsl_qe_udc.c +3 -0
index 37c8575..3d6c956 100644
@@ -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;
Signature · 5 lines
1.5.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html