Hi, On Sun, Jul 08, 2012 at 11:53:08AM -0700, Kevin Cernekee wrote: > 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. I have applied this one instead: commit a3ab51013e6af38b3e2f9f20bf469cf8c595391b Author: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> Date: Wed Aug 8 11:48:10 2012 +0200 usb: gadget: u_ether: fix kworker 100% CPU issue with still used interfaces in eth_stop This patch fixes an issue introduced by patch: 72c973d usb: gadget: add usb_endpoint_descriptor to struct usb_ep Without this patch we see a kworker taking 100% CPU, after this sequence: - Connect gadget to a windows host - load g_ether - ifconfig up <ip>; ifconfig down; ifconfig up - ping <windows host> The "ifconfig down" results in calling eth_stop(), which will call usb_ep_disable() and, if the carrier is still ok, usb_ep_enable(): usb_ep_disable(link->in_ep); usb_ep_disable(link->out_ep); if (netif_carrier_ok(net)) { usb_ep_enable(link->in_ep); usb_ep_enable(link->out_ep); } The ep should stay enabled, but will not, as ep_disable set the desc pointer to NULL, therefore the subsequent ep_enable will fail. This leads to permanent rescheduling of the eth_work() worker as usb_ep_queue() (called by the worker) will fail due to the unconfigured endpoint. We fix this issue by saving the ep descriptors and re-assign them before usb_ep_enable(). Cc: Tatyana Brokhman <tlinder@xxxxxxxxxxxxxx> Cc: stable@xxxxxxxxxxxxxxx Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> Signed-off-by: Felipe Balbi <balbi@xxxxxx> diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c index 90e82e2..0e52309 100644 --- a/drivers/usb/gadget/u_ether.c +++ b/drivers/usb/gadget/u_ether.c @@ -669,6 +669,8 @@ static int eth_stop(struct net_device *net) spin_lock_irqsave(&dev->lock, flags); if (dev->port_usb) { struct gether *link = dev->port_usb; + const struct usb_endpoint_descriptor *in; + const struct usb_endpoint_descriptor *out; if (link->close) link->close(link); @@ -682,10 +684,14 @@ static int eth_stop(struct net_device *net) * their own pace; the network stack can handle old packets. * For the moment we leave this here, since it works. */ + in = link->in_ep->desc; + out = link->out_ep->desc; 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"); + link->in_ep->desc = in; + link->out_ep->desc = out; usb_ep_enable(link->in_ep); usb_ep_enable(link->out_ep); } does it solve the issue you found ? -- balbi
Attachment:
signature.asc
Description: Digital signature