[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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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-GvhC2dPhHPQdnm+***@public.gmane.org>
Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/***@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+***@public.gmane.org>
---
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);

[Alan Stern]

Post by 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.

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

Post by Ming Lei
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
* 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);
}
}

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.

Post by Ming Lei
@@ -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.

Post by Ming Lei
+ 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.

Post by Ming Lei
+ 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;

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.

Post by Ming Lei
@@ -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.

Post by Ming Lei
- 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;

Post by Ming Lei
- 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.

Post by Ming Lei
+ }
+
+ 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.

Post by Ming Lei
+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.

Post by Ming Lei
+
+ __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.

Post by Ming Lei
+
+ kfree(hcd->periodic_bh);
+ kfree(hcd->async_bh);
+}
+
/**
* usb_create_shared_hcd - create and initialize an HCD structure
@@ -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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by 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.

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

Post by Ming Lei
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
* 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);
}
}

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?

Post by Alan Stern
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.

Post by Alan Stern
continue to handle root-hub URBs the way we always have.

Post by Ming Lei
@@ -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.

Post by Alan Stern

Post by Ming Lei
+ 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.

Post by Alan Stern

Post by Ming Lei
+ 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;

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.

Post by Alan Stern

Post by Ming Lei
@@ -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.

Post by Alan Stern

Post by Ming Lei
- 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;

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

Post by Ming Lei
- 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.

Post by Alan Stern

Post by Ming Lei
+ }
+
+ 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.
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.

Post by Alan Stern
Or you could turn this into a "switch" statement.

Post by Ming Lei
+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, :-)

Post by Alan Stern

Post by Ming Lei
+
+ __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.

Post by Alan Stern

Post by Ming Lei
+
+ kfree(hcd->periodic_bh);
+ kfree(hcd->async_bh);
+}
+
/**
* usb_create_shared_hcd - create and initialize an HCD structure
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Ming Lei

Post by Alan Stern

Post by 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.

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

Post by Ming Lei
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
* 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);
}
}

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.

Post by Ming Lei
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei
@@ -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);
}
}

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.

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei
@@ -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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei
@@ -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);
}
}

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?

Post by Alan Stern
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.

Post by Alan Stern

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei
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.

Post by Ming Lei

Post by Alan Stern
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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern
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.

Post by Alan Stern
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Ming Lei

Post by Alan Stern
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.

Post by Ming Lei

Post by Alan Stern
(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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by Ming Lei

Post by Alan Stern
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?

Post by Alan Stern

Post by Ming Lei

Post by Alan Stern
(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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei

Post by Alan Stern
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Ming Lei

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei

Post by Alan Stern
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?

- 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Oliver Neukum]

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei
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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Oliver Neukum

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Oliver Neukum]

Post by Ming Lei

Post by Oliver Neukum

Post by Ming Lei

Post by Alan Stern

Post by Ming Lei
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.

Post by Ming Lei
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


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Oliver Neukum
This is highly problematic. It is bound to cause resource leaks.

Post by Ming Lei
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.

Post by Oliver Neukum
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Ming Lei
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Ming Lei

Post by Oliver Neukum
This is highly problematic. It is bound to cause resource leaks.

Post by Ming Lei
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.

Post by Oliver Neukum
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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern
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.

Post by Alan Stern
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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-GvhC2dPhHPQdnm+***@public.gmane.org>
Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/***@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+***@public.gmane.org>
---
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);

[Sergei Shtylyov]

Hello.

Post by 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.
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by 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.

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

Post by Ming Lei
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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by 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.

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.

Post by Alan Stern

Post by Ming Lei
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Ming Lei

Post by Alan Stern

Post by 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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-GvhC2dPhHPQdnm+***@public.gmane.org>
Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/***@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+***@public.gmane.org>
---
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.

[Alan Stern]

Post by 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.
@@ -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.

Post by Ming Lei
+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.

Post by Ming Lei
+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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by 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.
@@ -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.

Post by Alan Stern
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.

Post by Alan Stern

Post by Ming Lei
+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.

Post by Alan Stern

Post by Ming Lei
+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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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-nwvwT67g6+6dFdvTe/***@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+***@public.gmane.org>
---
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);

[Alan Stern]

Post by 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().

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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by 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().

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.

Post by Alan Stern
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Ming Lei

Post by Alan Stern

Post by 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().

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.

Post by Ming Lei

Post by Alan Stern
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.

Post by Ming Lei
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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by Ming Lei

Post by Alan Stern

Post by 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().

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.

Post by Ming Lei

Post by Alan Stern
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.

Post by Ming Lei
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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-nwvwT67g6+6dFdvTe/***@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+***@public.gmane.org>
---
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 */

[Alan Stern]

Post by 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by 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.

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

Post by Alan Stern
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().

Post by Alan Stern
the QH was completely unlinked. Instead, you should wait first, then
do the entire unlink.

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

Post by Alan Stern
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.

Post by Alan Stern
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Ming Lei

Post by Alan Stern
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.

Post by Ming Lei
- 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

Post by Alan Stern
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.

Post by Ming Lei

Post by Alan Stern
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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by Ming Lei

Post by Alan Stern
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.

Post by Alan Stern
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.

Post by Alan Stern
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.

Post by Alan Stern

Post by Ming Lei
- 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

Post by Alan Stern
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Ming Lei
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.

Post by Ming Lei
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?

Post by Ming Lei
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.

Post by Ming Lei
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.

Post by Ming Lei
- 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.

Post by Ming Lei
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.

Post by Ming Lei
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.

Post by Ming Lei
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...

Post by Ming Lei
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.

Post by Ming Lei
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.

Post by Ming Lei
@@ -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.

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

Here you need to add:

ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;

Post by Ming Lei
+ hrtimer_cancel(&ehci->hrtimer);
+}

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

Post by Ming Lei
@@ -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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by Ming Lei
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.

Post by Ming Lei
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?

Post by Ming Lei
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.

Post by Ming Lei
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.

Post by Ming Lei
- 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.

Post by Ming Lei
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.

Post by Ming Lei
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.

Post by Ming Lei
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.

Post by Alan Stern

Post by Ming Lei
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.

Post by Alan Stern

Post by Ming Lei
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().

Post by Alan Stern

Post by Ming Lei
@@ -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.

Post by Alan Stern

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

ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;

Good catch.

Post by Alan Stern

Post by Ming Lei
+ 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.

Post by Alan Stern

Post by Ming Lei
@@ -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.

Post by Alan Stern
Otherwise this seems to be about right.

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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-nwvwT67g6+6dFdvTe/***@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+***@public.gmane.org>
---
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

[Alan Stern]

Post by Ming Lei
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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ming Lei]

Post by Alan Stern

Post by Ming Lei
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html