[RFC PATCH v1 0/6] USB: HCD/EHCI: giveback of URB in tasklet context

[RFC PATCH v1 0/6] USB: HCD/EHCI: giveback of URB in tasklet context

[Ming Lei]

Hi,

The patchset supports to run giveback of URB in tasklet context, so that
DMA unmapping/mapping on transfer buffer and compelte() callback can be
run with interrupt enabled, then time of HCD interrupt handler(IRQs
disabled time) can be saved much. Also this approach may simplify HCD
since HCD lock needn't be released any more before calling
usb_hcd_giveback_urb().

But, some drivers may assume IRQs are disabled in complete(),
and they may use spin_lock() to hold lock, so deadlock might be caused
when the lock is acquired in hard interrupt context. So currently patch
2/6 disables IRQs when calling complete() in tasklet, and at the same time
we can start to clean up drivers by converting spin_lock() in complete()
to spin_lock_irqsave() if the deadlock situation may exist. After the
cleanup is completed, complete() in tasklet will be called with IRQs
enabled.

Patch 5/6 improves interrupt qh unlink on EHCI HCD when the mechanism
is introduced.

The patchset enables the mechanism on EHCI HCD.

In the commit log of patch 6/6, detailed test result on three machines
(ARM A9/A15 dual core, X86) are provided about transfer performance and
ehci irq handling time. From the result, no transfer performance loss
is found and ehci irq handling time drops much with the patchset.


V1:
	- change percput tasklet into tasklet in HCD to avoid out of order
	of URB->complete() for same endpoint

	- disable local IRQs when calling complete() from tasklet to
	avoid deadlock which is caused by holding lock via spin_lock
	and the same lock might be acquired in hard irq context

	- document coming change about calling complete() with irq enabled
	so that we can start to clean up USB drivers which call spin_lock()
	in complete()


 Documentation/usb/URB.txt         |   30 +++---
 drivers/usb/core/hcd.c            |  196 ++++++++++++++++++++++++++++++++-----
 drivers/usb/host/ehci-fsl.c       |    2 +-
 drivers/usb/host/ehci-grlib.c     |    2 +-
 drivers/usb/host/ehci-hcd.c       |   18 +++-
 drivers/usb/host/ehci-hub.c       |    1 +
 drivers/usb/host/ehci-mv.c        |    2 +-
 drivers/usb/host/ehci-octeon.c    |    2 +-
 drivers/usb/host/ehci-pmcmsp.c    |    2 +-
 drivers/usb/host/ehci-ppc-of.c    |    2 +-
 drivers/usb/host/ehci-ps3.c       |    2 +-
 drivers/usb/host/ehci-q.c         |    6 +-
 drivers/usb/host/ehci-sched.c     |   60 +++++++++++-
 drivers/usb/host/ehci-sead3.c     |    2 +-
 drivers/usb/host/ehci-sh.c        |    2 +-
 drivers/usb/host/ehci-tegra.c     |    2 +-
 drivers/usb/host/ehci-tilegx.c    |    2 +-
 drivers/usb/host/ehci-timer.c     |   43 ++++++++
 drivers/usb/host/ehci-w90x900.c   |    2 +-
 drivers/usb/host/ehci-xilinx-of.c |    2 +-
 drivers/usb/host/ehci.h           |    3 +
 include/linux/usb/hcd.h           |   17 ++++
 22 files changed, 341 insertions(+), 59 deletions(-)

Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

This patch implements the mechanism of giveback of URB in
tasklet context, so that hardware interrupt handling time for
usb host controller can be saved much, and HCD interrupt handling
can be simplified.

Motivations:

1), on some arch(such as ARM), DMA mapping/unmapping is a bit
time-consuming, for example: when accessing usb mass storage
via EHCI on pandaboard, the common length of transfer buffer is 120KB,
the time consumed on DMA unmapping may reach hundreds of microseconds;
even on A15 based box, the time is still about scores of microseconds

2), on some arch, reading DMA coherent memoery is very time-consuming,
the most common example is usb video class driver[1]

3), driver's complete() callback may do much things which is driver
specific, so the time is consumed unnecessarily in hardware irq context.

4), running driver's complete() callback in hardware irq context causes
that host controller driver has to release its lock in interrupt handler,
so reacquiring the lock after return may busy wait a while and increase
interrupt handling time. More seriously, releasing the HCD lock makes
HCD becoming quite complicated to deal with introduced races.

So the patch proposes to run giveback of URB in tasklet context, then
time consumed in HCD irq handling doesn't depend on drivers' complete and
DMA mapping/unmapping any more, also we can simplify HCD since the HCD
lock isn't needed to be released during irq handling.

The patch should be reasonable and doable:

1), for drivers, they don't care if the complete() is called in hard irq
context or softirq context

2), the biggest change is the situation in which usb_submit_urb() is called
in complete() callback, so the introduced tasklet schedule delay might be a
con, but it shouldn't be a big deal:

	- control/bulk asynchronous transfer isn't sensitive to schedule
	  delay

	- the patch schedules giveback of periodic URBs using
	  tasklet_hi_schedule, so the introduced delay should be very
	  small

	- for ISOC transfer, generally, drivers submit several URBs
	  concurrently to avoid interrupt delay, so it is OK with the
	  little schedule delay.

	- for interrupt transfer, generally, drivers only submit one URB
	  at the same time, but interrupt transfer is often used in event
	  report, polling, ... situations, and a little delay should be OK.

Considered that HCDs may optimize on submitting URB in complete(), the
patch may cause the optimization not working, so introduces one flag to mark
if the HCD supports to run giveback URB in tasklet context. When all HCDs
are ready, the flag can be removed.

[1], http://marc.info/?t=136438111600010&r=1&w=2

Cc: Oliver Neukum <oliver@neukum.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/core/hcd.c  |  175 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/usb/hcd.h |   17 +++++
 2 files changed, 169 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 014dc99..a272968 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -699,9 +699,11 @@ error:
 	 * Avoiding calls to local_irq_disable/enable makes the code
 	 * RT-friendly.
 	 */
-	spin_unlock(&hcd_root_hub_lock);
+	if (!hcd_giveback_urb_in_bh(hcd))
+		spin_unlock(&hcd_root_hub_lock);
 	usb_hcd_giveback_urb(hcd, urb, status);
-	spin_lock(&hcd_root_hub_lock);
+	if (!hcd_giveback_urb_in_bh(hcd))
+		spin_lock(&hcd_root_hub_lock);
 
 	spin_unlock_irq(&hcd_root_hub_lock);
 	return 0;
@@ -742,9 +744,11 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
 			memcpy(urb->transfer_buffer, buffer, length);
 
 			usb_hcd_unlink_urb_from_ep(hcd, urb);
-			spin_unlock(&hcd_root_hub_lock);
+			if (!hcd_giveback_urb_in_bh(hcd))
+				spin_unlock(&hcd_root_hub_lock);
 			usb_hcd_giveback_urb(hcd, urb, 0);
-			spin_lock(&hcd_root_hub_lock);
+			if (!hcd_giveback_urb_in_bh(hcd))
+				spin_lock(&hcd_root_hub_lock);
 		} else {
 			length = 0;
 			set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags);
@@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 			hcd->status_urb = NULL;
 			usb_hcd_unlink_urb_from_ep(hcd, urb);
 
-			spin_unlock(&hcd_root_hub_lock);
+			if (!hcd_giveback_urb_in_bh(hcd))
+				spin_unlock(&hcd_root_hub_lock);
 			usb_hcd_giveback_urb(hcd, urb, status);
-			spin_lock(&hcd_root_hub_lock);
+			if (!hcd_giveback_urb_in_bh(hcd))
+				spin_lock(&hcd_root_hub_lock);
 		}
 	}
  done:
@@ -1648,6 +1654,60 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
 
 /*-------------------------------------------------------------------------*/
 
+static void __usb_hcd_giveback_urb(struct urb *urb)
+{
+	struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
+	int status = urb->status;
+
+	urb->hcpriv = NULL;
+	if (unlikely(urb->unlinked))
+		status = urb->unlinked;
+	else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
+			urb->actual_length < urb->transfer_buffer_length &&
+			!status))
+		status = -EREMOTEIO;
+
+	unmap_urb_for_dma(hcd, urb);
+	usbmon_urb_complete(&hcd->self, urb, status);
+	usb_unanchor_urb(urb);
+
+	/* pass ownership to the completion handler */
+	urb->status = status;
+	urb->complete (urb);
+	atomic_dec (&urb->use_count);
+	if (unlikely(atomic_read(&urb->reject)))
+		wake_up (&usb_kill_urb_queue);
+	usb_put_urb (urb);
+}
+
+static void usb_giveback_urb_bh(unsigned long param)
+{
+	struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param;
+	unsigned long flags;
+	struct list_head local_list;
+
+	spin_lock_irqsave(&bh->lock, flags);
+	bh->running = 1;
+restart:
+	list_replace_init(&bh->head, &local_list);
+	spin_unlock_irqrestore(&bh->lock, flags);
+
+	while (!list_empty(&local_list)) {
+		struct urb *urb;
+
+		urb = list_entry(local_list.next, struct urb, urb_list);
+		list_del_init(&urb->urb_list);
+		__usb_hcd_giveback_urb(urb);
+	}
+
+	/* check if there are new URBs to giveback */
+	spin_lock_irqsave(&bh->lock, flags);
+	if (!list_empty(&bh->head))
+		goto restart;
+	bh->running = 0;
+	spin_unlock_irqrestore(&bh->lock, flags);
+}
+
 /**
  * usb_hcd_giveback_urb - return URB from HCD to device driver
  * @hcd: host controller returning the URB
@@ -1667,25 +1727,33 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
  */
 void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 {
-	urb->hcpriv = NULL;
-	if (unlikely(urb->unlinked))
-		status = urb->unlinked;
-	else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
-			urb->actual_length < urb->transfer_buffer_length &&
-			!status))
-		status = -EREMOTEIO;
+	struct giveback_urb_bh *bh = hcd->async_bh;
+	bool async = 1;
+	bool sched = 1;
 
-	unmap_urb_for_dma(hcd, urb);
-	usbmon_urb_complete(&hcd->self, urb, status);
-	usb_unanchor_urb(urb);
-
-	/* pass ownership to the completion handler */
 	urb->status = status;
-	urb->complete (urb);
-	atomic_dec (&urb->use_count);
-	if (unlikely(atomic_read(&urb->reject)))
-		wake_up (&usb_kill_urb_queue);
-	usb_put_urb (urb);
+	if (!hcd_giveback_urb_in_bh(hcd)) {
+		__usb_hcd_giveback_urb(urb);
+		return;
+	}
+
+	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) {
+		bh = hcd->periodic_bh;
+		async = 0;
+	}
+
+	spin_lock(&bh->lock);
+	list_add_tail(&urb->urb_list, &bh->head);
+	if (bh->running)
+		sched = 0;
+	spin_unlock(&bh->lock);
+
+	if (sched) {
+		if (async)
+			tasklet_schedule(&bh->bh);
+		else
+			tasklet_hi_schedule(&bh->bh);
+	}
 }
 EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);
 
@@ -2307,6 +2375,52 @@ EXPORT_SYMBOL_GPL (usb_hc_died);
 
 /*-------------------------------------------------------------------------*/
 
+static void __init_giveback_urb_bh(struct giveback_urb_bh *bh)
+{
+
+	spin_lock_init(&bh->lock);
+	INIT_LIST_HEAD(&bh->head);
+	tasklet_init(&bh->bh, usb_giveback_urb_bh, (unsigned long)bh);
+}
+
+static int init_giveback_urb_bh(struct usb_hcd *hcd)
+{
+	if (!hcd_giveback_urb_in_bh(hcd))
+		return 0;
+
+	hcd->periodic_bh = kzalloc(sizeof(*hcd->periodic_bh), GFP_KERNEL);
+	if (!hcd->periodic_bh)
+		return -ENOMEM;
+
+	hcd->async_bh = kzalloc(sizeof(*hcd->async_bh), GFP_KERNEL);
+	if (!hcd->async_bh) {
+		kfree(hcd->periodic_bh);
+		return -ENOMEM;
+	}
+
+	__init_giveback_urb_bh(hcd->periodic_bh);
+	__init_giveback_urb_bh(hcd->async_bh);
+
+	return 0;
+}
+
+static void __exit_giveback_urb_bh(struct giveback_urb_bh *bh)
+{
+	tasklet_kill(&bh->bh);
+}
+
+static void exit_giveback_urb_bh(struct usb_hcd *hcd)
+{
+	if (!hcd_giveback_urb_in_bh(hcd))
+		return;
+
+	__exit_giveback_urb_bh(hcd->periodic_bh);
+	__exit_giveback_urb_bh(hcd->async_bh);
+
+	kfree(hcd->periodic_bh);
+	kfree(hcd->async_bh);
+}
+
 /**
  * usb_create_shared_hcd - create and initialize an HCD structure
  * @driver: HC driver that will use this hcd
@@ -2573,6 +2687,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
 			&& device_can_wakeup(&hcd->self.root_hub->dev))
 		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
 
+	if (usb_hcd_is_primary_hcd(hcd)) {
+		retval = init_giveback_urb_bh(hcd);
+		if (retval)
+			goto err_init_giveback_bh;
+	} else {
+		/* share tasklet handling with primary hcd */
+		hcd->async_bh = hcd->primary_hcd->async_bh;
+		hcd->periodic_bh = hcd->primary_hcd->periodic_bh;
+	}
+
 	/* enable irqs just before we start the controller,
 	 * if the BIOS provides legacy PCI irqs.
 	 */
@@ -2636,6 +2760,8 @@ err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);
 err_request_irq:
+	exit_giveback_urb_bh(hcd);
+err_init_giveback_bh:
 err_hcd_driver_setup:
 err_set_rh_speed:
 	usb_put_dev(hcd->self.root_hub);
@@ -2681,6 +2807,9 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 	usb_disconnect(&rhdev);		/* Sets rhdev to NULL */
 	mutex_unlock(&usb_bus_list_lock);
 
+	if (usb_hcd_is_primary_hcd(hcd))
+		exit_giveback_urb_bh(hcd);
+
 	/* Prevent any more root-hub status calls from the timer.
 	 * The HCD might still restart the timer (if a port status change
 	 * interrupt occurs), but usb_hcd_poll_rh_status() won't invoke
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 1e88377..9fd3409 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -22,6 +22,7 @@
 #ifdef __KERNEL__
 
 #include <linux/rwsem.h>
+#include <linux/interrupt.h>
 
 #define MAX_TOPO_LEVEL		6
 
@@ -67,6 +68,13 @@
 
 /*-------------------------------------------------------------------------*/
 
+struct giveback_urb_bh {
+	bool running;
+	spinlock_t lock;
+	struct list_head  head;
+	struct tasklet_struct bh;
+};
+
 struct usb_hcd {
 
 	/*
@@ -139,6 +147,9 @@ struct usb_hcd {
 	resource_size_t		rsrc_len;	/* memory/io resource length */
 	unsigned		power_budget;	/* in mA, 0 = no limit */
 
+	struct giveback_urb_bh  *periodic_bh;
+	struct giveback_urb_bh  *async_bh;
+
 	/* bandwidth_mutex should be taken before adding or removing
 	 * any new bus bandwidth constraints:
 	 *   1. Before adding a configuration for a new device.
@@ -221,6 +232,7 @@ struct hc_driver {
 #define	HCD_USB25	0x0030		/* Wireless USB 1.0 (USB 2.5)*/
 #define	HCD_USB3	0x0040		/* USB 3.0 */
 #define	HCD_MASK	0x0070
+#define	HCD_BH		0x0100		/* URB complete in BH context */
 
 	/* called to init HCD and root hub */
 	int	(*reset) (struct usb_hcd *hcd);
@@ -361,6 +373,11 @@ struct hc_driver {
 	int	(*find_raw_port_number)(struct usb_hcd *, int);
 };
 
+static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
+{
+	return hcd->driver->flags & HCD_BH;
+}
+
 extern int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd, struct urb *urb);
 extern int usb_hcd_check_unlink_urb(struct usb_hcd *hcd, struct urb *urb,
 		int status);
-- 
1.7.9.5

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Alan Stern]

On Tue, 18 Jun 2013, Ming Lei wrote:

> This patch implements the mechanism of giveback of URB in
> tasklet context, so that hardware interrupt handling time for
> usb host controller can be saved much, and HCD interrupt handling
> can be simplified.

Okay.  I'd make a few relatively minor changes.

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 014dc99..a272968 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -699,9 +699,11 @@ error:
>  	 * Avoiding calls to local_irq_disable/enable makes the code
>  	 * RT-friendly.
>  	 */
> -	spin_unlock(&hcd_root_hub_lock);
> +	if (!hcd_giveback_urb_in_bh(hcd))
> +		spin_unlock(&hcd_root_hub_lock);
>  	usb_hcd_giveback_urb(hcd, urb, status);
> -	spin_lock(&hcd_root_hub_lock);
> +	if (!hcd_giveback_urb_in_bh(hcd))
> +		spin_lock(&hcd_root_hub_lock);
>  
>  	spin_unlock_irq(&hcd_root_hub_lock);
>  	return 0;
> @@ -742,9 +744,11 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
>  			memcpy(urb->transfer_buffer, buffer, length);
>  
>  			usb_hcd_unlink_urb_from_ep(hcd, urb);
> -			spin_unlock(&hcd_root_hub_lock);
> +			if (!hcd_giveback_urb_in_bh(hcd))
> +				spin_unlock(&hcd_root_hub_lock);
>  			usb_hcd_giveback_urb(hcd, urb, 0);
> -			spin_lock(&hcd_root_hub_lock);
> +			if (!hcd_giveback_urb_in_bh(hcd))
> +				spin_lock(&hcd_root_hub_lock);
>  		} else {
>  			length = 0;
>  			set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags);
> @@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>  			hcd->status_urb = NULL;
>  			usb_hcd_unlink_urb_from_ep(hcd, urb);
>  
> -			spin_unlock(&hcd_root_hub_lock);
> +			if (!hcd_giveback_urb_in_bh(hcd))
> +				spin_unlock(&hcd_root_hub_lock);
>  			usb_hcd_giveback_urb(hcd, urb, status);
> -			spin_lock(&hcd_root_hub_lock);
> +			if (!hcd_giveback_urb_in_bh(hcd))
> +				spin_lock(&hcd_root_hub_lock);
>  		}
>  	}
>   done:

None of these tests are necessary.  Root hubs are different from normal
devices; their URBs are handled mostly by usbcore.  The only part done
by the HCD is always synchronous.  And we know that root-hub URBs
always have a very short completion handler.  So we may as well
continue to handle root-hub URBs the way we always have.

> @@ -1648,6 +1654,60 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static void __usb_hcd_giveback_urb(struct urb *urb)
> +{
> +	struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
> +	int status = urb->status;
> +
> +	urb->hcpriv = NULL;
> +	if (unlikely(urb->unlinked))
> +		status = urb->unlinked;

The way you're storing the status value isn't good.  We decided to make 
the status a separate argument because of drivers that abuse the 
urb->status field (they poll it instead of waiting for the completion 
handler to be called).  Hopefully there aren't any examples of drivers 
still doing this, but...

This means you shouldn't store anything in urb->status until just
before calling the completion handler.  What you can do instead is
always store the status value in urb->unlinked.

> +	else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
> +			urb->actual_length < urb->transfer_buffer_length &&
> +			!status))
> +		status = -EREMOTEIO;
> +
> +	unmap_urb_for_dma(hcd, urb);
> +	usbmon_urb_complete(&hcd->self, urb, status);
> +	usb_unanchor_urb(urb);
> +
> +	/* pass ownership to the completion handler */
> +	urb->status = status;
> +	urb->complete (urb);

You are supposed to disable local interrupts around the call to the 
completion handler.

> +	atomic_dec (&urb->use_count);
> +	if (unlikely(atomic_read(&urb->reject)))
> +		wake_up (&usb_kill_urb_queue);
> +	usb_put_urb (urb);
> +}
> +
> +static void usb_giveback_urb_bh(unsigned long param)
> +{
> +	struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param;
> +	unsigned long flags;
> +	struct list_head local_list;
> +
> +	spin_lock_irqsave(&bh->lock, flags);
> +	bh->running = 1;
> +restart:

I like to have statement labels indented by a single space character, 
so that tools like diff don't think the label marks the start of a new 
function.

> @@ -1667,25 +1727,33 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
>   */
>  void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
>  {
> -	urb->hcpriv = NULL;
> -	if (unlikely(urb->unlinked))
> -		status = urb->unlinked;
> -	else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
> -			urb->actual_length < urb->transfer_buffer_length &&
> -			!status))
> -		status = -EREMOTEIO;
> +	struct giveback_urb_bh *bh = hcd->async_bh;
> +	bool async = 1;
> +	bool sched = 1;

I know this is a common way of doing things, but to me it makes more 
sense in this case to set these values later on, in an "if ... else 
..." statement.

>  
> -	unmap_urb_for_dma(hcd, urb);
> -	usbmon_urb_complete(&hcd->self, urb, status);
> -	usb_unanchor_urb(urb);
> -
> -	/* pass ownership to the completion handler */
>  	urb->status = status;

This should now be:

	if (urb->unlinked == 0)
		urb->unlinked = status;

> -	urb->complete (urb);
> -	atomic_dec (&urb->use_count);
> -	if (unlikely(atomic_read(&urb->reject)))
> -		wake_up (&usb_kill_urb_queue);
> -	usb_put_urb (urb);
> +	if (!hcd_giveback_urb_in_bh(hcd)) {
> +		__usb_hcd_giveback_urb(urb);
> +		return;
> +	}
> +
> +	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) {
> +		bh = hcd->periodic_bh;
> +		async = 0;

I don't like these names, for several reasons.  The difference between 
the two tasklets isn't that one is meant specifically for periodic URBs 
and the other for async URBs; the difference is that one runs with 
higher priority than the other.  The names should reflect this.  For 
example, you could call them hcd->low_prio_bh and hcd->high_prio_bh.  

> +	}
> +
> +	spin_lock(&bh->lock);
> +	list_add_tail(&urb->urb_list, &bh->head);
> +	if (bh->running)
> +		sched = 0;
> +	spin_unlock(&bh->lock);
> +
> +	if (sched) {
> +		if (async)
> +			tasklet_schedule(&bh->bh);
> +		else
> +			tasklet_hi_schedule(&bh->bh);
> +	}


Also, you could combine async and sched into a single variable.
Using an enumerated type for sched, this would become:

	if (!sched)
		;	/* tasklet doesn't need to be scheduled */
	else if (sched == HIGH_PRIO)
		tasklet_hi_schedule(&bh->bh);
	else
		tasklet_schedule(&bh->bh);

Or you could turn this into a "switch" statement.

> +static int init_giveback_urb_bh(struct usb_hcd *hcd)
> +{
> +	if (!hcd_giveback_urb_in_bh(hcd))
> +		return 0;
> +
> +	hcd->periodic_bh = kzalloc(sizeof(*hcd->periodic_bh), GFP_KERNEL);
> +	if (!hcd->periodic_bh)
> +		return -ENOMEM;
> +
> +	hcd->async_bh = kzalloc(sizeof(*hcd->async_bh), GFP_KERNEL);
> +	if (!hcd->async_bh) {
> +		kfree(hcd->periodic_bh);
> +		return -ENOMEM;
> +	}

I think these things can be stored directly in the usb_hcd struct
instead of allocated separately.

> +
> +	__init_giveback_urb_bh(hcd->periodic_bh);
> +	__init_giveback_urb_bh(hcd->async_bh);
> +
> +	return 0;
> +}
> +
> +static void __exit_giveback_urb_bh(struct giveback_urb_bh *bh)
> +{
> +	tasklet_kill(&bh->bh);
> +}
> +
> +static void exit_giveback_urb_bh(struct usb_hcd *hcd)
> +{
> +	if (!hcd_giveback_urb_in_bh(hcd))
> +		return;
> +
> +	__exit_giveback_urb_bh(hcd->periodic_bh);
> +	__exit_giveback_urb_bh(hcd->async_bh);

You might as well put the tasklet_kill() call directly inline instead 
of going through a separate function.

> +
> +	kfree(hcd->periodic_bh);
> +	kfree(hcd->async_bh);
> +}
> +
>  /**
>   * usb_create_shared_hcd - create and initialize an HCD structure
>   * @driver: HC driver that will use this hcd
> @@ -2573,6 +2687,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  			&& device_can_wakeup(&hcd->self.root_hub->dev))
>  		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
>  
> +	if (usb_hcd_is_primary_hcd(hcd)) {
> +		retval = init_giveback_urb_bh(hcd);
> +		if (retval)
> +			goto err_init_giveback_bh;
> +	} else {
> +		/* share tasklet handling with primary hcd */
> +		hcd->async_bh = hcd->primary_hcd->async_bh;
> +		hcd->periodic_bh = hcd->primary_hcd->periodic_bh;
> +	}

Is there any reason why a secondary HCD can't have its own tasklets?

Alan Stern

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Wed, Jun 19, 2013 at 12:05 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 18 Jun 2013, Ming Lei wrote:
>
>> This patch implements the mechanism of giveback of URB in
>> tasklet context, so that hardware interrupt handling time for
>> usb host controller can be saved much, and HCD interrupt handling
>> can be simplified.
>
> Okay.  I'd make a few relatively minor changes.
>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 014dc99..a272968 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -699,9 +699,11 @@ error:
>>        * Avoiding calls to local_irq_disable/enable makes the code
>>        * RT-friendly.
>>        */
>> -     spin_unlock(&hcd_root_hub_lock);
>> +     if (!hcd_giveback_urb_in_bh(hcd))
>> +             spin_unlock(&hcd_root_hub_lock);
>>       usb_hcd_giveback_urb(hcd, urb, status);
>> -     spin_lock(&hcd_root_hub_lock);
>> +     if (!hcd_giveback_urb_in_bh(hcd))
>> +             spin_lock(&hcd_root_hub_lock);
>>
>>       spin_unlock_irq(&hcd_root_hub_lock);
>>       return 0;
>> @@ -742,9 +744,11 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
>>                       memcpy(urb->transfer_buffer, buffer, length);
>>
>>                       usb_hcd_unlink_urb_from_ep(hcd, urb);
>> -                     spin_unlock(&hcd_root_hub_lock);
>> +                     if (!hcd_giveback_urb_in_bh(hcd))
>> +                             spin_unlock(&hcd_root_hub_lock);
>>                       usb_hcd_giveback_urb(hcd, urb, 0);
>> -                     spin_lock(&hcd_root_hub_lock);
>> +                     if (!hcd_giveback_urb_in_bh(hcd))
>> +                             spin_lock(&hcd_root_hub_lock);
>>               } else {
>>                       length = 0;
>>                       set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags);
>> @@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>>                       hcd->status_urb = NULL;
>>                       usb_hcd_unlink_urb_from_ep(hcd, urb);
>>
>> -                     spin_unlock(&hcd_root_hub_lock);
>> +                     if (!hcd_giveback_urb_in_bh(hcd))
>> +                             spin_unlock(&hcd_root_hub_lock);
>>                       usb_hcd_giveback_urb(hcd, urb, status);
>> -                     spin_lock(&hcd_root_hub_lock);
>> +                     if (!hcd_giveback_urb_in_bh(hcd))
>> +                             spin_lock(&hcd_root_hub_lock);
>>               }
>>       }
>>   done:
>
> None of these tests are necessary.  Root hubs are different from normal
> devices; their URBs are handled mostly by usbcore.  The only part done
> by the HCD is always synchronous.  And we know that root-hub URBs

Looks not always synchronous, control transfer is synchronous, and
interrupt transfer is still asynchronous. No drivers(hub, usbfs) depend
on that,  and IMO, treating root hub same as hub will simplify HCD core,
and finally we can remove all the above lock releasing & acquiring if
all HCDs set HCD_BH.

Also there is very less roothub transfers and always letting tasklet
handle URB giveback of roothub won't have performance problem, so
how about keeping the above tests?

> always have a very short completion handler.  So we may as well

root hub's completion handler is same with hub's, and usb_submit_urb()
is still called to submit new schedule.

> continue to handle root-hub URBs the way we always have.



>
>> @@ -1648,6 +1654,60 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
>>
>>  /*-------------------------------------------------------------------------*/
>>
>> +static void __usb_hcd_giveback_urb(struct urb *urb)
>> +{
>> +     struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
>> +     int status = urb->status;
>> +
>> +     urb->hcpriv = NULL;
>> +     if (unlikely(urb->unlinked))
>> +             status = urb->unlinked;
>
> The way you're storing the status value isn't good.  We decided to make
> the status a separate argument because of drivers that abuse the
> urb->status field (they poll it instead of waiting for the completion
> handler to be called).  Hopefully there aren't any examples of drivers
> still doing this, but...
>
> This means you shouldn't store anything in urb->status until just
> before calling the completion handler.  What you can do instead is
> always store the status value in urb->unlinked.

Good point, will do it.

>
>> +     else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
>> +                     urb->actual_length < urb->transfer_buffer_length &&
>> +                     !status))
>> +             status = -EREMOTEIO;
>> +
>> +     unmap_urb_for_dma(hcd, urb);
>> +     usbmon_urb_complete(&hcd->self, urb, status);
>> +     usb_unanchor_urb(urb);
>> +
>> +     /* pass ownership to the completion handler */
>> +     urb->status = status;
>> +     urb->complete (urb);
>
> You are supposed to disable local interrupts around the call to the
> completion handler.

I did it in the 2nd patch to highlight the change.

>
>> +     atomic_dec (&urb->use_count);
>> +     if (unlikely(atomic_read(&urb->reject)))
>> +             wake_up (&usb_kill_urb_queue);
>> +     usb_put_urb (urb);
>> +}
>> +
>> +static void usb_giveback_urb_bh(unsigned long param)
>> +{
>> +     struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param;
>> +     unsigned long flags;
>> +     struct list_head local_list;
>> +
>> +     spin_lock_irqsave(&bh->lock, flags);
>> +     bh->running = 1;
>> +restart:
>
> I like to have statement labels indented by a single space character,
> so that tools like diff don't think the label marks the start of a new
> function.

OK.

>
>> @@ -1667,25 +1727,33 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
>>   */
>>  void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
>>  {
>> -     urb->hcpriv = NULL;
>> -     if (unlikely(urb->unlinked))
>> -             status = urb->unlinked;
>> -     else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
>> -                     urb->actual_length < urb->transfer_buffer_length &&
>> -                     !status))
>> -             status = -EREMOTEIO;
>> +     struct giveback_urb_bh *bh = hcd->async_bh;
>> +     bool async = 1;
>> +     bool sched = 1;
>
> I know this is a common way of doing things, but to me it makes more
> sense in this case to set these values later on, in an "if ... else
> ..." statement.

OK, it is fine for me too.

>
>>
>> -     unmap_urb_for_dma(hcd, urb);
>> -     usbmon_urb_complete(&hcd->self, urb, status);
>> -     usb_unanchor_urb(urb);
>> -
>> -     /* pass ownership to the completion handler */
>>       urb->status = status;
>
> This should now be:
>
>         if (urb->unlinked == 0)
>                 urb->unlinked = status;
>
>> -     urb->complete (urb);
>> -     atomic_dec (&urb->use_count);
>> -     if (unlikely(atomic_read(&urb->reject)))
>> -             wake_up (&usb_kill_urb_queue);
>> -     usb_put_urb (urb);
>> +     if (!hcd_giveback_urb_in_bh(hcd)) {
>> +             __usb_hcd_giveback_urb(urb);
>> +             return;
>> +     }
>> +
>> +     if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) {
>> +             bh = hcd->periodic_bh;
>> +             async = 0;
>
> I don't like these names, for several reasons.  The difference between
> the two tasklets isn't that one is meant specifically for periodic URBs
> and the other for async URBs; the difference is that one runs with
> higher priority than the other.  The names should reflect this.  For
> example, you could call them hcd->low_prio_bh and hcd->high_prio_bh.

Yes, that is also what I am wanting to do, but forget to change the name,
thanks for pointing it out.

>
>> +     }
>> +
>> +     spin_lock(&bh->lock);
>> +     list_add_tail(&urb->urb_list, &bh->head);
>> +     if (bh->running)
>> +             sched = 0;
>> +     spin_unlock(&bh->lock);
>> +
>> +     if (sched) {
>> +             if (async)
>> +                     tasklet_schedule(&bh->bh);
>> +             else
>> +                     tasklet_hi_schedule(&bh->bh);
>> +     }
>
>
> Also, you could combine async and sched into a single variable.
> Using an enumerated type for sched, this would become:
>
>         if (!sched)
>                 ;       /* tasklet doesn't need to be scheduled */
>         else if (sched == HIGH_PRIO)
>                 tasklet_hi_schedule(&bh->bh);
>         else
>                 tasklet_schedule(&bh->bh);

Looks fine.

>
> Or you could turn this into a "switch" statement.
>
>> +static int init_giveback_urb_bh(struct usb_hcd *hcd)
>> +{
>> +     if (!hcd_giveback_urb_in_bh(hcd))
>> +             return 0;
>> +
>> +     hcd->periodic_bh = kzalloc(sizeof(*hcd->periodic_bh), GFP_KERNEL);
>> +     if (!hcd->periodic_bh)
>> +             return -ENOMEM;
>> +
>> +     hcd->async_bh = kzalloc(sizeof(*hcd->async_bh), GFP_KERNEL);
>> +     if (!hcd->async_bh) {
>> +             kfree(hcd->periodic_bh);
>> +             return -ENOMEM;
>> +     }
>
> I think these things can be stored directly in the usb_hcd struct
> instead of allocated separately.

Yes, the allocation is just inherited from last percpu allocation, :-)

>
>> +
>> +     __init_giveback_urb_bh(hcd->periodic_bh);
>> +     __init_giveback_urb_bh(hcd->async_bh);
>> +
>> +     return 0;
>> +}
>> +
>> +static void __exit_giveback_urb_bh(struct giveback_urb_bh *bh)
>> +{
>> +     tasklet_kill(&bh->bh);
>> +}
>> +
>> +static void exit_giveback_urb_bh(struct usb_hcd *hcd)
>> +{
>> +     if (!hcd_giveback_urb_in_bh(hcd))
>> +             return;
>> +
>> +     __exit_giveback_urb_bh(hcd->periodic_bh);
>> +     __exit_giveback_urb_bh(hcd->async_bh);
>
> You might as well put the tasklet_kill() call directly inline instead
> of going through a separate function.

OK.

>
>> +
>> +     kfree(hcd->periodic_bh);
>> +     kfree(hcd->async_bh);
>> +}
>> +
>>  /**
>>   * usb_create_shared_hcd - create and initialize an HCD structure
>>   * @driver: HC driver that will use this hcd
>> @@ -2573,6 +2687,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>                       && device_can_wakeup(&hcd->self.root_hub->dev))
>>               dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
>>
>> +     if (usb_hcd_is_primary_hcd(hcd)) {
>> +             retval = init_giveback_urb_bh(hcd);
>> +             if (retval)
>> +                     goto err_init_giveback_bh;
>> +     } else {
>> +             /* share tasklet handling with primary hcd */
>> +             hcd->async_bh = hcd->primary_hcd->async_bh;
>> +             hcd->periodic_bh = hcd->primary_hcd->periodic_bh;
>> +     }
>
> Is there any reason why a secondary HCD can't have its own tasklets?

I didn't do that because both primary and secondary HCDs share one
hard interrupt handler, so basically there is no obvious advantage to
do that.

Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Wed, Jun 19, 2013 at 10:59 AM, Ming Lei <ming.lei@canonical.com> wrote:
> On Wed, Jun 19, 2013 at 12:05 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Tue, 18 Jun 2013, Ming Lei wrote:
>>
>>> This patch implements the mechanism of giveback of URB in
>>> tasklet context, so that hardware interrupt handling time for
>>> usb host controller can be saved much, and HCD interrupt handling
>>> can be simplified.
>>
>> Okay.  I'd make a few relatively minor changes.
>>
>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>> index 014dc99..a272968 100644
>>> --- a/drivers/usb/core/hcd.c
>>> +++ b/drivers/usb/core/hcd.c
>>> @@ -699,9 +699,11 @@ error:
>>>        * Avoiding calls to local_irq_disable/enable makes the code
>>>        * RT-friendly.
>>>        */
>>> -     spin_unlock(&hcd_root_hub_lock);
>>> +     if (!hcd_giveback_urb_in_bh(hcd))
>>> +             spin_unlock(&hcd_root_hub_lock);
>>>       usb_hcd_giveback_urb(hcd, urb, status);
>>> -     spin_lock(&hcd_root_hub_lock);
>>> +     if (!hcd_giveback_urb_in_bh(hcd))
>>> +             spin_lock(&hcd_root_hub_lock);
>>>
>>>       spin_unlock_irq(&hcd_root_hub_lock);
>>>       return 0;
>>> @@ -742,9 +744,11 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
>>>                       memcpy(urb->transfer_buffer, buffer, length);
>>>
>>>                       usb_hcd_unlink_urb_from_ep(hcd, urb);
>>> -                     spin_unlock(&hcd_root_hub_lock);
>>> +                     if (!hcd_giveback_urb_in_bh(hcd))
>>> +                             spin_unlock(&hcd_root_hub_lock);
>>>                       usb_hcd_giveback_urb(hcd, urb, 0);
>>> -                     spin_lock(&hcd_root_hub_lock);
>>> +                     if (!hcd_giveback_urb_in_bh(hcd))
>>> +                             spin_lock(&hcd_root_hub_lock);
>>>               } else {
>>>                       length = 0;
>>>                       set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags);
>>> @@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>>>                       hcd->status_urb = NULL;
>>>                       usb_hcd_unlink_urb_from_ep(hcd, urb);
>>>
>>> -                     spin_unlock(&hcd_root_hub_lock);
>>> +                     if (!hcd_giveback_urb_in_bh(hcd))
>>> +                             spin_unlock(&hcd_root_hub_lock);
>>>                       usb_hcd_giveback_urb(hcd, urb, status);
>>> -                     spin_lock(&hcd_root_hub_lock);
>>> +                     if (!hcd_giveback_urb_in_bh(hcd))
>>> +                             spin_lock(&hcd_root_hub_lock);
>>>               }
>>>       }
>>>   done:
>>
>> None of these tests are necessary.  Root hubs are different from normal
>> devices; their URBs are handled mostly by usbcore.  The only part done
>> by the HCD is always synchronous.  And we know that root-hub URBs
>
> Looks not always synchronous, control transfer is synchronous, and
> interrupt transfer is still asynchronous. No drivers(hub, usbfs) depend
> on that,  and IMO, treating root hub same as hub will simplify HCD core,
> and finally we can remove all the above lock releasing & acquiring if
> all HCDs set HCD_BH.

Actually, we can remove the above releasing and acquiring lock
unconditionally now.

>
> Also there is very less roothub transfers and always letting tasklet
> handle URB giveback of roothub won't have performance problem, so
> how about keeping the above tests?

thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Alan Stern]

On Wed, 19 Jun 2013, Ming Lei wrote:

> >> @@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> >>                       hcd->status_urb = NULL;
> >>                       usb_hcd_unlink_urb_from_ep(hcd, urb);
> >>
> >> -                     spin_unlock(&hcd_root_hub_lock);
> >> +                     if (!hcd_giveback_urb_in_bh(hcd))
> >> +                             spin_unlock(&hcd_root_hub_lock);
> >>                       usb_hcd_giveback_urb(hcd, urb, status);
> >> -                     spin_lock(&hcd_root_hub_lock);
> >> +                     if (!hcd_giveback_urb_in_bh(hcd))
> >> +                             spin_lock(&hcd_root_hub_lock);
> >>               }
> >>       }
> >>   done:
> >
> > None of these tests are necessary.  Root hubs are different from normal
> > devices; their URBs are handled mostly by usbcore.  The only part done
> > by the HCD is always synchronous.  And we know that root-hub URBs
> 
> Looks not always synchronous, control transfer is synchronous, and
> interrupt transfer is still asynchronous. No drivers(hub, usbfs) depend
> on that,  and IMO, treating root hub same as hub will simplify HCD core,
> and finally we can remove all the above lock releasing & acquiring if
> all HCDs set HCD_BH.
> 
> Also there is very less roothub transfers and always letting tasklet
> handle URB giveback of roothub won't have performance problem, so
> how about keeping the above tests?
  
If you want to use the tasklets for root-hub URBs, okay.  There's no
reason to check the HCD_BH flag, though, because HCDs have only minimal
involvement in root-hub URBs.  In particular, HCD's don't call
usb_hcd_giveback_urb() for them.
  
So you can use the tasklets for _all_ root-hub URBs.  Then the tests   
above aren't necessary, and neither are the spinlock operations.

> >> @@ -2573,6 +2687,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
> >>                       && device_can_wakeup(&hcd->self.root_hub->dev))
> >>               dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
> >>
> >> +     if (usb_hcd_is_primary_hcd(hcd)) {
> >> +             retval = init_giveback_urb_bh(hcd);
> >> +             if (retval)
> >> +                     goto err_init_giveback_bh;
> >> +     } else {
> >> +             /* share tasklet handling with primary hcd */
> >> +             hcd->async_bh = hcd->primary_hcd->async_bh;
> >> +             hcd->periodic_bh = hcd->primary_hcd->periodic_bh;
> >> +     }
> >
> > Is there any reason why a secondary HCD can't have its own tasklets?
> 
> I didn't do that because both primary and secondary HCDs share one
> hard interrupt handler, so basically there is no obvious advantage to
> do that.

If the bh structures are embedded directly in the hcd structure, it 
won't be possible for a secondary hcd to share its tasklets with the 
primary hcd.  Not sharing seems simpler, and there's no obvious 
disadvantage either.

Alan Stern

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Wed, Jun 19, 2013 at 11:37 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 19 Jun 2013, Ming Lei wrote:
>
>> >> @@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>> >>                       hcd->status_urb = NULL;
>> >>                       usb_hcd_unlink_urb_from_ep(hcd, urb);
>> >>
>> >> -                     spin_unlock(&hcd_root_hub_lock);
>> >> +                     if (!hcd_giveback_urb_in_bh(hcd))
>> >> +                             spin_unlock(&hcd_root_hub_lock);
>> >>                       usb_hcd_giveback_urb(hcd, urb, status);
>> >> -                     spin_lock(&hcd_root_hub_lock);
>> >> +                     if (!hcd_giveback_urb_in_bh(hcd))
>> >> +                             spin_lock(&hcd_root_hub_lock);
>> >>               }
>> >>       }
>> >>   done:
>> >
>> > None of these tests are necessary.  Root hubs are different from normal
>> > devices; their URBs are handled mostly by usbcore.  The only part done
>> > by the HCD is always synchronous.  And we know that root-hub URBs
>>
>> Looks not always synchronous, control transfer is synchronous, and
>> interrupt transfer is still asynchronous. No drivers(hub, usbfs) depend
>> on that,  and IMO, treating root hub same as hub will simplify HCD core,
>> and finally we can remove all the above lock releasing & acquiring if
>> all HCDs set HCD_BH.
>>
>> Also there is very less roothub transfers and always letting tasklet
>> handle URB giveback of roothub won't have performance problem, so
>> how about keeping the above tests?
>
> If you want to use the tasklets for root-hub URBs, okay.  There's no
> reason to check the HCD_BH flag, though, because HCDs have only minimal
> involvement in root-hub URBs.  In particular, HCD's don't call
> usb_hcd_giveback_urb() for them.

Looks both root hub's control and interrupt transfer call usb_hcd_giveback_urb()
to complete URBs, don't they?

> So you can use the tasklets for _all_ root-hub URBs.  Then the tests
> above aren't necessary, and neither are the spinlock operations.

Yes, that is what I am going to do.

>
>> >> @@ -2573,6 +2687,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>> >>                       && device_can_wakeup(&hcd->self.root_hub->dev))
>> >>               dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
>> >>
>> >> +     if (usb_hcd_is_primary_hcd(hcd)) {
>> >> +             retval = init_giveback_urb_bh(hcd);
>> >> +             if (retval)
>> >> +                     goto err_init_giveback_bh;
>> >> +     } else {
>> >> +             /* share tasklet handling with primary hcd */
>> >> +             hcd->async_bh = hcd->primary_hcd->async_bh;
>> >> +             hcd->periodic_bh = hcd->primary_hcd->periodic_bh;
>> >> +     }
>> >
>> > Is there any reason why a secondary HCD can't have its own tasklets?
>>
>> I didn't do that because both primary and secondary HCDs share one
>> hard interrupt handler, so basically there is no obvious advantage to
>> do that.
>
> If the bh structures are embedded directly in the hcd structure, it
> won't be possible for a secondary hcd to share its tasklets with the
> primary hcd.  Not sharing seems simpler, and there's no obvious
> disadvantage either.

OK, I will let secondary HCD have its own tasklet in v2.

Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Alan Stern]

On Thu, 20 Jun 2013, Ming Lei wrote:

> >> Looks not always synchronous, control transfer is synchronous, and
> >> interrupt transfer is still asynchronous. No drivers(hub, usbfs) depend
> >> on that,  and IMO, treating root hub same as hub will simplify HCD core,
> >> and finally we can remove all the above lock releasing & acquiring if
> >> all HCDs set HCD_BH.
> >>
> >> Also there is very less roothub transfers and always letting tasklet
> >> handle URB giveback of roothub won't have performance problem, so
> >> how about keeping the above tests?
> >
> > If you want to use the tasklets for root-hub URBs, okay.  There's no
> > reason to check the HCD_BH flag, though, because HCDs have only minimal
> > involvement in root-hub URBs.  In particular, HCD's don't call
> > usb_hcd_giveback_urb() for them.
> 
> Looks both root hub's control and interrupt transfer call usb_hcd_giveback_urb()
> to complete URBs, don't they?

They do.  But those calls come from within usbcore, not from within the 
HCD.  Which means it doesn't matter whether the HCD_BH flag is set.

When working with URBs sent to a root hub, it many ways it's as though 
usbcore acts as its own HCD.  It takes care of all the locking and 
giving back the URBs.

> > So you can use the tasklets for _all_ root-hub URBs.  Then the tests
> > above aren't necessary, and neither are the spinlock operations.
> 
> Yes, that is what I am going to do.

Okay.

By the way, did you consider the race that Oliver pointed out?  When an
HCD is removed, all the outstanding URBs for all devices on its bus get
cancelled.  The core waits until the urb_list for each endpoint is
empty.

In the past this was good enough.  But now it looks like we will also
need to wait until the tasklet lists are empty and the tasklets aren't
running before they get killed.  Your __exit_giveback_urb_bh() routine 
doesn't seem to do that.

(Probably it's sufficient to wait until the tasklet lists are empty.  I 
assume tasklet_kill() won't stop a tasklet that's currently running.)

Alan Stern

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Thu, Jun 20, 2013 at 10:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> By the way, did you consider the race that Oliver pointed out?  When an
> HCD is removed, all the outstanding URBs for all devices on its bus get
> cancelled.  The core waits until the urb_list for each endpoint is
> empty.

This should be enough since during remove path usbcore will wait for all
URBs' completion which is only triggered by tasklet, so once all URBs are
finished, the tasklet list has been empty already.

>
> In the past this was good enough.  But now it looks like we will also
> need to wait until the tasklet lists are empty and the tasklets aren't
> running before they get killed.  Your __exit_giveback_urb_bh() routine
> doesn't seem to do that.
>
> (Probably it's sufficient to wait until the tasklet lists are empty.  I
> assume tasklet_kill() won't stop a tasklet that's currently running.)

>From the implementation of tasklet_kill(), it will wait for the completion
of the tasklet.

Actually the tasklet_kill() should not be necessary, I think.


Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Alan Stern]

On Thu, 20 Jun 2013, Ming Lei wrote:

> On Thu, Jun 20, 2013 at 10:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > By the way, did you consider the race that Oliver pointed out?  When an
> > HCD is removed, all the outstanding URBs for all devices on its bus get
> > cancelled.  The core waits until the urb_list for each endpoint is
> > empty.
> 
> This should be enough since during remove path usbcore will wait for all
> URBs' completion which is only triggered by tasklet, so once all URBs are
> finished, the tasklet list has been empty already.

No, usbcore only waits until the urb_list for each endpoint is empty.  
It doesn't keep track of the individual URB completions.  Look at
usb_hcd_flush_endpoint() and you'll see.

> > (Probably it's sufficient to wait until the tasklet lists are empty.  I
> > assume tasklet_kill() won't stop a tasklet that's currently running.)
> 
> From the implementation of tasklet_kill(), it will wait for the completion
> of the tasklet.
> 
> Actually the tasklet_kill() should not be necessary, I think.

You're probably right.  But we should wait until the tasklet's list is 
empty; we don't want to deallocate an hcd structure until all the URBs 
are taken off.

Alan Stern

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Fri, Jun 21, 2013 at 12:52 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 20 Jun 2013, Ming Lei wrote:
>
>> On Thu, Jun 20, 2013 at 10:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >
>> > By the way, did you consider the race that Oliver pointed out?  When an
>> > HCD is removed, all the outstanding URBs for all devices on its bus get
>> > cancelled.  The core waits until the urb_list for each endpoint is
>> > empty.
>>
>> This should be enough since during remove path usbcore will wait for all
>> URBs' completion which is only triggered by tasklet, so once all URBs are
>> finished, the tasklet list has been empty already.
>
> No, usbcore only waits until the urb_list for each endpoint is empty.
> It doesn't keep track of the individual URB completions.  Look at
> usb_hcd_flush_endpoint() and you'll see.

But, if URBs still aren't completed after usb_disconnect(), it should be
bug of usbcore or driver, shouldn't it?

>
>> > (Probably it's sufficient to wait until the tasklet lists are empty.  I
>> > assume tasklet_kill() won't stop a tasklet that's currently running.)
>>
>> From the implementation of tasklet_kill(), it will wait for the completion
>> of the tasklet.
>>
>> Actually the tasklet_kill() should not be necessary, I think.
>
> You're probably right.  But we should wait until the tasklet's list is
> empty; we don't want to deallocate an hcd structure until all the URBs
> are taken off.

I will keep the tasklet_kill() for safe reason.

Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Fri, Jun 21, 2013 at 9:12 AM, Ming Lei <ming.lei@canonical.com> wrote:
> On Fri, Jun 21, 2013 at 12:52 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Thu, 20 Jun 2013, Ming Lei wrote:
>>
>>> On Thu, Jun 20, 2013 at 10:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> >
>>> > By the way, did you consider the race that Oliver pointed out?  When an
>>> > HCD is removed, all the outstanding URBs for all devices on its bus get
>>> > cancelled.  The core waits until the urb_list for each endpoint is
>>> > empty.
>>>
>>> This should be enough since during remove path usbcore will wait for all
>>> URBs' completion which is only triggered by tasklet, so once all URBs are
>>> finished, the tasklet list has been empty already.
>>
>> No, usbcore only waits until the urb_list for each endpoint is empty.
>> It doesn't keep track of the individual URB completions.  Look at
>> usb_hcd_flush_endpoint() and you'll see.
>
> But, if URBs still aren't completed after usb_disconnect(), it should be
> bug of usbcore or driver, shouldn't it?

Thought about the problem further, there should be one problem:

- we can't let URBs survive from deleting the interface, otherwise
the module in which the completion handler lives might have been
removed

- so tasklet_kill() should be added into usb_hcd_synchronize_unlinks()
to make sure the above point, although it is a bit tricky since tasklet_kill()
flushes global list, but it does work.


Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Fri, Jun 21, 2013 at 12:46 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Fri, Jun 21, 2013 at 9:12 AM, Ming Lei <ming.lei@canonical.com> wrote:
>> On Fri, Jun 21, 2013 at 12:52 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> On Thu, 20 Jun 2013, Ming Lei wrote:
>>>
>>>> On Thu, Jun 20, 2013 at 10:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>>> >
>>>> > By the way, did you consider the race that Oliver pointed out?  When an
>>>> > HCD is removed, all the outstanding URBs for all devices on its bus get
>>>> > cancelled.  The core waits until the urb_list for each endpoint is
>>>> > empty.
>>>>
>>>> This should be enough since during remove path usbcore will wait for all
>>>> URBs' completion which is only triggered by tasklet, so once all URBs are
>>>> finished, the tasklet list has been empty already.
>>>
>>> No, usbcore only waits until the urb_list for each endpoint is empty.
>>> It doesn't keep track of the individual URB completions.  Look at
>>> usb_hcd_flush_endpoint() and you'll see.
>>
>> But, if URBs still aren't completed after usb_disconnect(), it should be
>> bug of usbcore or driver, shouldn't it?
>
> Thought about the problem further, there should be one problem:
>
> - we can't let URBs survive from deleting the interface, otherwise
> the module in which the completion handler lives might have been
> removed
>
> - so tasklet_kill() should be added into usb_hcd_synchronize_unlinks()
> to make sure the above point, although it is a bit tricky since tasklet_kill()
> flushes global list, but it does work.

Maybe usb_suspend_both() need to call usb_hcd_synchronize_unlinks(dev),
both the two cases(disconnect, suspend) suppose drivers don't kill their
URBs in remove() and suspend().

Even we can implement one function which only flushes URBs for one device
to optimize the cases.

Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Oliver Neukum]

On Friday 21 June 2013 09:12:38 Ming Lei wrote:
> >> This should be enough since during remove path usbcore will wait for all
> >> URBs' completion which is only triggered by tasklet, so once all URBs are
> >> finished, the tasklet list has been empty already.
> >
> > No, usbcore only waits until the urb_list for each endpoint is empty.
> > It doesn't keep track of the individual URB completions.  Look at
> > usb_hcd_flush_endpoint() and you'll see.
> 
> But, if URBs still aren't completed after usb_disconnect(), it should be
> bug of usbcore or driver, shouldn't it?

A driver cannot know what caused the call to disconnect(). That includes
driver unbind. Doing IO to a device that another driver may be bound to
is certainly a bug.
Nevertheless it usually works and it is desirable to keep that behavior.

	Regards
		Oliver

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Fri, Jun 21, 2013 at 4:33 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Friday 21 June 2013 09:12:38 Ming Lei wrote:
>> >> This should be enough since during remove path usbcore will wait for all
>> >> URBs' completion which is only triggered by tasklet, so once all URBs are
>> >> finished, the tasklet list has been empty already.
>> >
>> > No, usbcore only waits until the urb_list for each endpoint is empty.
>> > It doesn't keep track of the individual URB completions.  Look at
>> > usb_hcd_flush_endpoint() and you'll see.
>>
>> But, if URBs still aren't completed after usb_disconnect(), it should be
>> bug of usbcore or driver, shouldn't it?
>
> A driver cannot know what caused the call to disconnect(). That includes
> driver unbind. Doing IO to a device that another driver may be bound to
> is certainly a bug.
> Nevertheless it usually works and it is desirable to keep that behavior.

I mean once driver is unbound, URB's complete() in that driver shouldn't be
called.  The situation might happen when driver->remove() doesn't
kill the URBs with the patch applied.

Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Oliver Neukum]

On Friday 21 June 2013 17:13:48 Ming Lei wrote:
> On Fri, Jun 21, 2013 at 4:33 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > On Friday 21 June 2013 09:12:38 Ming Lei wrote:
> >> >> This should be enough since during remove path usbcore will wait for all
> >> >> URBs' completion which is only triggered by tasklet, so once all URBs are
> >> >> finished, the tasklet list has been empty already.
> >> >
> >> > No, usbcore only waits until the urb_list for each endpoint is empty.
> >> > It doesn't keep track of the individual URB completions.  Look at
> >> > usb_hcd_flush_endpoint() and you'll see.
> >>
> >> But, if URBs still aren't completed after usb_disconnect(), it should be
> >> bug of usbcore or driver, shouldn't it?
> >
> > A driver cannot know what caused the call to disconnect(). That includes
> > driver unbind. Doing IO to a device that another driver may be bound to
> > is certainly a bug.
> > Nevertheless it usually works and it is desirable to keep that behavior.
> 
> I mean once driver is unbound, URB's complete() in that driver shouldn't be

This is highly problematic. It is bound to cause resource leaks.

> called.  The situation might happen when driver->remove() doesn't
> kill the URBs with the patch applied.

Well, there is no good way to handle this. But we have a simple rule.
If usb_submit_urb() succeeds, complete() will be called.
Breaking this rule is a bad idea.

The best way is really to make sure that no URB survive.

	Regards
		Oliver

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Fri, Jun 21, 2013 at 5:20 PM, Oliver Neukum <oneukum@suse.de> wrote:
>
> This is highly problematic. It is bound to cause resource leaks.
>
>> called.  The situation might happen when driver->remove() doesn't
>> kill the URBs with the patch applied.
>
> Well, there is no good way to handle this. But we have a simple rule.
> If usb_submit_urb() succeeds, complete() will be called.
> Breaking this rule is a bad idea.

The rule should be enhanced by calling complete() before
completing driver unbind.

>
> The best way is really to make sure that no URB survive.

Drivers may let usbcore to do that by not setting driver->soft_unbind.

I will fix the problem in v2, one solution I thought of is to flush
the endpoint's URBs which have been added to tasklet list
at the end of usb_hcd_flush_endpoint(). Any comments?

Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Fri, Jun 21, 2013 at 5:43 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Fri, Jun 21, 2013 at 5:20 PM, Oliver Neukum <oneukum@suse.de> wrote:
>
> Drivers may let usbcore to do that by not setting driver->soft_unbind.
>
> I will fix the problem in v2, one solution I thought of is to flush
> the endpoint's URBs which have been added to tasklet list
> at the end of usb_hcd_flush_endpoint(). Any comments?

Or we can wait for completion of all URBs in the endpoint at the end
of usb_hcd_flush_endpoint(), I think this approach should be easier.

Thanks,
--
Ming Lei

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Alan Stern]

On Fri, 21 Jun 2013, Ming Lei wrote:

> On Fri, Jun 21, 2013 at 5:20 PM, Oliver Neukum <oneukum@suse.de> wrote:
> >
> > This is highly problematic. It is bound to cause resource leaks.
> >
> >> called.  The situation might happen when driver->remove() doesn't
> >> kill the URBs with the patch applied.
> >
> > Well, there is no good way to handle this. But we have a simple rule.
> > If usb_submit_urb() succeeds, complete() will be called.
> > Breaking this rule is a bad idea.
> 
> The rule should be enhanced by calling complete() before
> completing driver unbind.
> 
> >
> > The best way is really to make sure that no URB survive.
> 
> Drivers may let usbcore to do that by not setting driver->soft_unbind.
> 
> I will fix the problem in v2, one solution I thought of is to flush
> the endpoint's URBs which have been added to tasklet list
> at the end of usb_hcd_flush_endpoint(). Any comments?

Come to think of it, this shouldn't be a problem.  Drivers _must_
insure that all their URBs have completed before their disconnect
routine returns; otherwise the completion handler could get called
after the driver has been unloaded from memory.

Currently we have no way to enforce this in usbcore, although with the
tasklets we could enforce it.  But I don't think it is necessary.  It
would be sufficient to log a warning if the tasklet's URB list isn't
empty when exit_giveback_urb_bh() runs.  It won't happen unless there's
a buggy driver.

Besides, even if you try to flush all the URBs on the tasklet's list at 
the end of usb_hcd_flush_endpoint(), you'll still miss the URBs which 
have been moved to the local_list in usb_giveback_urb_bh().  You'd have 
to wait until the tasklet wasn't running, and it would most likely be a 
waste of time.

Alan Stern

[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context

[Ming Lei]

On Fri, Jun 21, 2013 at 10:48 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> Come to think of it, this shouldn't be a problem.  Drivers _must_
> insure that all their URBs have completed before their disconnect
> routine returns; otherwise the completion handler could get called
> after the driver has been unloaded from memory.
>
> Currently we have no way to enforce this in usbcore, although with the
> tasklets we could enforce it.  But I don't think it is necessary.  It

One way of enforcing this I thought of is to wait for completions of all
unlinking URBs at the end of usb_hcd_flush_endpoint().

But as you said, it may not be necessary.

> would be sufficient to log a warning if the tasklet's URB list isn't
> empty when exit_giveback_urb_bh() runs.  It won't happen unless there's
> a buggy driver.
>
> Besides, even if you try to flush all the URBs on the tasklet's list at
> the end of usb_hcd_flush_endpoint(), you'll still miss the URBs which
> have been moved to the local_list in usb_giveback_urb_bh().  You'd have
> to wait until the tasklet wasn't running, and it would most likely be a
> waste of time.


Thanks,
--
Ming Lei

[RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()

[Ming Lei]

We disable local IRQs here in case of running complete() by
tasklet to avoid possible deadlock because drivers may call
spin_lock() to hold lock which might be acquired in one hard
interrupt handler.

The local_irq_save()/local_irq_restore() around complete()
will be removed if current USB drivers have been cleaned up
and no one may trigger the above deadlock situation when
running complete() in tasklet.

Cc: Oliver Neukum <oliver@neukum.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/core/hcd.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index a272968..09a8263 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
 
 	/* pass ownership to the completion handler */
 	urb->status = status;
-	urb->complete (urb);
+
+	/*
+	 * We disable local IRQs here in case of running complete() by
+	 * tasklet to avoid possible deadlock because drivers may call
+	 * spin_lock() to hold lock which might be acquired in one hard
+	 * interrupt handler.
+	 *
+	 * The local_irq_save()/local_irq_restore() around complete()
+	 * will be removed if current USB drivers have been cleaned up
+	 * and no one may trigger the above deadlock situation when
+	 * running complete() in tasklet.
+	 */
+	if (hcd_giveback_urb_in_bh(hcd)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		urb->complete (urb);
+		local_irq_restore(flags);
+	} else {
+		urb->complete (urb);
+	}
+
 	atomic_dec (&urb->use_count);
 	if (unlikely(atomic_read(&urb->reject)))
 		wake_up (&usb_kill_urb_queue);
-- 
1.7.9.5

[RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()

[Sergei Shtylyov]

Hello.

On 18-06-2013 19:03, Ming Lei wrote:

> We disable local IRQs here in case of running complete() by
> tasklet to avoid possible deadlock because drivers may call
> spin_lock() to hold lock which might be acquired in one hard
> interrupt handler.

> The local_irq_save()/local_irq_restore() around complete()
> will be removed if current USB drivers have been cleaned up
> and no one may trigger the above deadlock situation when
> running complete() in tasklet.

> Cc: Oliver Neukum <oliver@neukum.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>   drivers/usb/core/hcd.c |   23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a272968..09a8263 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>
>   	/* pass ownership to the completion handler */
>   	urb->status = status;
> -	urb->complete (urb);
> +
> +	/*
> +	 * We disable local IRQs here in case of running complete() by
> +	 * tasklet to avoid possible deadlock because drivers may call
> +	 * spin_lock() to hold lock which might be acquired in one hard
> +	 * interrupt handler.
> +	 *
> +	 * The local_irq_save()/local_irq_restore() around complete()
> +	 * will be removed if current USB drivers have been cleaned up
> +	 * and no one may trigger the above deadlock situation when
> +	 * running complete() in tasklet.
> +	 */
> +	if (hcd_giveback_urb_in_bh(hcd)) {
> +		unsigned long flags;
> +
> +		local_irq_save(flags);
> +		urb->complete (urb);

    I guess you didn't run the patch thru scripts/checkpatch.pl, did you?
It would complain about the space before (.

WBR, Sergei

[RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()

[Alan Stern]

On Tue, 18 Jun 2013, Ming Lei wrote:

> We disable local IRQs here in case of running complete() by
> tasklet to avoid possible deadlock because drivers may call
> spin_lock() to hold lock which might be acquired in one hard
> interrupt handler.
> 
> The local_irq_save()/local_irq_restore() around complete()
> will be removed if current USB drivers have been cleaned up
> and no one may trigger the above deadlock situation when
> running complete() in tasklet.

This should be part of the first patch, not added on separately.

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a272968..09a8263 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>  
>  	/* pass ownership to the completion handler */
>  	urb->status = status;
> -	urb->complete (urb);
> +
> +	/*
> +	 * We disable local IRQs here in case of running complete() by
> +	 * tasklet to avoid possible deadlock because drivers may call
> +	 * spin_lock() to hold lock which might be acquired in one hard
> +	 * interrupt handler.
> +	 *
> +	 * The local_irq_save()/local_irq_restore() around complete()
> +	 * will be removed if current USB drivers have been cleaned up
> +	 * and no one may trigger the above deadlock situation when
> +	 * running complete() in tasklet.
> +	 */
> +	if (hcd_giveback_urb_in_bh(hcd)) {
> +		unsigned long flags;
> +
> +		local_irq_save(flags);
> +		urb->complete (urb);
> +		local_irq_restore(flags);
> +	} else {
> +		urb->complete (urb);
> +	}
> +

There's no reason to bother with the test.  You might as well always do 
the local_irq_save.

Alan Stern

[RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()

[Ming Lei]

On Wed, Jun 19, 2013 at 12:36 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 18 Jun 2013, Ming Lei wrote:
>
>> We disable local IRQs here in case of running complete() by
>> tasklet to avoid possible deadlock because drivers may call
>> spin_lock() to hold lock which might be acquired in one hard
>> interrupt handler.
>>
>> The local_irq_save()/local_irq_restore() around complete()
>> will be removed if current USB drivers have been cleaned up
>> and no one may trigger the above deadlock situation when
>> running complete() in tasklet.
>
> This should be part of the first patch, not added on separately.

Yes, but I want to highlight the change since that will be removed
after drivers have been cleaned up.

>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index a272968..09a8263 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>>
>>       /* pass ownership to the completion handler */
>>       urb->status = status;
>> -     urb->complete (urb);
>> +
>> +     /*
>> +      * We disable local IRQs here in case of running complete() by
>> +      * tasklet to avoid possible deadlock because drivers may call
>> +      * spin_lock() to hold lock which might be acquired in one hard
>> +      * interrupt handler.
>> +      *
>> +      * The local_irq_save()/local_irq_restore() around complete()
>> +      * will be removed if current USB drivers have been cleaned up
>> +      * and no one may trigger the above deadlock situation when
>> +      * running complete() in tasklet.
>> +      */
>> +     if (hcd_giveback_urb_in_bh(hcd)) {
>> +             unsigned long flags;
>> +
>> +             local_irq_save(flags);
>> +             urb->complete (urb);
>> +             local_irq_restore(flags);
>> +     } else {
>> +             urb->complete (urb);
>> +     }
>> +
>
> There's no reason to bother with the test.  You might as well always do
> the local_irq_save.

Looks fine.

Thanks,
--
Ming Lei

[RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()

[Alan Stern]

On Wed, 19 Jun 2013, Ming Lei wrote:

> On Wed, Jun 19, 2013 at 12:36 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 18 Jun 2013, Ming Lei wrote:
> >
> >> We disable local IRQs here in case of running complete() by
> >> tasklet to avoid possible deadlock because drivers may call
> >> spin_lock() to hold lock which might be acquired in one hard
> >> interrupt handler.
> >>
> >> The local_irq_save()/local_irq_restore() around complete()
> >> will be removed if current USB drivers have been cleaned up
> >> and no one may trigger the above deadlock situation when
> >> running complete() in tasklet.
> >
> > This should be part of the first patch, not added on separately.
> 
> Yes, but I want to highlight the change since that will be removed
> after drivers have been cleaned up.

I don't think it's necessary to highlight anything, and it seems silly 
to add new code in one patch and then change it in the very next patch.

Alan Stern

[RFC PATCH v1 3/6] USB: URB documentation: claim complete() may be run with IRQs enabled

[Ming Lei]

There is no good reason to run complete() in hard interrupt
disabled context.

After running complete() in tasklet, we will enable local IRQs
when calling complete() since we can do it now.

Even though we still disable IRQs now when calling complete()
in tasklet, the URB documentation is updated to claim complete()
may be run in tasklet context and local IRQs may be enabled, so
that USB drivers can know the change and avoid one deadlock caused
by: assume IRQs disabled in complete() and call spin_lock() to
hold lock which might be acquired in interrupt context.

Current spin_lock() usages in drivers' complete() will be cleaned
up at the same time, and when the cleanup is finished, local IRQs
will be enabled when calling complete() in tasklet.

Also fix description about type of usb_complete_t.

Cc: Oliver Neukum <oliver@neukum.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 Documentation/usb/URB.txt |   30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/Documentation/usb/URB.txt b/Documentation/usb/URB.txt
index 00d2c64..454db87 100644
--- a/Documentation/usb/URB.txt
+++ b/Documentation/usb/URB.txt
@@ -195,13 +195,12 @@ by the completion handler.
 
 The handler is of the following type:
 
-	typedef void (*usb_complete_t)(struct urb *, struct pt_regs *)
+	typedef void (*usb_complete_t)(struct urb *)
 
-I.e., it gets the URB that caused the completion call, plus the
-register values at the time of the corresponding interrupt (if any).
-In the completion handler, you should have a look at urb->status to
-detect any USB errors. Since the context parameter is included in the URB,
-you can pass information to the completion handler. 
+I.e., it gets the URB that caused the completion call. In the completion
+handler, you should have a look at urb->status to detect any USB errors.
+Since the context parameter is included in the URB, you can pass
+information to the completion handler.
 
 Note that even when an error (or unlink) is reported, data may have been
 transferred.  That's because USB transfers are packetized; it might take
@@ -210,12 +209,19 @@ have transferred successfully before the completion was called.
 
 
 NOTE:  ***** WARNING *****
-NEVER SLEEP IN A COMPLETION HANDLER.  These are normally called
-during hardware interrupt processing.  If you can, defer substantial
-work to a tasklet (bottom half) to keep system latencies low.  You'll
-probably need to use spinlocks to protect data structures you manipulate
-in completion handlers.
-
+NEVER SLEEP IN A COMPLETION HANDLER.  These are called during hardware
+interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise
+called in tasklet context. If you can, defer substantial work to a tasklet
+(bottom half) to keep system latencies low.  You'll probably need to use
+spinlocks to protect data structures you manipulate in completion handlers.
+
+Driver can't assume that local IRQs are disabled any more when running
+complete(), for example, if drivers want to hold a lock which might be
+acquired in hard interrupt handler, they have to use spin_lock_irqsave()
+instead of spin_lock() to hold the lock.
+
+In the future, all HCD might set HCD_BH to run complete() in tasklet so
+that system latencies can be kept low.
 
 1.8. How to do isochronous (ISO) transfers?
 
-- 
1.7.9.5

[RFC PATCH v1 3/6] USB: URB documentation: claim complete() may be run with IRQs enabled

[Alan Stern]

On Tue, 18 Jun 2013, Ming Lei wrote:

> There is no good reason to run complete() in hard interrupt
> disabled context.
> 
> After running complete() in tasklet, we will enable local IRQs
> when calling complete() since we can do it now.
> 
> Even though we still disable IRQs now when calling complete()
> in tasklet, the URB documentation is updated to claim complete()
> may be run in tasklet context and local IRQs may be enabled, so
> that USB drivers can know the change and avoid one deadlock caused
> by: assume IRQs disabled in complete() and call spin_lock() to
> hold lock which might be acquired in interrupt context.
> 
> Current spin_lock() usages in drivers' complete() will be cleaned
> up at the same time, and when the cleanup is finished, local IRQs
> will be enabled when calling complete() in tasklet.
> 
> Also fix description about type of usb_complete_t.

> @@ -210,12 +209,19 @@ have transferred successfully before the completion was called.
>  
>  
>  NOTE:  ***** WARNING *****
> -NEVER SLEEP IN A COMPLETION HANDLER.  These are normally called
> -during hardware interrupt processing.  If you can, defer substantial
> -work to a tasklet (bottom half) to keep system latencies low.  You'll
> -probably need to use spinlocks to protect data structures you manipulate
> -in completion handlers.
> -
> +NEVER SLEEP IN A COMPLETION HANDLER.  These are called during hardware
> +interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise
> +called in tasklet context. If you can, defer substantial work to a tasklet
> +(bottom half) to keep system latencies low.  You'll probably need to use
> +spinlocks to protect data structures you manipulate in completion handlers.

You shouldn't go into so much detail here, partly because it doesn't 
matter for driver authors and partly because it is subject to change.  
Just say that completion handlers are usually called in an atomic 
context, so they must not sleep.

Also, the advice about deferring work to a tasklet seems out of place 
now, since many completion handlers will already be running in a 
tasklet.

> +Driver can't assume that local IRQs are disabled any more when running
> +complete(), for example, if drivers want to hold a lock which might be
> +acquired in hard interrupt handler, they have to use spin_lock_irqsave()
> +instead of spin_lock() to hold the lock.

Don't say so much.  Just mention that in the current kernel (3.10),
completion handlers always run with local interrupts disabled, but in
the future this is likely to change.  Therefore drivers should not make
any assumptions.

> +In the future, all HCD might set HCD_BH to run complete() in tasklet so
> +that system latencies can be kept low.

This does not need to be in the documentation.

Alan Stern

[RFC PATCH v1 3/6] USB: URB documentation: claim complete() may be run with IRQs enabled

[Ming Lei]

On Wed, Jun 19, 2013 at 12:42 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 18 Jun 2013, Ming Lei wrote:
>
>> There is no good reason to run complete() in hard interrupt
>> disabled context.
>>
>> After running complete() in tasklet, we will enable local IRQs
>> when calling complete() since we can do it now.
>>
>> Even though we still disable IRQs now when calling complete()
>> in tasklet, the URB documentation is updated to claim complete()
>> may be run in tasklet context and local IRQs may be enabled, so
>> that USB drivers can know the change and avoid one deadlock caused
>> by: assume IRQs disabled in complete() and call spin_lock() to
>> hold lock which might be acquired in interrupt context.
>>
>> Current spin_lock() usages in drivers' complete() will be cleaned
>> up at the same time, and when the cleanup is finished, local IRQs
>> will be enabled when calling complete() in tasklet.
>>
>> Also fix description about type of usb_complete_t.
>
>> @@ -210,12 +209,19 @@ have transferred successfully before the completion was called.
>>
>>
>>  NOTE:  ***** WARNING *****
>> -NEVER SLEEP IN A COMPLETION HANDLER.  These are normally called
>> -during hardware interrupt processing.  If you can, defer substantial
>> -work to a tasklet (bottom half) to keep system latencies low.  You'll
>> -probably need to use spinlocks to protect data structures you manipulate
>> -in completion handlers.
>> -
>> +NEVER SLEEP IN A COMPLETION HANDLER.  These are called during hardware
>> +interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise
>> +called in tasklet context. If you can, defer substantial work to a tasklet
>> +(bottom half) to keep system latencies low.  You'll probably need to use
>> +spinlocks to protect data structures you manipulate in completion handlers.
>
> You shouldn't go into so much detail here, partly because it doesn't
> matter for driver authors and partly because it is subject to change.
> Just say that completion handlers are usually called in an atomic
> context, so they must not sleep.

Right.

>
> Also, the advice about deferring work to a tasklet seems out of place
> now, since many completion handlers will already be running in a
> tasklet.

Good catch.

>
>> +Driver can't assume that local IRQs are disabled any more when running
>> +complete(), for example, if drivers want to hold a lock which might be
>> +acquired in hard interrupt handler, they have to use spin_lock_irqsave()
>> +instead of spin_lock() to hold the lock.
>
> Don't say so much.  Just mention that in the current kernel (3.10),
> completion handlers always run with local interrupts disabled, but in
> the future this is likely to change.  Therefore drivers should not make
> any assumptions.

OK.

>
>> +In the future, all HCD might set HCD_BH to run complete() in tasklet so
>> +that system latencies can be kept low.
>
> This does not need to be in the documentation.

Right, since drivers don't care HCD.

Thanks,
--
Ming Lei

[RFC PATCH v1 4/6] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context

[Ming Lei]

If HCD_BH is set for HC driver's flags, URB giveback will be
done in tasklet context instead of interrupt context, so the
ehci->lock needn't to be released any more before calling
usb_hcd_giveback_urb().

Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/host/ehci-q.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index d34b399..0387a81 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -283,9 +283,11 @@ __acquires(ehci->lock)
 
 	/* complete() can reenter this HCD */
 	usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
-	spin_unlock (&ehci->lock);
+	if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci)))
+		spin_unlock(&ehci->lock);
 	usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status);
-	spin_lock (&ehci->lock);
+	if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci)))
+		spin_lock(&ehci->lock);
 }
 
 static int qh_schedule (struct ehci_hcd *ehci, struct ehci_qh *qh);
-- 
1.7.9.5

[RFC PATCH v1 4/6] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context

[Alan Stern]

On Tue, 18 Jun 2013, Ming Lei wrote:

> If HCD_BH is set for HC driver's flags, URB giveback will be
> done in tasklet context instead of interrupt context, so the
> ehci->lock needn't to be released any more before calling
> usb_hcd_giveback_urb().

It is premature to do this now.  This should be part of the 6/6 patch, 
when it won't be necessary to test hcd_giveback_urb_in_bh().

Alan Stern

[RFC PATCH v1 4/6] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context

[Ming Lei]

On Wed, Jun 19, 2013 at 12:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 18 Jun 2013, Ming Lei wrote:
>
>> If HCD_BH is set for HC driver's flags, URB giveback will be
>> done in tasklet context instead of interrupt context, so the
>> ehci->lock needn't to be released any more before calling
>> usb_hcd_giveback_urb().
>
> It is premature to do this now.  This should be part of the 6/6 patch,

It is fine since HCD_BH isn't enabled.

> when it won't be necessary to test hcd_giveback_urb_in_bh().

Keeping it is easy to compare test results between running complete()
in tasklet and in hard irq handler.

Also it might be helpful for out of tree drivers.

But it isn't a big deal, I can merge it to 6/6 if you care.

Thanks,
--
Ming Lei

[RFC PATCH v1 4/6] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context

[Alan Stern]

On Wed, 19 Jun 2013, Ming Lei wrote:

> On Wed, Jun 19, 2013 at 12:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 18 Jun 2013, Ming Lei wrote:
> >
> >> If HCD_BH is set for HC driver's flags, URB giveback will be
> >> done in tasklet context instead of interrupt context, so the
> >> ehci->lock needn't to be released any more before calling
> >> usb_hcd_giveback_urb().
> >
> > It is premature to do this now.  This should be part of the 6/6 patch,
> 
> It is fine since HCD_BH isn't enabled.

It's not "fine".  It will work -- nothing will crash -- but that 
doesn't mean it is good style.

> > when it won't be necessary to test hcd_giveback_urb_in_bh().
> 
> Keeping it is easy to compare test results between running complete()
> in tasklet and in hard irq handler.

It's just as easy to do your tests by keeping two copies of ehci-hcd.ko 
lying around; one with your changes and one without.  You can insmod 
whichever one you want to test.

> Also it might be helpful for out of tree drivers.
> 
> But it isn't a big deal, I can merge it to 6/6 if you care.

Yes, please.

Alan Stern

[RFC PATCH v1 4/6] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context

[Ming Lei]

On Wed, Jun 19, 2013 at 11:47 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 19 Jun 2013, Ming Lei wrote:
>
>> On Wed, Jun 19, 2013 at 12:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Tue, 18 Jun 2013, Ming Lei wrote:
>> >
>> >> If HCD_BH is set for HC driver's flags, URB giveback will be
>> >> done in tasklet context instead of interrupt context, so the
>> >> ehci->lock needn't to be released any more before calling
>> >> usb_hcd_giveback_urb().
>> >
>> > It is premature to do this now.  This should be part of the 6/6 patch,
>>
>> It is fine since HCD_BH isn't enabled.
>
> It's not "fine".  It will work -- nothing will crash -- but that
> doesn't mean it is good style.
>
>> > when it won't be necessary to test hcd_giveback_urb_in_bh().
>>
>> Keeping it is easy to compare test results between running complete()
>> in tasklet and in hard irq handler.
>
> It's just as easy to do your tests by keeping two copies of ehci-hcd.ko
> lying around; one with your changes and one without.  You can insmod
> whichever one you want to test.
>
>> Also it might be helpful for out of tree drivers.
>>
>> But it isn't a big deal, I can merge it to 6/6 if you care.
>
> Yes, please.

OK.

Thanks,
--
Ming Lei

[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink

[Ming Lei]

Given interrupt URB will be resubmitted from tasklet context which
is scheduled by ehci hardware interrupt handler, and commonly only
one interrupt URB is scheduled on qh, so the qh may be unlinked
immediately once qh_completions() returns from ehci_irq(), then
the intr URB to be resubmitted in complete() can only be scheduled
and linked to hardware until the qh unlink is completed.

This patch applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh)
state to improve this above situation, and the qh will wait for 5
milliseconds before being unlinked from hardware, if one URB is submitted
during the period, the qh is move out of unlink wait state and the
interrupt transfer can be scheduled immediately, not like before: the
transfer is only linked to hardware until previous unlink is completed.

Only enable the improvement in case that HCD supports to run
giveback of URB in tasklet context.

Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/host/ehci-hcd.c   |   16 ++++++++---
 drivers/usb/host/ehci-hub.c   |    1 +
 drivers/usb/host/ehci-sched.c |   60 ++++++++++++++++++++++++++++++++++++++---
 drivers/usb/host/ehci-timer.c |   43 +++++++++++++++++++++++++++++
 drivers/usb/host/ehci.h       |    3 +++
 5 files changed, 115 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 246e124..0ab82de 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -304,7 +304,9 @@ static void end_unlink_async(struct ehci_hcd *ehci);
 static void unlink_empty_async(struct ehci_hcd *ehci);
 static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
 static void ehci_work(struct ehci_hcd *ehci);
-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
+static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
+			      bool wait);
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
 static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
 
 #include "ehci-timer.c"
@@ -484,6 +486,7 @@ static int ehci_init(struct usb_hcd *hcd)
 	ehci->periodic_size = DEFAULT_I_TDPS;
 	INIT_LIST_HEAD(&ehci->async_unlink);
 	INIT_LIST_HEAD(&ehci->async_idle);
+	INIT_LIST_HEAD(&ehci->intr_unlink_wait);
 	INIT_LIST_HEAD(&ehci->intr_unlink);
 	INIT_LIST_HEAD(&ehci->intr_qh_list);
 	INIT_LIST_HEAD(&ehci->cached_itd_list);
@@ -908,15 +911,20 @@ static int ehci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		switch (qh->qh_state) {
 		case QH_STATE_LINKED:
 			if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT)
-				start_unlink_intr(ehci, qh);
+				start_unlink_intr(ehci, qh, false);
 			else
 				start_unlink_async(ehci, qh);
 			break;
 		case QH_STATE_COMPLETING:
 			qh->dequeue_during_giveback = 1;
 			break;
-		case QH_STATE_UNLINK:
 		case QH_STATE_UNLINK_WAIT:
+			if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) {
+				cancel_unlink_wait_intr(ehci, qh);
+				start_unlink_intr(ehci, qh, false);
+			}
+			break;
+		case QH_STATE_UNLINK:
 			/* already started */
 			break;
 		case QH_STATE_IDLE:
@@ -1042,7 +1050,7 @@ ehci_endpoint_reset(struct usb_hcd *hcd, struct usb_host_endpoint *ep)
 			if (eptype == USB_ENDPOINT_XFER_BULK)
 				start_unlink_async(ehci, qh);
 			else
-				start_unlink_intr(ehci, qh);
+				start_unlink_intr(ehci, qh, false);
 		}
 	}
 	spin_unlock_irqrestore(&ehci->lock, flags);
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index b2f6450..4104c66 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -345,6 +345,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
 
 	end_unlink_async(ehci);
 	unlink_empty_async_suspended(ehci);
+	ehci_handle_start_intr_unlinks(ehci);
 	ehci_handle_intr_unlinks(ehci);
 	end_free_itds(ehci);
 
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f80d033..5b9ca31 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -601,10 +601,10 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci, struct ehci_qh *qh)
 	list_del(&qh->intr_node);
 }
 
-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
 {
-	/* If the QH isn't linked then there's nothing we can do. */
-	if (qh->qh_state != QH_STATE_LINKED)
+	/* If the QH isn't unlink wait then there's nothing we can do. */
+	if (qh->qh_state != QH_STATE_UNLINK_WAIT)
 		return;
 
 	qh_unlink_periodic (ehci, qh);
@@ -632,6 +632,55 @@ static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
 	}
 }
 
+static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
+			      bool wait)
+{
+	/* If the QH isn't linked then there's nothing we can do. */
+	if (qh->qh_state != QH_STATE_LINKED)
+		return;
+
+	/*
+	 * It is common only one intr URB is scheduled on one qh, and given
+	 * complete() is run in tasklet context, introduce QH_STATE_UNLINK_WAIT
+	 * to avoid unlink qh too early.
+	 */
+	qh->qh_state = QH_STATE_UNLINK_WAIT;
+
+	/* bypass the optimization without URB giveback in tasklet */
+	if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci)) || !wait) {
+		start_do_unlink_intr(ehci, qh);
+		return;
+	}
+
+	qh->unlink_cycle = ehci->intr_unlink_wait_cycle;
+
+	/* New entries go at the end of the intr_unlink_wait list */
+	list_add_tail(&qh->unlink_node, &ehci->intr_unlink_wait);
+
+	if (ehci->rh_state < EHCI_RH_RUNNING)
+		ehci_handle_start_intr_unlinks(ehci);
+	else if (ehci->intr_unlink_wait.next == &qh->unlink_node) {
+		ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+		++ehci->intr_unlink_wait_cycle;
+	}
+}
+
+/* must be called with holding ehci->lock */
+static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+	/* If the QH isn't waited for unlink then do nothing */
+	if (qh->qh_state != QH_STATE_UNLINK_WAIT)
+		return;
+
+	/* restored to linked state */
+	list_del(&qh->unlink_node);
+	qh->qh_state = QH_STATE_LINKED;
+
+	/* avoid unnecessary CPU wakeup */
+	if (list_empty(&ehci->intr_unlink_wait))
+		ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR);
+}
+
 static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
 {
 	struct ehci_qh_hw	*hw = qh->hw;
@@ -881,6 +930,9 @@ static int intr_submit (
 			goto done;
 	}
 
+	/* put back qh from unlink wait list */
+	cancel_unlink_wait_intr(ehci, qh);
+
 	/* then queue the urb's tds to the qh */
 	qh = qh_append_tds(ehci, urb, qtd_list, epnum, &urb->ep->hcpriv);
 	BUG_ON (qh == NULL);
@@ -926,7 +978,7 @@ static void scan_intr(struct ehci_hcd *ehci)
 			temp = qh_completions(ehci, qh);
 			if (unlikely(temp || (list_empty(&qh->qtd_list) &&
 					qh->qh_state == QH_STATE_LINKED)))
-				start_unlink_intr(ehci, qh);
+				start_unlink_intr(ehci, qh, true);
 		}
 	}
 }
diff --git a/drivers/usb/host/ehci-timer.c b/drivers/usb/host/ehci-timer.c
index 11e5b32..c2320f3 100644
--- a/drivers/usb/host/ehci-timer.c
+++ b/drivers/usb/host/ehci-timer.c
@@ -72,6 +72,7 @@ static unsigned event_delays_ns[] = {
 	1 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_POLL_DEAD */
 	1125 * NSEC_PER_USEC,	/* EHCI_HRTIMER_UNLINK_INTR */
 	2 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_FREE_ITDS */
+	5 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_START_UNLINK_INTR */
 	6 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_ASYNC_UNLINKS */
 	10 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_IAA_WATCHDOG */
 	10 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_DISABLE_PERIODIC */
@@ -98,6 +99,17 @@ static void ehci_enable_event(struct ehci_hcd *ehci, unsigned event,
 	}
 }
 
+/* Warning: don't call this function from hrtimer handler context */
+static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
+{
+	if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci)))
+		return;
+
+	ehci->enabled_hrtimer_events &= ~(1 << event);
+	if (!ehci->enabled_hrtimer_events)
+		hrtimer_cancel(&ehci->hrtimer);
+}
+
 
 /* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */
 static void ehci_poll_ASS(struct ehci_hcd *ehci)
@@ -215,6 +227,36 @@ static void ehci_handle_controller_death(struct ehci_hcd *ehci)
 	/* Not in process context, so don't try to reset the controller */
 }
 
+/* start to unlink interrupt QHs  */
+static void ehci_handle_start_intr_unlinks(struct ehci_hcd *ehci)
+{
+	bool		stopped = (ehci->rh_state < EHCI_RH_RUNNING);
+
+	/*
+	 * Process all the QHs on the intr_unlink list that were added
+	 * before the current unlink cycle began.  The list is in
+	 * temporal order, so stop when we reach the first entry in the
+	 * current cycle.  But if the root hub isn't running then
+	 * process all the QHs on the list.
+	 */
+	while (!list_empty(&ehci->intr_unlink_wait)) {
+		struct ehci_qh	*qh;
+
+		qh = list_first_entry(&ehci->intr_unlink_wait,
+				      struct ehci_qh, unlink_node);
+		if (!stopped &&
+		    qh->unlink_cycle == ehci->intr_unlink_wait_cycle)
+			break;
+		list_del(&qh->unlink_node);
+		start_do_unlink_intr(ehci, qh);
+	}
+
+	/* Handle remaining entries later */
+	if (!list_empty(&ehci->intr_unlink_wait)) {
+		ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+		++ehci->intr_unlink_wait_cycle;
+	}
+}
 
 /* Handle unlinked interrupt QHs once they are gone from the hardware */
 static void ehci_handle_intr_unlinks(struct ehci_hcd *ehci)
@@ -363,6 +405,7 @@ static void (*event_handlers[])(struct ehci_hcd *) = {
 	ehci_handle_controller_death,	/* EHCI_HRTIMER_POLL_DEAD */
 	ehci_handle_intr_unlinks,	/* EHCI_HRTIMER_UNLINK_INTR */
 	end_free_itds,			/* EHCI_HRTIMER_FREE_ITDS */
+	ehci_handle_start_intr_unlinks,	/* EHCI_HRTIMER_START_UNLINK_INTR */
 	unlink_empty_async,		/* EHCI_HRTIMER_ASYNC_UNLINKS */
 	ehci_iaa_watchdog,		/* EHCI_HRTIMER_IAA_WATCHDOG */
 	ehci_disable_PSE,		/* EHCI_HRTIMER_DISABLE_PERIODIC */
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 7c978b2..1d2c164 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -88,6 +88,7 @@ enum ehci_hrtimer_event {
 	EHCI_HRTIMER_POLL_DEAD,		/* Wait for dead controller to stop */
 	EHCI_HRTIMER_UNLINK_INTR,	/* Wait for interrupt QH unlink */
 	EHCI_HRTIMER_FREE_ITDS,		/* Wait for unused iTDs and siTDs */
+	EHCI_HRTIMER_START_UNLINK_INTR, /* wait for new intr schedule */
 	EHCI_HRTIMER_ASYNC_UNLINKS,	/* Unlink empty async QHs */
 	EHCI_HRTIMER_IAA_WATCHDOG,	/* Handle lost IAA interrupts */
 	EHCI_HRTIMER_DISABLE_PERIODIC,	/* Wait to disable periodic sched */
@@ -143,6 +144,8 @@ struct ehci_hcd {			/* one per controller */
 	unsigned		i_thresh;	/* uframes HC might cache */
 
 	union ehci_shadow	*pshadow;	/* mirror hw periodic table */
+	struct list_head	intr_unlink_wait;
+	unsigned		intr_unlink_wait_cycle;
 	struct list_head	intr_unlink;
 	unsigned		intr_unlink_cycle;
 	unsigned		now_frame;	/* frame from HC hardware */
-- 
1.7.9.5

[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink

[Alan Stern]

On Tue, 18 Jun 2013, Ming Lei wrote:

> Given interrupt URB will be resubmitted from tasklet context which
> is scheduled by ehci hardware interrupt handler, and commonly only
> one interrupt URB is scheduled on qh, so the qh may be unlinked
> immediately once qh_completions() returns from ehci_irq(), then
> the intr URB to be resubmitted in complete() can only be scheduled
> and linked to hardware until the qh unlink is completed.
> 
> This patch applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh)
> state to improve this above situation, and the qh will wait for 5
> milliseconds before being unlinked from hardware, if one URB is submitted
> during the period, the qh is move out of unlink wait state and the
> interrupt transfer can be scheduled immediately, not like before: the
> transfer is only linked to hardware until previous unlink is completed.
> 
> Only enable the improvement in case that HCD supports to run
> giveback of URB in tasklet context.

The approach used in this patch is wrong.  You shouldn't start the 
unlink, then wait, then finish the unlink.  Consider what would happen 
if an URB were submitted during the delay: It would have to wait until 
the QH was completely unlinked.  Instead, you should wait first, then 
do the entire unlink.

For example, scan_async() in ehci-q.c doesn't immediately begin to 
unlink empty async QHs.  It merely marks them as being empty and starts 
a timer to check them later.  It they are still empty at that point, 
then they are unlinked.

Also, it's silly to check whether or not giveback uses a tasklet.  We 
know that following the 6/6 patch it will.  Even if it doesn't, there's 
no harm in waiting a little while before unlinking an empty interrupt 
QH.

Alan Stern

[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink

[Ming Lei]

On Wed, Jun 19, 2013 at 12:51 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 18 Jun 2013, Ming Lei wrote:
>
>> Given interrupt URB will be resubmitted from tasklet context which
>> is scheduled by ehci hardware interrupt handler, and commonly only
>> one interrupt URB is scheduled on qh, so the qh may be unlinked
>> immediately once qh_completions() returns from ehci_irq(), then
>> the intr URB to be resubmitted in complete() can only be scheduled
>> and linked to hardware until the qh unlink is completed.
>>
>> This patch applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh)
>> state to improve this above situation, and the qh will wait for 5
>> milliseconds before being unlinked from hardware, if one URB is submitted
>> during the period, the qh is move out of unlink wait state and the
>> interrupt transfer can be scheduled immediately, not like before: the
>> transfer is only linked to hardware until previous unlink is completed.
>>
>> Only enable the improvement in case that HCD supports to run
>> giveback of URB in tasklet context.
>
> The approach used in this patch is wrong.  You shouldn't start the
> unlink, then wait, then finish the unlink.  Consider what would happen

What you mentioned above is just what the patch is doing, :-)

start_unlink_intr() only sets the qh as QH_STATE_UNLINK_WAIT, puts
the qh into one wait list and starts a timer, if it is expired the qh will be
started to unlink, otherwise the qh can be recovered to QH_STATE_LINKED
immediately if there is one URB submitted.

So unlinking intr qh becomes a 3-stage process:

       - wait(qh return to linked state if URB is submitted during the period,
                  otherwise go to start unlink)
       - start unlink(do unlink, and wait for end of unlink)
       - end unlink

> if an URB were submitted during the delay: It would have to wait until

If an URB were submitted during the delay, the previous wait is canceled
immediately, and the qh state is recovered to linked state, please see
cancel_unlink_wait_intr() called in intr_submit().

> the QH was completely unlinked.  Instead, you should wait first, then
> do the entire unlink.

Yes, it is just what the patch is doing, :-)

>
> For example, scan_async() in ehci-q.c doesn't immediately begin to
> unlink empty async QHs.  It merely marks them as being empty and starts
> a timer to check them later.  It they are still empty at that point,
> then they are unlinked.

Yes, the patch starts to use QH_STATE_UNLINK_WAIT state for intr qh,
and previously the state is only used by async qh.

>
> Also, it's silly to check whether or not giveback uses a tasklet.  We
> know that following the 6/6 patch it will.  Even if it doesn't, there's
> no harm in waiting a little while before unlinking an empty interrupt
> QH.

It is still for the test reason, since the patch may cause recursion if
HCD_BH isn't set.

Thanks,
--
Ming Lei

[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink

[Alan Stern]

On Wed, 19 Jun 2013, Ming Lei wrote:

> > The approach used in this patch is wrong.  You shouldn't start the
> > unlink, then wait, then finish the unlink.  Consider what would happen
> 
> What you mentioned above is just what the patch is doing, :-)
> 
> start_unlink_intr() only sets the qh as QH_STATE_UNLINK_WAIT, puts
> the qh into one wait list and starts a timer, if it is expired the qh will be
> started to unlink, otherwise the qh can be recovered to QH_STATE_LINKED
> immediately if there is one URB submitted.

Okay, maybe I was fooled by your use of QH_STATE_UNLINK_WAIT.  Setting
the state to that value means that the QH is going to be unlinked after
a time delay.  However, that's not what you mean; you mean that after a
time delay you will decide whether or not to unlink the QH.

I think you should copy the approach used for the async QHs.

> So unlinking intr qh becomes a 3-stage process:
> 
>        - wait(qh return to linked state if URB is submitted during the period,
>                   otherwise go to start unlink)
>        - start unlink(do unlink, and wait for end of unlink)
>        - end unlink
> 
> > if an URB were submitted during the delay: It would have to wait until
> 
> If an URB were submitted during the delay, the previous wait is canceled
> immediately, and the qh state is recovered to linked state, please see
> cancel_unlink_wait_intr() called in intr_submit().

Right.  But you're not allowed to do that after changing qh->state.  
One of the invariants of the driver is that once qh->state takes on any
value other than QH_STATE_LINKED (or, temporarily,
QH_STATE_COMPLETING), the QH must be unlinked.  The state can't change
back to QH_STATE_LINKED without first passing through QH_STATE_IDLE.

> > Also, it's silly to check whether or not giveback uses a tasklet.  We
> > know that following the 6/6 patch it will.  Even if it doesn't, there's
> > no harm in waiting a little while before unlinking an empty interrupt
> > QH.
> 
> It is still for the test reason, since the patch may cause recursion if
> HCD_BH isn't set.

How can it cause recursion?  The async unlink code doesn't.

Alan Stern

[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink

[Ming Lei]

On Wed, Jun 19, 2013 at 11:44 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 19 Jun 2013, Ming Lei wrote:
>
>> > The approach used in this patch is wrong.  You shouldn't start the
>> > unlink, then wait, then finish the unlink.  Consider what would happen
>>
>> What you mentioned above is just what the patch is doing, :-)
>>
>> start_unlink_intr() only sets the qh as QH_STATE_UNLINK_WAIT, puts
>> the qh into one wait list and starts a timer, if it is expired the qh will be
>> started to unlink, otherwise the qh can be recovered to QH_STATE_LINKED
>> immediately if there is one URB submitted.
>
> Okay, maybe I was fooled by your use of QH_STATE_UNLINK_WAIT.  Setting
> the state to that value means that the QH is going to be unlinked after

Here the meaning of intr qh's QH_STATE_UNLINK_WAIT is a bit different
with async qh's.

> a time delay.  However, that's not what you mean; you mean that after a
> time delay you will decide whether or not to unlink the QH.

Yes, that is the difference with async QH.

>
> I think you should copy the approach used for the async QHs.

Yes, we can copy, but current async unlinking has some disadvantages,
see below.

>> So unlinking intr qh becomes a 3-stage process:
>>
>>        - wait(qh return to linked state if URB is submitted during the period,
>>                   otherwise go to start unlink)
>>        - start unlink(do unlink, and wait for end of unlink)
>>        - end unlink
>>
>> > if an URB were submitted during the delay: It would have to wait until
>>
>> If an URB were submitted during the delay, the previous wait is canceled
>> immediately, and the qh state is recovered to linked state, please see
>> cancel_unlink_wait_intr() called in intr_submit().
>
> Right.  But you're not allowed to do that after changing qh->state.
> One of the invariants of the driver is that once qh->state takes on any
> value other than QH_STATE_LINKED (or, temporarily,
> QH_STATE_COMPLETING), the QH must be unlinked.  The state can't change
> back to QH_STATE_LINKED without first passing through QH_STATE_IDLE.

IMO, there is no any side effect when we change the state to
QH_STATE_UNLINK_WAIT and later change back to QH_STATE_LINKED
later under this situation.

The reason why I use QH_STATE_UNLINK_WAIT here is for avoiding
unnecessary CPU wakeup.  Without the state, the unlink wait timer
is still triggered to check if there are intr QHs to be unlinked, but in most of
situations, there aren't QHs to be unlinked since tasklet is surely run
before the wait timer is expired. So generally, without knowing the wait state,
CPU wakeup events may be introduced unnecessarily.

Considered that the interval of interrupt endpoint might be very
small(e.g. 125us,
1ms) and some devices have very frequent intr event, I think we
need to pay attention to the problem.

Even on async QH situation, we might need to consider the problem too
since some application(eg. bulk only mass storage on xhci) may have
thousands of interrupts per seconds during data transfer, so CPU wakeup
events could be increased much by letting wait timer expire unnecessarily.

Also the async QH unlink approach has another disadvantage:

- if there are several QHs which are become empty during one wait period,
the hrtimer has to be scheduled several times for starting unlink one qh
each time. And after introducing the unlink wait list like the patch, we only
need schedule the hrtimer one time for unlinking all these empty QHs.

Finally, unlink wait for intr qh is only needed when the qh is completed
right now, and other cases(eg. unlink) needn't the wait.

The attached patch removes the QH_STATE_UNLINK_WAIT, and can
avoid the above disadvantages of async QH unlink, could you comment
on the new patch?

---
 drivers/usb/host/ehci-hcd.c   |    8 +++---
 drivers/usb/host/ehci-hub.c   |    1 +
 drivers/usb/host/ehci-sched.c |   54 ++++++++++++++++++++++++++++++++++++++---
 drivers/usb/host/ehci-timer.c |   45 +++++++++++++++++++++++++++++++++-
 drivers/usb/host/ehci.h       |    3 +++
 5 files changed, 103 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 246e124..35b4148 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -304,7 +304,8 @@ static void end_unlink_async(struct ehci_hcd *ehci);
 static void unlink_empty_async(struct ehci_hcd *ehci);
 static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
 static void ehci_work(struct ehci_hcd *ehci);
-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
+static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
+			      bool wait);
 static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);

 #include "ehci-timer.c"
@@ -484,6 +485,7 @@ static int ehci_init(struct usb_hcd *hcd)
 	ehci->periodic_size = DEFAULT_I_TDPS;
 	INIT_LIST_HEAD(&ehci->async_unlink);
 	INIT_LIST_HEAD(&ehci->async_idle);
+	INIT_LIST_HEAD(&ehci->intr_unlink_wait);
 	INIT_LIST_HEAD(&ehci->intr_unlink);
 	INIT_LIST_HEAD(&ehci->intr_qh_list);
 	INIT_LIST_HEAD(&ehci->cached_itd_list);
@@ -908,7 +910,7 @@ static int ehci_urb_dequeue(struct usb_hcd *hcd,
struct urb *urb, int status)
 		switch (qh->qh_state) {
 		case QH_STATE_LINKED:
 			if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT)
-				start_unlink_intr(ehci, qh);
+				start_unlink_intr(ehci, qh, false);
 			else
 				start_unlink_async(ehci, qh);
 			break;
@@ -1042,7 +1044,7 @@ ehci_endpoint_reset(struct usb_hcd *hcd, struct
usb_host_endpoint *ep)
 			if (eptype == USB_ENDPOINT_XFER_BULK)
 				start_unlink_async(ehci, qh);
 			else
-				start_unlink_intr(ehci, qh);
+				start_unlink_intr(ehci, qh, false);
 		}
 	}
 	spin_unlock_irqrestore(&ehci->lock, flags);
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index b2f6450..4104c66 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -345,6 +345,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)

 	end_unlink_async(ehci);
 	unlink_empty_async_suspended(ehci);
+	ehci_handle_start_intr_unlinks(ehci);
 	ehci_handle_intr_unlinks(ehci);
 	end_free_itds(ehci);

diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index acff5b8..5dfda56 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -548,6 +548,9 @@ static void qh_link_periodic(struct ehci_hcd
*ehci, struct ehci_qh *qh)

 	list_add(&qh->intr_node, &ehci->intr_qh_list);

+	/* unlink need this node to be initialized */
+	INIT_LIST_HEAD(&qh->unlink_node);
+
 	/* maybe enable periodic schedule processing */
 	++ehci->intr_count;
 	enable_periodic(ehci);
@@ -601,12 +604,24 @@ static void qh_unlink_periodic(struct ehci_hcd
*ehci, struct ehci_qh *qh)
 	list_del(&qh->intr_node);
 }

-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+/* must be called with holding ehci->lock */
+static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
 {
-	/* If the QH isn't linked then there's nothing we can do. */
-	if (qh->qh_state != QH_STATE_LINKED)
+	if (qh->qh_state != QH_STATE_LINKED || list_empty(&qh->unlink_node))
 		return;

+	list_del_init(&qh->unlink_node);
+
+	/* avoid unnecessary CPU wakeup */
+	if (list_empty(&ehci->intr_unlink_wait))
+		ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR);
+}
+
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+	/* if the qh is waitting for unlink, cancel it now */
+	cancel_unlink_wait_intr(ehci, qh);
+
 	qh_unlink_periodic (ehci, qh);

 	/* Make sure the unlinks are visible before starting the timer */
@@ -632,6 +647,34 @@ static void start_unlink_intr(struct ehci_hcd
*ehci, struct ehci_qh *qh)
 	}
 }

+/*
+ * It is common only one intr URB is scheduled on one qh, and
+ * given complete() is run in tasklet context, introduce a bit
+ * delay to avoid unlink qh too early.
+ */
+static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
+			      bool wait)
+{
+	/* If the QH isn't linked then there's nothing we can do. */
+	if (qh->qh_state != QH_STATE_LINKED)
+		return;
+
+	if (!wait)
+		return start_do_unlink_intr(ehci, qh);
+
+	qh->unlink_cycle = ehci->intr_unlink_wait_cycle;
+
+	/* New entries go at the end of the intr_unlink_wait list */
+	list_add_tail(&qh->unlink_node, &ehci->intr_unlink_wait);
+
+	if (ehci->rh_state < EHCI_RH_RUNNING)
+		ehci_handle_start_intr_unlinks(ehci);
+	else if (ehci->intr_unlink_wait.next == &qh->unlink_node) {
+		ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+		++ehci->intr_unlink_wait_cycle;
+	}
+}
+
 static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
 {
 	struct ehci_qh_hw	*hw = qh->hw;
@@ -876,6 +919,9 @@ static int intr_submit (
 			goto done;
 	}

+	/* put back qh from unlink wait list */
+	cancel_unlink_wait_intr(ehci, qh);
+
 	/* then queue the urb's tds to the qh */
 	qh = qh_append_tds(ehci, urb, qtd_list, epnum, &urb->ep->hcpriv);
 	BUG_ON (qh == NULL);
@@ -921,7 +967,7 @@ static void scan_intr(struct ehci_hcd *ehci)
 			temp = qh_completions(ehci, qh);
 			if (unlikely(temp || (list_empty(&qh->qtd_list) &&
 					qh->qh_state == QH_STATE_LINKED)))
-				start_unlink_intr(ehci, qh);
+				start_unlink_intr(ehci, qh, true);
 		}
 	}
 }
diff --git a/drivers/usb/host/ehci-timer.c b/drivers/usb/host/ehci-timer.c
index 11e5b32..b8aadb9 100644
--- a/drivers/usb/host/ehci-timer.c
+++ b/drivers/usb/host/ehci-timer.c
@@ -72,6 +72,7 @@ static unsigned event_delays_ns[] = {
 	1 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_POLL_DEAD */
 	1125 * NSEC_PER_USEC,	/* EHCI_HRTIMER_UNLINK_INTR */
 	2 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_FREE_ITDS */
+	5 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_START_UNLINK_INTR */
 	6 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_ASYNC_UNLINKS */
 	10 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_IAA_WATCHDOG */
 	10 * NSEC_PER_MSEC,	/* EHCI_HRTIMER_DISABLE_PERIODIC */
@@ -98,6 +99,17 @@ static void ehci_enable_event(struct ehci_hcd
*ehci, unsigned event,
 	}
 }

+/* Warning: don't call this function from hrtimer handler context */
+static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
+{
+	if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci)))
+		return;
+
+	ehci->enabled_hrtimer_events &= ~(1 << event);
+	if (!ehci->enabled_hrtimer_events)
+		hrtimer_cancel(&ehci->hrtimer);
+}
+

 /* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */
 static void ehci_poll_ASS(struct ehci_hcd *ehci)
@@ -215,6 +227,36 @@ static void ehci_handle_controller_death(struct
ehci_hcd *ehci)
 	/* Not in process context, so don't try to reset the controller */
 }

+/* start to unlink interrupt QHs  */
+static void ehci_handle_start_intr_unlinks(struct ehci_hcd *ehci)
+{
+	bool		stopped = (ehci->rh_state < EHCI_RH_RUNNING);
+
+	/*
+	 * Process all the QHs on the intr_unlink list that were added
+	 * before the current unlink cycle began.  The list is in
+	 * temporal order, so stop when we reach the first entry in the
+	 * current cycle.  But if the root hub isn't running then
+	 * process all the QHs on the list.
+	 */
+	while (!list_empty(&ehci->intr_unlink_wait)) {
+		struct ehci_qh	*qh;
+
+		qh = list_first_entry(&ehci->intr_unlink_wait,
+				      struct ehci_qh, unlink_node);
+		if (!stopped &&
+		    qh->unlink_cycle == ehci->intr_unlink_wait_cycle)
+			break;
+		list_del_init(&qh->unlink_node);
+		start_unlink_intr(ehci, qh, false);
+	}
+
+	/* Handle remaining entries later */
+	if (!list_empty(&ehci->intr_unlink_wait)) {
+		ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+		++ehci->intr_unlink_wait_cycle;
+	}
+}

 /* Handle unlinked interrupt QHs once they are gone from the hardware */
 static void ehci_handle_intr_unlinks(struct ehci_hcd *ehci)
@@ -236,7 +278,7 @@ static void ehci_handle_intr_unlinks(struct ehci_hcd *ehci)
 				unlink_node);
 		if (!stopped && qh->unlink_cycle == ehci->intr_unlink_cycle)
 			break;
-		list_del(&qh->unlink_node);
+		list_del_init(&qh->unlink_node);
 		end_unlink_intr(ehci, qh);
 	}

@@ -363,6 +405,7 @@ static void (*event_handlers[])(struct ehci_hcd *) = {
 	ehci_handle_controller_death,	/* EHCI_HRTIMER_POLL_DEAD */
 	ehci_handle_intr_unlinks,	/* EHCI_HRTIMER_UNLINK_INTR */
 	end_free_itds,			/* EHCI_HRTIMER_FREE_ITDS */
+	ehci_handle_start_intr_unlinks,	/* EHCI_HRTIMER_START_UNLINK_INTR */
 	unlink_empty_async,		/* EHCI_HRTIMER_ASYNC_UNLINKS */
 	ehci_iaa_watchdog,		/* EHCI_HRTIMER_IAA_WATCHDOG */
 	ehci_disable_PSE,		/* EHCI_HRTIMER_DISABLE_PERIODIC */
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 7c978b2..1d2c164 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -88,6 +88,7 @@ enum ehci_hrtimer_event {
 	EHCI_HRTIMER_POLL_DEAD,		/* Wait for dead controller to stop */
 	EHCI_HRTIMER_UNLINK_INTR,	/* Wait for interrupt QH unlink */
 	EHCI_HRTIMER_FREE_ITDS,		/* Wait for unused iTDs and siTDs */
+	EHCI_HRTIMER_START_UNLINK_INTR, /* wait for new intr schedule */
 	EHCI_HRTIMER_ASYNC_UNLINKS,	/* Unlink empty async QHs */
 	EHCI_HRTIMER_IAA_WATCHDOG,	/* Handle lost IAA interrupts */
 	EHCI_HRTIMER_DISABLE_PERIODIC,	/* Wait to disable periodic sched */
@@ -143,6 +144,8 @@ struct ehci_hcd {			/* one per controller */
 	unsigned		i_thresh;	/* uframes HC might cache */

 	union ehci_shadow	*pshadow;	/* mirror hw periodic table */
+	struct list_head	intr_unlink_wait;
+	unsigned		intr_unlink_wait_cycle;
 	struct list_head	intr_unlink;
 	unsigned		intr_unlink_cycle;
 	unsigned		now_frame;	/* frame from HC hardware */
-- 
1.7.9.5



Thanks,
--
Ming Lei

[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink

[Alan Stern]

On Thu, 20 Jun 2013, Ming Lei wrote:

> IMO, there is no any side effect when we change the state to
> QH_STATE_UNLINK_WAIT and later change back to QH_STATE_LINKED
> later under this situation.

I don't like the idea of changing an invariant.

> The reason why I use QH_STATE_UNLINK_WAIT here is for avoiding
> unnecessary CPU wakeup.  Without the state, the unlink wait timer
> is still triggered to check if there are intr QHs to be unlinked, but in most of
> situations, there aren't QHs to be unlinked since tasklet is surely run
> before the wait timer is expired. So generally, without knowing the wait state,
> CPU wakeup events may be introduced unnecessarily.

Avoiding unnecessary timer events is a good thing, but I don't 
understand how it is connected with QH_STATE_UNLINK_WAIT.  Doesn't this
revised patch avoid the events without using this state?

Besides, don't you already know the wait state by checking whether 
qh->unlink_node is empty?

> Considered that the interval of interrupt endpoint might be very
> small(e.g. 125us,
> 1ms) and some devices have very frequent intr event, I think we
> need to pay attention to the problem.

Yes, we do.  The hrtimer code in ehci-timer is written specifically to 
handle that sort of situation.

> Even on async QH situation, we might need to consider the problem too
> since some application(eg. bulk only mass storage on xhci) may have
> thousands of interrupts per seconds during data transfer, so CPU wakeup
> events could be increased much by letting wait timer expire unnecessarily.

I suspect it's the other way around.  Letting the timer expire
_reduces_ the amount of work, because we don't have to start and stop
the timer as often.

It's a tradeoff.  One approach starts and cancels the timer N times
(where N can be fairly large); the other approach starts the timer
once and lets it expire, and then the handler routine does almost no
work.  Which approach uses more CPU time?  I don't know; I haven't
measured it.

> Also the async QH unlink approach has another disadvantage:
> 
> - if there are several QHs which are become empty during one wait period,
> the hrtimer has to be scheduled several times for starting unlink one qh
> each time.

That's because the driver unlinks only one async QH at a time.  It is
unavoidable.  In earlier kernels the driver would unlink multiple async
QHs simultaneously, and it needed to schedule the timer only once.  
For some reason (I still don't understand why), this did not work on
some systems.

>  And after introducing the unlink wait list like the patch, we only
> need schedule the hrtimer one time for unlinking all these empty QHs.

The async code used to work that way, before I changed it to unlink
only one async QH at a time.

> Finally, unlink wait for intr qh is only needed when the qh is completed
> right now, and other cases(eg. unlink) needn't the wait.

Right.

> The attached patch removes the QH_STATE_UNLINK_WAIT, and can
> avoid the above disadvantages of async QH unlink, could you comment
> on the new patch?

Okay...

> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 246e124..35b4148 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -304,7 +304,8 @@ static void end_unlink_async(struct ehci_hcd *ehci);
>  static void unlink_empty_async(struct ehci_hcd *ehci);
>  static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
>  static void ehci_work(struct ehci_hcd *ehci);
> -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
> +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
> +			      bool wait);

Adding a "wait" argument is a bad approach.  You should create a new 
function: start_unlink_intr_wait() or something like that.  After all, 
it is used in only one place.

> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index acff5b8..5dfda56 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -548,6 +548,9 @@ static void qh_link_periodic(struct ehci_hcd
> *ehci, struct ehci_qh *qh)
> 
>  	list_add(&qh->intr_node, &ehci->intr_qh_list);
> 
> +	/* unlink need this node to be initialized */
> +	INIT_LIST_HEAD(&qh->unlink_node);

This should be done only once, when the QH is created.  And the comment 
is unnecessary.

> @@ -98,6 +99,17 @@ static void ehci_enable_event(struct ehci_hcd
> *ehci, unsigned event,
>  	}
>  }
> 
> +/* Warning: don't call this function from hrtimer handler context */
> +static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
> +{
> +	if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci)))
> +		return;

This test looks really ugly, and it won't be needed after the driver
switches over to tasklets.  Of course, there's a problem before we
switch over: this routine will be called by an interrupt URB
submission, which could occur during a giveback in the timer handler
context.

Perhaps the best approach is to leave this routine out until after the 
driver switches over.

> +
> +	ehci->enabled_hrtimer_events &= ~(1 << event);
> +	if (!ehci->enabled_hrtimer_events)

Here you need to add:

		ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;

> +		hrtimer_cancel(&ehci->hrtimer);
> +}

This could also be useful for the IAA watchdog timer in the STS_IAA
code in ehci_irq().

> @@ -215,6 +227,36 @@ static void ehci_handle_controller_death(struct
> ehci_hcd *ehci)
>  	/* Not in process context, so don't try to reset the controller */
>  }
> 
> +/* start to unlink interrupt QHs  */
> +static void ehci_handle_start_intr_unlinks(struct ehci_hcd *ehci)
> +{
> +	bool		stopped = (ehci->rh_state < EHCI_RH_RUNNING);
> +
> +	/*
> +	 * Process all the QHs on the intr_unlink list that were added
> +	 * before the current unlink cycle began.  The list is in
> +	 * temporal order, so stop when we reach the first entry in the
> +	 * current cycle.  But if the root hub isn't running then
> +	 * process all the QHs on the list.
> +	 */
> +	while (!list_empty(&ehci->intr_unlink_wait)) {
> +		struct ehci_qh	*qh;
> +
> +		qh = list_first_entry(&ehci->intr_unlink_wait,
> +				      struct ehci_qh, unlink_node);
> +		if (!stopped &&
> +		    qh->unlink_cycle == ehci->intr_unlink_wait_cycle)
> +			break;

The style in this driver is to add two tab stops to continuation lines, 
not to line them up with respect to the previous line.

Otherwise this seems to be about right.

Alan Stern

[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink

[Ming Lei]

On Sat, Jun 22, 2013 at 4:16 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 20 Jun 2013, Ming Lei wrote:
>
>> IMO, there is no any side effect when we change the state to
>> QH_STATE_UNLINK_WAIT and later change back to QH_STATE_LINKED
>> later under this situation.
>
> I don't like the idea of changing an invariant.
>
>> The reason why I use QH_STATE_UNLINK_WAIT here is for avoiding
>> unnecessary CPU wakeup.  Without the state, the unlink wait timer
>> is still triggered to check if there are intr QHs to be unlinked, but in most of
>> situations, there aren't QHs to be unlinked since tasklet is surely run
>> before the wait timer is expired. So generally, without knowing the wait state,
>> CPU wakeup events may be introduced unnecessarily.
>
> Avoiding unnecessary timer events is a good thing, but I don't
> understand how it is connected with QH_STATE_UNLINK_WAIT.  Doesn't this
> revised patch avoid the events without using this state?
>
> Besides, don't you already know the wait state by checking whether
> qh->unlink_node is empty?
>
>> Considered that the interval of interrupt endpoint might be very
>> small(e.g. 125us,
>> 1ms) and some devices have very frequent intr event, I think we
>> need to pay attention to the problem.
>
> Yes, we do.  The hrtimer code in ehci-timer is written specifically to
> handle that sort of situation.
>
>> Even on async QH situation, we might need to consider the problem too
>> since some application(eg. bulk only mass storage on xhci) may have
>> thousands of interrupts per seconds during data transfer, so CPU wakeup
>> events could be increased much by letting wait timer expire unnecessarily.
>
> I suspect it's the other way around.  Letting the timer expire
> _reduces_ the amount of work, because we don't have to start and stop
> the timer as often.
>
> It's a tradeoff.  One approach starts and cancels the timer N times
> (where N can be fairly large); the other approach starts the timer
> once and lets it expire, and then the handler routine does almost no
> work.  Which approach uses more CPU time?  I don't know; I haven't
> measured it.
>
>> Also the async QH unlink approach has another disadvantage:
>>
>> - if there are several QHs which are become empty during one wait period,
>> the hrtimer has to be scheduled several times for starting unlink one qh
>> each time.
>
> That's because the driver unlinks only one async QH at a time.  It is
> unavoidable.  In earlier kernels the driver would unlink multiple async
> QHs simultaneously, and it needed to schedule the timer only once.
> For some reason (I still don't understand why), this did not work on
> some systems.
>
>>  And after introducing the unlink wait list like the patch, we only
>> need schedule the hrtimer one time for unlinking all these empty QHs.
>
> The async code used to work that way, before I changed it to unlink
> only one async QH at a time.
>
>> Finally, unlink wait for intr qh is only needed when the qh is completed
>> right now, and other cases(eg. unlink) needn't the wait.
>
> Right.
>
>> The attached patch removes the QH_STATE_UNLINK_WAIT, and can
>> avoid the above disadvantages of async QH unlink, could you comment
>> on the new patch?
>
> Okay...

Thanks for your review.

>
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index 246e124..35b4148 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -304,7 +304,8 @@ static void end_unlink_async(struct ehci_hcd *ehci);
>>  static void unlink_empty_async(struct ehci_hcd *ehci);
>>  static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
>>  static void ehci_work(struct ehci_hcd *ehci);
>> -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
>> +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
>> +                           bool wait);
>
> Adding a "wait" argument is a bad approach.  You should create a new
> function: start_unlink_intr_wait() or something like that.  After all,
> it is used in only one place.

OK.

>
>> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
>> index acff5b8..5dfda56 100644
>> --- a/drivers/usb/host/ehci-sched.c
>> +++ b/drivers/usb/host/ehci-sched.c
>> @@ -548,6 +548,9 @@ static void qh_link_periodic(struct ehci_hcd
>> *ehci, struct ehci_qh *qh)
>>
>>       list_add(&qh->intr_node, &ehci->intr_qh_list);
>>
>> +     /* unlink need this node to be initialized */
>> +     INIT_LIST_HEAD(&qh->unlink_node);
>
> This should be done only once, when the QH is created.  And the comment
> is unnecessary.

OK, I will move it into ehci_qh_alloc().

>
>> @@ -98,6 +99,17 @@ static void ehci_enable_event(struct ehci_hcd
>> *ehci, unsigned event,
>>       }
>>  }
>>
>> +/* Warning: don't call this function from hrtimer handler context */
>> +static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
>> +{
>> +     if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci)))
>> +             return;
>
> This test looks really ugly, and it won't be needed after the driver
> switches over to tasklets.  Of course, there's a problem before we
> switch over: this routine will be called by an interrupt URB
> submission, which could occur during a giveback in the timer handler
> context.
>
> Perhaps the best approach is to leave this routine out until after the
> driver switches over.

Considered without this patch, enabling BH_HCD isn't very good
for interrupt transfer, I will remove the check in 6/6.

>
>> +
>> +     ehci->enabled_hrtimer_events &= ~(1 << event);
>> +     if (!ehci->enabled_hrtimer_events)
>
> Here you need to add:
>
>                 ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;

Good catch.

>
>> +             hrtimer_cancel(&ehci->hrtimer);
>> +}
>
> This could also be useful for the IAA watchdog timer in the STS_IAA
> code in ehci_irq().

Right, it might be useful for any ehci timers which won't expire at
most of times, especially when the timer's frequency is high.

>
>> @@ -215,6 +227,36 @@ static void ehci_handle_controller_death(struct
>> ehci_hcd *ehci)
>>       /* Not in process context, so don't try to reset the controller */
>>  }
>>
>> +/* start to unlink interrupt QHs  */
>> +static void ehci_handle_start_intr_unlinks(struct ehci_hcd *ehci)
>> +{
>> +     bool            stopped = (ehci->rh_state < EHCI_RH_RUNNING);
>> +
>> +     /*
>> +      * Process all the QHs on the intr_unlink list that were added
>> +      * before the current unlink cycle began.  The list is in
>> +      * temporal order, so stop when we reach the first entry in the
>> +      * current cycle.  But if the root hub isn't running then
>> +      * process all the QHs on the list.
>> +      */
>> +     while (!list_empty(&ehci->intr_unlink_wait)) {
>> +             struct ehci_qh  *qh;
>> +
>> +             qh = list_first_entry(&ehci->intr_unlink_wait,
>> +                                   struct ehci_qh, unlink_node);
>> +             if (!stopped &&
>> +                 qh->unlink_cycle == ehci->intr_unlink_wait_cycle)
>> +                     break;
>
> The style in this driver is to add two tab stops to continuation lines,
> not to line them up with respect to the previous line.

OK.

> Otherwise this seems to be about right.

Thanks,
--
Ming Lei

[RFC PATCH v1 6/6] USB: EHCI: support running URB giveback in tasklet context

[Ming Lei]

Both 4 transfers can work well on EHCI HCD after switching to run
URB giveback in tasklet context, so mark all HCD drivers to support
it.

>From below test results on 3 machines(2 ARM and one x86), time
consumed by EHCI interrupt handler droped much without performance
loss.

1 test description
1.1 mass storage performance test:
- run below command 10 times and compute the average performance

    dd if=/dev/sdN iflag=direct of=/dev/null bs=200M count=1

- two usb mass storage device:
A: sandisk extreme USB 3.0 16G(used in test case 1 & case 2)
B: kingston DataTraveler G2 4GB(only used in test case 2)

1.2 uvc function test:
- run one simple capture program in the below link

   http://kernel.ubuntu.com/~ming/up/capture.c

- capture format 640*480 and results in High Bandwidth mode on the
uvc device: Z-Star 0x0ac8/0x3450

- on T410(x86) laptop, also use guvcview to watch video capture/playback

1.3 about test2 and test4
- both two devices involved are tested concurrently by above test items

1.4 how to compute irq time(the time consumed by ehci_irq)
- use trace points of irq:irq_handler_entry and irq:irq_handler_exit

1.5 kernel
3.10.0-rc3-next-20130528

1.6 test machines
Pandaboard A1: ARM CortexA9 dural core
Arndale board: ARM CortexA15 dural core
T410: i5 CPU 2.67GHz quad core

2 test result
2.1 test case1: single mass storage device performance test
--------------------------------------------------------------------
		upstream 		| patched
		perf(MB/s)+irq time(us)	| perf(MB/s)+irq time(us)
--------------------------------------------------------------------
Pandaboard A1:  25.280(avg:145,max:772)	| 25.540(avg:14, max:75)
Arndale board:  29.700(avg:33, max:129)	| 29.700(avg:10,  max:50)
T410: 		34.430(avg:17, max:154*)| 34.660(avg:12, max:155)
---------------------------------------------------------------------

2.2 test case2: two mass storage devices' performance test
--------------------------------------------------------------------
		upstream 			| patched
		perf(MB/s)+irq time(us)		| perf(MB/s)+irq time(us)
--------------------------------------------------------------------
Pandaboard A1:  15.840/15.580(avg:158,max:1216)	| 16.500/16.160(avg:15,max:139)
Arndale board:  17.370/16.220(avg:33 max:234)	| 17.480/16.200(avg:11, max:91)
T410: 		21.180/19.820(avg:18 max:160)	| 21.220/19.880(avg:11, max:149)
---------------------------------------------------------------------

2.3 test case3: one uvc streaming test
- uvc device works well(on x86, luvcview can be used too and has
same result with uvc capture)
--------------------------------------------------------------------
		upstream 		| patched
		irq time(us)		| irq time(us)
--------------------------------------------------------------------
Pandaboard A1:  (avg:445, max:873)	| (avg:33, max:44)
Arndale board:  (avg:316, max:630)	| (avg:20, max:27)
T410: 		(avg:39,  max:107)	| (avg:10, max:65)
---------------------------------------------------------------------

2.4 test case4: one uvc streaming plus one mass storage device test
--------------------------------------------------------------------
		upstream 		| patched
		perf(MB/s)+irq time(us)	| perf(MB/s)+irq time(us)
--------------------------------------------------------------------
Pandaboard A1:  20.340(avg:259,max:1704)| 20.390(avg:24, max:101)
Arndale board:  23.460(avg:124,max:726)	| 23.370(avg:15, max:52)
T410: 		28.520(avg:27, max:169)	| 28.630(avg:13, max:160)
---------------------------------------------------------------------

* On T410, sometimes read ehci status register in ehci_irq takes more
than 100us, and the problem has been reported on the link:

	http://marc.info/?t=137065867300001&r=1&w=2

Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/host/ehci-fsl.c       |    2 +-
 drivers/usb/host/ehci-grlib.c     |    2 +-
 drivers/usb/host/ehci-hcd.c       |    2 +-
 drivers/usb/host/ehci-mv.c        |    2 +-
 drivers/usb/host/ehci-octeon.c    |    2 +-
 drivers/usb/host/ehci-pmcmsp.c    |    2 +-
 drivers/usb/host/ehci-ppc-of.c    |    2 +-
 drivers/usb/host/ehci-ps3.c       |    2 +-
 drivers/usb/host/ehci-sead3.c     |    2 +-
 drivers/usb/host/ehci-sh.c        |    2 +-
 drivers/usb/host/ehci-tegra.c     |    2 +-
 drivers/usb/host/ehci-tilegx.c    |    2 +-
 drivers/usb/host/ehci-w90x900.c   |    2 +-
 drivers/usb/host/ehci-xilinx-of.c |    2 +-
 14 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index bd831ec..330274a 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -669,7 +669,7 @@ static const struct hc_driver ehci_fsl_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq = ehci_irq,
-	.flags = HCD_USB2 | HCD_MEMORY,
+	.flags = HCD_USB2 | HCD_MEMORY | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-grlib.c b/drivers/usb/host/ehci-grlib.c
index a77bd8d..2905004 100644
--- a/drivers/usb/host/ehci-grlib.c
+++ b/drivers/usb/host/ehci-grlib.c
@@ -43,7 +43,7 @@ static const struct hc_driver ehci_grlib_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
+	.flags			= HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 0ab82de..d856c5e 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1174,7 +1174,7 @@ static const struct hc_driver ehci_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq =			ehci_irq,
-	.flags =		HCD_MEMORY | HCD_USB2,
+	.flags =		HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-mv.c b/drivers/usb/host/ehci-mv.c
index 915c2db..ce18a36 100644
--- a/drivers/usb/host/ehci-mv.c
+++ b/drivers/usb/host/ehci-mv.c
@@ -96,7 +96,7 @@ static const struct hc_driver mv_ehci_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq = ehci_irq,
-	.flags = HCD_MEMORY | HCD_USB2,
+	.flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-octeon.c b/drivers/usb/host/ehci-octeon.c
index 45cc001..ab0397e 100644
--- a/drivers/usb/host/ehci-octeon.c
+++ b/drivers/usb/host/ehci-octeon.c
@@ -51,7 +51,7 @@ static const struct hc_driver ehci_octeon_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
+	.flags			= HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-pmcmsp.c b/drivers/usb/host/ehci-pmcmsp.c
index 363890e..3ab32a2 100644
--- a/drivers/usb/host/ehci-pmcmsp.c
+++ b/drivers/usb/host/ehci-pmcmsp.c
@@ -286,7 +286,7 @@ static const struct hc_driver ehci_msp_hc_driver = {
 #else
 	.irq =			ehci_irq,
 #endif
-	.flags =		HCD_MEMORY | HCD_USB2,
+	.flags =		HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-ppc-of.c b/drivers/usb/host/ehci-ppc-of.c
index 86da09c..014d37b 100644
--- a/drivers/usb/host/ehci-ppc-of.c
+++ b/drivers/usb/host/ehci-ppc-of.c
@@ -28,7 +28,7 @@ static const struct hc_driver ehci_ppc_of_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
+	.flags			= HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
index fd98377..8188542 100644
--- a/drivers/usb/host/ehci-ps3.c
+++ b/drivers/usb/host/ehci-ps3.c
@@ -71,7 +71,7 @@ static const struct hc_driver ps3_ehci_hc_driver = {
 	.product_desc		= "PS3 EHCI Host Controller",
 	.hcd_priv_size		= sizeof(struct ehci_hcd),
 	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
+	.flags			= HCD_MEMORY | HCD_USB2 | HCD_BH,
 	.reset			= ps3_ehci_hc_reset,
 	.start			= ehci_run,
 	.stop			= ehci_stop,
diff --git a/drivers/usb/host/ehci-sead3.c b/drivers/usb/host/ehci-sead3.c
index b2de52d..8a73449 100644
--- a/drivers/usb/host/ehci-sead3.c
+++ b/drivers/usb/host/ehci-sead3.c
@@ -55,7 +55,7 @@ const struct hc_driver ehci_sead3_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
+	.flags			= HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-sh.c b/drivers/usb/host/ehci-sh.c
index c4c0ee9..1691f8e 100644
--- a/drivers/usb/host/ehci-sh.c
+++ b/drivers/usb/host/ehci-sh.c
@@ -36,7 +36,7 @@ static const struct hc_driver ehci_sh_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq				= ehci_irq,
-	.flags				= HCD_USB2 | HCD_MEMORY,
+	.flags				= HCD_USB2 | HCD_MEMORY | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 8390c87..b71a496 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -377,7 +377,7 @@ static const struct hc_driver tegra_ehci_hc_driver = {
 	.description		= hcd_name,
 	.product_desc		= "Tegra EHCI Host Controller",
 	.hcd_priv_size		= sizeof(struct ehci_hcd),
-	.flags			= HCD_USB2 | HCD_MEMORY,
+	.flags			= HCD_USB2 | HCD_MEMORY | HCD_BH,
 
 	/* standard ehci functions */
 	.irq			= ehci_irq,
diff --git a/drivers/usb/host/ehci-tilegx.c b/drivers/usb/host/ehci-tilegx.c
index d72b292..204d3b6 100644
--- a/drivers/usb/host/ehci-tilegx.c
+++ b/drivers/usb/host/ehci-tilegx.c
@@ -61,7 +61,7 @@ static const struct hc_driver ehci_tilegx_hc_driver = {
 	 * Generic hardware linkage.
 	 */
 	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
+	.flags			= HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * Basic lifecycle operations.
diff --git a/drivers/usb/host/ehci-w90x900.c b/drivers/usb/host/ehci-w90x900.c
index 59e0e24..1c370df 100644
--- a/drivers/usb/host/ehci-w90x900.c
+++ b/drivers/usb/host/ehci-w90x900.c
@@ -108,7 +108,7 @@ static const struct hc_driver ehci_w90x900_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq = ehci_irq,
-	.flags = HCD_USB2|HCD_MEMORY,
+	.flags = HCD_USB2|HCD_MEMORY|HCD_BH,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/ehci-xilinx-of.c b/drivers/usb/host/ehci-xilinx-of.c
index 35c7f90..c6591ea 100644
--- a/drivers/usb/host/ehci-xilinx-of.c
+++ b/drivers/usb/host/ehci-xilinx-of.c
@@ -79,7 +79,7 @@ static const struct hc_driver ehci_xilinx_of_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
+	.flags			= HCD_MEMORY | HCD_USB2 | HCD_BH,
 
 	/*
 	 * basic lifecycle operations
-- 
1.7.9.5

[RFC PATCH v1 6/6] USB: EHCI: support running URB giveback in tasklet context

[Alan Stern]

On Tue, 18 Jun 2013, Ming Lei wrote:

> Both 4 transfers can work well on EHCI HCD after switching to run

"Both 4 transfers"?  The word "both" applies when there are two items, 
not more than two.  And what are the four transfers you are referring 
to?  Do you mean all four transfer _types_?

Alan Stern

[RFC PATCH v1 6/6] USB: EHCI: support running URB giveback in tasklet context

[Ming Lei]

On Wed, Jun 19, 2013 at 12:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 18 Jun 2013, Ming Lei wrote:
>
>> Both 4 transfers can work well on EHCI HCD after switching to run
>
> "Both 4 transfers"?  The word "both" applies when there are two items,
> not more than two.  And what are the four transfers you are referring
> to?  Do you mean all four transfer _types_?

Yes, I mean the four transfer types, and "Both 4 transfers" should have
been "all four transfer types", will fix it in v2.


Thanks,
--
Ming Lei