Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Oliver Neukum wrote: I grabbed a random HUB (usbhub4c from Linksys) and this made it work nicely even on UHCI-based system I am testing on. Is it a 1.1 hub or a 2.0 hub? 1.1, the device is still handled by uhci_hcd. -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
Am Montag, 25. Juni 2007 schrieb Jiri Kosina: On Fri, 22 Jun 2007, Oliver Neukum wrote: could you please run two tests? 1. set the autosuspend timeout to 0 (this'll kill usb mice) And it kills also my testing keyboard on the UHCI system. After the keyboard gets suspended and I hit a key, it wakes up (the LEDs come up), but no keypressess are produced and the keyboard gets suspended again. I had a hunch that UHCI handles remote wakeup incorrectly while the hub above the device is still active. This seems to kill that theory. 2. use a 1.1 hub I grabbed a random HUB (usbhub4c from Linksys) and this made it work nicely even on UHCI-based system I am testing on. I will do some more debugging to check what exactly goes wrong, but I am leaving for OLS tomorrow. Is it a 1.1 hub or a 2.0 hub? If the latter you just switch the device to ehci. Did you unload ehci_hcd for the test? BTW I don't know if you recall - I reported previously that the keypresses are lost only if I try to hit the key very soon after the keyboard gets suspended. If I wait for 2 seconds (looks like exact value), then no keypressess are lost and the keyboard wakes up properly. Alan, is there something in UHCI that uses that 2 second value? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Fri, 22 Jun 2007, Oliver Neukum wrote: could you please run two tests? 1. set the autosuspend timeout to 0 (this'll kill usb mice) And it kills also my testing keyboard on the UHCI system. After the keyboard gets suspended and I hit a key, it wakes up (the LEDs come up), but no keypressess are produced and the keyboard gets suspended again. 2. use a 1.1 hub I grabbed a random HUB (usbhub4c from Linksys) and this made it work nicely even on UHCI-based system I am testing on. I will do some more debugging to check what exactly goes wrong, but I am leaving for OLS tomorrow. BTW I don't know if you recall - I reported previously that the keypresses are lost only if I try to hit the key very soon after the keyboard gets suspended. If I wait for 2 seconds (looks like exact value), then no keypressess are lost and the keyboard wakes up properly. When I change the autosuspend values of all devices in system from 2 (default) to 5, the value described in the previous paragraph (i.e. the minimum time for which the keyboard must be suspended before it could be woken up flawlessly) is still 2 seconds. -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
Am Montag, 25. Juni 2007 schrieb Jiri Kosina: On Mon, 25 Jun 2007, Oliver Neukum wrote: I grabbed a random HUB (usbhub4c from Linksys) and this made it work nicely even on UHCI-based system I am testing on. Is it a 1.1 hub or a 2.0 hub? 1.1, the device is still handled by uhci_hcd. That indicates that something's wrong in uhci's root hub code. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Oliver Neukum wrote: 1.1, the device is still handled by uhci_hcd. That indicates that something's wrong in uhci's root hub code. Just for completness, below is what dev_dbg gives. This is the case which is OK - I turn the autosuspend on, let the keyboard suspend, wait for more than 2 seconds, hit a few keys, everything works: Jun 25 16:36:14 box kernel: uhci_hcd :00:1d.2: release dev 6 ep81-INT, period 8, phase 4, 118 us Jun 25 16:36:14 box kernel: usbhid 4-2:1.0: suspend Jun 25 16:36:14 box kernel: uhci_hcd :00:1d.2: release dev 6 ep82-INT, period 8, phase 4, 118 us Jun 25 16:36:14 box kernel: usbhid 4-2:1.1: suspend Jun 25 16:36:14 box kernel: usb 4-2: usb auto-suspend Jun 25 16:36:16 box kernel: hub 4-0:1.0: hub_suspend Jun 25 16:36:16 box kernel: usb usb4: suspend_rh Jun 25 16:36:16 box kernel: usb usb4: usb auto-suspend Jun 25 16:36:20 box kernel: usb usb4: usb resume Jun 25 16:36:20 box kernel: usb usb4: finish resume Jun 25 16:36:20 box kernel: hub 4-0:1.0: hub_resume Jun 25 16:36:20 box kernel: usb usb4: wakeup_rh Jun 25 16:36:20 box kernel: hub 4-0:1.0: state 7 ports 2 chg evt Jun 25 16:36:20 box kernel: uhci_hcd :00:1d.2: port 2 portsc 01a5,01 Jun 25 16:36:20 box kernel: usb 4-2: usb wakeup-resume Jun 25 16:36:20 box kernel: usb 4-2: finish resume Jun 25 16:36:20 box kernel: uhci_hcd :00:1d.2: reserve dev 6 ep81-INT, period 8, phase 4, 118 us Jun 25 16:36:20 box kernel: usbhid 4-2:1.0: resume status 0 Jun 25 16:36:20 box kernel: uhci_hcd :00:1d.2: reserve dev 6 ep82-INT, period 8, phase 4, 118 us Jun 25 16:36:20 box kernel: usbhid 4-2:1.1: resume status 0 Jun 25 16:36:20 box kernel: hub 4-0:1.0: resume on port 2, status 0 this is the failing case (i.e. hitting the keys without waiting two seconds after the keyboard is suspended): Jun 25 16:37:08 box kernel: uhci_hcd :00:1d.2: release dev 6 ep81-INT, period 8, phase 4, 118 us Jun 25 16:37:08 box kernel: usbhid 4-2:1.0: suspend Jun 25 16:37:08 box kernel: uhci_hcd :00:1d.2: release dev 6 ep82-INT, period 8, phase 4, 118 us Jun 25 16:37:08 box kernel: usbhid 4-2:1.1: suspend Jun 25 16:37:08 box kernel: usb 4-2: usb auto-suspend Jun 25 16:37:09 box kernel: hub 4-0:1.0: state 7 ports 2 chg evt 0004 Jun 25 16:37:09 box kernel: uhci_hcd :00:1d.2: port 2 portsc 0195,01 Jun 25 16:37:09 box kernel: usb 4-2: usb wakeup-resume Jun 25 16:37:09 box kernel: usb 4-2: finish resume Jun 25 16:37:09 box kernel: uhci_hcd :00:1d.2: reserve dev 6 ep81-INT, period 8, phase 4, 118 us Jun 25 16:37:09 box kernel: usbhid 4-2:1.0: resume status 0 Jun 25 16:37:09 box kernel: uhci_hcd :00:1d.2: reserve dev 6 ep82-INT, period 8, phase 4, 118 us Jun 25 16:37:09 box kernel: usbhid 4-2:1.1: resume status 0 Jun 25 16:37:09 box kernel: hub 4-0:1.0: resume on port 2, status 0 Jun 25 16:37:11 box kernel: uhci_hcd :00:1d.2: release dev 6 ep81-INT, period 8, phase 4, 118 us Jun 25 16:37:11 box kernel: usbhid 4-2:1.0: suspend Jun 25 16:37:11 box kernel: uhci_hcd :00:1d.2: release dev 6 ep82-INT, period 8, phase 4, 118 us Jun 25 16:37:11 box kernel: usbhid 4-2:1.1: suspend Jun 25 16:37:11 box kernel: usb 4-2: usb auto-suspend i.e. when the HUB is also suspended and then woken up, everything looks OK, but if it is only the device, without root hub going to suspend, it doesn't work. Alan, could this possibly be some race in uhci hub suspend handling (i.e. when the keyboard tries to resume while the root hub is trying to go to suspend, or something like that). -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Oliver Neukum wrote: Am Montag, 25. Juni 2007 schrieb Jiri Kosina: On Fri, 22 Jun 2007, Oliver Neukum wrote: could you please run two tests? 1. set the autosuspend timeout to 0 (this'll kill usb mice) And it kills also my testing keyboard on the UHCI system. After the keyboard gets suspended and I hit a key, it wakes up (the LEDs come up), but no keypressess are produced and the keyboard gets suspended again. I had a hunch that UHCI handles remote wakeup incorrectly while the hub above the device is still active. This seems to kill that theory. Setting the timeout to 0 would cause lots of weird things to happen. The driver might not even have time to query the device before it gets suspended again! 2. use a 1.1 hub I grabbed a random HUB (usbhub4c from Linksys) and this made it work nicely even on UHCI-based system I am testing on. I will do some more debugging to check what exactly goes wrong, but I am leaving for OLS tomorrow. Is it a 1.1 hub or a 2.0 hub? If the latter you just switch the device to ehci. Did you unload ehci_hcd for the test? BTW I don't know if you recall - I reported previously that the keypresses are lost only if I try to hit the key very soon after the keyboard gets suspended. If I wait for 2 seconds (looks like exact value), then no keypressess are lost and the keyboard wakes up properly. Alan, is there something in UHCI that uses that 2 second value? No. That indicates that something's wrong in uhci's root hub code. Could well be. I'll try duplicating the experiment: suspend the keyboard and less than 2 seconds later type some keys. I don't have the HID-autosuspend patch installed, but a manual suspend with remote wakeup should work pretty much the same. Jiri, I'll look for you at OLS. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Alan Stern wrote:
That indicates that something's wrong in uhci's root hub code.
Could well be. I'll try duplicating the experiment: suspend the
keyboard and less than 2 seconds later type some keys. I don't have the
HID-autosuspend patch installed, but a manual suspend with remote wakeup
should work pretty much the same.
It probably should, but just in case you are not able to reproduce the
problem in this way, I am attaching the Oliver's condensated patches I am
experimenting with, as one bigger patch below. It's based on 2.6.22-rc2.
Jiri, I'll look for you at OLS.
Great, I'll be having uhci-based notebook with me, and could easily bring
a tiny portable keyboard that reproduces the problem, so we could try to
figure out, if there is time. Thanks.
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 7f81789..3c5e7ba 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -940,6 +940,17 @@ int hidinput_find_field(struct hid_device *hid, unsigned
int type, unsigned int
}
EXPORT_SYMBOL_GPL(hidinput_find_field);
+int hidinput_has_ff(struct hid_device *hid)
+{
+ struct hid_input *hidinput;
+
+ list_for_each_entry(hidinput, hid-inputs, list)
+ if (test_bit(EV_FF, hidinput-input-evbit))
+ return 1;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(hidinput_has_ff);
+
static int hidinput_open(struct input_dev *dev)
{
struct hid_device *hid = input_get_drvdata(dev);
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index d91b9da..f646a9c 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -5,6 +5,7 @@
* Copyright (c) 2000-2005 Vojtech Pavlik [EMAIL PROTECTED]
* Copyright (c) 2005 Michael Haboustak [EMAIL PROTECTED] for Concept2, Inc
* Copyright (c) 2006-2007 Jiri Kosina
+ * Copyright (c) 2007 Oliver Neukum
*/
/*
@@ -26,6 +27,7 @@
#include asm/byteorder.h
#include linux/input.h
#include linux/wait.h
+#include linux/workqueue.h
#include linux/usb.h
@@ -63,8 +65,12 @@ MODULE_PARM_DESC(quirks, Add/modify USB HID quirks by
specifying
/*
* Input submission and I/O error handler.
*/
+static DEFINE_MUTEX(hid_open_mut);
+static struct workqueue_struct *resumption_waker;
static void hid_io_error(struct hid_device *hid);
+static int hid_submit_out(struct hid_device *hid);
+static int hid_submit_ctrl(struct hid_device *hid);
/* Start up the input URB */
static int hid_start_in(struct hid_device *hid)
@@ -74,7 +80,7 @@ static int hid_start_in(struct hid_device *hid)
struct usbhid_device *usbhid = hid-driver_data;
spin_lock_irqsave(usbhid-inlock, flags);
- if (hid-open 0 !test_bit(HID_SUSPENDED, usbhid-iofl)
+ if (hid-open 0 !test_bit(HID_REPORTED_IDLE, usbhid-iofl)
!test_and_set_bit(HID_IN_RUNNING, usbhid-iofl)) {
rc = usb_submit_urb(usbhid-urbin, GFP_ATOMIC);
if (rc != 0)
@@ -178,6 +184,50 @@ done:
spin_unlock_irqrestore(usbhid-inlock, flags);
}
+static void usbhid_mark_busy(struct usbhid_device *usbhid)
+{
+ struct usb_interface *intf = usbhid-intf;
+
+ usb_mark_last_busy(interface_to_usbdev(intf));
+}
+
+static int usbhid_restart_out_queue(struct usbhid_device *usbhid)
+{
+ struct hid_device *hid = usb_get_intfdata(usbhid-intf);
+ int kicked;
+
+ if (!hid)
+ return 0;
+
+ if ((kicked = (usbhid-outhead != usbhid-outtail))) {
+ dbg(Kicking head %d tail %d, usbhid-outhead,
usbhid-outtail);
+ if (hid_submit_out(hid)) {
+ clear_bit(HID_OUT_RUNNING, usbhid-iofl);
+ wake_up(hid-wait);
+ }
+ }
+ return kicked;
+}
+
+static int usbhid_restart_ctrl_queue(struct usbhid_device *usbhid)
+{
+ struct hid_device *hid = usb_get_intfdata(usbhid-intf);
+ int kicked;
+
+ WARN_ON(hid == NULL);
+ if (!hid)
+ return 0;
+
+ if ((kicked = (usbhid-ctrlhead != usbhid-ctrltail))) {
+ dbg(Kicking head %d tail %d, usbhid-ctrlhead,
usbhid-ctrltail);
+ if (hid_submit_ctrl(hid)) {
+ clear_bit(HID_CTRL_RUNNING, usbhid-iofl);
+ wake_up(hid-wait);
+ }
+ }
+ return kicked;
+}
+
/*
* Input interrupt completion handler.
*/
@@ -190,12 +240,14 @@ static void hid_irq_in(struct urb *urb)
switch (urb-status) {
case 0: /* success */
+ usbhid_mark_busy(usbhid);
usbhid-retry_delay = 0;
hid_input_report(urb-context, HID_INPUT_REPORT,
urb-transfer_buffer,
urb-actual_length, 1);
break;
case -EPIPE:/* stall */
+
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Jiri Kosina wrote: On Mon, 25 Jun 2007, Oliver Neukum wrote: 1.1, the device is still handled by uhci_hcd. That indicates that something's wrong in uhci's root hub code. Just for completness, below is what dev_dbg gives. This is the case which is OK - I turn the autosuspend on, let the keyboard suspend, wait for more than 2 seconds, hit a few keys, everything works: ... this is the failing case (i.e. hitting the keys without waiting two seconds after the keyboard is suspended): ... i.e. when the HUB is also suspended and then woken up, everything looks OK, but if it is only the device, without root hub going to suspend, it doesn't work. These logs look correct. The difference might have something to do with the timing of the USB messages relative to the keystrokes. Or maybe the keyboard itself has some weird 2-second timer. Alan, could this possibly be some race in uhci hub suspend handling (i.e. when the keyboard tries to resume while the root hub is trying to go to suspend, or something like that). I don't think so. These transitions are protected by mutexes. In case of a race like you suggest, either the keyboard would win and would resume, thereby preventing the root hub from suspending (which is what actually did occur in your second log), or else the root hub would win and would suspend, then immediately resume because of the wakeup request from the keyboard. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Alan Stern wrote: These logs look correct. The difference might have something to do with the timing of the USB messages relative to the keystrokes. Or maybe the keyboard itself has some weird 2-second timer. Hi Alan, thanks for looking at it. I have tried with two different keyboards, both behave in the very same way - i.e. on OHCI, or when connected through 1.1 hub to UHCI, everything works nicely. Only when connected directly to UHCI root hub, the keystrokes are lost if pressed within that 2-sec period, etc. I'd be inclined to rule out purely keyboard issue here. I'll put some printk()s into the uhci root hub code to understand better what is going on. If you have any idea, please let me know. Thanks, -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
Am Montag, 25. Juni 2007 schrieb Jiri Kosina: Just for completness, below is what dev_dbg gives. This is the case which is OK - I turn the autosuspend on, let the keyboard suspend, wait for more than 2 seconds, hit a few keys, everything works: What does the trace look like if you increase the autosuspend timeout and press a key after two seconds but while the root hub has not been suspended? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Jiri Kosina wrote: On Mon, 25 Jun 2007, Alan Stern wrote: These logs look correct. The difference might have something to do with the timing of the USB messages relative to the keystrokes. Or maybe the keyboard itself has some weird 2-second timer. Hi Alan, thanks for looking at it. I have tried with two different keyboards, both behave in the very same way - i.e. on OHCI, or when connected through 1.1 hub to UHCI, everything works nicely. Only when connected directly to UHCI root hub, the keystrokes are lost if pressed within that 2-sec period, etc. I'd be inclined to rule out purely keyboard issue here. I'll put some printk()s into the uhci root hub code to understand better what is going on. If you have any idea, please let me know. I tried with a keyboard here (manual suspend, not using Oliver's changes). The keyboard is different from yours becaue it has a built-in hub. Nevertheless, it almost always happens that keystrokes can get lost during a resume. It depends on how fast I type and which devices have been suspended: the keyboard along, the keyboard and its internal hub, or both plus the root hub. With usbmon it's possible to see exactly what packets are transferred. The packets used for doing the resume always follow the same pattern. The only difference is the Interrupt data carrying the keystroke information. The device simply does not send the data. It'll be a lot easier to show you all this in person. And maybe I'll see something new when trying out your keyboard. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Alan Stern wrote: With usbmon it's possible to see exactly what packets are transferred. The packets used for doing the resume always follow the same pattern. The only difference is the Interrupt data carrying the keystroke information. The device simply does not send the data. Do you see how could the fact that the device is connected either to ohci or uhci root hub influence whether the device itself does or does not send the data? It'll be a lot easier to show you all this in person. And maybe I'll see something new when trying out your keyboard. Sure, see you in Ottawa. Thanks, -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 25 Jun 2007, Jiri Kosina wrote: On Mon, 25 Jun 2007, Alan Stern wrote: With usbmon it's possible to see exactly what packets are transferred. The packets used for doing the resume always follow the same pattern. The only difference is the Interrupt data carrying the keystroke information. The device simply does not send the data. Do you see how could the fact that the device is connected either to ohci or uhci root hub influence whether the device itself does or does not send the data? The only affect should be in the timing of the packets, or their timing relative to the keystrokes. I'll try to do some more testing. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
I tried another test, this time using Oliver's patch. With both UHCI and OHCI controllers the results were about the same. Sometimes my keystrokes would be received and sometimes they wouldn't. It may have been connected with how quickly I typed, but there also appeared to be a certain degree of randomness. It seems likely that my keyboard, at least, is a little flakey. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
Am Freitag, 22. Juni 2007 schrieb Jiri Kosina: On Mon, 28 May 2007, Oliver Neukum wrote: Sure, it still could be a HW issue (I have experienced this with two random keyboards I used for testing), but I'd guess it would be something different than what you describe. What do you think? Have you varied the computer or only the keyboard? Hi Oliver, sorry for not touching this for quite some time. I have now tested with the very same keyboards on OHCI machine (I have seen the issues I reported in this thread on UHCI), and there seem to be no problems at all - the keyboard is always woken up immediately and no loss of keypressess happens. I will verify on more various systems whether it is really related only to UHCI, but so far it seems like the best guess. OK, could you please run two tests? 1. set the autosuspend timeout to 0 (this'll kill usb mice) 2. use a 1.1 hub Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 28 May 2007, Oliver Neukum wrote: Sure, it still could be a HW issue (I have experienced this with two random keyboards I used for testing), but I'd guess it would be something different than what you describe. What do you think? Have you varied the computer or only the keyboard? Hi Oliver, sorry for not touching this for quite some time. I have now tested with the very same keyboards on OHCI machine (I have seen the issues I reported in this thread on UHCI), and there seem to be no problems at all - the keyboard is always woken up immediately and no loss of keypressess happens. I will verify on more various systems whether it is really related only to UHCI, but so far it seems like the best guess. Thanks, -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #5
On Wed, 23 May 2007, Oliver Neukum wrote: del_timer_sync(usbhid-io_retry); + cancel_work_sync(usbhid-restart_work); flush_scheduled_work(); Hi Oliver, isn't the call to flush_scheduled_work() after cancel_work_sync() redundant? Also, we could very probably use the hotplug-safer cancel_work_sync() elsewhere in your patch where you introduce flush_scheduled_work(), right? Thanks, -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #5
Am Mittwoch, 30. Mai 2007 17:25 schrieb Jiri Kosina: On Wed, 23 May 2007, Oliver Neukum wrote: del_timer_sync(usbhid-io_retry); + cancel_work_sync(usbhid-restart_work); flush_scheduled_work(); Hi Oliver, isn't the call to flush_scheduled_work() after cancel_work_sync() redundant? No, we need to wait for the other works. Also, we could very probably use the hotplug-safer cancel_work_sync() elsewhere in your patch where you introduce flush_scheduled_work(), right? We certainly could. Do you think it could deadlock? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #5
Am Mittwoch, 30. Mai 2007 17:25 schrieb Jiri Kosina: On Wed, 23 May 2007, Oliver Neukum wrote: del_timer_sync(usbhid-io_retry); + cancel_work_sync(usbhid-restart_work); flush_scheduled_work(); Hi Oliver, isn't the call to flush_scheduled_work() after cancel_work_sync() redundant? Also, we could very probably use the hotplug-safer cancel_work_sync() elsewhere in your patch where you introduce flush_scheduled_work(), right? Alan's last mail has addressed the issue. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Wed, 23 May 2007, Alan Stern wrote: I suspect it is keyboard-dependent. For example, the keyboard's internal buffer might be able to hold no more than one event, because the designers expected the host to poll frequently. Since the polling can't occur during the wakeup interval, multiple events from that time will get lost. in such situation, I'd however expect the first N events to be lost, but the last events to arrive without problem. What I am experiencing, however, is that the missing events are usually the middle ones. It depends on how the buffer is implemented. It might work this way: The first keystroke is detected, goes into the buffer, and causes a remote wakeup request to be sent. Later keystrokes can't go into the buffer because the buffer is still full, so they are dropped. When the resume sequence is complete, the host reads the original keystroke and the buffer is once again empty. Hence any further keystrokes are processed as usual. Hi Alan, you are right, however there is still a reason I think that this is not the case. What I am seeing is that the keypresses are lost only if I hit a key (and thus wake the autosuspended keyboard up) only after a short time it goes to suspend. When I leave it autosuspended for 2 or more seconds (approximately), it behaves nicely. Sure, it still could be a HW issue (I have experienced this with two random keyboards I used for testing), but I'd guess it would be something different than what you describe. What do you think? Thanks, -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
Am Montag, 28. Mai 2007 16:37 schrieb Jiri Kosina: Sure, it still could be a HW issue (I have experienced this with two random keyboards I used for testing), but I'd guess it would be something different than what you describe. What do you think? Have you varied the computer or only the keyboard? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Mon, 28 May 2007, Jiri Kosina wrote: Hi Alan, you are right, however there is still a reason I think that this is not the case. What I am seeing is that the keypresses are lost only if I hit a key (and thus wake the autosuspended keyboard up) only after a short time it goes to suspend. When I leave it autosuspended for 2 or more seconds (approximately), it behaves nicely. Sure, it still could be a HW issue (I have experienced this with two random keyboards I used for testing), but I'd guess it would be something different than what you describe. What do you think? I don't know. Perhaps there's some strange interaction with the parent hub? It would get autosuspended 2 seconds after the keyboard, unless there was some other unsuspended USB device plugged into it. You can prevent the parent hub from being autosuspended by doing: echo on /sys/bus/usb/devices/D/power/level where D is the device name of the parent hub. Try various combinations to see what works. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
Am Dienstag, 22. Mai 2007 18:50 schrieb Jiri Kosina: I think that this is unfortunately not true. Let's take system with multiple keyboards and their LEDs as an excellent example again. If you kill the interrupt URB, there is nothing what will prevent other keyboard (PS/2 or even USB keyboard, which is not yet suspended) to generate an input event (user presses CapsLock/NumLock at a 'right' time). Then you'll get out of sync, won't you? Not only will you get out of sync, you'll activate DMA on a system going to snapshot or switching off DMA coherence. This box will suffer memory corruption. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
Am Dienstag, 22. Mai 2007 19:18 schrieb Alan Stern: On Tue, 22 May 2007, Oliver Neukum wrote: Yes, but if you are in pre_reset, chances are something is wrong with the devices. Outright slaughter of all outstanding URBs is better than waiting for them to complete. Fair enough. I wouldn't call post_reset broken, though. True. Here the difference lies only in updating last_busy. This is more a matter of making a clean distinction. In fact, failing a non-auto suspend is not a good idea. You could easily end up preventing somebody's laptop from hibernating when they close the lid and put it in their knapsack. Yes, and I don't. That's one reason I want to separate the auto and system cases. What happens if usbhid_wait_io() times out? That I treat as a genuine error because it should not happen. Jiri, what is your comment? Kill the URBs in that case? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
Am Dienstag, 22. Mai 2007 18:50 schrieb Jiri Kosina: On Tue, 22 May 2007, Alan Stern wrote: But if you kill the interrupt URB then there will be no more inputs and hence nothing to generate any more output. Thus, when suspending you should kill the inputs and wait for the outputs to drain (or else explicitly plug the output queue). Then it will be safe to autoresume whenever a new output queue entry arrives. Hi Alan, I think that this is unfortunately not true. Let's take system with multiple keyboards and their LEDs as an excellent example again. If you kill the interrupt URB, there is nothing what will prevent other keyboard (PS/2 or even USB keyboard, which is not yet suspended) to generate an input event (user presses CapsLock/NumLock at a 'right' time). Then you'll get out of sync, won't you? That gives me an idea. Resumption of a device has to be done in task context. So if I allocate a private freezable work queue for HID resumption, the freezer would guard against illicit resumptions, wouldn't it? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
[linux-usb-devel] autosuspend for HID devices, take #4
Hi,
this may be it.
open issues:
- waiting on Jiri's comment about what to do if waiting for io gets
a timeout during suspend()
Other than that it has all features I planned for and were requested.
It should work now. I am now taking comments on coding style :-)
I request testing and remind you that cruelty is a virtue in that regard.
Regards
Oliver
--- linux-2.6.22-rc2/include/linux/hid.h2007-05-22 14:50:58.0
+0200
+++ linux-2.6.22-rc2-autosuspend/include/linux/hid.h2007-05-22
15:04:41.0 +0200
@@ -401,6 +401,7 @@ struct hid_control_fifo {
#define HID_RESET_PENDING 4
#define HID_SUSPENDED 5
#define HID_CLEAR_HALT 6
+#define HID_REPORTED_IDLE 7
struct hid_input {
struct list_head list;
@@ -500,6 +501,7 @@ void hid_input_field(struct hid_device *
void hid_output_report(struct hid_report *report, __u8 *data);
void hid_free_device(struct hid_device *device);
struct hid_device *hid_parse_report(__u8 *start, unsigned size);
+int hidinput_has_ff(struct hid_device *hid);
/* HID quirks API */
u32 usbhid_lookup_quirk(const u16 idVendor, const u16 idProduct);
--- linux-2.6.22-rc2/drivers/hid/usbhid/usbhid.h2007-05-22
14:50:09.0 +0200
+++ linux-2.6.22-rc2-autosuspend/drivers/hid/usbhid/usbhid.h2007-05-22
11:15:44.0 +0200
@@ -36,7 +36,10 @@ int usbhid_wait_io(struct hid_device* hi
void usbhid_close(struct hid_device *hid);
int usbhid_open(struct hid_device *hid);
void usbhid_init_reports(struct hid_device *hid);
-void usbhid_submit_report(struct hid_device *hid, struct hid_report *report,
unsigned char dir);
+void usbhid_submit_report
+(struct hid_device *hid, struct hid_report *report, unsigned char dir);
+int usbhid_get_power(struct hid_device *hid);
+void usbhid_put_power(struct hid_device *hid);
/*
* USB-specific HID struct, to be pointed to
@@ -44,40 +47,69 @@ void usbhid_submit_report(struct hid_dev
*/
struct usbhid_device {
- struct hid_device *hid; /*
pointer to corresponding HID dev */
-
- struct usb_interface *intf; /* USB
interface */
- int ifnum; /* USB
interface number */
-
- unsigned int bufsize; /* URB
buffer size */
-
- struct urb *urbin; /*
Input URB */
- char *inbuf;/*
Input buffer */
- dma_addr_t inbuf_dma; /*
Input buffer dma */
- spinlock_t inlock; /*
Input fifo spinlock */
-
- struct urb *urbctrl;/*
Control URB */
- struct usb_ctrlrequest *cr; /*
Control request struct */
- dma_addr_t cr_dma; /*
Control request struct dma */
- struct hid_control_fifo ctrl[HID_CONTROL_FIFO_SIZE];/*
Control fifo */
- unsigned char ctrlhead, ctrltail; /*
Control fifo head tail */
- char *ctrlbuf; /*
Control buffer */
- dma_addr_t ctrlbuf_dma; /*
Control buffer dma */
- spinlock_t ctrllock;/*
Control fifo spinlock */
-
- struct urb *urbout; /*
Output URB */
- struct hid_report *out[HID_CONTROL_FIFO_SIZE]; /*
Output pipe fifo */
- 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 */
-
- unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
- struct timer_list io_retry; /*
Retry timer */
- unsigned long stop_retry; /* Time
to give up, in jiffies */
- unsigned int retry_delay; /*
Delay length in ms */
- struct work_struct reset_work; /* Task
context for resets */
+ /* pointer to corresponding HID dev */
+ struct hid_device *hid;
+ /* USB interface */
+ struct usb_interface *intf;
+ /* USB interface number */
+ int ifnum;
+
+ /* URB buffer size */
+ unsigned int bufsize;
+
+ /* Input URB */
+ struct urb *urbin;
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Wed, 23 May 2007, Oliver Neukum wrote: - waiting on Jiri's comment about what to do if waiting for io gets a timeout during suspend() Hi Oliver, I think that whenever usbhid_wait_io() times out during suspend, it is fine to kill the URB, as it doesn't seem to make any more harm compared to the situation when usbhid_wait_io() times out without suspend in progress. Other than that it has all features I planned for and were requested. It should work now. I am now taking comments on coding style :-) I request testing and remind you that cruelty is a virtue in that regard. I have quite a lot of things pending, but will test this ASAP. Thanks, -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
Am Mittwoch, 23. Mai 2007 13:36 schrieb Jiri Kosina:
On Wed, 23 May 2007, Oliver Neukum wrote:
- waiting on Jiri's comment about what to do if waiting for io gets
a timeout during suspend()
Hi Oliver,
I think that whenever usbhid_wait_io() times out during suspend, it is
fine to kill the URB, as it doesn't seem to make any more harm compared to
the situation when usbhid_wait_io() times out without suspend in progress.
Very well, this version does so.
Other than that it has all features I planned for and were requested. It
should work now. I am now taking comments on coding style :-) I request
testing and remind you that cruelty is a virtue in that regard.
I have quite a lot of things pending, but will test this ASAP.
I've tested normal operations on OHCI and EHCI and STR with a mouse,
a keyboard and a PID joystick (just plugged in)
Regards
Oliver
-
--- linux-2.6.22-rc2/include/linux/hid.h2007-05-22 14:50:58.0
+0200
+++ linux-2.6.22-rc2-autosuspend/include/linux/hid.h2007-05-22
15:04:41.0 +0200
@@ -401,6 +401,7 @@ struct hid_control_fifo {
#define HID_RESET_PENDING 4
#define HID_SUSPENDED 5
#define HID_CLEAR_HALT 6
+#define HID_REPORTED_IDLE 7
struct hid_input {
struct list_head list;
@@ -500,6 +501,7 @@ void hid_input_field(struct hid_device *
void hid_output_report(struct hid_report *report, __u8 *data);
void hid_free_device(struct hid_device *device);
struct hid_device *hid_parse_report(__u8 *start, unsigned size);
+int hidinput_has_ff(struct hid_device *hid);
/* HID quirks API */
u32 usbhid_lookup_quirk(const u16 idVendor, const u16 idProduct);
--- linux-2.6.22-rc2/drivers/hid/usbhid/usbhid.h2007-05-22
14:50:09.0 +0200
+++ linux-2.6.22-rc2-autosuspend/drivers/hid/usbhid/usbhid.h2007-05-22
11:15:44.0 +0200
@@ -36,7 +36,10 @@ int usbhid_wait_io(struct hid_device* hi
void usbhid_close(struct hid_device *hid);
int usbhid_open(struct hid_device *hid);
void usbhid_init_reports(struct hid_device *hid);
-void usbhid_submit_report(struct hid_device *hid, struct hid_report *report,
unsigned char dir);
+void usbhid_submit_report
+(struct hid_device *hid, struct hid_report *report, unsigned char dir);
+int usbhid_get_power(struct hid_device *hid);
+void usbhid_put_power(struct hid_device *hid);
/*
* USB-specific HID struct, to be pointed to
@@ -44,40 +47,69 @@ void usbhid_submit_report(struct hid_dev
*/
struct usbhid_device {
- struct hid_device *hid; /*
pointer to corresponding HID dev */
-
- struct usb_interface *intf; /* USB
interface */
- int ifnum; /* USB
interface number */
-
- unsigned int bufsize; /* URB
buffer size */
-
- struct urb *urbin; /*
Input URB */
- char *inbuf;/*
Input buffer */
- dma_addr_t inbuf_dma; /*
Input buffer dma */
- spinlock_t inlock; /*
Input fifo spinlock */
-
- struct urb *urbctrl;/*
Control URB */
- struct usb_ctrlrequest *cr; /*
Control request struct */
- dma_addr_t cr_dma; /*
Control request struct dma */
- struct hid_control_fifo ctrl[HID_CONTROL_FIFO_SIZE];/*
Control fifo */
- unsigned char ctrlhead, ctrltail; /*
Control fifo head tail */
- char *ctrlbuf; /*
Control buffer */
- dma_addr_t ctrlbuf_dma; /*
Control buffer dma */
- spinlock_t ctrllock;/*
Control fifo spinlock */
-
- struct urb *urbout; /*
Output URB */
- struct hid_report *out[HID_CONTROL_FIFO_SIZE]; /*
Output pipe fifo */
- 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 */
-
- unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
- struct timer_list io_retry; /*
Retry timer */
- unsigned long stop_retry; /* Time
to give
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Wed, 23 May 2007, Oliver Neukum wrote: I have quite a lot of things pending, but will test this ASAP. I've tested normal operations on OHCI and EHCI and STR with a mouse, a keyboard and a PID joystick (just plugged in) Hi Oliver, well, I remember to having this reported also against one of the very first versions of your patch - I still experience loss of events from the device that is being woken up shortly after it goes to suspend. I.e. as soon as the device goes to suspend, I type 'asdf' on the keyboard, it wakes up and reports only 'af' or something like that. It seems that this is not reproducible after some time (a few seconds) the device is suspended. Are you able to reproduce this? I will try to figure out what is going on. -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
Am Mittwoch, 23. Mai 2007 16:23 schrieb Jiri Kosina: On Wed, 23 May 2007, Oliver Neukum wrote: I have quite a lot of things pending, but will test this ASAP. I've tested normal operations on OHCI and EHCI and STR with a mouse, a keyboard and a PID joystick (just plugged in) Hi Oliver, well, I remember to having this reported also against one of the very first versions of your patch - I still experience loss of events from the device that is being woken up shortly after it goes to suspend. I.e. as soon as the device goes to suspend, I type 'asdf' on the keyboard, it wakes up and reports only 'af' or something like that. It seems that this is not reproducible after some time (a few seconds) the device is suspended. Are you able to reproduce this? I will try to figure out what is going on. No, sorry, but maybe I am just clumsy at hitting the keys at the correct time. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Wed, 23 May 2007, Oliver Neukum wrote: well, I remember to having this reported also against one of the very first versions of your patch - I still experience loss of events from the device that is being woken up shortly after it goes to suspend. I.e. as soon as the device goes to suspend, I type 'asdf' on the keyboard, it wakes up and reports only 'af' or something like that. It seems that this is not reproducible after some time (a few seconds) the device is suspended. Are you able to reproduce this? I will try to figure out what is going on. No, sorry, but maybe I am just clumsy at hitting the keys at the correct time. I hit the keys as soon as LED diods go off. Then the events get lost. If I wait for approximately two seconds, everything works fine (waiting one second is not enough, it still loses data). I will debug. Maybe just this particular keyboard is clumsy. I will try with various different hardware. But if there are lots of HID hardware out there which expose this broken behavior, it would be bad. -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
Am Mittwoch, 23. Mai 2007 16:37 schrieb Jiri Kosina: On Wed, 23 May 2007, Oliver Neukum wrote: well, I remember to having this reported also against one of the very first versions of your patch - I still experience loss of events from the device that is being woken up shortly after it goes to suspend. I.e. as soon as the device goes to suspend, I type 'asdf' on the keyboard, it wakes up and reports only 'af' or something like that. It seems that this is not reproducible after some time (a few seconds) the device is suspended. Are you able to reproduce this? I will try to figure out what is going on. No, sorry, but maybe I am just clumsy at hitting the keys at the correct time. I hit the keys as soon as LED diods go off. Then the events get lost. If I wait for approximately two seconds, everything works fine (waiting one second is not enough, it still loses data). An obvious method. :-) Sometimes I am stupid. I will debug. Maybe just this particular keyboard is clumsy. I will try with various different hardware. But if there are lots of HID hardware out there which expose this broken behavior, it would be bad. No, I cannot replicate this. I see all keys. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Wed, 23 May 2007, Oliver Neukum wrote: I will debug. Maybe just this particular keyboard is clumsy. I will try with various different hardware. But if there are lots of HID hardware out there which expose this broken behavior, it would be bad. No, I cannot replicate this. I see all keys. I suspect it is keyboard-dependent. For example, the keyboard's internal buffer might be able to hold no more than one event, because the designers expected the host to poll frequently. Since the polling can't occur during the wakeup interval, multiple events from that time will get lost. It wouldn't be surprising to find lots of USB HID devices suffering from this kind of problem. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
On Wed, 23 May 2007, Oliver Neukum wrote: That gives me an idea. Resumption of a device has to be done in task context. So if I allocate a private freezable work queue for HID resumption, the freezer would guard against illicit resumptions, wouldn't it? You can use the ksuspend_usb_wq workqueue. It already exists, is freezable, and is used for suspending and resuming USB devices. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
Am Mittwoch, 23. Mai 2007 17:42 schrieb Alan Stern: On Wed, 23 May 2007, Oliver Neukum wrote: That gives me an idea. Resumption of a device has to be done in task context. So if I allocate a private freezable work queue for HID resumption, the freezer would guard against illicit resumptions, wouldn't it? You can use the ksuspend_usb_wq workqueue. It already exists, is freezable, and is used for suspending and resuming USB devices. It shall be done. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Wed, 23 May 2007, Alan Stern wrote: I suspect it is keyboard-dependent. For example, the keyboard's internal buffer might be able to hold no more than one event, because the designers expected the host to poll frequently. Since the polling can't occur during the wakeup interval, multiple events from that time will get lost. Hi Alan, in such situation, I'd however expect the first N events to be lost, but the last events to arrive without problem. What I am experiencing, however, is that the missing events are usually the middle ones. I have in the meantime tested with another USB keyboard on the same system, still the very same behavior with lost keypresses. I will try the same keyboards on another system now, to verify whether it could be a USB hub's fault instead. It wouldn't be surprising to find lots of USB HID devices suffering from this kind of problem. Which unfortunately would render suspending them quite impossible, as losing keypresses this often is a big no-no :( I will work on this a little bit more. Thanks, -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #4
On Wed, 23 May 2007, Jiri Kosina wrote: On Wed, 23 May 2007, Alan Stern wrote: I suspect it is keyboard-dependent. For example, the keyboard's internal buffer might be able to hold no more than one event, because the designers expected the host to poll frequently. Since the polling can't occur during the wakeup interval, multiple events from that time will get lost. Hi Alan, in such situation, I'd however expect the first N events to be lost, but the last events to arrive without problem. What I am experiencing, however, is that the missing events are usually the middle ones. It depends on how the buffer is implemented. It might work this way: The first keystroke is detected, goes into the buffer, and causes a remote wakeup request to be sent. Later keystrokes can't go into the buffer because the buffer is still full, so they are dropped. When the resume sequence is complete, the host reads the original keystroke and the buffer is once again empty. Hence any further keystrokes are processed as usual. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
[linux-usb-devel] autosuspend for HID devices, take #5
Hi,
(on0001)
this version uses usbcore pm workqueue as Alan suggested. It is relative
to the last patch I sent, which I retroactively christen on.
Regards
Oliver
--- linux-2.6.22-rc2/include/linux/hid.h2007-05-22 14:50:58.0
+0200
+++ linux-2.6.22-rc2-autosuspend/include/linux/hid.h2007-05-22
15:04:41.0 +0200
@@ -401,6 +401,7 @@ struct hid_control_fifo {
#define HID_RESET_PENDING 4
#define HID_SUSPENDED 5
#define HID_CLEAR_HALT 6
+#define HID_REPORTED_IDLE 7
struct hid_input {
struct list_head list;
@@ -500,6 +501,7 @@ void hid_input_field(struct hid_device *
void hid_output_report(struct hid_report *report, __u8 *data);
void hid_free_device(struct hid_device *device);
struct hid_device *hid_parse_report(__u8 *start, unsigned size);
+int hidinput_has_ff(struct hid_device *hid);
/* HID quirks API */
u32 usbhid_lookup_quirk(const u16 idVendor, const u16 idProduct);
--- linux-2.6.22-rc2/drivers/hid/usbhid/usbhid.h2007-05-22
14:50:09.0 +0200
+++ linux-2.6.22-rc2-autosuspend/drivers/hid/usbhid/usbhid.h2007-05-22
11:15:44.0 +0200
@@ -36,7 +36,10 @@ int usbhid_wait_io(struct hid_device* hi
void usbhid_close(struct hid_device *hid);
int usbhid_open(struct hid_device *hid);
void usbhid_init_reports(struct hid_device *hid);
-void usbhid_submit_report(struct hid_device *hid, struct hid_report *report,
unsigned char dir);
+void usbhid_submit_report
+(struct hid_device *hid, struct hid_report *report, unsigned char dir);
+int usbhid_get_power(struct hid_device *hid);
+void usbhid_put_power(struct hid_device *hid);
/*
* USB-specific HID struct, to be pointed to
@@ -44,40 +47,69 @@ void usbhid_submit_report(struct hid_dev
*/
struct usbhid_device {
- struct hid_device *hid; /*
pointer to corresponding HID dev */
-
- struct usb_interface *intf; /* USB
interface */
- int ifnum; /* USB
interface number */
-
- unsigned int bufsize; /* URB
buffer size */
-
- struct urb *urbin; /*
Input URB */
- char *inbuf;/*
Input buffer */
- dma_addr_t inbuf_dma; /*
Input buffer dma */
- spinlock_t inlock; /*
Input fifo spinlock */
-
- struct urb *urbctrl;/*
Control URB */
- struct usb_ctrlrequest *cr; /*
Control request struct */
- dma_addr_t cr_dma; /*
Control request struct dma */
- struct hid_control_fifo ctrl[HID_CONTROL_FIFO_SIZE];/*
Control fifo */
- unsigned char ctrlhead, ctrltail; /*
Control fifo head tail */
- char *ctrlbuf; /*
Control buffer */
- dma_addr_t ctrlbuf_dma; /*
Control buffer dma */
- spinlock_t ctrllock;/*
Control fifo spinlock */
-
- struct urb *urbout; /*
Output URB */
- struct hid_report *out[HID_CONTROL_FIFO_SIZE]; /*
Output pipe fifo */
- 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 */
-
- unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
- struct timer_list io_retry; /*
Retry timer */
- unsigned long stop_retry; /* Time
to give up, in jiffies */
- unsigned int retry_delay; /*
Delay length in ms */
- struct work_struct reset_work; /* Task
context for resets */
+ /* pointer to corresponding HID dev */
+ struct hid_device *hid;
+ /* USB interface */
+ struct usb_interface *intf;
+ /* USB interface number */
+ int ifnum;
+
+ /* URB buffer size */
+ unsigned int bufsize;
+
+ /* Input URB */
+ struct urb *urbin;
+ /* Input buffer */
+ char *inbuf;
+ /* Input buffer dma */
+ dma_addr_t inbuf_dma;
+ /* Input fifo spinlock */
+ spinlock_t inlock;
+
+ /* Control
[linux-usb-devel] autosuspend for HID devices, take #2
Hi,
this is the newest version of autosuspend for HID devices. It fixes:
- Pete: removal of dead members of a struct
- Alan: open/suspend race
- Jiri: PID devices are exempt
- Jiri: any device that is served by hiddev is exempt
- fixed a race between close and resume
- proper error handling in resume
open questions:
- Jiri, is there a _category_ of HID devices that should not be
autosuspended at all?
- do we need to care about any common PID devices?
- Alan, do you have strong feelings about putting all conditions
for failing to suspend into one condition? I consider it conceptually
cleaner to have seperate branches for wanting to check for failure
TODO
- pre/post_reset is currently broken, it can no longer be a parasite
on suspend/resume
- adapt to hibernate
- adapt to Jiri's new HID access driver
There is a principal issue. What is to be done with output requests?
Starting the queue as soon as one arrives seems the safest thing to do,
but it is fatal in a subset of situations, that is, it must not be done during
snapshotting and when going for system suspend. The freezer is insufficient
to prevent new requests from coming in.
How is a driver to learn when it may restart the queue? This needs
support from usbcore. udev-auto_pm will become obsolete, so it
is no help.
Alan, are you going to scream and leap with talons extended if I
suggest yet another pair of methods for USB devices to facilitate
that?
Regards
Oliver
-
--- linux-2.6.22-rc1/include/linux/hid.h2007-05-13 03:45:56.0
+0200
+++ linux-2.6.22-rc1-autosuspend/include/linux/hid.h2007-05-15
10:03:12.0 +0200
@@ -401,6 +401,7 @@ struct hid_control_fifo {
#define HID_RESET_PENDING 4
#define HID_SUSPENDED 5
#define HID_CLEAR_HALT 6
+#define HID_REPORTED_IDLE 7
struct hid_input {
struct list_head list;
--- linux-2.6.22-rc1/drivers/hid/usbhid/usbhid.h2007-05-13
03:45:56.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/usbhid.h2007-05-22
09:52:29.0 +0200
@@ -36,7 +36,10 @@ int usbhid_wait_io(struct hid_device* hi
void usbhid_close(struct hid_device *hid);
int usbhid_open(struct hid_device *hid);
void usbhid_init_reports(struct hid_device *hid);
-void usbhid_submit_report(struct hid_device *hid, struct hid_report *report,
unsigned char dir);
+void usbhid_submit_report
+(struct hid_device *hid, struct hid_report *report, unsigned char dir);
+int usbhid_get_power(struct hid_device *hid);
+void usbhid_put_power(struct hid_device *hid);
/*
* USB-specific HID struct, to be pointed to
@@ -44,40 +47,69 @@ void usbhid_submit_report(struct hid_dev
*/
struct usbhid_device {
- struct hid_device *hid; /*
pointer to corresponding HID dev */
-
- struct usb_interface *intf; /* USB
interface */
- int ifnum; /* USB
interface number */
-
- unsigned int bufsize; /* URB
buffer size */
-
- struct urb *urbin; /*
Input URB */
- char *inbuf;/*
Input buffer */
- dma_addr_t inbuf_dma; /*
Input buffer dma */
- spinlock_t inlock; /*
Input fifo spinlock */
-
- struct urb *urbctrl;/*
Control URB */
- struct usb_ctrlrequest *cr; /*
Control request struct */
- dma_addr_t cr_dma; /*
Control request struct dma */
- struct hid_control_fifo ctrl[HID_CONTROL_FIFO_SIZE];/*
Control fifo */
- unsigned char ctrlhead, ctrltail; /*
Control fifo head tail */
- char *ctrlbuf; /*
Control buffer */
- dma_addr_t ctrlbuf_dma; /*
Control buffer dma */
- spinlock_t ctrllock;/*
Control fifo spinlock */
-
- struct urb *urbout; /*
Output URB */
- struct hid_report *out[HID_CONTROL_FIFO_SIZE]; /*
Output pipe fifo */
- 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 */
-
- unsigned long iofl; /* I/O
flags (CTRL_RUNNING,
Re: [linux-usb-devel] autosuspend for HID devices, take #2
On Tue, 22 May 2007, Oliver Neukum wrote:
- Jiri, is there a _category_ of HID devices that should not be
autosuspended at all?
Hi Oliver,
thanks for updating the patch. I am not quite sure right now whether I can
come up with a category that should not be suspended at all, but I will
think a little bit more about this.
There is a principal issue. What is to be done with output requests?
Starting the queue as soon as one arrives seems the safest thing to do,
but it is fatal in a subset of situations, that is, it must not be done
during snapshotting and when going for system suspend. The freezer is
insufficient to prevent new requests from coming in.
Starting the queue immediately would be my preferred solution, as has been
already discussed previously in this thread.
Is anyone aware of any better solution for assuring whether it is possible
to restart the queue, apart from adding special hooks into usbcore? Don't
other USB drivers suffer from similar problems?
--- linux-2.6.22-rc1/drivers/hid/usbhid/hiddev.c 2007-05-13
03:45:56.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/hiddev.c 2007-05-22
09:51:08.0 +0200
@@ -248,10 +248,12 @@ static int hiddev_release(struct inode *
spin_unlock_irqrestore(list-hiddev-list_lock, flags);
if (!--list-hiddev-open) {
- if (list-hiddev-exist)
+ if (list-hiddev-exist) {
usbhid_close(list-hiddev-hid);
- else
+ usbhid_put_power(list-hiddev-hid);
+ } else {
kfree(list-hiddev);
+ }
}
What is the purpose of this hunk please?
It'd maybe be nice if you provide the cleanups (and this also includes
moving the comments around so that they are wrapped at 80 cols, etc) in a
separate patch. Thanks.
+static int hidinput_has_ff(struct hid_device *hid)
+{
+ struct hid_input *hidinput;
+
+ list_for_each_entry(hidinput, hid-inputs, list)
+ if (test_bit(EV_FF, hidinput-input-evbit))
+ return 1;
+ return 0;
+}
This should better go into hid-input.c if possible.
@@ -406,49 +497,53 @@ void usbhid_submit_report(struct hid_dev
if (usbhid-urbout dir == USB_DIR_OUT report-type ==
HID_OUTPUT_REPORT) {
- spin_lock_irqsave(usbhid-outlock, flags);
-
- if ((head = (usbhid-outhead + 1) (HID_OUTPUT_FIFO_SIZE - 1))
== usbhid-outtail) {
- spin_unlock_irqrestore(usbhid-outlock, flags);
- warn(output queue full);
- return;
- }
-
- usbhid-out[usbhid-outhead] = report;
- usbhid-outhead = head;
+ usbhid_queue_report_out(hid, report);
Is it OK that we don't check the return value of
usbhid_queue_report_out()? What if the queue becomes full, aren't we
losing the report? (applies also to usbhid_queue_report_ctrl() elsewhere).
Thanks,
--
Jiri Kosina
SUSE Labs
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
Am Dienstag, 22. Mai 2007 13:28 schrieb Jiri Kosina:
Hi,
There is a principal issue. What is to be done with output requests?
Starting the queue as soon as one arrives seems the safest thing to do,
but it is fatal in a subset of situations, that is, it must not be done
during snapshotting and when going for system suspend. The freezer is
insufficient to prevent new requests from coming in.
Starting the queue immediately would be my preferred solution, as has been
already discussed previously in this thread.
I agree that starting the queue as soon as possible is the sensible
approach. The question is what as soon as possible means.
Is anyone aware of any better solution for assuring whether it is possible
to restart the queue, apart from adding special hooks into usbcore? Don't
other USB drivers suffer from similar problems?
No, HID is pretty unique in causing output from its interrupt handlers.
--- linux-2.6.22-rc1/drivers/hid/usbhid/hiddev.c 2007-05-13
03:45:56.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/hiddev.c 2007-05-22
09:51:08.0 +0200
@@ -248,10 +248,12 @@ static int hiddev_release(struct inode *
spin_unlock_irqrestore(list-hiddev-list_lock, flags);
if (!--list-hiddev-open) {
- if (list-hiddev-exist)
+ if (list-hiddev-exist) {
usbhid_close(list-hiddev-hid);
- else
+ usbhid_put_power(list-hiddev-hid);
+ } else {
kfree(list-hiddev);
+ }
}
What is the purpose of this hunk please?
To make sure devices opened through hiddev are bot suspended.
It'd maybe be nice if you provide the cleanups (and this also includes
moving the comments around so that they are wrapped at 80 cols, etc) in a
separate patch. Thanks.
I did that and Pavel complained about it. I'll work on the
comments.
+static int hidinput_has_ff(struct hid_device *hid)
+{
+ struct hid_input *hidinput;
+
+ list_for_each_entry(hidinput, hid-inputs, list)
+ if (test_bit(EV_FF, hidinput-input-evbit))
+ return 1;
+ return 0;
+}
This should better go into hid-input.c if possible.
Ok.
@@ -406,49 +497,53 @@ void usbhid_submit_report(struct hid_dev
if (usbhid-urbout dir == USB_DIR_OUT report-type ==
HID_OUTPUT_REPORT) {
- spin_lock_irqsave(usbhid-outlock, flags);
-
- if ((head = (usbhid-outhead + 1) (HID_OUTPUT_FIFO_SIZE -
1)) == usbhid-outtail) {
- spin_unlock_irqrestore(usbhid-outlock, flags);
- warn(output queue full);
- return;
- }
-
- usbhid-out[usbhid-outhead] = report;
- usbhid-outhead = head;
+ usbhid_queue_report_out(hid, report);
Is it OK that we don't check the return value of
usbhid_queue_report_out()? What if the queue becomes full, aren't we
losing the report? (applies also to usbhid_queue_report_ctrl() elsewhere).
We are. But we are already. What would you do if we overflow the queue?
At present this can only happen if the device is very slow processing
requests, but it is possible. Flow control would be a good idea, but it is
too late at that stage. The prototypes would have to be changed to report
back errors, or the space left.
Regards
Oliver
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
[linux-usb-devel] autosuspend for HID devices, take #3
Hi,
this is the newest version of autosuspend for HID devices. It fixes:
- Jiri: shifting code around
- forgot to kill output urbs upon suspend
- pre/post_reset methods
Regards
Oliver
--
--- linux-2.6.22-rc2/include/linux/hid.h2007-05-22 14:50:58.0
+0200
+++ linux-2.6.22-rc2-autosuspend/include/linux/hid.h2007-05-22
15:04:41.0 +0200
@@ -401,6 +401,7 @@ struct hid_control_fifo {
#define HID_RESET_PENDING 4
#define HID_SUSPENDED 5
#define HID_CLEAR_HALT 6
+#define HID_REPORTED_IDLE 7
struct hid_input {
struct list_head list;
@@ -500,6 +501,7 @@ void hid_input_field(struct hid_device *
void hid_output_report(struct hid_report *report, __u8 *data);
void hid_free_device(struct hid_device *device);
struct hid_device *hid_parse_report(__u8 *start, unsigned size);
+int hidinput_has_ff(struct hid_device *hid);
/* HID quirks API */
u32 usbhid_lookup_quirk(const u16 idVendor, const u16 idProduct);
--- linux-2.6.22-rc2/drivers/hid/usbhid/usbhid.h2007-05-22
14:50:09.0 +0200
+++ linux-2.6.22-rc2-autosuspend/drivers/hid/usbhid/usbhid.h2007-05-22
11:15:44.0 +0200
@@ -36,7 +36,10 @@ int usbhid_wait_io(struct hid_device* hi
void usbhid_close(struct hid_device *hid);
int usbhid_open(struct hid_device *hid);
void usbhid_init_reports(struct hid_device *hid);
-void usbhid_submit_report(struct hid_device *hid, struct hid_report *report,
unsigned char dir);
+void usbhid_submit_report
+(struct hid_device *hid, struct hid_report *report, unsigned char dir);
+int usbhid_get_power(struct hid_device *hid);
+void usbhid_put_power(struct hid_device *hid);
/*
* USB-specific HID struct, to be pointed to
@@ -44,40 +47,69 @@ void usbhid_submit_report(struct hid_dev
*/
struct usbhid_device {
- struct hid_device *hid; /*
pointer to corresponding HID dev */
-
- struct usb_interface *intf; /* USB
interface */
- int ifnum; /* USB
interface number */
-
- unsigned int bufsize; /* URB
buffer size */
-
- struct urb *urbin; /*
Input URB */
- char *inbuf;/*
Input buffer */
- dma_addr_t inbuf_dma; /*
Input buffer dma */
- spinlock_t inlock; /*
Input fifo spinlock */
-
- struct urb *urbctrl;/*
Control URB */
- struct usb_ctrlrequest *cr; /*
Control request struct */
- dma_addr_t cr_dma; /*
Control request struct dma */
- struct hid_control_fifo ctrl[HID_CONTROL_FIFO_SIZE];/*
Control fifo */
- unsigned char ctrlhead, ctrltail; /*
Control fifo head tail */
- char *ctrlbuf; /*
Control buffer */
- dma_addr_t ctrlbuf_dma; /*
Control buffer dma */
- spinlock_t ctrllock;/*
Control fifo spinlock */
-
- struct urb *urbout; /*
Output URB */
- struct hid_report *out[HID_CONTROL_FIFO_SIZE]; /*
Output pipe fifo */
- 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 */
-
- unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
- struct timer_list io_retry; /*
Retry timer */
- unsigned long stop_retry; /* Time
to give up, in jiffies */
- unsigned int retry_delay; /*
Delay length in ms */
- struct work_struct reset_work; /* Task
context for resets */
+ /* pointer to corresponding HID dev */
+ struct hid_device *hid;
+ /* USB interface */
+ struct usb_interface *intf;
+ /* USB interface number */
+ int ifnum;
+
+ /* URB buffer size */
+ unsigned int bufsize;
+
+ /* Input URB */
+ struct urb *urbin;
+ /* Input buffer */
+ char *inbuf;
+ /* Input buffer dma */
+ dma_addr_t inbuf_dma;
+ /* Input fifo spinlock */
+ spinlock_t inlock;
Re: [linux-usb-devel] autosuspend for HID devices, take #2
On Tue, 22 May 2007, Oliver Neukum wrote: - Alan, do you have strong feelings about putting all conditions for failing to suspend into one condition? I consider it conceptually cleaner to have seperate branches for wanting to check for failure It looks like you've got two conditions for failing to suspend: The driver is still busy doing something like error recovery when the autosuspend timer expires; I/O in progress or on the queue takes too long to complete. Those are sufficiently different in nature that it makes sense to have separate branches. On the other hand, I still think you'd be better off with only one spin_lock_irq() call in hid_suspend(). After all, you always have to synchronize with the error handler, no matter what kind of suspend it is. In addition there would be nothing really wrong with calling usbhid_wait_io() for an autosuspend, since it should return right away. If you follow this advice, you'll find that the two branches share quite a lot more code than they do now. TODO - pre/post_reset is currently broken, it can no longer be a parasite on suspend/resume Why not? The only difference I can see is that you're allowed to fail a suspend but not a pre_reset. In fact, failing a non-auto suspend is not a good idea. You could easily end up preventing somebody's laptop from hibernating when they close the lid and put it in their knapsack. - adapt to hibernate What adaptations are needed? There is a principal issue. What is to be done with output requests? Starting the queue as soon as one arrives seems the safest thing to do, but it is fatal in a subset of situations, that is, it must not be done during snapshotting and when going for system suspend. The freezer is insufficient to prevent new requests from coming in. Because new requests are generated as a result of interrupt processing. But if you kill the interrupt URB then there will be no more inputs and hence nothing to generate any more output. Thus, when suspending you should kill the inputs and wait for the outputs to drain (or else explicitly plug the output queue). Then it will be safe to autoresume whenever a new output queue entry arrives. Come to think of it, it would be safest to have an explicit way of plugging the queues. But those details are up to you. Alan, are you going to scream and leap with talons extended if I suggest yet another pair of methods for USB devices to facilitate that? Probably. Besides, it's not just a USB problem. Other drivers -- and also some kernel threads -- will need to know when a hibernation is about to start. Isn't there going to be a notifier chain added for this? Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
Am Dienstag, 22. Mai 2007 16:59 schrieb Alan Stern: On Tue, 22 May 2007, Oliver Neukum wrote: On the other hand, I still think you'd be better off with only one spin_lock_irq() call in hid_suspend(). After all, you always have to synchronize with the error handler, no matter what kind of suspend it is. In addition there would be nothing really wrong with calling usbhid_wait_io() for an autosuspend, since it should return right away. If you follow this advice, you'll find that the two branches share quite a lot more code than they do now. I'll think about it. TODO - pre/post_reset is currently broken, it can no longer be a parasite on suspend/resume Why not? The only difference I can see is that you're allowed to fail a suspend but not a pre_reset. Yes, but if you are in pre_reset, chances are something is wrong with the devices. Outright slaughter of all outstanding URBs is better than waiting for them to complete. In fact, failing a non-auto suspend is not a good idea. You could easily end up preventing somebody's laptop from hibernating when they close the lid and put it in their knapsack. Yes, and I don't. That's one reason I want to separate the auto and system cases. - adapt to hibernate What adaptations are needed? Do I need to kill remote wakeup? There is a principal issue. What is to be done with output requests? Starting the queue as soon as one arrives seems the safest thing to do, but it is fatal in a subset of situations, that is, it must not be done during snapshotting and when going for system suspend. The freezer is insufficient to prevent new requests from coming in. Because new requests are generated as a result of interrupt processing. Yes. But if you kill the interrupt URB then there will be no more inputs and hence nothing to generate any more output. Thus, when suspending you should kill the inputs and wait for the outputs to drain (or else explicitly plug the output queue). Then it will be safe to autoresume whenever a new output queue entry arrives. Nope. Devices share LED status. Now think of the case of having two keyboards, one already processed and the other still active. Come to think of it, it would be safest to have an explicit way of plugging the queues. But those details are up to you. What tells me when to do so? Alan, are you going to scream and leap with talons extended if I suggest yet another pair of methods for USB devices to facilitate that? Probably. Besides, it's not just a USB problem. Other drivers -- and also some kernel threads -- will need to know when a hibernation is about to start. Isn't there going to be a notifier chain added for this? I have no idea. Should I ask on the pm list? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
On Tue, 22 May 2007, Alan Stern wrote: But if you kill the interrupt URB then there will be no more inputs and hence nothing to generate any more output. Thus, when suspending you should kill the inputs and wait for the outputs to drain (or else explicitly plug the output queue). Then it will be safe to autoresume whenever a new output queue entry arrives. Hi Alan, I think that this is unfortunately not true. Let's take system with multiple keyboards and their LEDs as an excellent example again. If you kill the interrupt URB, there is nothing what will prevent other keyboard (PS/2 or even USB keyboard, which is not yet suspended) to generate an input event (user presses CapsLock/NumLock at a 'right' time). Then you'll get out of sync, won't you? Thanks, -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
On Tue, 22 May 2007, Oliver Neukum wrote: TODO - pre/post_reset is currently broken, it can no longer be a parasite on suspend/resume Why not? The only difference I can see is that you're allowed to fail a suspend but not a pre_reset. Yes, but if you are in pre_reset, chances are something is wrong with the devices. Outright slaughter of all outstanding URBs is better than waiting for them to complete. Fair enough. I wouldn't call post_reset broken, though. In fact, failing a non-auto suspend is not a good idea. You could easily end up preventing somebody's laptop from hibernating when they close the lid and put it in their knapsack. Yes, and I don't. That's one reason I want to separate the auto and system cases. What happens if usbhid_wait_io() times out? - adapt to hibernate What adaptations are needed? Do I need to kill remote wakeup? No. It should be handled at a higher level. (Right now we don't really handle it properly; this is partly the fault of the PM core.) There is a principal issue. What is to be done with output requests? Starting the queue as soon as one arrives seems the safest thing to do, but it is fatal in a subset of situations, that is, it must not be done during snapshotting and when going for system suspend. The freezer is insufficient to prevent new requests from coming in. Because new requests are generated as a result of interrupt processing. Yes. But if you kill the interrupt URB then there will be no more inputs and hence nothing to generate any more output. Thus, when suspending you should kill the inputs and wait for the outputs to drain (or else explicitly plug the output queue). Then it will be safe to autoresume whenever a new output queue entry arrives. Nope. Devices share LED status. Now think of the case of having two keyboards, one already processed and the other still active. Yes, I had forgotten. Then you will have to rely on plugging. Come to think of it, it would be safest to have an explicit way of plugging the queues. But those details are up to you. What tells me when to do so? The best you can do is to plug the queues whenever you do a non-auto suspend. But that won't help if the device is already autosuspended when a hibernate begins. This is a good question; we should bring it to the attention of the PM people. Alan, are you going to scream and leap with talons extended if I suggest yet another pair of methods for USB devices to facilitate that? Probably. Besides, it's not just a USB problem. Other drivers -- and also some kernel threads -- will need to know when a hibernation is about to start. Isn't there going to be a notifier chain added for this? I have no idea. Should I ask on the pm list? Go ahead. It's connected to the previous question. Be sure to include enough details to make the nature of the problem clear to everyone, even people not familiar with the input subsystem. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for HID devices, take #2
On Tue, 22 May 2007, Alan Stern wrote: - adapt to hibernate What adaptations are needed? Do I need to kill remote wakeup? No. It should be handled at a higher level. (Right now we don't really handle it properly; this is partly the fault of the PM core.) This isn't is bad as it may sound. Our handling of remote wakeup isn't truly _bad_ -- it just doesn't quite match the USB spec. There's no danger of a remote wakeup request accidentally activating a device in the middle of snapshotting, or even in the middle of preparing for the snapshot, since khubd will already be frozen. The USB spec puts pretty stringent limitations on how long a host is allowed to ignore a remote wakeup request. In fact, the limit is 1 ms (TURSM; see 7.1.7.7). If a device does send a wakeup request during snapshotting, we will almost certainly violate that limit. The correct approach is to disable remote wakeup before starting the snapshot, but we don't currently do this. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Mon, 21 May 2007, Oliver Neukum wrote:
- what to do with devices with force feedback? (or PID devices in
general). The effect could have up to infinite duration (until it is
stopped by appropriate stop method), and we definitely absolutely
must not autosuspend a device that is currently in a process of
playback.
I suggest we don't suspend them. We'd increment the pm usage count on
open and decrement it on close, basically the same we need to do for
generic HID devices. I just need a way to identify PID devices.
Hi Oliver,
what about something like
int hidinput_has_ff(struct hid_device *hid)
{
struct hid_input *hidinput;
list_for_each_entry(hidinput, hid-inputs, list)
if (test_bit(EV_FF, hidinput-input-evbit))
return 1;
return 0;
}
- think again about the output reports queuing. I am now inclined to think
that we should simply wake the device up once the output report is to be
delivered to it. There might be different situations other than just
keyboard LEDs (there can be simply any kind of exotic HID device being
controlled through hiddev and userspace could want to deliver the output
report to it immediately, without any queuing)
If we do busy style autosuspend only for devices we positively identify,
is the point rendered moot?
Sorry, could you be please more specific? I don't think I understand the
point here. Thanks.
- (and of course coding style)
It shall be done, but not until the principal features are clear.
Of course.
Thanks,
--
Jiri Kosina
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 17:20 schrieb Alan Stern:
@@ -592,8 +604,21 @@ static int hid_get_class_descriptor(stru
int usbhid_open(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid-driver_data;
+ int res;
- hid-open++;
+ mutex_lock(hid_open_mut);
+ if (!hid-open++) {
+ res = usb_autopm_get_interface(usbhid-intf);
+ /* the device must be awake to reliable request remote wakeup
*/
+ if (res 0) {
+ hid-open--;
+ mutex_unlock(hid_open_mut);
+ return -EIO;
+ }
+ usbhid-intf-needs_remote_wakeup = 1;
+ usb_autopm_put_interface(usbhid-intf);
+ }
+ mutex_unlock(hid_open_mut);
if (hid_start_in(hid))
hid_io_error(hid);
return 0;
Don't you need to do usb_autopm_get_interface() before hid_start_in()?
hid_start_in() takes the spinlock and checks for a suspension. In the
unlikely case that the device already has been suspended, remote
wakeup must be active and resume() will call hid_start_in() again.
@@ -1154,13 +1184,26 @@ static int hid_suspend(struct usb_interf
struct usbhid_device *usbhid = hid-driver_data;
struct usb_device *udev = interface_to_usbdev(intf);
-
- spin_lock_irq(usbhid-inlock);
- set_bit(HID_REPORTED_IDLE, usbhid-iofl);
- spin_unlock_irq(usbhid-inlock);
- if (usbhid_wait_io(hid) 0)
- return -EIO;
-
+ if (udev-auto_pm) {
+ spin_lock_irq(usbhid-inlock); /* Sync with error handler */
+ if (!test_bit(HID_RESET_PENDING, usbhid-iofl)
+ !test_bit(HID_CLEAR_HALT, usbhid-iofl)
+ !test_bit(HID_OUT_RUNNING, usbhid-iofl)
+ !test_bit(HID_CTRL_RUNNING, usbhid-iofl))
+ {
+ set_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ spin_unlock_irq(usbhid-inlock);
+ } else {
+ spin_unlock_irq(usbhid-inlock);
+ return -EBUSY;
+ }
+ } else {
+ spin_lock_irq(usbhid-inlock);
+ set_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ spin_unlock_irq(usbhid-inlock);
+ if (usbhid_wait_io(hid) 0)
+ return -EIO;
+ }
del_timer(usbhid-io_retry);
usb_kill_urb(usbhid-urbin);
flush_scheduled_work();
This might be simpler if you inverted the order of the tests. That is,
first get the spinlock, then test for
udev-auto_pm (test_bit() || test_bit() || ...)
But that would put code needlessly under lock.
to see if you need to fail right away. You probably don't even need to
check udev-auto_pm again before calling usbhid_wait_io.
@@ -1175,6 +1218,7 @@ static int hid_resume(struct usb_interfa
int status;
clear_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ usbhid_mark_busy(usbhid);
usbhid-retry_delay = 0;
status = hid_start_in(hid);
usbhid_restart_queues(usbhid);
Again, don't you need to call usb_autopm_get_interface() before
hid_start_in()?
resume() and suspend() have mutual exclusion, don't they? So
the device must be awake while resume() is running.
One last thing, about the race between a last-minute URB completion and
autosuspend. The USB autosuspend core routine doesn't check the
last_busy value after suspending the interface drivers and before
suspending the device. Do we need to?
As the lock is per device, the resume() call would wait and proceed
immediately after suspend() drops the lock. I see no problem.
Regards
Oliver
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Hi Oliver,
what about something like
int hidinput_has_ff(struct hid_device *hid)
{
struct hid_input *hidinput;
list_for_each_entry(hidinput, hid-inputs, list)
if (test_bit(EV_FF, hidinput-input-evbit))
return 1;
return 0;
}
Taken.
- think again about the output reports queuing. I am now inclined to
think
that we should simply wake the device up once the output report is to
be
delivered to it. There might be different situations other than just
keyboard LEDs (there can be simply any kind of exotic HID device being
controlled through hiddev and userspace could want to deliver the
output
report to it immediately, without any queuing)
If we do busy style autosuspend only for devices we positively identify,
is the point rendered moot?
Sorry, could you be please more specific? I don't think I understand the
point here. Thanks.
Such devices would be claimed by hiddev, wouldn't they be?
So if hiddev's open() bumps the count they won't be autosuspended
anyway.
Regards
Oliver
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 15:07 schrieb Pete Zaitcev: On Wed, 16 May 2007 07:37:04 +0200, Oliver Neukum [EMAIL PROTECTED] wrote: + /* Task context for reporting idleness */ + struct work_struct idle_work; This does not seem to be used anywhere. Yes. Killed. Thanks Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Mon, 21 May 2007, Oliver Neukum wrote:
Am Mittwoch, 16. Mai 2007 17:20 schrieb Alan Stern:
@@ -592,8 +604,21 @@ static int hid_get_class_descriptor(stru
int usbhid_open(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid-driver_data;
+ int res;
- hid-open++;
+ mutex_lock(hid_open_mut);
+ if (!hid-open++) {
+ res = usb_autopm_get_interface(usbhid-intf);
+ /* the device must be awake to reliable request remote
wakeup */
+ if (res 0) {
+ hid-open--;
+ mutex_unlock(hid_open_mut);
+ return -EIO;
+ }
+ usbhid-intf-needs_remote_wakeup = 1;
+ usb_autopm_put_interface(usbhid-intf);
+ }
+ mutex_unlock(hid_open_mut);
if (hid_start_in(hid))
hid_io_error(hid);
return 0;
Don't you need to do usb_autopm_get_interface() before hid_start_in()?
hid_start_in() takes the spinlock and checks for a suspension. In the
unlikely case that the device already has been suspended, remote
wakeup must be active and resume() will call hid_start_in() again.
But this code runs whenever a process opens the device file. If the
device is suspended at that time, there might not be a remote wakeup
request pending. So you'd run into trouble unless you did an
autoresume before calling hid_start_in().
@@ -1154,13 +1184,26 @@ static int hid_suspend(struct usb_interf
struct usbhid_device *usbhid = hid-driver_data;
struct usb_device *udev = interface_to_usbdev(intf);
-
- spin_lock_irq(usbhid-inlock);
- set_bit(HID_REPORTED_IDLE, usbhid-iofl);
- spin_unlock_irq(usbhid-inlock);
- if (usbhid_wait_io(hid) 0)
- return -EIO;
-
+ if (udev-auto_pm) {
+ spin_lock_irq(usbhid-inlock); /* Sync with error handler
*/
+ if (!test_bit(HID_RESET_PENDING, usbhid-iofl)
+ !test_bit(HID_CLEAR_HALT, usbhid-iofl)
+ !test_bit(HID_OUT_RUNNING, usbhid-iofl)
+ !test_bit(HID_CTRL_RUNNING, usbhid-iofl))
+ {
+ set_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ spin_unlock_irq(usbhid-inlock);
+ } else {
+ spin_unlock_irq(usbhid-inlock);
+ return -EBUSY;
+ }
+ } else {
+ spin_lock_irq(usbhid-inlock);
+ set_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ spin_unlock_irq(usbhid-inlock);
+ if (usbhid_wait_io(hid) 0)
+ return -EIO;
+ }
del_timer(usbhid-io_retry);
usb_kill_urb(usbhid-urbin);
flush_scheduled_work();
This might be simpler if you inverted the order of the tests. That is,
first get the spinlock, then test for
udev-auto_pm (test_bit() || test_bit() || ...)
But that would put code needlessly under lock.
A single test of udev-auto_pm! I think it would be worthwhile for the
improvements in comprehensibility and code size.
to see if you need to fail right away. You probably don't even need to
check udev-auto_pm again before calling usbhid_wait_io.
@@ -1175,6 +1218,7 @@ static int hid_resume(struct usb_interfa
int status;
clear_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ usbhid_mark_busy(usbhid);
usbhid-retry_delay = 0;
status = hid_start_in(hid);
usbhid_restart_queues(usbhid);
Again, don't you need to call usb_autopm_get_interface() before
hid_start_in()?
resume() and suspend() have mutual exclusion, don't they? So
the device must be awake while resume() is running.
Oh, yes. My mistake.
One last thing, about the race between a last-minute URB completion and
autosuspend. The USB autosuspend core routine doesn't check the
last_busy value after suspending the interface drivers and before
suspending the device. Do we need to?
As the lock is per device, the resume() call would wait and proceed
immediately after suspend() drops the lock. I see no problem.
I guess it's just a theoretical problem. The whole point of the
last_busy field is to prevent autosuspend too soon after any I/O. So
if an URB does complete and nevertheless the device is suspended a few
milliseconds later, then last_busy hasn't fulfilled its role. Like
this:
The autosuspend timer expires.
last_busy is sufficiently old.
hid_suspend() is called.
The interrupt URB completes.
last_busy is set to jiffies.
hid_suspend() kills the
interrupt URB and returns.
usbcore doesn't notice that
last_busy has been changed.
usbcore suspends the
Re: [linux-usb-devel] autosuspend for hid
Am Montag, 21. Mai 2007 17:32 schrieb Alan Stern: On Mon, 21 May 2007, Oliver Neukum wrote: Am Mittwoch, 16. Mai 2007 17:20 schrieb Alan Stern: Don't you need to do usb_autopm_get_interface() before hid_start_in()? hid_start_in() takes the spinlock and checks for a suspension. In the unlikely case that the device already has been suspended, remote wakeup must be active and resume() will call hid_start_in() again. But this code runs whenever a process opens the device file. If the device is suspended at that time, there might not be a remote wakeup request pending. So you'd run into trouble unless you did an autoresume before calling hid_start_in(). Which trouble? Remote wakeup must be activated as it is requested with a bumped pm count. If there's no wakeup, what's the point in pumping data in? There won't be any data. This might be simpler if you inverted the order of the tests. That is, first get the spinlock, then test for udev-auto_pm (test_bit() || test_bit() || ...) But that would put code needlessly under lock. A single test of udev-auto_pm! I think it would be worthwhile for the improvements in comprehensibility and code size. Code size yes. Comprehensibility, no. It might lead to the conclusions that udev-auto_pm needs to be locked. One last thing, about the race between a last-minute URB completion and autosuspend. The USB autosuspend core routine doesn't check the last_busy value after suspending the interface drivers and before suspending the device. Do we need to? As the lock is per device, the resume() call would wait and proceed immediately after suspend() drops the lock. I see no problem. I guess it's just a theoretical problem. The whole point of the last_busy field is to prevent autosuspend too soon after any I/O. So if an URB does complete and nevertheless the device is suspended a few milliseconds later, then last_busy hasn't fulfilled its role. Like this: The autosuspend timer expires. last_busy is sufficiently old. hid_suspend() is called. The interrupt URB completes. last_busy is set to jiffies. hid_suspend() kills the interrupt URB and returns. usbcore doesn't notice that last_busy has been changed. usbcore suspends the device. It's not actually _wrong_, but it isn't optimal. Yes, the race exists. The original version scheduled work to reawake the device in this case. Pete spotted the last vestige of that. Should I put it back in? It seemed a pointless waste of perfection on a heuristics that can't be perfect. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Montag, 21. Mai 2007 17:49 schrieb Oliver Neukum: But this code runs whenever a process opens the device file. If the device is suspended at that time, there might not be a remote wakeup request pending. So you'd run into trouble unless you did an autoresume before calling hid_start_in(). Which trouble? Remote wakeup must be activated as it is requested with a bumped pm count. If there's no wakeup, what's the point in pumping data in? There won't be any data. Oh, I see. In fact, the hid_start_in() in hid_open() itself is mostly a nop. resume() will restart the queue. Unless the device is not suspended before the driver opens. In that case the buffers filled before the data pump can be started may be lost. OK, I'll move it under the lock. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Mon, 21 May 2007, Oliver Neukum wrote: I guess it's just a theoretical problem. The whole point of the last_busy field is to prevent autosuspend too soon after any I/O. So if an URB does complete and nevertheless the device is suspended a few milliseconds later, then last_busy hasn't fulfilled its role. Like this: The autosuspend timer expires. last_busy is sufficiently old. hid_suspend() is called. The interrupt URB completes. last_busy is set to jiffies. hid_suspend() kills the interrupt URB and returns. usbcore doesn't notice that last_busy has been changed. usbcore suspends the device. It's not actually _wrong_, but it isn't optimal. Yes, the race exists. The original version scheduled work to reawake the device in this case. Pete spotted the last vestige of that. Should I put it back in? It seemed a pointless waste of perfection on a heuristics that can't be perfect. No, don't put it back in. My question was whether usbcore should look to see whether last_busy had changed. But it is, after all, a race -- and like you say, we'll never be perfect. So there's probably no point in worrying too much about it. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 15:41 schrieb Jiri Kosina: On Wed, 16 May 2007, Pete Zaitcev wrote: I did not verify the function in detail, but the patch looks sane at a quick glance. Hi Pete and Oliver, (BTW, why was I again dropped from CC? :) ) I consider the things below crucial before this could be merged: - what to do with devices with force feedback? (or PID devices in general). The effect could have up to infinite duration (until it is stopped by appropriate stop method), and we definitely absolutely must not autosuspend a device that is currently in a process of playback. I suggest we don't suspend them. We'd increment the pm usage count on open and decrement it on close, basically the same we need to do for generic HID devices. I just need a way to identify PID devices. - think again about the output reports queuing. I am now inclined to think that we should simply wake the device up once the output report is to be delivered to it. There might be different situations other than just keyboard LEDs (there can be simply any kind of exotic HID device being controlled through hiddev and userspace could want to deliver the output report to it immediately, without any queuing) If we do busy style autosuspend only for devices we positively identify, is the point rendered moot? It seems to me waking needs modifications to be safe, as freezing the tasks does not prevent io for HID devices. - (and of course coding style) It shall be done, but not until the principal features are clear. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Oliver Neukum schrieb: Am Dienstag, 15. Mai 2007 16:43 schrieb Robert Marquardt: Oliver Neukum schrieb: The main problem implementing this is LEDs. HID has the very though property of initiating output to them without user space's involvement from interrupt context to all attached devices. Doing this with suspended devices is problematic. Is that setting the keyboard LEDs? That I've tested it with. But the HID spec is very versatile. Other devices can use it, too. Regards Oliver Writing reports to a device from kernel? Why would that be needed? I even see no reason to do it at all. The virtual keyboard should live at the border to user mode or even fully in user mode. There should only two system devices (like in Windows) mouse and keyboard. Only keyboards are writable at all (mice may be writable, but not from the virtual mouse point of view). - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 08:41 schrieb Robert Marquardt: Oliver Neukum schrieb: Am Dienstag, 15. Mai 2007 16:43 schrieb Robert Marquardt: Oliver Neukum schrieb: The main problem implementing this is LEDs. HID has the very though property of initiating output to them without user space's involvement from interrupt context to all attached devices. Doing this with suspended devices is problematic. Is that setting the keyboard LEDs? That I've tested it with. But the HID spec is very versatile. Other devices can use it, too. Regards Oliver Writing reports to a device from kernel? Why would that be needed? It is in my experience mainly needed for setting and clearing the LEDs. But I don't claim to know all input devices in the kernel tree. I even see no reason to do it at all. The virtual keyboard should live at the border to user mode or even fully in user mode. There should only two system devices (like in Windows) mouse and keyboard. Only keyboards The mouse is not a system device in that sense. are writable at all (mice may be writable, but not from the virtual mouse point of view). Preserving the LEDs working under the worst circumstances is important. I am not sure whether it is a good thing that all keyboards share CAPS LOCK, etc... , but that's a wider issue. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Oliver Neukum schrieb: Writing reports to a device from kernel? Why would that be needed? It is in my experience mainly needed for setting and clearing the LEDs. But I don't claim to know all input devices in the kernel tree. Does hat really have to be handled on low-level? It should be possible to forbid such interactions on the lower levels without getting regressions. Preserving the LEDs working under the worst circumstances is important. I am not sure whether it is a good thing that all keyboards share CAPS LOCK, etc... , but that's a wider issue. If handled at higher levels there are two way to handle suspended keyboard. Either first wake it up or decide to not write to it. I do know about special keyboards which do not have a concept of CAPS LOCK, SCROLL LOCK or NUM LOCK at all for the simple reason of not having the corresponding keys. Personally i do not think the keyboards should be held in sync (aka having a single virtual keyboard). - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 10:24 schrieb Robert Marquardt: Oliver Neukum schrieb: Writing reports to a device from kernel? Why would that be needed? It is in my experience mainly needed for setting and clearing the LEDs. But I don't claim to know all input devices in the kernel tree. Does hat really have to be handled on low-level? It should be possible to forbid such interactions on the lower levels without getting regressions. Where would you put it? To make things easier it would have to be on a work queue and thus subject to kernel threads being scheduled. Preserving the LEDs working under the worst circumstances is important. I am not sure whether it is a good thing that all keyboards share CAPS LOCK, etc... , but that's a wider issue. If handled at higher levels there are two way to handle suspended keyboard. Either first wake it up or decide to not write to it. Yes, but we cannot drop the LEDs if CAPS_LOCK is common to all keyboards. I do know about special keyboards which do not have a concept of CAPS LOCK, SCROLL LOCK or NUM LOCK at all for the simple reason of not having the corresponding keys. Personally i do not think the keyboards should be held in sync (aka having a single virtual keyboard). That is a whole new debate. Feel free to start it on the input list. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Robert Marquardt wrote: Writing reports to a device from kernel? Why would that be needed? It is in my experience mainly needed for setting and clearing the LEDs. But I don't claim to know all input devices in the kernel tree. Does hat really have to be handled on low-level? It should be possible to forbid such interactions on the lower levels without getting regressions. Hi Robert, first, why did you remove me from CC when responding? Anyway, what would be your proposal here? Currently the output reports (for changing the keyboard LEDs, for example) are being submitted as soon as the corresponding input event occurs (on different keyboard), which makes sense. Could you be more elaborate on what are the exact drawbacks you see here, and what would your proposal be? (postponing it into workqueue? why?). Thanks. If handled at higher levels there are two way to handle suspended keyboard. Either first wake it up or decide to not write to it. If you decide not to write to it, it will get out of sync with all other keyboards attached to the system once it is woken up. I do know about special keyboards which do not have a concept of CAPS LOCK, SCROLL LOCK or NUM LOCK at all for the simple reason of not having the corresponding keys. Personally i do not think the keyboards should be held in sync (aka having a single virtual keyboard). If you think so, please raise this on linux-input mailing list (and CC at least me and Dmitry). Thanks, -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Tue, 15 May 2007, Oliver Neukum wrote: this is autosuspend for HID devices. It uses the new last_busy facility for USB devices. And for a few functions the pm counter. Hi Oliver, just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. I will investigate. Thanks, -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Jiri Kosina schrieb: first, why did you remove me from CC when responding? Sorry, on too many lists with widely varying reply rules. Just goofed. Anyway, what would be your proposal here? Currently the output reports (for changing the keyboard LEDs, for example) are being submitted as soon as the corresponding input event occurs (on different keyboard), which makes sense. Could you be more elaborate on what are the exact drawbacks you see here, and what would your proposal be? (postponing it into workqueue? why?). The basic problem is the difference between CAPS LOCK event and CAPS LOCK state. The state may be in user space only. Windows even has it on a per process base (At least theoretically. It is not very well documented what the difference between GetKeyState and GetAsyncKeyState is). I do know about special keyboards which do not have a concept of CAPS LOCK, SCROLL LOCK or NUM LOCK at all for the simple reason of not having the corresponding keys. Personally i do not think the keyboards should be held in sync (aka having a single virtual keyboard). If you think so, please raise this on linux-input mailing list (and CC at least me and Dmitry). When i find time (yet another list to subscribe to). - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 11:24 schrieb Jiri Kosina: On Tue, 15 May 2007, Oliver Neukum wrote: this is autosuspend for HID devices. It uses the new last_busy facility for USB devices. And for a few functions the pm counter. Hi Oliver, just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. I will investigate. You have compiled it with CONFIG_USB_SUSPEND, haven't you? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Robert Marquardt wrote: Anyway, what would be your proposal here? Currently the output reports (for changing the keyboard LEDs, for example) are being submitted as soon as the corresponding input event occurs (on different keyboard), which makes sense. Could you be more elaborate on what are the exact drawbacks you see here, and what would your proposal be? (postponing it into workqueue? why?). The basic problem is the difference between CAPS LOCK event and CAPS LOCK state. The state may be in user space only. Windows even has it on We have the LEDs states per-terminal. I do not think that changing this is going to happen. If you think so, please raise this on linux-input mailing list (and CC at least me and Dmitry). When i find time (yet another list to subscribe to). You don't have to be subscribed to post into this mailinglist. Thanks, -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 11:24 schrieb Jiri Kosina: just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. I will investigate. And as a sanity check, please check with lsusb whether your keyboard can do remote wakeup. I've never seen one that doesn't but it is within spec. Regards Oliver -- SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg) This signature is a legal requirement - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Oliver Neukum wrote: just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. I will investigate. You have compiled it with CONFIG_USB_SUSPEND, haven't you? I have, of course :) Otherwise I won't even have the 'autosuspend' file in sysfs, for example. -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Oliver Neukum wrote: And as a sanity check, please check with lsusb whether your keyboard can do remote wakeup. I've never seen one that doesn't but it is within spec. Suspending of this keyboard used to work with older versions of your patches you sent me some weeks/months ago. -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Jiri Kosina schrieb: just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. Ah, that was nagging me. Is it at all possible to suspend a keyboard if you want to have the LEDs always in sync with their state? A suspended bus-powered USB device cannot drive LEDs so suspending an USB keyboard with CAPS LOCK on means it gets out of sync. - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Robert Marquardt wrote: just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. Ah, that was nagging me. Is it at all possible to suspend a keyboard if you want to have the LEDs always in sync with their state? A suspended bus-powered USB device cannot drive LEDs so suspending an USB keyboard with CAPS LOCK on means it gets out of sync. I don't think that there is a better approach than what Oliver's code is currently doing - i.e. if the keyboard goes to sleep, queue the output reports which would change the state of the leds, and send them either when the keyboard wakes up, or when the queue is too long. Or we can simply just make any output report wake the device up. So this is going to make the leds state consistent as soon as the keyboard is woken up. This is also a reason why this is not going to fly with PID devices - we really want the force feedback to happen immediately, for example. -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 11:41 schrieb Jiri Kosina: On Wed, 16 May 2007, Oliver Neukum wrote: And as a sanity check, please check with lsusb whether your keyboard can do remote wakeup. I've never seen one that doesn't but it is within spec. Suspending of this keyboard used to work with older versions of your patches you sent me some weeks/months ago. Yes, then it should keep working. My keyboard does suspend. Could you post your syslog? Regards Oliver PS: Sorry about the stupid questions. I've just seen to many things go stupidly wrong - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 11:40 schrieb Robert Marquardt: Jiri Kosina schrieb: just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. Ah, that was nagging me. Is it at all possible to suspend a keyboard if you want to have the LEDs always in sync with their state? The current code will resync when the keyboard is resumed. It should do so automatically if too many output requests are queued. That doesn't work yet and I am hunting that bug. A suspended bus-powered USB device cannot drive LEDs so suspending an USB keyboard with CAPS LOCK on means it gets out of sync. How much power does an LED need? You have 2mA in suspended mode. Regards Oliver -- SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg) This signature is a legal requirement - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 11:48 schrieb Jiri Kosina: On Wed, 16 May 2007, Robert Marquardt wrote: just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. Ah, that was nagging me. Is it at all possible to suspend a keyboard if you want to have the LEDs always in sync with their state? A suspended bus-powered USB device cannot drive LEDs so suspending an USB keyboard with CAPS LOCK on means it gets out of sync. I don't think that there is a better approach than what Oliver's code is currently doing - i.e. if the keyboard goes to sleep, queue the output reports which would change the state of the leds, and send them either when the keyboard wakes up, or when the queue is too long. Or we can simply just make any output report wake the device up. Actually, there's currently a race in the code that can trigger if you have multiple keyboards. Suppose the system is going to hibernate. One keyboard is suspended and the other is not yet. You hit a key causing an LED message and the queue reaches the threshold. You'd have active DMA while a snapshot is done. So this is going to make the leds state consistent as soon as the keyboard is woken up. This is also a reason why this is not going to fly with PID devices - we really want the force feedback to happen immediately, for example. It is my understanding that a PID effect can be active long after the message went out to the device. Is that correct? Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Oliver Neukum schrieb: Am Mittwoch, 16. Mai 2007 11:40 schrieb Robert Marquardt: Jiri Kosina schrieb: just a quick immediate note - I just tested it on my testing box, and it doesn't seem to work for me - the USB keyboard still seems to be awake (leds are on), even if I echo 0 into 'autosuspend' file in sysfs in order to suspend it immediately. Ah, that was nagging me. Is it at all possible to suspend a keyboard if you want to have the LEDs always in sync with their state? The current code will resync when the keyboard is resumed. It should do so automatically if too many output requests are queued. That doesn't work yet and I am hunting that bug. A suspended bus-powered USB device cannot drive LEDs so suspending an USB keyboard with CAPS LOCK on means it gets out of sync. How much power does an LED need? You have 2mA in suspended mode. A standard LED uses about 20 mA and you have 3 of them for a keyboard. That is the reason why wireless keyboard do not have LEDs. So in suspend the keyboard must switch off the LEDs which in turn means either keyboards cannot autosuspend at all or you have to allow that the LEDs obn the keyboard are sometimes not in sync with their state. BTW is the keyboard not required restore the the LEDs on its own when resuming? - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Oliver Neukum wrote: Actually, there's currently a race in the code that can trigger if you It is my understanding that a PID effect can be active long after the message went out to the device. Is that correct? Yes, I am afraid you can't assume that the effect playback has been already completed. -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Dienstag, 15. Mai 2007 16:37 schrieb Alan Stern:
Can you break this up into two patches? If the first adds the queuing
code and the second adds autosuspend support, it will be much easier to
appraise and test them.
Here's part #2, the core autosuspend.
Regards
Oliver
--
--- linux-2.6.22-rc1/drivers/hid/usbhid/hid-core.c 2007-05-16
12:23:26.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/hid-core.c 2007-05-15
19:22:13.0 +0200
@@ -64,6 +64,8 @@ MODULE_PARM_DESC(quirks, Add/modify USB
/*
* Input submission and I/O error handler.
*/
+static DEFINE_MUTEX(hid_open_mut);
+
static void hid_io_error(struct hid_device *hid);
static int hid_submit_out(struct hid_device *hid);
static int hid_submit_ctrl(struct hid_device *hid);
@@ -180,6 +182,13 @@ done:
spin_unlock_irqrestore(usbhid-inlock, flags);
}
+static void usbhid_mark_busy(struct usbhid_device *usbhid)
+{
+ struct usb_interface *intf = usbhid-intf;
+
+ usb_mark_last_busy(interface_to_usbdev(intf));
+}
+
static int usbhid_restart_out_queue(struct usbhid_device *usbhid)
{
struct hid_device *hid = usb_get_intfdata(usbhid-intf);
@@ -230,12 +239,14 @@ static void hid_irq_in(struct urb *urb)
switch (urb-status) {
case 0: /* success */
+ usbhid_mark_busy(usbhid);
usbhid-retry_delay = 0;
hid_input_report(urb-context, HID_INPUT_REPORT,
urb-transfer_buffer,
urb-actual_length, 1);
break;
case -EPIPE:/* stall */
+ usbhid_mark_busy(usbhid);
clear_bit(HID_IN_RUNNING, usbhid-iofl);
set_bit(HID_CLEAR_HALT, usbhid-iofl);
schedule_work(usbhid-reset_work);
@@ -249,6 +260,7 @@ static void hid_irq_in(struct urb *urb)
case -EPROTO: /* protocol error or unplug */
case -ETIME:/* protocol error or unplug */
case -ETIMEDOUT:/* Should never happen, but... */
+ usbhid_mark_busy(usbhid);
clear_bit(HID_IN_RUNNING, usbhid-iofl);
hid_io_error(hid);
return;
@@ -592,8 +604,21 @@ static int hid_get_class_descriptor(stru
int usbhid_open(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid-driver_data;
+ int res;
- hid-open++;
+ mutex_lock(hid_open_mut);
+ if (!hid-open++) {
+ res = usb_autopm_get_interface(usbhid-intf);
+ /* the device must be awake to reliable request remote wakeup */
+ if (res 0) {
+ hid-open--;
+ mutex_unlock(hid_open_mut);
+ return -EIO;
+ }
+ usbhid-intf-needs_remote_wakeup = 1;
+ usb_autopm_put_interface(usbhid-intf);
+ }
+ mutex_unlock(hid_open_mut);
if (hid_start_in(hid))
hid_io_error(hid);
return 0;
@@ -603,10 +628,13 @@ void usbhid_close(struct hid_device *hid
{
struct usbhid_device *usbhid = hid-driver_data;
+ mutex_lock(hid_open_mut);
if (!--hid-open) {
usb_kill_urb(usbhid-urbin);
flush_scheduled_work();
+ usbhid-intf-needs_remote_wakeup = 0;
}
+ mutex_unlock(hid_open_mut);
}
/*
@@ -730,7 +758,9 @@ static void __usbhid_restart_queues(stru
struct usbhid_device *usbhid =
container_of(work, struct usbhid_device, restart_work);
+ usb_autopm_get_interface(usbhid-intf);
usbhid_restart_queues(usbhid);
+ usb_autopm_put_interface(usbhid-intf);
}
static void hid_free_buffers(struct usb_device *dev, struct hid_device *hid)
@@ -1154,13 +1184,26 @@ static int hid_suspend(struct usb_interf
struct usbhid_device *usbhid = hid-driver_data;
struct usb_device *udev = interface_to_usbdev(intf);
-
- spin_lock_irq(usbhid-inlock);
- set_bit(HID_REPORTED_IDLE, usbhid-iofl);
- spin_unlock_irq(usbhid-inlock);
- if (usbhid_wait_io(hid) 0)
- return -EIO;
-
+ if (udev-auto_pm) {
+ spin_lock_irq(usbhid-inlock); /* Sync with error handler */
+ if (!test_bit(HID_RESET_PENDING, usbhid-iofl)
+!test_bit(HID_CLEAR_HALT, usbhid-iofl)
+!test_bit(HID_OUT_RUNNING, usbhid-iofl)
+!test_bit(HID_CTRL_RUNNING, usbhid-iofl))
+ {
+ set_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ spin_unlock_irq(usbhid-inlock);
+ } else {
+ spin_unlock_irq(usbhid-inlock);
+ return
Re: [linux-usb-devel] autosuspend for hid
Am Mittwoch, 16. Mai 2007 12:01 schrieb Robert Marquardt: A suspended bus-powered USB device cannot drive LEDs so suspending an USB keyboard with CAPS LOCK on means it gets out of sync. How much power does an LED need? You have 2mA in suspended mode. A standard LED uses about 20 mA and you have 3 of them for a keyboard. That is the reason why wireless keyboard do not have LEDs. That's useful info. So in suspend the keyboard must switch off the LEDs which in turn means either keyboards cannot autosuspend at all or you have to allow that the LEDs obn the keyboard are sometimes not in sync with their state. Yes. The same way a blanked screen is not in sync with X's view of screen content. BTW is the keyboard not required restore the the LEDs on its own when resuming? The keyboard's view of the status might be outdated. Regards Oliver - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Oliver Neukum wrote: Suspending of this keyboard used to work with older versions of your patches you sent me some weeks/months ago. Yes, then it should keep working. My keyboard does suspend. Could you post your syslog? I have had also one very strange (and broken in many ways) usb mouse attached in a situation when things didn't suspend. After unplugging it, it seems to work. I will look at it in more detail now, thanks. -- Jiri Kosina SUSE Labs - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007 07:37:04 +0200, Oliver Neukum [EMAIL PROTECTED] wrote: + /* Task context for reporting idleness */ + struct work_struct idle_work; This does not seem to be used anywhere. I did not verify the function in detail, but the patch looks sane at a quick glance. -- Pete - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Pete Zaitcev wrote: I did not verify the function in detail, but the patch looks sane at a quick glance. Hi Pete and Oliver, (BTW, why was I again dropped from CC? :) ) I consider the things below crucial before this could be merged: - what to do with devices with force feedback? (or PID devices in general). The effect could have up to infinite duration (until it is stopped by appropriate stop method), and we definitely absolutely must not autosuspend a device that is currently in a process of playback. - think again about the output reports queuing. I am now inclined to think that we should simply wake the device up once the output report is to be delivered to it. There might be different situations other than just keyboard LEDs (there can be simply any kind of exotic HID device being controlled through hiddev and userspace could want to deliver the output report to it immediately, without any queuing) - (and of course coding style) Thanks, -- Jiri Kosina - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wednesday 16 May 2007, Oliver Neukum wrote: Am Mittwoch, 16. Mai 2007 11:40 schrieb Robert Marquardt: A suspended bus-powered USB device cannot drive LEDs so suspending an USB keyboard with CAPS LOCK on means it gets out of sync. How much power does an LED need? You have 2mA in suspended mode. Correction: more like 0.5mA in suspended mode. For a high-power device, it can be more ... but most keyboards aren't. - Dave - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
On Wed, 16 May 2007, Oliver Neukum wrote:
Here's part #2, the core autosuspend.
@@ -230,12 +239,14 @@ static void hid_irq_in(struct urb *urb)
switch (urb-status) {
case 0: /* success */
+ usbhid_mark_busy(usbhid);
usbhid-retry_delay = 0;
hid_input_report(urb-context, HID_INPUT_REPORT,
urb-transfer_buffer,
urb-actual_length, 1);
break;
case -EPIPE:/* stall */
+ usbhid_mark_busy(usbhid);
clear_bit(HID_IN_RUNNING, usbhid-iofl);
set_bit(HID_CLEAR_HALT, usbhid-iofl);
schedule_work(usbhid-reset_work);
@@ -249,6 +260,7 @@ static void hid_irq_in(struct urb *urb)
case -EPROTO: /* protocol error or unplug */
case -ETIME:/* protocol error or unplug */
case -ETIMEDOUT:/* Should never happen, but... */
+ usbhid_mark_busy(usbhid);
clear_bit(HID_IN_RUNNING, usbhid-iofl);
hid_io_error(hid);
return;
At first I thought it would be easier to call usbhid_mark_busy() at
every URB completion. But obviously you don't want to do it when an
URB is unlinked.
@@ -592,8 +604,21 @@ static int hid_get_class_descriptor(stru
int usbhid_open(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid-driver_data;
+ int res;
- hid-open++;
+ mutex_lock(hid_open_mut);
+ if (!hid-open++) {
+ res = usb_autopm_get_interface(usbhid-intf);
+ /* the device must be awake to reliable request remote wakeup */
+ if (res 0) {
+ hid-open--;
+ mutex_unlock(hid_open_mut);
+ return -EIO;
+ }
+ usbhid-intf-needs_remote_wakeup = 1;
+ usb_autopm_put_interface(usbhid-intf);
+ }
+ mutex_unlock(hid_open_mut);
if (hid_start_in(hid))
hid_io_error(hid);
return 0;
Don't you need to do usb_autopm_get_interface() before hid_start_in()?
@@ -1154,13 +1184,26 @@ static int hid_suspend(struct usb_interf
struct usbhid_device *usbhid = hid-driver_data;
struct usb_device *udev = interface_to_usbdev(intf);
-
- spin_lock_irq(usbhid-inlock);
- set_bit(HID_REPORTED_IDLE, usbhid-iofl);
- spin_unlock_irq(usbhid-inlock);
- if (usbhid_wait_io(hid) 0)
- return -EIO;
-
+ if (udev-auto_pm) {
+ spin_lock_irq(usbhid-inlock); /* Sync with error handler */
+ if (!test_bit(HID_RESET_PENDING, usbhid-iofl)
+ !test_bit(HID_CLEAR_HALT, usbhid-iofl)
+ !test_bit(HID_OUT_RUNNING, usbhid-iofl)
+ !test_bit(HID_CTRL_RUNNING, usbhid-iofl))
+ {
+ set_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ spin_unlock_irq(usbhid-inlock);
+ } else {
+ spin_unlock_irq(usbhid-inlock);
+ return -EBUSY;
+ }
+ } else {
+ spin_lock_irq(usbhid-inlock);
+ set_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ spin_unlock_irq(usbhid-inlock);
+ if (usbhid_wait_io(hid) 0)
+ return -EIO;
+ }
del_timer(usbhid-io_retry);
usb_kill_urb(usbhid-urbin);
flush_scheduled_work();
This might be simpler if you inverted the order of the tests. That is,
first get the spinlock, then test for
udev-auto_pm (test_bit() || test_bit() || ...)
to see if you need to fail right away. You probably don't even need to
check udev-auto_pm again before calling usbhid_wait_io.
@@ -1175,6 +1218,7 @@ static int hid_resume(struct usb_interfa
int status;
clear_bit(HID_REPORTED_IDLE, usbhid-iofl);
+ usbhid_mark_busy(usbhid);
usbhid-retry_delay = 0;
status = hid_start_in(hid);
usbhid_restart_queues(usbhid);
Again, don't you need to call usb_autopm_get_interface() before
hid_start_in()?
One last thing, about the race between a last-minute URB completion and
autosuspend. The USB autosuspend core routine doesn't check the
last_busy value after suspending the interface drivers and before
suspending the device. Do we need to?
Alan Stern
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form
[linux-usb-devel] autosuspend for hid
Hi,
this is autosuspend for HID devices. It uses the new last_busy facility
for USB devices. And for a few functions the pm counter.
The main problem implementing this is LEDs. HID has the very though
property of initiating output to them without user space's involvement
from interrupt context to all attached devices. Doing this with suspended
devices is problematic.
Output requests now go through different code paths depending on whether
the device they are intended for is asleep or not. In the suspended case
they are merely queued and the queue processed on resume.
Upon every input recieved, whether faulty or not, the device is marked
busy. The race with output requests is handled with a spinlock. The
suspend handler will report a failure if autosuspend wins the race against
output, thus solving the problem.
Some races opened by sleeping in the open path were fixed with a mutex.
Tested:
- basic functionality
- devices with output devices (a keyboard)
- multiple devices
- devices without remote wakeup
- suspend to ram
known limitations:
- will crash with PID devices
- too many queued requests should resume - they don't, cause unknown
needed testing:
- stress testing - this is the only USB driver whose suspend() method
is designed to fail at times. This code path is nearly untested
- exotic devices
- hiddev
- reading from a joystick
- suspend to disk
- cooperation with input devices on other busses (eg. bluetooth)
Any comment on the code is appreciated (though I know it doesn't yet
conform to coding style)
Blast away
Oliver
--
--- linux-2.6.22-rc1/drivers/hid/usbhid/usbhid.h2007-05-13
03:45:56.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/usbhid.h2007-05-15
09:53:12.0 +0200
@@ -63,21 +63,24 @@ struct usbhid_device {
unsigned char ctrlhead, ctrltail; /*
Control fifo head tail */
char *ctrlbuf; /*
Control buffer */
dma_addr_t ctrlbuf_dma; /*
Control buffer dma */
- spinlock_t ctrllock;/*
Control fifo spinlock */
struct urb *urbout; /*
Output URB */
struct hid_report *out[HID_CONTROL_FIFO_SIZE]; /*
Output pipe fifo */
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 */
- unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
- struct timer_list io_retry; /*
Retry timer */
- unsigned long stop_retry; /* Time
to give up, in jiffies */
- unsigned int retry_delay; /*
Delay length in ms */
- struct work_struct reset_work; /* Task
context for resets */
+ spinlock_t outputlock; /*
Output fifo spinlock */
+ unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
+ struct timer_list io_retry; /*
Retry timer */
+ struct timer_list idle_timer; /*
timer to determine idleness */
+ unsigned long stop_retry; /* Time
to give up, in jiffies */
+ unsigned int idle_time; /* Time
to determine idleness, in seconds */
+ unsigned int retry_delay; /*
Delay length in ms */
+ struct work_struct reset_work; /* Task
context for resets */
+ struct work_struct idle_work; /* Task
context for reporting idleness */
+ struct work_struct restart_work;/*
waking up for output to be done in task context */
};
#definehid_to_usb_dev(hid_dev) \
--- linux-2.6.22-rc1/drivers/hid/usbhid/hid-core.c 2007-05-13
03:45:56.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/hid-core.c 2007-05-15
08:57:38.0 +0200
@@ -5,6 +5,7 @@
* Copyright (c) 2000-2005 Vojtech Pavlik [EMAIL PROTECTED]
* Copyright (c) 2005 Michael Haboustak [EMAIL PROTECTED] for Concept2, Inc
* Copyright (c) 2006-2007 Jiri Kosina
+ * Copyright (c) 2007 Oliver Neukum
*/
/*
@@ -63,8 +64,11 @@ MODULE_PARM_DESC(quirks, Add/modify USB
/*
* Input submission and I/O error
Re: [linux-usb-devel] autosuspend for hid
On Tue, 15 May 2007, Oliver Neukum wrote: Hi, this is autosuspend for HID devices. It uses the new last_busy facility for USB devices. And for a few functions the pm counter. The main problem implementing this is LEDs. HID has the very though property of initiating output to them without user space's involvement from interrupt context to all attached devices. Doing this with suspended devices is problematic. Output requests now go through different code paths depending on whether the device they are intended for is asleep or not. In the suspended case they are merely queued and the queue processed on resume. Upon every input recieved, whether faulty or not, the device is marked busy. The race with output requests is handled with a spinlock. The suspend handler will report a failure if autosuspend wins the race against output, thus solving the problem. Can you break this up into two patches? If the first adds the queuing code and the second adds autosuspend support, it will be much easier to appraise and test them. Alan Stern - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Oliver Neukum schrieb: The main problem implementing this is LEDs. HID has the very though property of initiating output to them without user space's involvement from interrupt context to all attached devices. Doing this with suspended devices is problematic. Is that setting the keyboard LEDs? - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Dienstag, 15. Mai 2007 16:37 schrieb Alan Stern: Can you break this up into two patches? If the first adds the queuing code and the second adds autosuspend support, it will be much easier to appraise and test them. Very well. I'll do so. Regards Oliver -- SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg) This signature is a legal requirement - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Dienstag, 15. Mai 2007 16:43 schrieb Robert Marquardt: Oliver Neukum schrieb: The main problem implementing this is LEDs. HID has the very though property of initiating output to them without user space's involvement from interrupt context to all attached devices. Doing this with suspended devices is problematic. Is that setting the keyboard LEDs? That I've tested it with. But the HID spec is very versatile. Other devices can use it, too. Regards Oliver -- SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg) This signature is a legal requirement - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Dienstag, 15. Mai 2007 16:37 schrieb Alan Stern:
Can you break this up into two patches? If the first adds the queuing
code and the second adds autosuspend support, it will be much easier to
appraise and test them.
Hi,
this code implements delayed execution of output requests arriving
for suspended HID devices.
Regards
Oliver
--
--- linux-2.6.22-rc1/include/linux/hid.h2007-05-15 19:12:37.0
+0200
+++ linux-2.6.22-rc1-autosuspend/include/linux/hid.h2007-05-15
10:03:12.0 +0200
@@ -401,6 +401,7 @@ struct hid_control_fifo {
#define HID_RESET_PENDING 4
#define HID_SUSPENDED 5
#define HID_CLEAR_HALT 6
+#define HID_REPORTED_IDLE 7
struct hid_input {
struct list_head list;
--- linux-2.6.22-rc1/drivers/hid/usbhid/usbhid.h2007-05-15
19:12:37.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/usbhid.h2007-05-15
13:49:14.0 +0200
@@ -63,21 +63,23 @@ struct usbhid_device {
unsigned char ctrlhead, ctrltail; /*
Control fifo head tail */
char *ctrlbuf; /*
Control buffer */
dma_addr_t ctrlbuf_dma; /*
Control buffer dma */
- spinlock_t ctrllock;/*
Control fifo spinlock */
struct urb *urbout; /*
Output URB */
struct hid_report *out[HID_CONTROL_FIFO_SIZE]; /*
Output pipe fifo */
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 */
- unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
- struct timer_list io_retry; /*
Retry timer */
- unsigned long stop_retry; /* Time
to give up, in jiffies */
- unsigned int retry_delay; /*
Delay length in ms */
- struct work_struct reset_work; /* Task
context for resets */
+ spinlock_t outputlock; /*
Output fifo spinlock */
+ unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
+ struct timer_list io_retry; /*
Retry timer */
+ struct timer_list idle_timer; /*
timer to determine idleness */
+ unsigned long stop_retry; /* Time
to give up, in jiffies */
+ unsigned int retry_delay; /*
Delay length in ms */
+ struct work_struct reset_work; /* Task
context for resets */
+ struct work_struct idle_work; /* Task
context for reporting idleness */
+ struct work_struct restart_work;/*
waking up for output to be done in task context */
};
#definehid_to_usb_dev(hid_dev) \
--- linux-2.6.22-rc1/drivers/hid/usbhid/hid-core.c 2007-05-15
19:12:37.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/hid-core.c 2007-05-15
21:09:26.0 +0200
@@ -5,6 +5,7 @@
* Copyright (c) 2000-2005 Vojtech Pavlik [EMAIL PROTECTED]
* Copyright (c) 2005 Michael Haboustak [EMAIL PROTECTED] for Concept2, Inc
* Copyright (c) 2006-2007 Jiri Kosina
+ * Copyright (c) 2007 Oliver Neukum
*/
/*
@@ -63,8 +64,9 @@ MODULE_PARM_DESC(quirks, Add/modify USB
/*
* Input submission and I/O error handler.
*/
-
static void hid_io_error(struct hid_device *hid);
+static int hid_submit_out(struct hid_device *hid);
+static int hid_submit_ctrl(struct hid_device *hid);
/* Start up the input URB */
static int hid_start_in(struct hid_device *hid)
@@ -74,7 +76,7 @@ static int hid_start_in(struct hid_devic
struct usbhid_device *usbhid = hid-driver_data;
spin_lock_irqsave(usbhid-inlock, flags);
- if (hid-open 0 !test_bit(HID_SUSPENDED, usbhid-iofl)
+ if (hid-open 0 !test_bit(HID_REPORTED_IDLE, usbhid-iofl)
!test_and_set_bit(HID_IN_RUNNING, usbhid-iofl)) {
rc = usb_submit_urb(usbhid-urbin, GFP_ATOMIC);
if (rc != 0)
@@ -178,6 +180,44 @@ done:
spin_unlock_irqrestore(usbhid-inlock, flags);
}
+static int usbhid_restart_out_queue(struct usbhid_device *usbhid)
+{
+ struct hid_device *hid =
Re: [linux-usb-devel] autosuspend for hid
Hi! Can you break this up into two patches? If the first adds the queuing code and the second adds autosuspend support, it will be much easier to appraise and test them. Hi, this code implements delayed execution of output requests arriving for suspended HID devices. usbhid.h violates 80-column rule... in a way that is not funny. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Re: [linux-usb-devel] autosuspend for hid
Am Dienstag, 15. Mai 2007 22:45 schrieb Pavel Machek:
Hi!
Can you break this up into two patches? If the first adds the queuing
code and the second adds autosuspend support, it will be much easier to
appraise and test them.
Hi,
this code implements delayed execution of output requests arriving
for suspended HID devices.
usbhid.h violates 80-column rule... in a way that is not funny.
Here's a version that does not do that.
Regards
Oliver
-
--- linux-2.6.22-rc1/include/linux/hid.h2007-05-15 19:12:37.0
+0200
+++ linux-2.6.22-rc1-autosuspend/include/linux/hid.h2007-05-15
10:03:12.0 +0200
@@ -401,6 +401,7 @@ struct hid_control_fifo {
#define HID_RESET_PENDING 4
#define HID_SUSPENDED 5
#define HID_CLEAR_HALT 6
+#define HID_REPORTED_IDLE 7
struct hid_input {
struct list_head list;
--- linux-2.6.22-rc1/drivers/hid/usbhid/hid-core.c 2007-05-15
19:12:37.0 +0200
+++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/hid-core.c 2007-05-15
21:09:26.0 +0200
@@ -5,6 +5,7 @@
* Copyright (c) 2000-2005 Vojtech Pavlik [EMAIL PROTECTED]
* Copyright (c) 2005 Michael Haboustak [EMAIL PROTECTED] for Concept2, Inc
* Copyright (c) 2006-2007 Jiri Kosina
+ * Copyright (c) 2007 Oliver Neukum
*/
/*
@@ -63,8 +64,9 @@ MODULE_PARM_DESC(quirks, Add/modify USB
/*
* Input submission and I/O error handler.
*/
-
static void hid_io_error(struct hid_device *hid);
+static int hid_submit_out(struct hid_device *hid);
+static int hid_submit_ctrl(struct hid_device *hid);
/* Start up the input URB */
static int hid_start_in(struct hid_device *hid)
@@ -74,7 +76,7 @@ static int hid_start_in(struct hid_devic
struct usbhid_device *usbhid = hid-driver_data;
spin_lock_irqsave(usbhid-inlock, flags);
- if (hid-open 0 !test_bit(HID_SUSPENDED, usbhid-iofl)
+ if (hid-open 0 !test_bit(HID_REPORTED_IDLE, usbhid-iofl)
!test_and_set_bit(HID_IN_RUNNING, usbhid-iofl)) {
rc = usb_submit_urb(usbhid-urbin, GFP_ATOMIC);
if (rc != 0)
@@ -178,6 +180,44 @@ done:
spin_unlock_irqrestore(usbhid-inlock, flags);
}
+static int usbhid_restart_out_queue(struct usbhid_device *usbhid)
+{
+ struct hid_device *hid = usb_get_intfdata(usbhid-intf);
+ int kicked;
+
+ WARN_ON(hid == NULL);
+ if (!hid)
+ return 0;
+
+ if ((kicked = (usbhid-outhead != usbhid-outtail))) {
+ dbg(Kicking head %d tail %d, usbhid-outhead,
usbhid-outtail);
+ if (hid_submit_out(hid)) {
+ clear_bit(HID_OUT_RUNNING, usbhid-iofl);
+ wake_up(hid-wait);
+ }
+ }
+ return kicked;
+}
+
+static int usbhid_restart_ctrl_queue(struct usbhid_device *usbhid)
+{
+ struct hid_device *hid = usb_get_intfdata(usbhid-intf);
+ int kicked;
+
+ WARN_ON(hid == NULL);
+ if (!hid)
+ return 0;
+
+ if ((kicked = (usbhid-ctrlhead != usbhid-ctrltail))) {
+ dbg(Kicking head %d tail %d, usbhid-ctrlhead,
usbhid-ctrltail);
+ if (hid_submit_ctrl(hid)) {
+ clear_bit(HID_CTRL_RUNNING, usbhid-iofl);
+ wake_up(hid-wait);
+ }
+ }
+ return kicked;
+}
+
/*
* Input interrupt completion handler.
*/
@@ -234,9 +274,11 @@ static int hid_submit_out(struct hid_dev
struct hid_report *report;
struct usbhid_device *usbhid = hid-driver_data;
+ WARN_ON(usbhid == NULL);
report = usbhid-out[usbhid-outtail];
-
+ WARN_ON(report == NULL);
hid_output_report(report, usbhid-outbuf);
+ BUG_ON(usbhid-urbout == NULL);
usbhid-urbout-transfer_buffer_length = ((report-size - 1) 3) + 1
+ (report-id 0);
usbhid-urbout-dev = hid_to_usb_dev(hid);
@@ -324,24 +366,20 @@ static void hid_irq_out(struct urb *urb)
warn(output irq status %d received, urb-status);
}
- spin_lock_irqsave(usbhid-outlock, flags);
+ spin_lock_irqsave(usbhid-outputlock, flags);
if (unplug)
usbhid-outtail = usbhid-outhead;
else
usbhid-outtail = (usbhid-outtail + 1) (HID_OUTPUT_FIFO_SIZE
- 1);
- if (usbhid-outhead != usbhid-outtail) {
- if (hid_submit_out(hid)) {
- clear_bit(HID_OUT_RUNNING, usbhid-iofl);
- wake_up(hid-wait);
- }
- spin_unlock_irqrestore(usbhid-outlock, flags);
+ if (usbhid_restart_out_queue(usbhid)) {
+ spin_unlock_irqrestore(usbhid-outputlock, flags);
return;
}
clear_bit(HID_OUT_RUNNING, usbhid-iofl);
- spin_unlock_irqrestore(usbhid-outlock, flags);
+ spin_unlock_irqrestore(usbhid-outputlock, flags);
