Re: [PATCH] usb: hub: add I/O error retry & reset routine

[Nicolas Saenz Julienne]

Hi Alan,
thanks for the review.

On Thu, 2018-11-15 at 14:24 -0500, Alan Stern wrote:
> On Thu, 15 Nov 2018, Nicolas Saenz Julienne wrote:
> 
> > An URB submission error in the HUB's endpoint completion function
> > renders the whole HUB device unresponsive. This patch introduces a
> > routine that retries the submission for 1s to then, as a last
> resort,
> > reset the whole device.
> > 
> > The implementation is based on usbhid/hid_core.c's, which
> implements the
> > same functionality. It was tested with the help of BCC's error
> injection
> > tool (inject.py).
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
> 
> Why do you think this is needed?  Are you experiencing these 
> sorts of URB submission errors?

Sorry, I should have been more explicit on where I come from. I've been
playing around injecting atomic allocation errors on the USB stack. For
example any URB submission marked with GFP_ATOMIC that ends up into
xhci will allocate some memory with that flag.

Most subsystems, after facing a burst of memory allocation failures,
seem to recover well (usbnet/hid/uas/alsa/serial). But I found out that
it's not the case for USB hubs: In the event of detecting a new device
the hub will complete an int URB which was previously sent to it. The
event data is saved by the host and the URB is resubmitted for further
event passing. In the case that URB submission failed, we lose all
further events.

It is indeed pretty hard to find this issue in the wild, since you have
to time plugging or unplugging an USB device with the system running
out of memory. But I don't think it's unrealistic to think it might
happen.

As I comment in the patch description, I'm injecting the errors using
BCC and eBPF's function override capabilities.  

> 
> Why do you handle only errors during submission but not during
> completion?  And if you keep on getting errors during submission, why
> would resetting the hub make any difference?

Well, as far as I know, errors during completion are handled. The error
is marked in hub->error, which later-on, in hub-event(), triggers a
device reset. 

While implementing the solution I took into account the hub's
completion error processing behavior and HID's implementation of the
submission error handling (see hid_irq_in() in usbhid/hid-core.c). My
rationale was that since both HID and hub are USB devices with a
similar behavior there was no point in reinventing a mechanism. That
said I have no spec data to back the "1s retry window to then reset the
device".

One could argue that in the event of an error having a timer running
forever is not the best design. It has to stop sometime. If that's the
case, the HUB will be in a unknown state, i.e. a device might have
disappeared. Resetting the hub will at least unbind all the USB devices
attached to it and retry the enumeration. Regardless of the
enumeration's success we'll at least be in a "safe" state.

> 
> The patch doesn't delete the io_retry timer when the hub is removed.

Right, that was silly of me...

Kind Regards,
Nicolas

> 
> Alan Stern
> 
> 
> > ---
> >  drivers/usb/core/hub.c | 43
> +++++++++++++++++++++++++++++++++++++++++-
> >  drivers/usb/core/hub.h |  3 +++
> >  2 files changed, 45 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index d9bd7576786a..1447bdba59ec 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -607,6 +607,44 @@ static int hub_port_status(struct usb_hub
> *hub, int port1,
> >                                  status, change, NULL);
> >  }
> >  
> > +static void hub_io_error(struct usb_hub *hub)
> > +{
> > +     /*
> > +      * If it has been a while since the last error, we'll assume
> > +      * this a brand new error and reset the retry timeout.
> > +      */
> > +     if (time_after(jiffies, hub->stop_retry + HZ/2))
> > +             hub->retry_delay = 0;
> > +
> > +     /* When an error occurs, retry at increasing intervals */
> > +     if (hub->retry_delay == 0) {
> > +             hub->retry_delay = 13;  /* Then 26, 52, 104, 104, ...
> */
> > +             hub->stop_retry = jiffies + msecs_to_jiffies(1000);
> > +     } else if (hub->retry_delay < 100)
> > +             hub->retry_delay *= 2;
> > +
> > +     if (time_after(jiffies, hub->stop_retry)) {
> > +             /* Retries failed, so do a port reset */
> > +             usb_queue_reset_device(to_usb_interface(hub-
> >intfdev));
> > +             return;
> > +     }
> > +
> > +     mod_timer(&hub->io_retry,
> > +                     jiffies + msecs_to_jiffies(hub-
> >retry_delay));
> > +}
> > +
> > +static void hub_retry_timeout(struct timer_list *t)
> > +{
> > +     struct usb_hub *hub = from_timer(hub, t, io_retry);
> > +
> > +     if (hub->disconnected || hub->quiescing)
> > +             return;
> > +
> > +     dev_err(hub->intfdev, "retrying int urb\n");
> > +     if (usb_submit_urb(hub->urb, GFP_ATOMIC))
> > +             hub_io_error(hub);
> > +}
> > +
> >  static void kick_hub_wq(struct usb_hub *hub)
> >  {
> >       struct usb_interface *intf;
> > @@ -713,8 +751,10 @@ static void hub_irq(struct urb *urb)
> >               return;
> >  
> >       status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> > -     if (status != 0 && status != -ENODEV && status != -EPERM)
> > +     if (status != 0 && status != -ENODEV && status != -EPERM) {
> >               dev_err(hub->intfdev, "resubmit --> %d\n", status);
> > +             hub_io_error(hub);
> > +     }
> >  }
> >  
> >  /* USB 2.0 spec Section 11.24.2.3 */
> > @@ -1800,6 +1840,7 @@ static int hub_probe(struct usb_interface
> *intf, const struct usb_device_id *id)
> >       INIT_DELAYED_WORK(&hub->leds, led_work);
> >       INIT_DELAYED_WORK(&hub->init_work, NULL);
> >       INIT_WORK(&hub->events, hub_event);
> > +     timer_setup(&hub->io_retry, hub_retry_timeout, 0);
> >       usb_get_intf(intf);
> >       usb_get_dev(hdev);
> >  
> > diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> > index 4accfb63f7dc..7dbd19c2c8d9 100644
> > --- a/drivers/usb/core/hub.h
> > +++ b/drivers/usb/core/hub.h
> > @@ -69,6 +69,9 @@ struct usb_hub {
> >       struct delayed_work     leds;
> >       struct delayed_work     init_work;
> >       struct work_struct      events;
> > +     struct timer_list       io_retry;
> > +     unsigned long           stop_retry;
> > +     unsigned int            retry_delay;
> >       struct usb_port         **ports;
> >  };
> >  
> > 
>

Attachment: signature.asc
Description: This is a digitally signed message part