On Thu, Mar 09, 2017 at 09:16:06AM +0100, Benjamin Tissoires wrote: > We have a "clamp()" function in the kernel that does the job directly > and which is more readable. Also, this makes testing the out of range > values twice. > > How about: > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index cf8256a..781f400 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct > > /* > * Ignore out-of-range values as per HID specification, > - * section 5.10 and 6.2.25. > + * section 5.10 and 6.2.25, when NULL state bit is present. > + * When it's not, clamp the value to match Microsoft's input > + * driver as mentioned in "Required HID usages for digitizers": > + * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp > * > * The logical_minimum < logical_maximum check is done so that we > * don't unintentionally discard values sent by devices which > * don't specify logical min and max. > */ > if ((field->flags & HID_MAIN_ITEM_VARIABLE) && > - (field->flags & HID_MAIN_ITEM_NULL_STATE) && > - (field->logical_minimum < field->logical_maximum) && > - (value < field->logical_minimum || > - value > field->logical_maximum)) { > - dbg_hid("Ignoring out-of-range value %x\n", value); > - return; > + (field->logical_minimum < field->logical_maximum)) { > } Yes, I don't mind the expansion of the comment and the usage of clamp (I didn't know this existed, but I will use it in the future). However if there is anything I would change, it would be this: --- drivers/hid/hid-input.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index cf8256aac2bd..a1ebdd7d4d4d 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct /* * Ignore out-of-range values as per HID specification, - * section 5.10 and 6.2.25. + * section 5.10 and 6.2.25, when NULL state bit is present. + * When it's not, clamp the value to match Microsoft's input + * driver as mentioned in "Required HID usages for digitizers": + * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp * * The logical_minimum < logical_maximum check is done so that we * don't unintentionally discard values sent by devices which * don't specify logical min and max. */ if ((field->flags & HID_MAIN_ITEM_VARIABLE) && - (field->flags & HID_MAIN_ITEM_NULL_STATE) && - (field->logical_minimum < field->logical_maximum) && - (value < field->logical_minimum || - value > field->logical_maximum)) { - dbg_hid("Ignoring out-of-range value %x\n", value); - return; + (field->logical_minimum < field->logical_maximum)) { + if (field->flags & HID_MAIN_ITEM_NULL_STATE && + (value < field->logical_minimum || + value > field->logical_maximum)) { + dbg_hid("Ignoring out-of-range value %x\n", value); + return; + } + value = clamp(value, + field->logical_minimum, + field->logical_maximum); } /* -- 2.12.0 For me it is a bit clearer on what is happening and still avoids doing the range check twice. But ultimately it is all up to you guys. I can get both versions of this patch tested at some point in the next few days and re-submit whichever one you prefer as a v2. I'm not sure what the procedures are on this, should I put a "Suggested-by:" for your suggested change to my patch, or is that not applicable here? As always, thanks for your time. -- Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
