Linux USB HID should ignore values outside Logical Minimum/Maximum range

Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Denilson Figueiredo de Sá]

Short description:

An absolute pointing device using USB HID defines a LOGICAL_MINIMUM and
a LOGICAL_MAXIMUM for X, Y axes, and then sends a HID report containing
values outside that range.

Linux kernel should ignore values outside that range, as they are not
meaningful.

Just for comparison, Windows ignores such values. (and I hate this kind
of comparison)


Long description:

I'm building a homebrew USB Absolute Pointing Device using an AVR
ATmega8 and the V-USB firmware. This device will work almost like a
mouse, with one (obvious) difference: while a mouse sends "relative"
movements (move the pointer x,y units, relative to its current
position), an absolute pointing device sends "absolute" positions (move
the mouse exactly to this x,y location).

This device is using two 16-bit fields to report the X and Y axes, and
limit the meaningful values to those within the [0..32767] range. This
is the relevant part of the USB HID Descriptor:

	0x09, 0x30,        //     USAGE (X)
	0x09, 0x31,        //     USAGE (Y)
	0x15, 0x00,        //     LOGICAL_MINIMUM (0)
	0x26, 0xff, 0x7f,  //     LOGICAL_MAXIMUM (32767)
	0x75, 0x10,        //     REPORT_SIZE (16)
	0x95, 0x02,        //     REPORT_COUNT (2)
	0x81, 0x42,        //     INPUT (Data,Var,Abs,Null)

However, Linux kernel is not behaving as expected when receiving
out-of-range values. Instead of ignoring such coordinates, it moves the
pointer to the bottom-right corner of the screen.

Each HID report for this device has 1 byte for the Report ID (with value
02), 2 bytes for the X position, 2 bytes for the Y position, and finally
1 byte for the button presses. They shows up as "02XXXXYY YYbb" in
usbmon.

Look at this usbmon output:

ffff88000c9a0600 1465973871 C Ii:5:093:1 0:8 6 = 02ff0fff 0f00
ffff88000c9a0600 1465973936 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1467317888 C Ii:5:093:1 0:8 6 = 02ff1fff 1f00
ffff88000c9a0600 1467317950 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1467725888 C Ii:5:093:1 0:8 6 = 02ff2fff 2f00
ffff88000c9a0600 1467725931 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1468661905 C Ii:5:093:1 0:8 6 = 02ff3fff 3f00
ffff88000c9a0600 1468661961 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1469429914 C Ii:5:093:1 0:8 6 = 02ff4fff 4f00
ffff88000c9a0600 1469429969 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1470133907 C Ii:5:093:1 0:8 6 = 02ff5fff 5f00
ffff88000c9a0600 1470133948 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1470837931 C Ii:5:093:1 0:8 6 = 02ff6fff 6f00
ffff88000c9a0600 1470837986 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1471445923 C Ii:5:093:1 0:8 6 = 02ff7fff 7f00
ffff88000c9a0600 1471445981 S Ii:5:093:1 -115:8 6 <

Here, the pointer is moving from 0x0fff, 0x0fff (near the top-left of
the screen) up to the maximum possible value: 0x7fff, 0x7fff (the
bottom-right corner). When receiving these reports, the pointer was
correctly moving diagonally across my screen.

Up until here, no problem yet. The problem starts now:

ffff88000c9a0600 1472261948 C Ii:5:093:1 0:8 6 = 02ff8fff 8f00
ffff88000c9a0600 1472262007 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1472965956 C Ii:5:093:1 0:8 6 = 02ff9fff 9f00
ffff88000c9a0600 1472966010 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1473621964 C Ii:5:093:1 0:8 6 = 02ffafff af00
ffff88000c9a0600 1473622019 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1474285972 C Ii:5:093:1 0:8 6 = 02ffbfff bf00
ffff88000c9a0600 1474286027 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1474885980 C Ii:5:093:1 0:8 6 = 02ffcfff cf00
ffff88000c9a0600 1474886034 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1475517975 C Ii:5:093:1 0:8 6 = 02ffdfff df00
ffff88000c9a0600 1475518040 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1476333997 C Ii:5:093:1 0:8 6 = 02ffefff ef00
ffff88000c9a0600 1476334055 S Ii:5:093:1 -115:8 6 <
ffff88000c9a0600 1477366011 C Ii:5:093:1 0:8 6 = 02ffffff ff00
ffff88000c9a0600 1477366069 S Ii:5:093:1 -115:8 6 <

These reports send values outside the range defined in the HID
Descriptor. The values here go from 0x8fff up to 0xffff. All of these
values are invalid and should have been ignored by the Linux kernel.
Instead, what happened was that the kernel insisted into moving the
pointer to the bottom-right corner of the screen.

Just to be sure about what I'm saying, I used my touchpad to move the
pointer away from the corner before each one of those reports, and upon
receiving them, the pointer went back to the corner.

It is worth noting that changing this:

	0x81, 0x42,        //     INPUT (Data,Var,Abs,Null)

to this:

	0x81, 0x02,        //     INPUT (Data,Var,Abs)

makes no difference at all.


I've also tried this device on a Windows XP system, and it works as
expected there (out-of-range values are ignored and don't move the
pointer).


It's also important to remember that an out-of-range value should be
ignored, but not the full report. For instance, the device could have
sent "02 ffff ffff 01", meaning an invalid X,Y position (which should be
ignored), but a valid button press of the first button (which should be
processed). In this case, the kernel should send a button_press event,
but not a motion event.

It may even happen to send an out-of-range value for one axis, but a
valid value for another axis. The code should be prepared for that
(ignore one, but keep the other).


Found this bug in Linux kernel 2.6.38


If someone wants to build the hardware to reproduce this bug, go to the
revision 662baa542e07 of this project:
https://bitbucket.org/denilsonsa/atmega8-magnetometer-usb-mouse/
It will probably need a few tweaks, though. Basically, after removing
the non-relevant portions of the code (sensor, menu, keyemu), it should
work using a fairly minimalistic ATmega8 circuit.


-- 
Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Chris Friesen]

On 10/22/2011 05:42 AM, Denilson Figueiredo de Sá wrote:

> It may even happen to send an out-of-range value for one axis, but a
> valid value for another axis. The code should be prepared for that
> (ignore one, but keep the other).

In this case what should be used for the "invalid" axis value?  The 
previous value?

Chris

-- 
Chris Friesen
Software Developer
GENBAND
chris.friesen@genband.com
www.genband.com

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Chris Friesen]

On 10/22/2011 05:42 AM, Denilson Figueiredo de Sá wrote:

> It may even happen to send an out-of-range value for one axis, but a
> valid value for another axis. The code should be prepared for that
> (ignore one, but keep the other).

In this case what should be used for the "invalid" axis value?  The 
previous value?

Chris

-- 
Chris Friesen
Software Developer
GENBAND
chris.friesen-b7o/lNNmKxtBDgjK7y7TUQ@public.gmane.org
www.genband.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Denilson Figueiredo de Sá]

On Mon, Oct 24, 2011 at 14:24, Chris Friesen <chris.friesen@genband.com> wrote:
> On 10/22/2011 05:42 AM, Denilson Figueiredo de Sá wrote:
>
>> It may even happen to send an out-of-range value for one axis, but a
>> valid value for another axis. The code should be prepared for that
>> (ignore one, but keep the other).
>
> In this case what should be used for the "invalid" axis value?  The previous
> value?

If that's an absolute pointing device, can we supply only one axis? I
mean, can we do this: "move the pointer to this X position, but leave
the Y position where it currently is". (note that the pointer might
have moved since last input from this device, due to other devices
also being present)

If that's not possible, then I'm not sure about what should be the
correct behavior. Should we ignore both axes if one of them is
invalid? Should we use the last good value from the other axis? (note
that relative pointing devices like proper mice shouldn't have this
issue, as we can just use zero for invalid values, which means "no
movement")

I haven't tested that on Windows (yet), so I don't know how it
behaves. If I test it, I'll post my results here.

I know, this is a corner case difficult to decide about.



By the way, my report was about absolute pointing devices, but I
haven't tested how Linux USB HID behaves for other kinds of devices
(keyboards, gamepads, anything else).


-- 
Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Denilson Figueiredo de Sá]

On Mon, Oct 24, 2011 at 14:24, Chris Friesen <chris.friesen@genband.com> wrote:
> On 10/22/2011 05:42 AM, Denilson Figueiredo de Sá wrote:
>
>> It may even happen to send an out-of-range value for one axis, but a
>> valid value for another axis. The code should be prepared for that
>> (ignore one, but keep the other).
>
> In this case what should be used for the "invalid" axis value?  The previous
> value?

If that's an absolute pointing device, can we supply only one axis? I
mean, can we do this: "move the pointer to this X position, but leave
the Y position where it currently is". (note that the pointer might
have moved since last input from this device, due to other devices
also being present)

If that's not possible, then I'm not sure about what should be the
correct behavior. Should we ignore both axes if one of them is
invalid? Should we use the last good value from the other axis? (note
that relative pointing devices like proper mice shouldn't have this
issue, as we can just use zero for invalid values, which means "no
movement")

I haven't tested that on Windows (yet), so I don't know how it
behaves. If I test it, I'll post my results here.

I know, this is a corner case difficult to decide about.



By the way, my report was about absolute pointing devices, but I
haven't tested how Linux USB HID behaves for other kinds of devices
(keyboards, gamepads, anything else).


-- 
Denilson Figueiredo de Sá
Rio de Janeiro - Brasil
--
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

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Denilson Figueiredo de Sá]

On Mon, Oct 24, 2011 at 15:09, Christoph Fritz <chf.fritz@googlemail.com> wrote:
>
>  * Note that input core does not clamp reported values to the
>  * [minimum, maximum] limits, such task is left to userspace.

That means this is a bug in X.org, instead?

-- 
Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Christoph Fritz]

Hi Denilson,

 please see my comment below.

On Sat, 2011-10-22 at 09:42 -0200, Denilson Figueiredo de Sá wrote:
> Short description:
> 
> An absolute pointing device using USB HID defines a LOGICAL_MINIMUM and
> a LOGICAL_MAXIMUM for X, Y axes, and then sends a HID report containing
> values outside that range.
> 
> Linux kernel should ignore values outside that range, as they are not
> meaningful.

To answer with a quote from the comment above input.h:"struct
input_absinfo":

 *
 * Note that input core does not clamp reported values to the
 * [minimum, maximum] limits, such task is left to userspace.
 *

to do so, use this from input.h:

#define EVIOCGABS(abs)		_IOR('E', 0x40 + (abs), struct input_absinfo)	/* get abs value/limits */
#define EVIOCSABS(abs)		_IOW('E', 0xc0 + (abs), struct input_absinfo)	/* set abs value/limits */


> <SNIP>

Thanks,
 -- chf

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Christoph Fritz]

Hi Denilson,

 please see my comment below.

On Sat, 2011-10-22 at 09:42 -0200, Denilson Figueiredo de Sá wrote:
> Short description:
> 
> An absolute pointing device using USB HID defines a LOGICAL_MINIMUM and
> a LOGICAL_MAXIMUM for X, Y axes, and then sends a HID report containing
> values outside that range.
> 
> Linux kernel should ignore values outside that range, as they are not
> meaningful.

To answer with a quote from the comment above input.h:"struct
input_absinfo":

 *
 * Note that input core does not clamp reported values to the
 * [minimum, maximum] limits, such task is left to userspace.
 *

to do so, use this from input.h:

#define EVIOCGABS(abs)		_IOR('E', 0x40 + (abs), struct input_absinfo)	/* get abs value/limits */
#define EVIOCSABS(abs)		_IOW('E', 0xc0 + (abs), struct input_absinfo)	/* set abs value/limits */


> <SNIP>

Thanks,
 -- chf


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

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Dmitry Torokhov]

Hi Christoph,

On Mon, Oct 24, 2011 at 07:09:43PM +0200, Christoph Fritz wrote:
> Hi Denilson,
> 
>  please see my comment below.
> 
> On Sat, 2011-10-22 at 09:42 -0200, Denilson Figueiredo de Sá wrote:
> > Short description:
> > 
> > An absolute pointing device using USB HID defines a LOGICAL_MINIMUM and
> > a LOGICAL_MAXIMUM for X, Y axes, and then sends a HID report containing
> > values outside that range.
> > 
> > Linux kernel should ignore values outside that range, as they are not
> > meaningful.
> 
> To answer with a quote from the comment above input.h:"struct
> input_absinfo":
> 
>  *
>  * Note that input core does not clamp reported values to the
>  * [minimum, maximum] limits, such task is left to userspace.
>  *
> 
> to do so, use this from input.h:
> 
> #define EVIOCGABS(abs)		_IOR('E', 0x40 + (abs), struct input_absinfo)	/* get abs value/limits */
> #define EVIOCSABS(abs)		_IOW('E', 0xc0 + (abs), struct input_absinfo)	/* set abs value/limits */

While input core does not clam or validate the values reported by the
driver it might make sense to do so in hid-input; I am not sure how
often these limits programmed intorrectly. Jiri i sprobably the best
person to answer this question.

Thanks.

-- 
Dmitry

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Dmitry Torokhov]

On Mon, Oct 24, 2011 at 02:39:31PM -0200, Denilson Figueiredo de Sá wrote:
> On Mon, Oct 24, 2011 at 14:24, Chris Friesen <chris.friesen@genband.com> wrote:
> > On 10/22/2011 05:42 AM, Denilson Figueiredo de Sá wrote:
> >
> >> It may even happen to send an out-of-range value for one axis, but a
> >> valid value for another axis. The code should be prepared for that
> >> (ignore one, but keep the other).
> >
> > In this case what should be used for the "invalid" axis value?  The previous
> > value?
> 
> If that's an absolute pointing device, can we supply only one axis? I
> mean, can we do this: "move the pointer to this X position, but leave
> the Y position where it currently is". (note that the pointer might

Yes, input protocol is stateful and it will actually suppress duplicate
events itself. I.e. if device moves from (10, 20) to (40, 20) usespace
will see:

EV_ABS/ABS_X/10
EV_ABS/ABS_Y/20
EV_SYN/SYN_REPORT
EV_ABS/ABS_X/40
EV_SYN/SYN_REPORT

even if driver emits 2nd EV_ABS/ABS_Y/20 event.

> have moved since last input from this device, due to other devices
> also being present)

The other devices have no effect on this device state.

Thanks.

-- 
Dmitry

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Denilson Figueiredo de Sá]

On Mon, 24 Oct 2011 14:39:31 -0200, Denilson Figueiredo de Sá wrote:
> On Mon, Oct 24, 2011 at 14:24, Chris Friesen wrote:
>> On 10/22/2011 05:42 AM, Denilson Figueiredo de Sá wrote:
>>
>>> It may even happen to send an out-of-range value for one axis, but a
>>> valid value for another axis. The code should be prepared for that
>>> (ignore one, but keep the other).
>>
>> In this case what should be used for the "invalid" axis value?  The  
>> previous value?

> I haven't tested that on Windows (yet), so I don't know how it
> behaves. If I test it, I'll post my results here.

I've modified the firmware of my device to do some more tests. I've  
modified it to send invalid values in X axis, but keep Y axis with valid  
values.

In Linux, the pointer moved to the right-most position of the screen  
(which is consistent with the behavior I previously described), not  
ignoring the out-of-range value.

In Windows, the pointer moved to the left-most position of the screen. It  
means Windows can't send a movement of only one axis, and fills the other  
axis with zero when receiving an invalid value. This behavior doesn't seem  
very helpful.


If anyone cares, this was the firmware modification for this test:
https://bitbucket.org/denilsonsa/atmega8-magnetometer-usb-mouse/changeset/31027323fa0b

-- 
Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Denilson Figueiredo de Sá]

On Mon, 24 Oct 2011 14:39:31 -0200, Denilson Figueiredo de Sá wrote:
> On Mon, Oct 24, 2011 at 14:24, Chris Friesen wrote:
>> On 10/22/2011 05:42 AM, Denilson Figueiredo de Sá wrote:
>>
>>> It may even happen to send an out-of-range value for one axis, but a
>>> valid value for another axis. The code should be prepared for that
>>> (ignore one, but keep the other).
>>
>> In this case what should be used for the "invalid" axis value?  The  
>> previous value?

> I haven't tested that on Windows (yet), so I don't know how it
> behaves. If I test it, I'll post my results here.

I've modified the firmware of my device to do some more tests. I've  
modified it to send invalid values in X axis, but keep Y axis with valid  
values.

In Linux, the pointer moved to the right-most position of the screen  
(which is consistent with the behavior I previously described), not  
ignoring the out-of-range value.

In Windows, the pointer moved to the left-most position of the screen. It  
means Windows can't send a movement of only one axis, and fills the other  
axis with zero when receiving an invalid value. This behavior doesn't seem  
very helpful.


If anyone cares, this was the firmware modification for this test:
https://bitbucket.org/denilsonsa/atmega8-magnetometer-usb-mouse/changeset/31027323fa0b

-- 
Denilson Figueiredo de Sá
Rio de Janeiro - Brasil
--
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

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Jiri Kosina]

On Mon, 24 Oct 2011, Dmitry Torokhov wrote:

> While input core does not clam or validate the values reported by the 
> driver it might make sense to do so in hid-input; I am not sure how 
> often these limits programmed intorrectly. Jiri i sprobably the best 
> person to answer this question.

I haven't met such a device myself yet.

On the other hand I feel like just dropping the report on the floor should 
be the proper thing to do.


Christoph, how about the (untested at my side yet) patch below? It'd be 
nice if you could test with the device you are seeing the problem with and 
let me know.

It's definitely not something I'll be pushing for 3.1 though; I'd first 
like to verify the batch of the device records I have (or I have received) 
to see whehter we can't introduce regressions this way.


From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] HID: ignore absolute values which don't fit between logical min and max

Linux should ignore values outside logical min/max range, as they are not
meaningful. This is what at least some of other OSes do, and it also makes
sense (currently the value gets misinterpreted larger up the stack).

Reported-by: Denilson Figueiredo de Sá <denilsonsa@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/hid/hid-input.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index f333139..ca923de 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -822,6 +822,13 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
+	/* Ignore absolute values that are out of bounds */
+	if ((usage->type == EV_ABS && (value < field->logical_minimum ||
+					value > field->logical_maximum))) {
+		dbg_hid("Ignoring out-of-range value %x\n", value);
+		return;
+	}
+
 	/* report the usage code as scancode if the key status has changed */
 	if (usage->type == EV_KEY && !!test_bit(usage->code, input->key) != value)
 		input_event(input, EV_MSC, MSC_SCAN, usage->hid);
-- 
Jiri Kosina
SUSE Labs

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Jiri Kosina]

On Mon, 24 Oct 2011, Dmitry Torokhov wrote:

> While input core does not clam or validate the values reported by the 
> driver it might make sense to do so in hid-input; I am not sure how 
> often these limits programmed intorrectly. Jiri i sprobably the best 
> person to answer this question.

I haven't met such a device myself yet.

On the other hand I feel like just dropping the report on the floor should 
be the proper thing to do.


Christoph, how about the (untested at my side yet) patch below? It'd be 
nice if you could test with the device you are seeing the problem with and 
let me know.

It's definitely not something I'll be pushing for 3.1 though; I'd first 
like to verify the batch of the device records I have (or I have received) 
to see whehter we can't introduce regressions this way.


From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] HID: ignore absolute values which don't fit between logical min and max

Linux should ignore values outside logical min/max range, as they are not
meaningful. This is what at least some of other OSes do, and it also makes
sense (currently the value gets misinterpreted larger up the stack).

Reported-by: Denilson Figueiredo de Sá <denilsonsa@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/hid/hid-input.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index f333139..ca923de 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -822,6 +822,13 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
+	/* Ignore absolute values that are out of bounds */
+	if ((usage->type == EV_ABS && (value < field->logical_minimum ||
+					value > field->logical_maximum))) {
+		dbg_hid("Ignoring out-of-range value %x\n", value);
+		return;
+	}
+
 	/* report the usage code as scancode if the key status has changed */
 	if (usage->type == EV_KEY && !!test_bit(usage->code, input->key) != value)
 		input_event(input, EV_MSC, MSC_SCAN, usage->hid);
-- 
Jiri Kosina
SUSE Labs
--
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

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Christoph Fritz]

On Fri, 2011-10-28 at 18:23 +0200, Jiri Kosina wrote:
> On Mon, 24 Oct 2011, Dmitry Torokhov wrote:
> 
> > While input core does not clam or validate the values reported by the 
> > driver it might make sense to do so in hid-input; I am not sure how 
> > often these limits programmed intorrectly. Jiri i sprobably the best 
> > person to answer this question.
> 
> I haven't met such a device myself yet.
> 
> On the other hand I feel like just dropping the report on the floor should 
> be the proper thing to do.
> 
> 
> Christoph, how about the (untested at my side yet) patch below? It'd be 
> nice if you could test with the device you are seeing the problem with and 
> let me know.


I don't have this device and I'm not the person who started this thread.
It's Denilson Figueiredo de Sá <denilsonsa@gmail.com> , just see above.

Denilson, please comment.

Thanks,
 -- Christoph

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Christoph Fritz]

On Fri, 2011-10-28 at 18:23 +0200, Jiri Kosina wrote:
> On Mon, 24 Oct 2011, Dmitry Torokhov wrote:
> 
> > While input core does not clam or validate the values reported by the 
> > driver it might make sense to do so in hid-input; I am not sure how 
> > often these limits programmed intorrectly. Jiri i sprobably the best 
> > person to answer this question.
> 
> I haven't met such a device myself yet.
> 
> On the other hand I feel like just dropping the report on the floor should 
> be the proper thing to do.
> 
> 
> Christoph, how about the (untested at my side yet) patch below? It'd be 
> nice if you could test with the device you are seeing the problem with and 
> let me know.


I don't have this device and I'm not the person who started this thread.
It's Denilson Figueiredo de Sá <denilsonsa@gmail.com> , just see above.

Denilson, please comment.

Thanks,
 -- Christoph


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

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Denilson Figueiredo de Sá]

On Fri, 28 Oct 2011 14:23:17 -0200, Jiri Kosina <jkosina@suse.cz> wrote:

> On the other hand I feel like just dropping the report on the floor  
> should be the proper thing to do.

I'd say not the entire report, but just the invalid fields.

The HID specification (HID1_11.pdf) talks about "Null Values" at the  
section 5.10 (page 20).

[quote]
HID class devices support the ability to ignore selected fields in a  
report at run-
time. This is accomplished by declaring bit field in a report that is  
capable of
containing a range of values larger than those actually generated by the  
control. If
the host or the device receives an out-of-range value then the current  
value for the
respective control will not be modified.
[/quote]


(sidenote: Why didn't I mention this spec earlier? Because I knew I have  
read that information somewhere, but didn't find it again until today.)


> Christoph, how about the (untested at my side yet) patch below? It'd be
> nice if you could test with the device you are seeing the problem with  
> and let me know.

I can't test it *right now*, but I can probably test it in a week or two.


> +	/* Ignore absolute values that are out of bounds */
> +	if ((usage->type == EV_ABS && (value < field->logical_minimum ||
> +					value > field->logical_maximum))) {

Given what the HID spec says (as I quoted above), I'd change this  
condition to just:

> +	if (value < field->logical_minimum ||
> +					value > field->logical_maximum) {

On the other hand, HID spec also defines the "Null" bit of Input fields at  
page 31. This Null bit seems redundant for me, as (in my understanding)  
any value outside the range is automatically a null value. We wouldn't  
need a Null bit for that.

Anyway, another solution is to change that condition to something like  
this:

> +	if ( THIS_INPUT_FIELD_HAS_NULL_BIT_SET &&
> +                      (value < field->logical_minimum ||
> +					value > field->logical_maximum)) {

What do you think?


-- 
Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Jiri Kosina]

On Fri, 28 Oct 2011, Denilson Figueiredo de Sá wrote:

> > On the other hand I feel like just dropping the report on the floor should
> > be the proper thing to do.
> 
> I'd say not the entire report, but just the invalid fields.

Right, which is what my patch actually does, just my fingers on keyboard 
were faster than my brain while writing that.

> The HID specification (HID1_11.pdf) talks about "Null Values" at the section
> 5.10 (page 20).
> 
> [quote]
> HID class devices support the ability to ignore selected fields in a 
> report at run- time. This is accomplished by declaring bit field in a 
> report that is capable of containing a range of values larger than those 
> actually generated by the control. If the host or the device receives an 
> out-of-range value then the current value for the respective control 
> will not be modified.
> [/quote]
> 
> (sidenote: Why didn't I mention this spec earlier? Because I knew I have 
> read that information somewhere, but didn't find it again until today.)

I personally find the wording of the spec here a bit unfortunate (the 
'declaring bit field in a report that is capable of containing a range of 
values largen than those actually generated by the control' seems to be a 
bit too foggy and vague).

> > Christoph, how about the (untested at my side yet) patch below? It'd be
> > nice if you could test with the device you are seeing the problem with and
> > let me know.
> 
> I can't test it *right now*, but I can probably test it in a week or two.

Thanks. No real rush here, as I will definitely not be queuing this for 
3.2 anyway, 3.3 is the first release I'd consider this for.

> > +	/* Ignore absolute values that are out of bounds */
> > +	if ((usage->type == EV_ABS && (value < field->logical_minimum ||
> > +					value > field->logical_maximum))) {
> 
> Given what the HID spec says (as I quoted above), I'd change this condition to
> just:
> 
> > +	if (value < field->logical_minimum ||
> > +					value > field->logical_maximum) {

After thinking about it a little bit more, I think I agree.

I will still try to evaluate this against a number of devices I have (or I 
have reports for), to see if we potentially really can't break something.

So please let me know the result of your testing.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Jiri Kosina]

On Fri, 28 Oct 2011, Denilson Figueiredo de Sá wrote:

> > On the other hand I feel like just dropping the report on the floor should
> > be the proper thing to do.
> 
> I'd say not the entire report, but just the invalid fields.

Right, which is what my patch actually does, just my fingers on keyboard 
were faster than my brain while writing that.

> The HID specification (HID1_11.pdf) talks about "Null Values" at the section
> 5.10 (page 20).
> 
> [quote]
> HID class devices support the ability to ignore selected fields in a 
> report at run- time. This is accomplished by declaring bit field in a 
> report that is capable of containing a range of values larger than those 
> actually generated by the control. If the host or the device receives an 
> out-of-range value then the current value for the respective control 
> will not be modified.
> [/quote]
> 
> (sidenote: Why didn't I mention this spec earlier? Because I knew I have 
> read that information somewhere, but didn't find it again until today.)

I personally find the wording of the spec here a bit unfortunate (the 
'declaring bit field in a report that is capable of containing a range of 
values largen than those actually generated by the control' seems to be a 
bit too foggy and vague).

> > Christoph, how about the (untested at my side yet) patch below? It'd be
> > nice if you could test with the device you are seeing the problem with and
> > let me know.
> 
> I can't test it *right now*, but I can probably test it in a week or two.

Thanks. No real rush here, as I will definitely not be queuing this for 
3.2 anyway, 3.3 is the first release I'd consider this for.

> > +	/* Ignore absolute values that are out of bounds */
> > +	if ((usage->type == EV_ABS && (value < field->logical_minimum ||
> > +					value > field->logical_maximum))) {
> 
> Given what the HID spec says (as I quoted above), I'd change this condition to
> just:
> 
> > +	if (value < field->logical_minimum ||
> > +					value > field->logical_maximum) {

After thinking about it a little bit more, I think I agree.

I will still try to evaluate this against a number of devices I have (or I 
have reports for), to see if we potentially really can't break something.

So please let me know the result of your testing.

Thanks,

-- 
Jiri Kosina
SUSE Labs
--
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

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Denilson Figueiredo de Sá]

On Mon, 31 Oct 2011 13:24:09 -0200, Jiri Kosina <jkosina@suse.cz> wrote:

> I personally find the wording of the spec here a bit unfortunate (the
> 'declaring bit field in a report that is capable of containing a range of
> values largen than those actually generated by the control' seems to be a
> bit too foggy and vague).

I don't think it is vague.

"declaring bit field in a report" -> Input()
"that is capable of containing a range of values" -> Report_size()
"larger than those actually generated by the control" -> The entire range  
repreented using Report_size bits is larger than the Logical  
minimum/maximum,

For me, that paragraph is not foggy. Maybe other pieces of the spec are,  
but that one seemed clear for me.

>> > +	if (value < field->logical_minimum ||
>> > +					value > field->logical_maximum) {
>
> After thinking about it a little bit more, I think I agree.
[...]
> So please let me know the result of your testing.

Seems to work on my device, thanks!
I haven't done more than a couple of minutes of testing, though. But it  
seems to work.


(another test below)

I've also tested the behavior when one axis is valid, while the other one  
as an invalid (and thus discarded) value.

This was based on a question from Chris Friesen:  
https://lkml.org/lkml/2011/10/24/290

Running "evtest /dev/input/event9" only shows the valid axis. That's good,  
it seems invalid values don't generate events.

But Xorg moves the pointer in both axes at once, instead of just one of  
them.
IMHO, that's not perfect, but it seems good enough, specially considering  
this is a somewhat unlikely edge-case (remember I'm talking about one axis  
valid and another discarded). It might also be a bug in Xorg.


(if anything in this message doesn't make much sense, that's probably  
because I need some sleep - but I wanted to give you feedback as soon as I  
could)


-- 
Denilson Figueiredo de Sá
Rio de Janeiro - Brasil

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Jiri Kosina]

On Wed, 2 Nov 2011, Denilson Figueiredo de Sá wrote:

> > > > +	if (value < field->logical_minimum ||
> > > > +					value > field->logical_maximum) {
> > 
> > After thinking about it a little bit more, I think I agree.
> [...]
> > So please let me know the result of your testing.
> 
> Seems to work on my device, thanks!

Thank you. I have now queued the patch with your Tested-by.

-- 
Jiri Kosina
SUSE Labs

Re: Linux USB HID should ignore values outside Logical Minimum/Maximum range

[Jiri Kosina]

On Wed, 2 Nov 2011, Denilson Figueiredo de Sá wrote:

> > > > +	if (value < field->logical_minimum ||
> > > > +					value > field->logical_maximum) {
> > 
> > After thinking about it a little bit more, I think I agree.
> [...]
> > So please let me know the result of your testing.
> 
> Seems to work on my device, thanks!

Thank you. I have now queued the patch with your Tested-by.

-- 
Jiri Kosina
SUSE Labs
--
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