This is a follow-up to an earlier thread: "gadget: Clearing ep->desc in
struct usb_ep on EP disable?"
I took a closer look at u_ether.c and I had some concerns about the
synchronization between USB connection/disconnection vs. netdev
open/close. For instance:
If somebody yanks the USB cable in the middle of eth_open() ->
eth_start() -> rx_fill(), triggering gether_disconnect() and
usb_ep_disable() on another CPU, will rx_fill() abort gracefully when
its requests start getting rejected? It looks like rx_fill() just
assumes that if rx_submit() returns an error, it is out of memory so it
schedules a worker to try again. That might be a bad idea if the
endpoint was disabled.
If somebody hotplugs the USB cable in the middle of eth_stop(),
triggering gether_connect(), is there a chance that the endpoints might
wind up disabled (and stay that way until eth_stop() executes again)?
Is there any chance of eth_start() never getting called? Both
eth_stop() and gether_connect() acquire dev->lock, but it doesn't
protect the entire usb_ep_enable()...netif_carrier_on() sequence.
Maybe as a first step, it is best to just eliminate the usb_ep_disable()
/ usb_ep_enable() code from eth_stop() entirely, and let the
connect/disconnect functions worry about the endpoint state.
-- 8< --
From: Kevin Cernekee <cernekee@xxxxxxxxx>
eth_stop() calls usb_ep_disable() then usb_ep_enable(), to flush out any
I/O in progress. There are a couple of issues with this:
1) UDC drivers can clear ep->desc on EP disable, causing usb_ep_enable()
to fail. This causes rx_fill() to start looping, and it makes the
system (mostly) non-responsive.
2) gether_connect() / gether_disconnect() also modify the endpoint
state, and they aren't holding dev->lock during ALL of the critical
operations. It may be possible for the endpoints to wind up in an
incorrect state if these events happen in the wrong order.
3) It probably isn't even necessary to flush these transactions unless
the USB link drops, and that case is already covered in
gether_disconnect().
For now, let's just delete these calls.
Signed-off-by: Kevin Cernekee <cernekee@xxxxxxxxx>
---
drivers/usb/gadget/u_ether.c | 17 -----------------
1 file changed, 17 deletions(-)
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index 5b46f02..0a30f23 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -672,23 +672,6 @@ static int eth_stop(struct net_device *net)
if (link->close)
link->close(link);
-
- /* NOTE: we have no abort-queue primitive we could use
- * to cancel all pending I/O. Instead, we disable then
- * reenable the endpoints ... this idiom may leave toggle
- * wrong, but that's a self-correcting error.
- *
- * REVISIT: we *COULD* just let the transfers complete at
- * their own pace; the network stack can handle old packets.
- * For the moment we leave this here, since it works.
- */
- usb_ep_disable(link->in_ep);
- usb_ep_disable(link->out_ep);
- if (netif_carrier_ok(net)) {
- DBG(dev, "host still using in/out endpoints\n");
- usb_ep_enable(link->in_ep);
- usb_ep_enable(link->out_ep);
- }
}
spin_unlock_irqrestore(&dev->lock, flags);
--
1.7.11.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
