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
