Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 7 Jan 2019 17:33:10 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc:     oneukum@suse.com, stern@rowland.harvard.edu,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH v3] usb: hub: add retry routine after intr URB submit
 error
Message-ID: <20190107163310.GA17519@kroah.com>
References: <20181120143438.18296-1-nsaenzjulienne@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181120143438.18296-1-nsaenzjulienne@suse.de>
User-Agent: Mutt/1.11.1 (2018-12-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 20, 2018 at 03:34:38PM +0100, Nicolas Saenz Julienne wrote:
> The hub sends hot-plug events to the host trough it's interrupt URB. The
> driver takes care of completing the URB and re-submitting it. Completion
> errors are handled in the hub_event() work, yet submission errors are
> ignored, rendering the device unresponsive. All further events are lost.
> 
> It is fairly hard to find this issue in the wild, since you have to time
> the USB hot-plug event with the URB submission failure. For instance it
> could be the system running out of memory or some malfunction in the USB
> controller driver. Nevertheless, it's pretty reasonable to think it'll
> happen sometime. One can trigger this issue using eBPF's function
> override feature (see BCC's inject.py script).
> 
> This patch adds a retry routine to the event of a submission error. The
> HUB driver will try to re-submit the URB once every second until it's
> successful or the HUB is disconnected.
> 
> As some USB subsystems already take care of this issue, the
> implementation was inspired from usbhid/hid_core.c's.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> 
> ---

What ever happened to this patch?  Is it still needed?  Oliver and Alan,
any objections about it anymore?

thanks,

greg k-h

> 
> v3: As per Oliver's request:
>   - Take care of race condition between disconnect and irq
> 
> v2: as per Alan's and Oliver's comments:
>   - Rename timer
>   - Delete the timer on disconnect
>   - Don't reset HUB nor exponential slowdown the timer, 1s fixed retry
>     period
>   - Check for -ESHUTDOWN prior kicking the timer
> 
>  drivers/usb/core/hub.c | 45 ++++++++++++++++++++++++++++++++++++------
>  drivers/usb/core/hub.h |  2 ++
>  2 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c6077d582d29..630490a35301 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -607,6 +607,38 @@ static int hub_port_status(struct usb_hub *hub, int port1,
>  				   status, change, NULL);
>  }
>  
> +static void hub_resubmit_irq_urb(struct usb_hub *hub)
> +{
> +	unsigned long flags;
> +	int status;
> +
> +	spin_lock_irqsave(&hub->irq_urb_lock, flags);
> +
> +	if (hub->quiescing) {
> +		spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
> +		return;
> +	}
> +
> +	status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> +	if (status && 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));
> +
> +	}
> +
> +	spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
> +}
> +
> +static void hub_retry_irq_urb(struct timer_list *t)
> +{
> +	struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
> +
> +	hub_resubmit_irq_urb(hub);
> +}
> +
> +
>  static void kick_hub_wq(struct usb_hub *hub)
>  {
>  	struct usb_interface *intf;
> @@ -709,12 +741,7 @@ static void hub_irq(struct urb *urb)
>  	kick_hub_wq(hub);
>  
>  resubmit:
> -	if (hub->quiescing)
> -		return;
> -
> -	status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> -	if (status != 0 && status != -ENODEV && status != -EPERM)
> -		dev_err(hub->intfdev, "resubmit --> %d\n", status);
> +	hub_resubmit_irq_urb(hub);
>  }
>  
>  /* USB 2.0 spec Section 11.24.2.3 */
> @@ -1254,10 +1281,13 @@ enum hub_quiescing_type {
>  static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
>  {
>  	struct usb_device *hdev = hub->hdev;
> +	unsigned long flags;
>  	int i;
>  
>  	/* hub_wq and related activity won't re-trigger */
> +	spin_lock_irqsave(&hub->irq_urb_lock, flags);
>  	hub->quiescing = 1;
> +	spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
>  
>  	if (type != HUB_SUSPEND) {
>  		/* Disconnect all the children */
> @@ -1268,6 +1298,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);
>  	usb_kill_urb(hub->urb);
>  	if (hub->has_indicators)
>  		cancel_delayed_work_sync(&hub->leds);
> @@ -1800,6 +1831,8 @@ 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);
> +	spin_lock_init(&hub->irq_urb_lock);
> +	timer_setup(&hub->irq_urb_retry, hub_retry_irq_urb, 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..a9e24e4b8df1 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -69,6 +69,8 @@ struct usb_hub {
>  	struct delayed_work	leds;
>  	struct delayed_work	init_work;
>  	struct work_struct      events;
> +	spinlock_t		irq_urb_lock;
> +	struct timer_list	irq_urb_retry;
>  	struct usb_port		**ports;
>  };
>  
> -- 
> 2.19.1
>