On Tue, 1 Apr 2008, Oliver Neukum wrote:
> this is a full implementation of autosuspend for USB HID devices.
> Contrary to the current implementation it tries to safe power even for
> open devices, like keyboards and mice. Please test.
Hi Oliver,
there is some deadlock in IRQ path, could you please look at it? I have
triggered it on my test system trivially by just attaching one PS2 and one
USB keyboard to the system, fiddling a bit with leds on PS2 keyboard and
then tried to perform remote-wakeup on the suspended USB keyboard.
Unfortunately I won't probably have time to debug this today.
Other than that, some trivial comments below.
BUG: spinlock cpu recursion on CPU#0, ksoftirqd/0/4
lock: ffff81001d8796a0, .magic: dead4ead, .owner: swapper/0, .owner_cpu:
0
Pid: 4, comm: ksoftirqd/0 Not tainted
2.6.25-rc7-default-00002-g9e50656-dirty #24
Call Trace:
[<ffffffff802fd0ca>] spin_bug+0xa2/0xaa
[<ffffffff802fd25d>] _raw_spin_lock+0x5d/0x125
[<ffffffff80426bdb>] _spin_lock_irqsave+0x34/0x3c
[<ffffffff8038e601>] input_event+0x3b/0x77
[<ffffffff880fc838>] :hid:hidinput_hid_event+0x2da/0x30a
[<ffffffff880f880e>] :hid:hid_process_event+0x44/0x78
[<ffffffff880f8a99>] :hid:hid_input_field+0x257/0x268
[<ffffffff880f8c94>] :hid:hid_input_report+0x1ea/0x223
[<ffffffff8810e309>] :usbhid:hid_irq_in+0xb9/0x1e4
[<ffffffff80370693>] usb_hcd_giveback_urb+0x84/0xb7
[<ffffffff80386f0c>] uhci_giveback_urb+0x10a/0x19e
[<ffffffff8038763f>] uhci_scan_schedule+0x58a/0x83f
[<ffffffff80389586>] uhci_irq+0x128/0x144
[<ffffffff803709be>] usb_hcd_irq+0x2b/0x5b
[<ffffffff8025e2bf>] handle_IRQ_event+0x20/0x55
[<ffffffff8025f673>] handle_fasteoi_irq+0x9d/0xdd
[<ffffffff8020ecb2>] do_IRQ+0x6d/0xd7
[<ffffffff8020c556>] ret_from_intr+0x0/0xf
[<ffffffff80231e6f>] ? __do_softirq+0x65/0xf1
[<ffffffff80231e67>] ? __do_softirq+0x5d/0xf1
[<ffffffff80231aa9>] ? ksoftirqd+0x0/0xa5
[<ffffffff8020d25c>] ? call_softirq+0x1c/0x28
[<ffffffff8020ebca>] ? do_softirq+0x39/0x8a
[<ffffffff80231aec>] ? ksoftirqd+0x43/0xa5
[<ffffffff8023fe10>] ? kthread+0x49/0x79
[<ffffffff8020cee8>] ? child_rip+0xa/0x12
[<ffffffff8020c5ff>] ? restore_args+0x0/0x30
[<ffffffff8023fc7b>] ? kthreadd+0x17b/0x1a0
[<ffffffff8023fdc7>] ? kthread+0x0/0x79
[<ffffffff8020cede>] ? child_rip+0x0/0x12
> --- linux-2.6.25-rc7-vanilla/drivers/hid/hid-core.c 2008-03-31 15:20:58.000000000 +0200
> +++ linux-2.6.25-rc7-work/drivers/hid/hid-core.c 2008-04-01 11:48:52.000000000 +0200
> @@ -1001,6 +1001,24 @@ int hid_input_report(struct hid_device *
> }
> EXPORT_SYMBOL_GPL(hid_input_report);
> +#define NBITS(x) ((((x)-1)/BITS_PER_LONG)+1)
Why is this needed (and it is definitely not nice to have it in the middle
of the file). We already have the same macro in input.h, and we include it
from this source anyway, don't we?
> --- linux-2.6.25-rc7-vanilla/drivers/hid/usbhid/hid-core.c 2008-04-01 15:52:10.000000000 +0200
> +++ linux-2.6.25-rc7-work/drivers/hid/usbhid/hid-core.c 2008-04-01 11:21:07.000000000 +0200
> @@ -5,6 +5,7 @@
> * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@xxxxxxx>
> * Copyright (c) 2005 Michael Haboustak <mike-@xxxxxxxxxxxx> for Concept2, Inc
> * Copyright (c) 2006-2007 Jiri Kosina
> + * Copyright (c) 2007 Oliver Neukum
It's 2008 these days already :)
> + /* autosuspend refused while keys are pressed*/
> + if (hid_check_keys_pressed(hid))
> + set_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> + else
> + clear_bit(HID_KEYS_PRESSED, &usbhid->iofl);
Maybe we will have to make this more general one day? I think it makes
more sense to disable autosuspend whenever any event having EV_REP set is
happening. But as this currently equals to keys, that should be probably
OK for now.
> + spin_lock_irqsave(&usbhid->outputlock, flags);
> + /* suspension will switch off LEDs, so count them */
> + if (value) {
> + /* increase pm counter */
> + } else {
Some code missing here?
> + if (udev->auto_pm && test_bit(HID_KEYS_PRESSED, &usbhid->iofl)) {
> + /* lost race against keypresses */
> +printk(KERN_ERR"lost race against keypresses.\n");
I guess this printk() should go away, right? I don't think anyone is
interested to know whether this happened, right?
> @@ -70,14 +73,14 @@ struct usbhid_device {
> unsigned char outhead, outtail; /* Output pipe fifo head & tail */
> char *outbuf; /* Output buffer */
> dma_addr_t outbuf_dma; /* Output buffer dma */
> - spinlock_t outlock; /* Output fifo spinlock */
> + spinlock_t outputlock; /* Output fifo spinlock */
Shouldn't ctrllock be removed as well?
Also, I think that a little bit more verbose changelog would be helpful
(i.e. short description what is changed and how), as the patch is not
completely trivial.
Thanks,
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
