Hi Oliver,
thanks for the reviews :).
On Mon, 2018-11-19 at 15:04 +0100, Oliver Neukum wrote:
> On Mo, 2018-11-19 at 15:02 +0100, Nicolas Saenz Julienne wrote:
> > +static void hub_retry_irq_urb(struct timer_list *t)
> > +{
> > + struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
> > + int status;
> > +
> > + if (hub->disconnected || hub->quiescing)
> > + return;
> > +
> > + dev_err(hub->intfdev, "retrying int urb\n");
> > + status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> > + if (status && status != -ENODEV && status != -EPERM &&
> > + status != -ESHUTDOWN)
> > + mod_timer(&hub->irq_urb_retry,
> > + jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> > +}
> > +
> > static void kick_hub_wq(struct usb_hub *hub)
> > {
> > struct usb_interface *intf;
> > @@ -713,8 +729,12 @@ 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 &&
> > + status != -ESHUTDOWN) {
> > dev_err(hub->intfdev, "resubmit --> %d\n", status);
> > + mod_timer(&hub->irq_urb_retry,
> > + jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> > + }
> > }
> >
> > /* USB 2.0 spec Section 11.24.2.3 */
> > @@ -1268,6 +1288,7 @@ static void hub_quiesce(struct usb_hub *hub,
> > enum hub_quiescing_type type)
> > }
> >
> > /* Stop hub_wq and related activity */
> > + del_timer_sync(&hub->irq_urb_retry);
>
> That is a race condition. You kill the timer here, but the URB may
> still be in flight. And if it fails, it will restart the error
> handler. You have to introduce a flag or poison the URB.
I see, wouldn't checking "hub->quiescing" in hub_irq()'s submit error
path do the work? Something the likes of this:
@@ -713,8 +729,12 @@ 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 &&
+ status != -ESHUTDOWN && !hub->quiescing) {
dev_err(hub->intfdev, "resubmit --> %d\n", status);
+ mod_timer(&hub->irq_urb_retry,
+ jiffies + msecs_to_jiffies(MSEC_PER_SEC));
+ }
>
> > usb_kill_urb(hub->urb);
> > if (hub->has_indicators)
> > cancel_delayed_work_sync(&hub->leds);
> >
>
> Regards
> Oliver
>
Also, looking at this made me realize that there is no real need to
check "hub->disconnected" in the timer's function, since "hub-
>quiescing" is also set in that code path before any actual cleanup.
I'll update it in the next revision.
Regards,
Nicolas
Attachment:
signature.asc
Description: This is a digitally signed message part
