USB: HCD/EHCI: giveback of URB in tasklet context

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.

drivers/usb/core/hcd.c +8 -4

index 014dc99..a272968 100644
@@ -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);
 

drivers/usb/core/hcd.c +5 -0

+	atomic_dec (&urb->use_count);
+	if (unlikely(atomic_read(&urb->reject)))
+		wake_up (&usb_kill_urb_queue);
+	usb_put_urb (urb);
+}

drivers/usb/core/hcd.c +0 -0

@@ -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)
 {

drivers/usb/core/hcd.c +4 -0

+	}
+
+	spin_lock(&bh->lock);
+	list_add_tail(&urb->urb_list, &bh->head);

drivers/usb/core/hcd.c +4 -0

+static int init_giveback_urb_bh(struct usb_hcd *hcd)
+{
+	if (!hcd_giveback_urb_in_bh(hcd))
+		return 0;

drivers/usb/core/hcd.c +9 -0

+
+	__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)
+{

drivers/usb/core/hcd.c +5 -0

+
+	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");

Quoted 4 lines · 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.

Quoted 36 lines

drivers/usb/core/hcd.c +8 -4

index 014dc99..a272968 100644
@@ -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);
 

drivers/usb/core/hcd.c static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) · line 849

Quoted 10 lines

drivers/usb/core/hcd.c +4 -2

-                     spin_unlock(&hcd_root_hub_lock);
+                     if (!hcd_giveback_urb_in_bh(hcd))
+                             spin_unlock(&hcd_root_hub_lock);
                      usb_hcd_giveback_urb(hcd, urb, status);
-                     spin_lock(&hcd_root_hub_lock);
+                     if (!hcd_giveback_urb_in_bh(hcd))
+                             spin_lock(&hcd_root_hub_lock);
              }
      }
  done:

None of these tests are necessary. Root hubs are different from normal devices; their URBs are handled mostly by usbcore. The only part done by the HCD is always synchronous. And we know that root-hub URBs

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

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

drivers/usb/core/hcd.c int usb_hcd_unlink_urb (struct urb *urb, int status) · line 1664

Quoted 12 lines

drivers/usb/core/hcd.c +8 -0

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

drivers/usb/core/hcd.c int usb_hcd_unlink_urb (struct urb *urb, int status) · line 1676

Quoted 13 lines

drivers/usb/core/hcd.c +12 -0

+     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.

Quoted 12 lines

drivers/usb/core/hcd.c +11 -0

+     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;

drivers/usb/core/hcd.c int usb_hcd_unlink_urb (struct urb *urb, int status) · line 1691

Quoted 4 lines

drivers/usb/core/hcd.c +4 -0

+
+     spin_lock_irqsave(&bh->lock, flags);
+     bh->running = 1;
+restart:

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

Quoted 11 lines

drivers/usb/core/hcd.c +0 -6

@@ -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))

drivers/usb/core/hcd.c int usb_hcd_unlink_urb (struct urb *urb, int status) · line 1732

Quoted 4 lines

drivers/usb/core/hcd.c +3 -1

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

drivers/usb/core/hcd.c int usb_hcd_unlink_urb (struct urb *urb, int status) · line 1734

Quoted 8 lines

drivers/usb/core/hcd.c +0 -5

-     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;

drivers/usb/core/hcd.c int usb_hcd_unlink_urb (struct urb *urb, int status) · line 1742

Quoted 13 lines

drivers/usb/core/hcd.c +8 -5

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

Quoted 5 lines

drivers/usb/core/hcd.c +4 -0

+     }
+
+     spin_lock(&bh->lock);
+     list_add_tail(&urb->urb_list, &bh->head);

drivers/usb/core/hcd.c int usb_hcd_unlink_urb (struct urb *urb, int status) · line 1756

Quoted 10 lines

drivers/usb/core/hcd.c +10 -0

+     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.

Quoted 9 lines

drivers/usb/core/hcd.c +9 -0

+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;
+

drivers/usb/core/hcd.c EXPORT_SYMBOL_GPL (usb_hc_died); · line 2399

Quoted 5 lines

drivers/usb/core/hcd.c +5 -0

+     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.

Quoted 15 lines

drivers/usb/core/hcd.c +14 -0

+
+     __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)
+{

drivers/usb/core/hcd.c EXPORT_SYMBOL_GPL (usb_hc_died); · line 2418

Quoted 5 lines

drivers/usb/core/hcd.c +5 -0

+     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.

Quoted 13 lines

drivers/usb/core/hcd.c +5 -0

+
+     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");
 

drivers/usb/core/hcd.c int usb_add_hcd(struct usb_hcd *hcd, · line 2698

Quoted 9 lines

drivers/usb/core/hcd.c +9 -0

+     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?

Quoted 45 lines · Alan Stern

Quoted 4 lines · 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.

Quoted 36 lines

drivers/usb/core/hcd.c +8 -4

index 014dc99..a272968 100644
@@ -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);
 

Quoted 14 lines

drivers/usb/core/hcd.c static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) · line 849

Quoted 10 lines

drivers/usb/core/hcd.c +4 -2

-                     spin_unlock(&hcd_root_hub_lock);
+                     if (!hcd_giveback_urb_in_bh(hcd))
+                             spin_unlock(&hcd_root_hub_lock);
                      usb_hcd_giveback_urb(hcd, urb, status);
-                     spin_lock(&hcd_root_hub_lock);
+                     if (!hcd_giveback_urb_in_bh(hcd))
+                             spin_lock(&hcd_root_hub_lock);
              }
      }
  done:

None of these tests are necessary. Root hubs are different from normal devices; their URBs are handled mostly by usbcore. The only part done by the HCD is always synchronous. And we know that root-hub URBs

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

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

Quoted 4 lines

Quoted 4 lines

drivers/usb/core/hcd.c +0 -0

@@ -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);
 

Quoted 14 lines

drivers/usb/core/hcd.c static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) · line 849

Quoted 10 lines

drivers/usb/core/hcd.c +4 -2

-                     spin_unlock(&hcd_root_hub_lock);
+                     if (!hcd_giveback_urb_in_bh(hcd))
+                             spin_unlock(&hcd_root_hub_lock);
                      usb_hcd_giveback_urb(hcd, urb, status);
-                     spin_lock(&hcd_root_hub_lock);
+                     if (!hcd_giveback_urb_in_bh(hcd))
+                             spin_lock(&hcd_root_hub_lock);
              }
      }
  done:

None of these tests are necessary. Root hubs are different from normal devices; their URBs are handled mostly by usbcore. The only part done by the HCD is always synchronous. And we know that root-hub URBs

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

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

Quoted 4 lines

Quoted 4 lines

drivers/usb/core/hcd.c +0 -0

@@ -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");
 

Quoted 11 lines

drivers/usb/core/hcd.c int usb_add_hcd(struct usb_hcd *hcd, · line 2698

Quoted 9 lines

drivers/usb/core/hcd.c +9 -0

+     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.

Quoted 25 lines · Ming Lei

Quoted 4 lines

Quoted 4 lines

drivers/usb/core/hcd.c +0 -0

@@ -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);
 

Quoted 14 lines

drivers/usb/core/hcd.c static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) · line 849

Quoted 10 lines

drivers/usb/core/hcd.c +4 -2

-                     spin_unlock(&hcd_root_hub_lock);
+                     if (!hcd_giveback_urb_in_bh(hcd))
+                             spin_unlock(&hcd_root_hub_lock);
                      usb_hcd_giveback_urb(hcd, urb, status);
-                     spin_lock(&hcd_root_hub_lock);
+                     if (!hcd_giveback_urb_in_bh(hcd))
+                             spin_lock(&hcd_root_hub_lock);
              }
      }
  done:

None of these tests are necessary. Root hubs are different from normal devices; their URBs are handled mostly by usbcore. The only part done by the HCD is always synchronous. And we know that root-hub URBs

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

Quoted 3 lines

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.

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

drivers/usb/core/hcd.c +0 -0

@@ -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");

drivers/usb/core/hcd.c int usb_add_hcd(struct usb_hcd *hcd, · line 2698

Quoted 10 lines

drivers/usb/core/hcd.c +9 -0

 
+     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?

Quoted 3 lines

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.

Quoted 10 lines

Quoted 9 lines

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?

Quoted 4 lines

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?

Quoted 2 lines

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.

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.)

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

Quoted 5 lines · 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.

Quoted 2 lines

(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.

Quoted 7 lines · Ming Lei

Quoted 5 lines · 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.

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 13 lines · Alan Stern

Quoted 10 lines · Ming Lei

Quoted 5 lines · 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.

Quoted 3 lines

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?

Quoted 14 lines · Ming Lei

Quoted 13 lines · Alan Stern

Quoted 10 lines · Ming Lei

Quoted 5 lines · 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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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?

Quoted 8 lines · On Friday 21 June 2013 09:12:38 Ming Lei wrote:

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 12 lines · Oliver Neukum

Quoted 10 lines · On Friday 21 June 2013 09:12:38 Ming Lei wrote:

Quoted 7 lines

Quoted 3 lines

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?

Quoted 4 lines

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.

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

Quoted 2 lines

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.

On Fri, Jun 21, 2013 at 5:20 PM, Oliver Neukum <[email protected]> wrote:

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

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

Quoted 9 lines · Oliver Neukum

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

Quoted 2 lines

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.

Quoted 1 line

Quoted 1 line

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.

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 <[email protected]>

Cc Alan Stern <[email protected]>

Signed-off-by Ming Lei <[email protected]>

---
  drivers/usb/core/hcd.c |   23 ++++++++++++++++++++++-
  1 file changed, 22 insertions(+), 1 deletion(-)

drivers/usb/core/hcd.c +7 -1

index a272968..09a8263 100644
@@ -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.
+	 *

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 +12 -1

index a272968..09a8263 100644
@@ -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.
+	 */

Quoted 5 lines · 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.

Quoted 4 lines

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.

Quoted 27 lines

drivers/usb/core/hcd.c +17 -1

index a272968..09a8263 100644
@@ -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);

drivers/usb/core/hcd.c static void __usb_hcd_giveback_urb(struct urb *urb) · line 1697

Quoted 5 lines

drivers/usb/core/hcd.c +5 -0

+             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.

Quoted 12 lines · Alan Stern

Quoted 9 lines · 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.

Quoted 1 line

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.

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.

Documentation/usb/URB.txt +0 -1

@@ -210,12 +209,19 @@ have transferred successfully before the completion was called.
 
 
 NOTE:  ***** WARNING *****
-NEVER SLEEP IN A COMPLETION HANDLER.  These are normally called

Quoted 31 lines · 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.

Documentation/usb/URB.txt +2 -6

@@ -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

Documentation/usb/URB.txt have transferred successfully before the completion was called. · line 216

Quoted 3 lines

Documentation/usb/URB.txt +3 -0

+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.

Documentation/usb/URB.txt have transferred successfully before the completion was called. · line 221

Quoted 5 lines

Documentation/usb/URB.txt +4 -0

+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.

Documentation/usb/URB.txt have transferred successfully before the completion was called. · line 224

Quoted 3 lines

Documentation/usb/URB.txt +2 -0

+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.

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

Quoted 4 lines · 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().

Quoted 8 lines · Alan Stern

Quoted 4 lines · 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.

Quoted 1 line

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.

Quoted 11 lines · Ming Lei

Quoted 8 lines · Alan Stern

Quoted 4 lines · 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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 3 lines

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.

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.

Quoted 14 lines · 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.

Quoted 2 lines

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.

Quoted 2 lines

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

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 5 lines · Ming Lei

Quoted 2 lines

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, :-)

Quoted 4 lines

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.

Quoted 7 lines

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

Quoted 5 lines

Quoted 1 line

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-sched.c +10 -0

@@ -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

Quoted 3 lines · 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.

Quoted 6 lines

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?

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

Also the async QH unlink approach has another disadvantage:

- if there are several QHs which are become empty during one wait period, the hrtimer has to be scheduled several times for starting unlink one qh each time.

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

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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

drivers/usb/host/ehci-hcd.c static void end_unlink_async(struct ehci_hcd *ehci); · line 308

Quoted 12 lines

drivers/usb/host/ehci-hcd.c +2 -1

index 246e124..35b4148 100644
@@ -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.

Quoted 8 lines

drivers/usb/host/ehci-sched.c +0 -0

index acff5b8..5dfda56 100644
@@ -548,6 +548,9 @@ static void qh_link_periodic(struct ehci_hcd
 *ehci, struct ehci_qh *qh)
 

drivers/usb/host/ehci-sched.c static void qh_link_periodic(struct ehci_hcd · line 553

Quoted 4 lines

drivers/usb/host/ehci-sched.c +2 -0

      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.

drivers/usb/host/ehci-sched.c static void ehci_enable_event(struct ehci_hcd · line 107

Quoted 11 lines

drivers/usb/host/ehci-sched.c +5 -0

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

drivers/usb/host/ehci-sched.c static void ehci_enable_event(struct ehci_hcd · line 110

Quoted 4 lines

drivers/usb/host/ehci-sched.c +3 -0

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

Here you need to add:

                ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;

drivers/usb/host/ehci-sched.c static void ehci_enable_event(struct ehci_hcd · line 112

Quoted 3 lines

drivers/usb/host/ehci-sched.c +2 -0

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

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

Quoted 21 lines

drivers/usb/host/ehci-sched.c +15 -0

@@ -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;
+

drivers/usb/host/ehci-sched.c static void ehci_handle_controller_death(struct · line 250

Quoted 5 lines

drivers/usb/host/ehci-sched.c +5 -0

+             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.

From below test results on 3 machines(2 ARM and one x86), time

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

Quoted 1 line · 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_?