* [PATCH] HID: usbhid: prevent unwanted events to be sent when re-opening the device
@ 2014-09-30 18:28 Benjamin Tissoires
2014-10-28 20:04 ` Benjamin Tissoires
2014-10-29 10:05 ` Jiri Kosina
0 siblings, 2 replies; 3+ messages in thread
From: Benjamin Tissoires @ 2014-09-30 18:28 UTC (permalink / raw)
To: Jiri Kosina, Hans de Goede; +Cc: Bastien Nocera, linux-kernel, linux-input
When events occurs while no one is listening to the node (hid->open == 0
and usb_kill_urb() called) some events are still stacked somewhere in
the USB (kernel or device?) stack. When the node gets reopened, these
events are drained, and this results in spurious touch down/up, or mouse
button clicks.
The problem was spotted with touchscreens in fdo bug #81781 [1], but it
actually occurs with any mouse using hid-generic or touchscreen.
A way to reproduce it is to call:
$ xinput disable 9 ; sleep 5 ; xinput enable 9
With 9 being the device ID for the touchscreen/mouse. During the "sleep",
produce some touch events or click events. When "xinput enable" is called,
at least one click is generated.
This patch tries to fix this by draining the queue for 50 msec and
during this time frame, not forwarding these old events to the hid layer.
Hans completed the explanation:
"""
Devices like mice (basically any hid device) will have a fifo
on the device side, when we stop submitting urbs to get hid reports from
it, that fifo will fill up, and when we resume we will get whatever
is there in that fifo.
"""
[1] https://bugs.freedesktop.org/show_bug.cgi?id=81781
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
OK, I know this is uggly, but I could not find a better way :(
Cheers,
Benjamin
drivers/hid/usbhid/hid-core.c | 36 ++++++++++++++++++++++++------------
drivers/hid/usbhid/usbhid.h | 1 +
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index ca6849a..04e34b9 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -278,18 +278,20 @@ static void hid_irq_in(struct urb *urb)
usbhid->retry_delay = 0;
if ((hid->quirks & HID_QUIRK_ALWAYS_POLL) && !hid->open)
break;
- hid_input_report(urb->context, HID_INPUT_REPORT,
- urb->transfer_buffer,
- urb->actual_length, 1);
- /*
- * autosuspend refused while keys are pressed
- * because most keyboards don't wake up when
- * a key is released
- */
- if (hid_check_keys_pressed(hid))
- set_bit(HID_KEYS_PRESSED, &usbhid->iofl);
- else
- clear_bit(HID_KEYS_PRESSED, &usbhid->iofl);
+ if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) {
+ hid_input_report(urb->context, HID_INPUT_REPORT,
+ urb->transfer_buffer,
+ urb->actual_length, 1);
+ /*
+ * autosuspend refused while keys are pressed
+ * because most keyboards don't wake up when
+ * a key is released
+ */
+ if (hid_check_keys_pressed(hid))
+ set_bit(HID_KEYS_PRESSED, &usbhid->iofl);
+ else
+ clear_bit(HID_KEYS_PRESSED, &usbhid->iofl);
+ }
break;
case -EPIPE: /* stall */
usbhid_mark_busy(usbhid);
@@ -688,6 +690,7 @@ int usbhid_open(struct hid_device *hid)
goto done;
}
usbhid->intf->needs_remote_wakeup = 1;
+ set_bit(HID_RESUME_RUNNING, &usbhid->iofl);
res = hid_start_in(hid);
if (res) {
if (res != -ENOSPC) {
@@ -701,6 +704,15 @@ int usbhid_open(struct hid_device *hid)
}
}
usb_autopm_put_interface(usbhid->intf);
+
+ /*
+ * In case events are generated while nobody was listening,
+ * some are released when the device is re-opened.
+ * Wait 50 msec for the queue to empty before allowing events
+ * to go through hid.
+ */
+ msleep(50);
+ clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
}
done:
mutex_unlock(&hid_open_mut);
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index f633c24..807922b 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -52,6 +52,7 @@ struct usb_interface *usbhid_find_interface(int minor);
#define HID_STARTED 8
#define HID_KEYS_PRESSED 10
#define HID_NO_BANDWIDTH 11
+#define HID_RESUME_RUNNING 12
/*
* USB-specific HID struct, to be pointed to
--
2.1.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: usbhid: prevent unwanted events to be sent when re-opening the device
2014-09-30 18:28 [PATCH] HID: usbhid: prevent unwanted events to be sent when re-opening the device Benjamin Tissoires
@ 2014-10-28 20:04 ` Benjamin Tissoires
2014-10-29 10:05 ` Jiri Kosina
1 sibling, 0 replies; 3+ messages in thread
From: Benjamin Tissoires @ 2014-10-28 20:04 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Hans de Goede, Bastien Nocera,
linux-kernel@vger.kernel.org, linux-input
On Tue, Sep 30, 2014 at 2:28 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> When events occurs while no one is listening to the node (hid->open == 0
> and usb_kill_urb() called) some events are still stacked somewhere in
> the USB (kernel or device?) stack. When the node gets reopened, these
> events are drained, and this results in spurious touch down/up, or mouse
> button clicks.
>
> The problem was spotted with touchscreens in fdo bug #81781 [1], but it
> actually occurs with any mouse using hid-generic or touchscreen.
>
> A way to reproduce it is to call:
>
> $ xinput disable 9 ; sleep 5 ; xinput enable 9
>
> With 9 being the device ID for the touchscreen/mouse. During the "sleep",
> produce some touch events or click events. When "xinput enable" is called,
> at least one click is generated.
>
> This patch tries to fix this by draining the queue for 50 msec and
> during this time frame, not forwarding these old events to the hid layer.
>
> Hans completed the explanation:
> """
> Devices like mice (basically any hid device) will have a fifo
> on the device side, when we stop submitting urbs to get hid reports from
> it, that fifo will fill up, and when we resume we will get whatever
> is there in that fifo.
> """
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=81781
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
My turn to raise patches lost in the mailboxes during the merge window....
Jiri, can you re-add this one to the pile of "things that you don't
want to do but some people are really relying on it so somebody still
needs to review it, sigh" list?
Cheers,
Benjamin
>
> OK, I know this is uggly, but I could not find a better way :(
>
> Cheers,
> Benjamin
>
> drivers/hid/usbhid/hid-core.c | 36 ++++++++++++++++++++++++------------
> drivers/hid/usbhid/usbhid.h | 1 +
> 2 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index ca6849a..04e34b9 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -278,18 +278,20 @@ static void hid_irq_in(struct urb *urb)
> usbhid->retry_delay = 0;
> if ((hid->quirks & HID_QUIRK_ALWAYS_POLL) && !hid->open)
> break;
> - hid_input_report(urb->context, HID_INPUT_REPORT,
> - urb->transfer_buffer,
> - urb->actual_length, 1);
> - /*
> - * autosuspend refused while keys are pressed
> - * because most keyboards don't wake up when
> - * a key is released
> - */
> - if (hid_check_keys_pressed(hid))
> - set_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> - else
> - clear_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) {
> + hid_input_report(urb->context, HID_INPUT_REPORT,
> + urb->transfer_buffer,
> + urb->actual_length, 1);
> + /*
> + * autosuspend refused while keys are pressed
> + * because most keyboards don't wake up when
> + * a key is released
> + */
> + if (hid_check_keys_pressed(hid))
> + set_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> + else
> + clear_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> + }
> break;
> case -EPIPE: /* stall */
> usbhid_mark_busy(usbhid);
> @@ -688,6 +690,7 @@ int usbhid_open(struct hid_device *hid)
> goto done;
> }
> usbhid->intf->needs_remote_wakeup = 1;
> + set_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> res = hid_start_in(hid);
> if (res) {
> if (res != -ENOSPC) {
> @@ -701,6 +704,15 @@ int usbhid_open(struct hid_device *hid)
> }
> }
> usb_autopm_put_interface(usbhid->intf);
> +
> + /*
> + * In case events are generated while nobody was listening,
> + * some are released when the device is re-opened.
> + * Wait 50 msec for the queue to empty before allowing events
> + * to go through hid.
> + */
> + msleep(50);
> + clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> }
> done:
> mutex_unlock(&hid_open_mut);
> diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
> index f633c24..807922b 100644
> --- a/drivers/hid/usbhid/usbhid.h
> +++ b/drivers/hid/usbhid/usbhid.h
> @@ -52,6 +52,7 @@ struct usb_interface *usbhid_find_interface(int minor);
> #define HID_STARTED 8
> #define HID_KEYS_PRESSED 10
> #define HID_NO_BANDWIDTH 11
> +#define HID_RESUME_RUNNING 12
>
> /*
> * USB-specific HID struct, to be pointed to
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: usbhid: prevent unwanted events to be sent when re-opening the device
2014-09-30 18:28 [PATCH] HID: usbhid: prevent unwanted events to be sent when re-opening the device Benjamin Tissoires
2014-10-28 20:04 ` Benjamin Tissoires
@ 2014-10-29 10:05 ` Jiri Kosina
1 sibling, 0 replies; 3+ messages in thread
From: Jiri Kosina @ 2014-10-29 10:05 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Hans de Goede, Bastien Nocera, linux-kernel, linux-input
On Tue, 30 Sep 2014, Benjamin Tissoires wrote:
> When events occurs while no one is listening to the node (hid->open == 0
> and usb_kill_urb() called) some events are still stacked somewhere in
> the USB (kernel or device?) stack. When the node gets reopened, these
> events are drained, and this results in spurious touch down/up, or mouse
> button clicks.
>
> The problem was spotted with touchscreens in fdo bug #81781 [1], but it
> actually occurs with any mouse using hid-generic or touchscreen.
>
> A way to reproduce it is to call:
>
> $ xinput disable 9 ; sleep 5 ; xinput enable 9
>
> With 9 being the device ID for the touchscreen/mouse. During the "sleep",
> produce some touch events or click events. When "xinput enable" is called,
> at least one click is generated.
>
> This patch tries to fix this by draining the queue for 50 msec and
> during this time frame, not forwarding these old events to the hid layer.
>
> Hans completed the explanation:
> """
> Devices like mice (basically any hid device) will have a fifo
> on the device side, when we stop submitting urbs to get hid reports from
> it, that fifo will fill up, and when we resume we will get whatever
> is there in that fifo.
> """
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=81781
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> OK, I know this is uggly, but I could not find a better way :(
I have been thinking about this quite a lot, but unfortunately wasn't able
to come up with anything better either, so I am queuing this now.
Thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-10-29 10:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-30 18:28 [PATCH] HID: usbhid: prevent unwanted events to be sent when re-opening the device Benjamin Tissoires
2014-10-28 20:04 ` Benjamin Tissoires
2014-10-29 10:05 ` Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).