[CC-ing the public mailing list and a few interested parties]
On Wed, 11 Jul 2012, Len Brown wrote:
> Hello Alan,
> I'm struggling with a system that consumes 20% more power when a USB keyboard is plugged in.
>
> Can you help me?
>
> thanks,
> -Len
> ----
>
> I find that even if no keyboard is plugged in, that I have to issue this
> command to drop power to 72 Watts from 88 Watts
>
> # echo auto > /sys/bus/usb/devices/2-1.4/power/control
>
> I find that if I have a physical keyboard connected,
> it is impossible to save this 16 Watts. (at least with
> the 3 keyboards that I've tried). Unplugging it is the only
> way to save that power.
I can't tell exactly what the problem is, but testing has revealed
several bugs in the usbhid driver related to autosuspend. The patch
below should fix them. (The biggest bug was that the gets and puts
weren't balanced, but there were others.)
Note that you'll still have to use that "echo auto ..." command before
anything will happen, because autosuspend is disabled by default for
most USB devices (including keyboards).
Also, you'll have to make sure that either all the keyboard LEDs are
off or else you modprobe usbhid with the ignoreled=1 option.
Jiri and Oliver: What do you think of the patch? I can break it up
into several pieces, each making a single change.
Alan Stern
Index: usb-3.5/drivers/hid/usbhid/hid-core.c
===================================================================
--- usb-3.5.orig/drivers/hid/usbhid/hid-core.c
+++ usb-3.5/drivers/hid/usbhid/hid-core.c
@@ -213,9 +213,20 @@ static int usbhid_restart_out_queue(stru
if ((kicked = (usbhid->outhead != usbhid->outtail))) {
hid_dbg(hid, "Kicking head %d tail %d", usbhid->outhead, usbhid->outtail);
+ /* Try to wake up from autosuspend... */
r = usb_autopm_get_interface_async(usbhid->intf);
if (r < 0)
return r;
+
+ /*
+ * If still suspended, don't submit. Submission will
+ * occur if/when resume drains the queue.
+ */
+ if (test_bit(HID_REPORTED_IDLE, &usbhid->iofl)) {
+ usb_autopm_put_interface_no_suspend(usbhid->intf);
+ return r;
+ }
+
/* Asynchronously flush queue. */
set_bit(HID_OUT_RUNNING, &usbhid->iofl);
if (hid_submit_out(hid)) {
@@ -240,9 +251,20 @@ static int usbhid_restart_ctrl_queue(str
if ((kicked = (usbhid->ctrlhead != usbhid->ctrltail))) {
hid_dbg(hid, "Kicking head %d tail %d", usbhid->ctrlhead, usbhid->ctrltail);
+ /* Try to wake up from autosuspend... */
r = usb_autopm_get_interface_async(usbhid->intf);
if (r < 0)
return r;
+
+ /*
+ * If still suspended, don't submit. Submission will
+ * occur if/when resume drains the queue.
+ */
+ if (test_bit(HID_REPORTED_IDLE, &usbhid->iofl)) {
+ usb_autopm_put_interface_no_suspend(usbhid->intf);
+ return r;
+ }
+
/* Asynchronously flush queue. */
set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
if (hid_submit_ctrl(hid)) {
@@ -331,9 +353,12 @@ static int hid_submit_out(struct hid_dev
usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) +
1 + (report->id > 0);
usbhid->urbout->dev = hid_to_usb_dev(hid);
- memcpy(usbhid->outbuf, raw_report,
- usbhid->urbout->transfer_buffer_length);
- kfree(raw_report);
+ if (raw_report) {
+ memcpy(usbhid->outbuf, raw_report,
+ usbhid->urbout->transfer_buffer_length);
+ kfree(raw_report);
+ usbhid->out[usbhid->outtail].raw_report = NULL;
+ }
dbg_hid("submitting out urb\n");
@@ -362,8 +387,11 @@ static int hid_submit_ctrl(struct hid_de
if (dir == USB_DIR_OUT) {
usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
usbhid->urbctrl->transfer_buffer_length = len;
- memcpy(usbhid->ctrlbuf, raw_report, len);
- kfree(raw_report);
+ if (raw_report) {
+ memcpy(usbhid->ctrlbuf, raw_report, len);
+ kfree(raw_report);
+ usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
+ }
} else {
int maxpacket, padlen;
@@ -407,16 +435,6 @@ static int hid_submit_ctrl(struct hid_de
* Output interrupt completion handler.
*/
-static int irq_out_pump_restart(struct hid_device *hid)
-{
- struct usbhid_device *usbhid = hid->driver_data;
-
- if (usbhid->outhead != usbhid->outtail)
- return hid_submit_out(hid);
- else
- return -1;
-}
-
static void hid_irq_out(struct urb *urb)
{
struct hid_device *hid = urb->context;
@@ -443,13 +461,14 @@ static void hid_irq_out(struct urb *urb)
if (unplug)
usbhid->outtail = usbhid->outhead;
- else
+ else {
usbhid->outtail = (usbhid->outtail + 1) & (HID_OUTPUT_FIFO_SIZE - 1);
- if (!irq_out_pump_restart(hid)) {
- /* Successfully submitted next urb in queue */
- spin_unlock_irqrestore(&usbhid->lock, flags);
- return;
+ if (usbhid->outhead != usbhid->outtail && hid_submit_out(hid) == 0) {
+ /* Successfully submitted next urb in queue */
+ spin_unlock_irqrestore(&usbhid->lock, flags);
+ return;
+ }
}
clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
@@ -461,15 +480,6 @@ static void hid_irq_out(struct urb *urb)
/*
* Control pipe completion handler.
*/
-static int ctrl_pump_restart(struct hid_device *hid)
-{
- struct usbhid_device *usbhid = hid->driver_data;
-
- if (usbhid->ctrlhead != usbhid->ctrltail)
- return hid_submit_ctrl(hid);
- else
- return -1;
-}
static void hid_ctrl(struct urb *urb)
{
@@ -500,13 +510,14 @@ static void hid_ctrl(struct urb *urb)
if (unplug)
usbhid->ctrltail = usbhid->ctrlhead;
- else
+ else {
usbhid->ctrltail = (usbhid->ctrltail + 1) & (HID_CONTROL_FIFO_SIZE - 1);
- if (!ctrl_pump_restart(hid)) {
- /* Successfully submitted next urb in queue */
- spin_unlock(&usbhid->lock);
- return;
+ if (usbhid->ctrlhead != usbhid->ctrltail && hid_submit_ctrl(hid) == 0) {
+ /* Successfully submitted next urb in queue */
+ spin_unlock(&usbhid->lock);
+ return;
+ }
}
clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
@@ -540,49 +551,29 @@ static void __usbhid_submit_report(struc
usbhid->out[usbhid->outhead].report = report;
usbhid->outhead = head;
- /* Try to awake from autosuspend... */
- if (usb_autopm_get_interface_async(usbhid->intf) < 0)
- return;
+ /* If the queue isn't running, restart it */
+ if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
+ usbhid_restart_out_queue(usbhid);
- /*
- * But if still suspended, leave urb enqueued, don't submit.
- * Submission will occur if/when resume() drains the queue.
- */
- if (test_bit(HID_REPORTED_IDLE, &usbhid->iofl))
- return;
+ /* Otherwise see if an earlier request has timed out */
+ } else if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
- if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
- if (hid_submit_out(hid)) {
- clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
- usb_autopm_put_interface_async(usbhid->intf);
- }
- wake_up(&usbhid->wait);
- } else {
- /*
- * the queue is known to run
- * but an earlier request may be stuck
- * we may need to time out
- * no race because the URB is blocked under
- * spinlock
- */
- if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
- usb_block_urb(usbhid->urbout);
- /* drop lock to not deadlock if the callback is called */
- spin_unlock(&usbhid->lock);
- usb_unlink_urb(usbhid->urbout);
- spin_lock(&usbhid->lock);
- usb_unblock_urb(usbhid->urbout);
- /*
- * if the unlinking has already completed
- * the pump will have been stopped
- * it must be restarted now
- */
- if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
- if (!irq_out_pump_restart(hid))
- set_bit(HID_OUT_RUNNING, &usbhid->iofl);
+ /* If the URB is about to finish, don't autosuspend */
+ usb_autopm_get_interface_no_resume(usbhid->intf);
+ /* drop lock to not deadlock if the callback is called */
+ usb_block_urb(usbhid->urbout);
+ spin_unlock(&usbhid->lock);
+ usb_unlink_urb(usbhid->urbout);
+ spin_lock(&usbhid->lock);
+ usb_unblock_urb(usbhid->urbout);
- }
+ /* If the unlink completed, the queue must be restarted */
+ if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
+ usbhid_restart_out_queue(usbhid);
+
+ /* Now we can allow autosuspend again */
+ usb_autopm_put_interface_async(usbhid->intf);
}
return;
}
@@ -604,47 +595,29 @@ static void __usbhid_submit_report(struc
usbhid->ctrl[usbhid->ctrlhead].dir = dir;
usbhid->ctrlhead = head;
- /* Try to awake from autosuspend... */
- if (usb_autopm_get_interface_async(usbhid->intf) < 0)
- return;
+ /* If the queue isn't running, restart it */
+ if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl)) {
+ usbhid_restart_ctrl_queue(usbhid);
- /*
- * If already suspended, leave urb enqueued, but don't submit.
- * Submission will occur if/when resume() drains the queue.
- */
- if (test_bit(HID_REPORTED_IDLE, &usbhid->iofl))
- return;
+ /* Otherwise see if an earlier request has timed out */
+ } else if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
- if (!test_and_set_bit(HID_CTRL_RUNNING, &usbhid->iofl)) {
- if (hid_submit_ctrl(hid)) {
- clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
- usb_autopm_put_interface_async(usbhid->intf);
- }
- wake_up(&usbhid->wait);
- } else {
- /*
- * the queue is known to run
- * but an earlier request may be stuck
- * we may need to time out
- * no race because the URB is blocked under
- * spinlock
- */
- if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
- usb_block_urb(usbhid->urbctrl);
- /* drop lock to not deadlock if the callback is called */
- spin_unlock(&usbhid->lock);
- usb_unlink_urb(usbhid->urbctrl);
- spin_lock(&usbhid->lock);
- usb_unblock_urb(usbhid->urbctrl);
- /*
- * if the unlinking has already completed
- * the pump will have been stopped
- * it must be restarted now
- */
- if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
- if (!ctrl_pump_restart(hid))
- set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
- }
+ /* If the URB is about to finish, don't autosuspend */
+ usb_autopm_get_interface_no_resume(usbhid->intf);
+
+ /* drop lock to not deadlock if the callback is called */
+ usb_block_urb(usbhid->urbctrl);
+ spin_unlock(&usbhid->lock);
+ usb_unlink_urb(usbhid->urbctrl);
+ spin_lock(&usbhid->lock);
+ usb_unblock_urb(usbhid->urbctrl);
+
+ /* If the unlink completed, the queue must be restarted */
+ if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
+ usbhid_restart_ctrl_queue(usbhid);
+
+ /* Now we can allow autosuspend again */
+ usb_autopm_put_interface_async(usbhid->intf);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
