[PATCH 00/12] HID: fix for generic input processing

[PATCH 00/12] HID: fix for generic input processing

[Benjamin Tissoires]

Hi,

This is a followup of the discussion we had between Wacom and
the maintainers, and a followup of those 2 patch series:

https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/

It took me a while to get it right, but I finally can submit the
series:

- the first 8 patches are some cleanup in the hid-input.c and
  hid-core.c code. They also create a list of input fields that
  is then used to process the event, in the priority we think
  is good.

  For instance, on multitouch devices, it is better to have
  Contact Count before processing all touches, and in each
  touch, having Contact ID first is better. This series doesn't
  cover hid-multitouch, but I have a series on top of this one that
  does cover it.

  Anyway, in our case, here, we need to process Invert before
  In Range for tablets so we can make a decision whether the user
  has the intend to erase or not.

- patch 9 enforces the invert usage before In Range as mentioned
  above

- patch 10 is the actual bulk of processing that should fix the
  generic tablet handling. Now that we have a reliable ordering
  of the fields, we can compute the state of the tool in a reliable
  way, and be kinder to userspace by not sending to it 2 tools at
  the same time.

  This patch has been extensively tested by hid-tools with the new
  MR I submitted that add tests for tablets [0].

- patch 11 is a nice to have that I need for my second series regarding
  hid-multitouch. It is not mandatory with that series, but given
  that it changes the format of the priorities in hid-input.c I thought
  it would be best to send it as part of this series.

  Note that now we are tagging the *reports* and the individual fields
  when they are part of a multitouch collection, which should help
  the drivers that implement this processing (hid-multitouch and wacom).

- last, patch 12 is an attempt at fixing the documentation regarding
  BTN_TOOL_* (requested by Peter).

  Dmitry, feel free to take this one through your tree if you prefer
  to do so (and if you are happy with it), otherwise we can take it
  through the hid tree.

As mentioned above, I have a followup series not entirely tidied up
that implements the processing of Win8 mutltiouch devices in
hid-input.c.
There are several benefits for that: we should be able to drop the
multitouch code in wacom.ko, we can simplify part of hid-multitouch,
and we will be able to quirk a particular device in a separate module,
without touching at the generic code (in hid-multitouch or hid-input).

Anyway, I am missing a few bits for that so that's coming in later.

Cheers,
Benjamin


[0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127

Benjamin Tissoires (12):
  HID: core: statically allocate read buffers
  HID: core: de-duplicate some code in hid_input_field()
  HID: core: split data fetching from processing in hid_input_field()
  HID: input: tag touchscreens as such if the physical is not there
  HID: input: rework spaghetti code with switch statements
  HID: input: move up out-of-range processing of input values
  HID: compute an ordered list of input fields to process
  HID: core: for input reports, process the usages by priority list
  HID: input: enforce Invert usage to be processed before InRange
  HID: input: remove the need for HID_QUIRK_INVERT
  HID: input: accommodate priorities for slotted devices
  Input: docs: add more details on the use of BTN_TOOL

 Documentation/input/event-codes.rst |   5 +-
 drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
 drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
 include/linux/hid.h                 |  23 +-
 4 files changed, 533 insertions(+), 105 deletions(-)

-- 
2.33.1

[PATCH 01/12] HID: core: statically allocate read buffers

[Benjamin Tissoires]

This is a preparation patch for rethinking the generic processing
of HID reports.

We can actually pre-allocate all of our memory instead of dynamically
allocating/freeing it whenever we parse a report.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 12 +++++-------
 include/linux/hid.h    |  1 +
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index f1aed5bbd000..75e7b8447bf7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
 
 	field = kzalloc((sizeof(struct hid_field) +
 			 usages * sizeof(struct hid_usage) +
-			 usages * sizeof(unsigned)), GFP_KERNEL);
+			 2 * usages * sizeof(unsigned int)), GFP_KERNEL);
 	if (!field)
 		return NULL;
 
@@ -109,6 +109,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
 	report->field[field->index] = field;
 	field->usage = (struct hid_usage *)(field + 1);
 	field->value = (s32 *)(field->usage + usages);
+	field->new_value = (s32 *)(field->value + usages);
 	field->report = report;
 
 	return field;
@@ -1541,9 +1542,8 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 	__s32 max = field->logical_maximum;
 	__s32 *value;
 
-	value = kmalloc_array(count, sizeof(__s32), GFP_ATOMIC);
-	if (!value)
-		return;
+	value = field->new_value;
+	memset(value, 0, count * sizeof(__s32));
 
 	for (n = 0; n < count; n++) {
 
@@ -1557,7 +1557,7 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 		    value[n] >= min && value[n] <= max &&
 		    value[n] - min < field->maxusage &&
 		    field->usage[value[n] - min].hid == HID_UP_KEYBOARD + 1)
-			goto exit;
+			return;
 	}
 
 	for (n = 0; n < count; n++) {
@@ -1581,8 +1581,6 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 	}
 
 	memcpy(field->value, value, count * sizeof(__s32));
-exit:
-	kfree(value);
 }
 
 /*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7487b0586fe6..3fbfe0986659 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -476,6 +476,7 @@ struct hid_field {
 	unsigned  report_count;		/* number of this field in the report */
 	unsigned  report_type;		/* (input,output,feature) */
 	__s32    *value;		/* last known value(s) */
+	__s32    *new_value;		/* newly read value(s) */
 	__s32     logical_minimum;
 	__s32     logical_maximum;
 	__s32     physical_minimum;
-- 
2.33.1

[PATCH 02/12] HID: core: de-duplicate some code in hid_input_field()

[Benjamin Tissoires]

I had to go twice through the history to get a grasp at this code.
De-duplicate the various tests in one common helper to make it
more explicit.

Note that the `HID_UP_KEYBOARD + 1` condition is tested through
https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/121

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 54 ++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 75e7b8447bf7..fad4dbdf6391 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1525,6 +1525,24 @@ static void hid_process_event(struct hid_device *hid, struct hid_field *field,
 		hid->hiddev_hid_event(hid, field, usage, value);
 }
 
+/*
+ * Checks if the given value is valid within this field
+ */
+static inline int hid_array_value_is_valid(struct hid_field *field,
+					   __s32 value)
+{
+	__s32 min = field->logical_minimum;
+
+	/*
+	 * Value needs to be between logical min and max, and
+	 * (value - min) is used as an index in the usage array.
+	 * This array is of size field->maxusage
+	 */
+	return value >= min &&
+	       value <= field->logical_maximum &&
+	       value - min < field->maxusage;
+}
+
 /*
  * Analyse a received field, and fetch the data from it. The field
  * content is stored for next report processing (we do differential
@@ -1539,7 +1557,6 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 	unsigned offset = field->report_offset;
 	unsigned size = field->report_size;
 	__s32 min = field->logical_minimum;
-	__s32 max = field->logical_maximum;
 	__s32 *value;
 
 	value = field->new_value;
@@ -1554,8 +1571,7 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 
 		/* Ignore report if ErrorRollOver */
 		if (!(field->flags & HID_MAIN_ITEM_VARIABLE) &&
-		    value[n] >= min && value[n] <= max &&
-		    value[n] - min < field->maxusage &&
+		    hid_array_value_is_valid(field, value[n]) &&
 		    field->usage[value[n] - min].hid == HID_UP_KEYBOARD + 1)
 			return;
 	}
@@ -1563,21 +1579,29 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 	for (n = 0; n < count; n++) {
 
 		if (HID_MAIN_ITEM_VARIABLE & field->flags) {
-			hid_process_event(hid, field, &field->usage[n], value[n], interrupt);
+			hid_process_event(hid,
+					  field,
+					  &field->usage[n],
+					  value[n],
+					  interrupt);
 			continue;
 		}
 
-		if (field->value[n] >= min && field->value[n] <= max
-			&& field->value[n] - min < field->maxusage
-			&& field->usage[field->value[n] - min].hid
-			&& search(value, field->value[n], count))
-				hid_process_event(hid, field, &field->usage[field->value[n] - min], 0, interrupt);
-
-		if (value[n] >= min && value[n] <= max
-			&& value[n] - min < field->maxusage
-			&& field->usage[value[n] - min].hid
-			&& search(field->value, value[n], count))
-				hid_process_event(hid, field, &field->usage[value[n] - min], 1, interrupt);
+		if (hid_array_value_is_valid(field, field->value[n]) &&
+		    search(value, field->value[n], count))
+			hid_process_event(hid,
+					  field,
+					  &field->usage[field->value[n] - min],
+					  0,
+					  interrupt);
+
+		if (hid_array_value_is_valid(field, value[n]) &&
+		    search(field->value, value[n], count))
+			hid_process_event(hid,
+					  field,
+					  &field->usage[value[n] - min],
+					  1,
+					  interrupt);
 	}
 
 	memcpy(field->value, value, count * sizeof(__s32));
-- 
2.33.1

[PATCH 03/12] HID: core: split data fetching from processing in hid_input_field()

[Benjamin Tissoires]

This is a preparatory patch for being able to process the usages
out of order. We split the retrieval of the data in a separate function
and also split out the processing of the usages depending if the field
is an array or a variable.

No functional changes from this patch.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 96 +++++++++++++++++++++++++++++++++---------
 include/linux/hid.h    |  3 +-
 2 files changed, 79 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index fad4dbdf6391..34188d7ac0f7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1544,13 +1544,12 @@ static inline int hid_array_value_is_valid(struct hid_field *field,
 }
 
 /*
- * Analyse a received field, and fetch the data from it. The field
- * content is stored for next report processing (we do differential
- * reporting to the layer).
+ * Fetch the field from the data. The field content is stored for next
+ * report processing (we do differential reporting to the layer).
  */
-
-static void hid_input_field(struct hid_device *hid, struct hid_field *field,
-			    __u8 *data, int interrupt)
+static void hid_input_fetch_field(struct hid_device *hid,
+				  struct hid_field *field,
+				  __u8 *data)
 {
 	unsigned n;
 	unsigned count = field->report_count;
@@ -1561,6 +1560,7 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 
 	value = field->new_value;
 	memset(value, 0, count * sizeof(__s32));
+	field->ignored = false;
 
 	for (n = 0; n < count; n++) {
 
@@ -1572,21 +1572,56 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 		/* Ignore report if ErrorRollOver */
 		if (!(field->flags & HID_MAIN_ITEM_VARIABLE) &&
 		    hid_array_value_is_valid(field, value[n]) &&
-		    field->usage[value[n] - min].hid == HID_UP_KEYBOARD + 1)
+		    field->usage[value[n] - min].hid == HID_UP_KEYBOARD + 1) {
+			field->ignored = true;
 			return;
+		}
 	}
+}
 
-	for (n = 0; n < count; n++) {
+/*
+ * Process a received variable field.
+ */
 
-		if (HID_MAIN_ITEM_VARIABLE & field->flags) {
-			hid_process_event(hid,
-					  field,
-					  &field->usage[n],
-					  value[n],
-					  interrupt);
-			continue;
-		}
+static void hid_input_var_field(struct hid_device *hid,
+				struct hid_field *field,
+				int interrupt)
+{
+	unsigned int count = field->report_count;
+	__s32 *value = field->new_value;
+	unsigned int n;
+
+	for (n = 0; n < count; n++)
+		hid_process_event(hid,
+				  field,
+				  &field->usage[n],
+				  value[n],
+				  interrupt);
+
+	memcpy(field->value, value, count * sizeof(__s32));
+}
 
+/*
+ * Process a received array field. The field content is stored for
+ * next report processing (we do differential reporting to the layer).
+ */
+
+static void hid_input_array_field(struct hid_device *hid,
+				  struct hid_field *field,
+				  int interrupt)
+{
+	unsigned int n;
+	unsigned int count = field->report_count;
+	__s32 min = field->logical_minimum;
+	__s32 *value;
+
+	value = field->new_value;
+
+	/* ErrorRollOver */
+	if (field->ignored)
+		return;
+
+	for (n = 0; n < count; n++) {
 		if (hid_array_value_is_valid(field, field->value[n]) &&
 		    search(value, field->value[n], count))
 			hid_process_event(hid,
@@ -1607,6 +1642,31 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 	memcpy(field->value, value, count * sizeof(__s32));
 }
 
+/*
+ * Analyse a received report, and fetch the data from it. The field
+ * content is stored for next report processing (we do differential
+ * reporting to the layer).
+ */
+static void hid_process_report(struct hid_device *hid,
+			       struct hid_report *report,
+			       __u8 *data,
+			       int interrupt)
+{
+	unsigned int a;
+	struct hid_field *field;
+
+	for (a = 0; a < report->maxfield; a++) {
+		field = report->field[a];
+
+		hid_input_fetch_field(hid, field, data);
+
+		if (field->flags & HID_MAIN_ITEM_VARIABLE)
+			hid_input_var_field(hid, field, interrupt);
+		else
+			hid_input_array_field(hid, field, interrupt);
+	}
+}
+
 /*
  * Output the field into the report.
  */
@@ -1768,7 +1828,6 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
 	struct hid_report_enum *report_enum = hid->report_enum + type;
 	struct hid_report *report;
 	struct hid_driver *hdrv;
-	unsigned int a;
 	u32 rsize, csize = size;
 	u8 *cdata = data;
 	int ret = 0;
@@ -1804,8 +1863,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
 	}
 
 	if (hid->claimed != HID_CLAIMED_HIDRAW && report->maxfield) {
-		for (a = 0; a < report->maxfield; a++)
-			hid_input_field(hid, report->field[a], cdata, interrupt);
+		hid_process_report(hid, report, cdata, interrupt);
 		hdrv = hid->driver;
 		if (hdrv && hdrv->report)
 			hdrv->report(hid, report);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 3fbfe0986659..cf79eb3da465 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -342,7 +342,7 @@ struct hid_item {
  * HID device quirks.
  */
 
-/* 
+/*
  * Increase this if you need to configure more HID quirks at module load time
  */
 #define MAX_USBHID_BOOT_QUIRKS 4
@@ -483,6 +483,7 @@ struct hid_field {
 	__s32     physical_maximum;
 	__s32     unit_exponent;
 	unsigned  unit;
+	bool      ignored;		/* this field is ignored in this event */
 	struct hid_report *report;	/* associated report */
 	unsigned index;			/* index into report->field[] */
 	/* hidinput data */
-- 
2.33.1

[PATCH 04/12] HID: input: tag touchscreens as such if the physical is not there

[Benjamin Tissoires]

Some devices (Elan, Synaptics...) are sometimes not setting a physical
in their finger collections. hid-input will consider them to be pen
devices, leading to some wrong behavior in user space.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 112901d2d8d2..d2562497a726 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -828,10 +828,31 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			break;
 
 		case 0x32: /* InRange */
-			switch (field->physical & 0xff) {
-			case 0x21: map_key(BTN_TOOL_MOUSE); break;
-			case 0x22: map_key(BTN_TOOL_FINGER); break;
-			default: map_key(BTN_TOOL_PEN); break;
+			switch (field->physical) {
+			case HID_DG_PUCK:
+				map_key(BTN_TOOL_MOUSE);
+				break;
+			case HID_DG_FINGER:
+				map_key(BTN_TOOL_FINGER);
+				break;
+			default:
+				/*
+				 * If the physical is not given,
+				 * rely on the application.
+				 */
+				if (!field->physical) {
+					switch (field->application) {
+					case HID_DG_TOUCHSCREEN:
+					case HID_DG_TOUCHPAD:
+						map_key_clear(BTN_TOOL_FINGER);
+						break;
+					default:
+						map_key_clear(BTN_TOOL_PEN);
+					}
+				} else {
+					map_key(BTN_TOOL_PEN);
+				}
+				break;
 			}
 			break;
 
-- 
2.33.1

[PATCH 05/12] HID: input: rework spaghetti code with switch statements

[Benjamin Tissoires]

Instead of using multiple `if (a == b)`, use the switch statement
which has been done exactly for that.

There should be no functional change (I don't expect moving down
HID_QUIRK_{X|Y}_INVERT having any impact.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 80 ++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index d2562497a726..54b3e9c5ccc4 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1354,12 +1354,6 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 
 	input = field->hidinput->input;
 
-	if (usage->type == EV_ABS &&
-	    (((*quirks & HID_QUIRK_X_INVERT) && usage->code == ABS_X) ||
-	     ((*quirks & HID_QUIRK_Y_INVERT) && usage->code == ABS_Y))) {
-		value = field->logical_maximum - value;
-	}
-
 	if (usage->hat_min < usage->hat_max || usage->hat_dir) {
 		int hat_dir = usage->hat_dir;
 		if (!hat_dir)
@@ -1370,12 +1364,12 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
-	if (usage->hid == HID_DG_INVERT) {
+	switch (usage->hid) {
+	case HID_DG_INVERT:
 		*quirks = value ? (*quirks | HID_QUIRK_INVERT) : (*quirks & ~HID_QUIRK_INVERT);
 		return;
-	}
 
-	if (usage->hid == HID_DG_INRANGE) {
+	case HID_DG_INRANGE:
 		if (value) {
 			input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1);
 			return;
@@ -1383,46 +1377,58 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		input_event(input, usage->type, usage->code, 0);
 		input_event(input, usage->type, BTN_TOOL_RUBBER, 0);
 		return;
-	}
 
-	if (usage->hid == HID_DG_TIPPRESSURE && (*quirks & HID_QUIRK_NOTOUCH)) {
-		int a = field->logical_minimum;
-		int b = field->logical_maximum;
-		input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
-	}
+	case HID_DG_TIPPRESSURE:
+		if (*quirks & HID_QUIRK_NOTOUCH) {
+			int a = field->logical_minimum;
+			int b = field->logical_maximum;
+
+			input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
+		}
+		break;
 
-	if (usage->hid == (HID_UP_PID | 0x83UL)) { /* Simultaneous Effects Max */
+	case HID_UP_PID | 0x83UL: /* Simultaneous Effects Max */
 		dbg_hid("Maximum Effects - %d\n",value);
 		return;
-	}
 
-	if (usage->hid == (HID_UP_PID | 0x7fUL)) {
+	case HID_UP_PID | 0x7fUL:
 		dbg_hid("PID Pool Report\n");
 		return;
 	}
 
-	if ((usage->type == EV_KEY) && (usage->code == 0)) /* Key 0 is "unassigned", not KEY_UNKNOWN */
-		return;
+	switch (usage->type) {
+	case EV_KEY:
+		if (usage->code == 0) /* Key 0 is "unassigned", not KEY_UNKNOWN */
+			return;
+		break;
 
-	if ((usage->type == EV_REL) && (usage->code == REL_WHEEL_HI_RES ||
-					usage->code == REL_HWHEEL_HI_RES)) {
-		hidinput_handle_scroll(usage, input, value);
-		return;
-	}
+	case EV_REL:
+		if (usage->code == REL_WHEEL_HI_RES ||
+		    usage->code == REL_HWHEEL_HI_RES) {
+			hidinput_handle_scroll(usage, input, value);
+			return;
+		}
+		break;
 
-	if ((usage->type == EV_ABS) && (field->flags & HID_MAIN_ITEM_RELATIVE) &&
-			(usage->code == ABS_VOLUME)) {
-		int count = abs(value);
-		int direction = value > 0 ? KEY_VOLUMEUP : KEY_VOLUMEDOWN;
-		int i;
+	case EV_ABS:
+		if ((field->flags & HID_MAIN_ITEM_RELATIVE) &&
+		    usage->code == ABS_VOLUME) {
+			int count = abs(value);
+			int direction = value > 0 ? KEY_VOLUMEUP : KEY_VOLUMEDOWN;
+			int i;
+
+			for (i = 0; i < count; i++) {
+				input_event(input, EV_KEY, direction, 1);
+				input_sync(input);
+				input_event(input, EV_KEY, direction, 0);
+				input_sync(input);
+			}
+			return;
 
-		for (i = 0; i < count; i++) {
-			input_event(input, EV_KEY, direction, 1);
-			input_sync(input);
-			input_event(input, EV_KEY, direction, 0);
-			input_sync(input);
-		}
-		return;
+		} else if (((*quirks & HID_QUIRK_X_INVERT) && usage->code == ABS_X) ||
+			   ((*quirks & HID_QUIRK_Y_INVERT) && usage->code == ABS_Y))
+			value = field->logical_maximum - value;
+		break;
 	}
 
 	/*
-- 
2.33.1

[PATCH 06/12] HID: input: move up out-of-range processing of input values

[Benjamin Tissoires]

It actually makes sense to clamp the value to its boundaries before
doing further processing.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 48 ++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 54b3e9c5ccc4..8770d9a2b2af 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1364,6 +1364,30 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
+	/*
+	 * Ignore out-of-range values as per HID specification,
+	 * 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->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);
+	}
+
 	switch (usage->hid) {
 	case HID_DG_INVERT:
 		*quirks = value ? (*quirks | HID_QUIRK_INVERT) : (*quirks & ~HID_QUIRK_INVERT);
@@ -1431,30 +1455,6 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		break;
 	}
 
-	/*
-	 * Ignore out-of-range values as per HID specification,
-	 * 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->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);
-	}
-
 	/*
 	 * Ignore reports for absolute data if the data didn't change. This is
 	 * not only an optimization but also fixes 'dead' key reports. Some
-- 
2.33.1

[PATCH 07/12] HID: compute an ordered list of input fields to process

[Benjamin Tissoires]

This will be used in a later commit:
we build a list of input fields (and usage_index) that is ordered based
on a usage priority.

Changing the usage priority allows to re-order the processed list, meaning
that we can enforce some usages to be process before others.

For instance, before processing InRange in the HID tablets, we need to
know if we are using the eraser (side or button). Enforcing a higher
(lower number) priority for Invert allows to force the input stack to
process that field before.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 105 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/hid.h    |  10 ++++
 2 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 34188d7ac0f7..b38528118642 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -81,6 +81,7 @@ struct hid_report *hid_register_report(struct hid_device *device,
 	report_enum->report_id_hash[id] = report;
 
 	list_add_tail(&report->list, &report_enum->report_list);
+	INIT_LIST_HEAD(&report->field_entry_list);
 
 	return report;
 }
@@ -101,7 +102,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
 
 	field = kzalloc((sizeof(struct hid_field) +
 			 usages * sizeof(struct hid_usage) +
-			 2 * usages * sizeof(unsigned int)), GFP_KERNEL);
+			 3 * usages * sizeof(unsigned int)), GFP_KERNEL);
 	if (!field)
 		return NULL;
 
@@ -110,6 +111,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
 	field->usage = (struct hid_usage *)(field + 1);
 	field->value = (s32 *)(field->usage + usages);
 	field->new_value = (s32 *)(field->value + usages);
+	field->usages_priorities = (s32 *)(field->new_value + usages);
 	field->report = report;
 
 	return field;
@@ -657,6 +659,8 @@ static void hid_free_report(struct hid_report *report)
 {
 	unsigned n;
 
+	kfree(report->field_entries);
+
 	for (n = 0; n < report->maxfield; n++)
 		kfree(report->field[n]);
 	kfree(report);
@@ -1667,6 +1671,103 @@ static void hid_process_report(struct hid_device *hid,
 	}
 }
 
+/*
+ * Insert a given usage_index in a field in the list
+ * of processed usages in the report.
+ *
+ * The elements of lower priority score are processed
+ * first.
+ */
+static void __hid_insert_field_entry(struct hid_device *hid,
+				     struct hid_report *report,
+				     struct hid_field_entry *entry,
+				     struct hid_field *field,
+				     unsigned int usage_index)
+{
+	struct hid_field_entry *next;
+
+	entry->field = field;
+	entry->index = usage_index;
+	entry->priority = field->usages_priorities[usage_index];
+
+	/* insert the element at the correct position */
+	list_for_each_entry(next,
+			    &report->field_entry_list,
+			    list) {
+		/*
+		 * the priority of our element is strictly higher
+		 * than the next one, insert it before
+		 */
+		if (entry->priority > next->priority) {
+			list_add_tail(&entry->list, &next->list);
+			return;
+		}
+	}
+
+	/* lowest priority score: insert at the end */
+	list_add_tail(&entry->list, &report->field_entry_list);
+}
+
+static void hid_report_process_ordering(struct hid_device *hid,
+					struct hid_report *report)
+{
+	struct hid_field *field;
+	struct hid_field_entry *entries;
+	unsigned int a, u, usages;
+	unsigned int count = 0;
+
+	/* count the number of individual fields in the report */
+	for (a = 0; a < report->maxfield; a++) {
+		field = report->field[a];
+
+		if (field->flags & HID_MAIN_ITEM_VARIABLE)
+			count += field->report_count;
+		else
+			count++;
+	}
+
+	/* allocate the memory to process the fields */
+	entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
+	if (!entries)
+		return;
+
+	report->field_entries = entries;
+
+	/*
+	 * walk through all fields in the report and
+	 * store them by priority order in report->field_entry_list
+	 *
+	 * - Var elements are individualized (field + usage_index)
+	 * - Arrays are taken as one, we can not chose an order for them
+	 */
+	usages = 0;
+	for (a = 0; a < report->maxfield; a++) {
+		field = report->field[a];
+
+		if (field->flags & HID_MAIN_ITEM_VARIABLE) {
+			for (u = 0; u < field->report_count; u++) {
+				__hid_insert_field_entry(hid, report,
+							 &entries[usages],
+							 field, u);
+				usages++;
+			}
+		} else {
+			__hid_insert_field_entry(hid, report, &entries[usages],
+						 field, 0);
+			usages++;
+		}
+	}
+}
+
+static void hid_process_ordering(struct hid_device *hid)
+{
+	struct hid_report *report;
+	struct hid_report_enum *report_enum = &hid->report_enum[HID_INPUT_REPORT];
+
+	list_for_each_entry(report, &report_enum->report_list, list)
+		hid_report_process_ordering(hid, report);
+}
+
 /*
  * Output the field into the report.
  */
@@ -2050,6 +2151,8 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 		return -ENODEV;
 	}
 
+	hid_process_ordering(hdev);
+
 	if ((hdev->claimed & HID_CLAIMED_INPUT) &&
 			(connect_mask & HID_CONNECT_FF) && hdev->ff_init)
 		hdev->ff_init(hdev);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index cf79eb3da465..f25020c0d6b8 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -477,6 +477,7 @@ struct hid_field {
 	unsigned  report_type;		/* (input,output,feature) */
 	__s32    *value;		/* last known value(s) */
 	__s32    *new_value;		/* newly read value(s) */
+	__s32    *usages_priorities;	/* priority of each usage when reading the report */
 	__s32     logical_minimum;
 	__s32     logical_maximum;
 	__s32     physical_minimum;
@@ -493,13 +494,22 @@ struct hid_field {
 
 #define HID_MAX_FIELDS 256
 
+struct hid_field_entry {
+	struct list_head list;
+	struct hid_field *field;
+	unsigned int index;
+	__s32 priority;
+};
+
 struct hid_report {
 	struct list_head list;
 	struct list_head hidinput_list;
+	struct list_head field_entry_list;		/* ordered list of input fields */
 	unsigned int id;				/* id of this report */
 	unsigned int type;				/* report type */
 	unsigned int application;			/* application usage for this report */
 	struct hid_field *field[HID_MAX_FIELDS];	/* fields of the report */
+	struct hid_field_entry *field_entries;		/* allocated memory of input field_entry */
 	unsigned maxfield;				/* maximum valid field index */
 	unsigned size;					/* size of the report (bits) */
 	struct hid_device *device;			/* associated device */
-- 
2.33.1

[PATCH 08/12] HID: core: for input reports, process the usages by priority list

[Benjamin Tissoires]

Now that we have a list of fields/usages by priority order,
walk through that list to process the inputs instead of using the
order provided by the manufacturer.

Note that this changes the way we update the values in the struct
hid_field:
Previously, once a field was processed, we updated the new values.
Now we need to wait for the entire report to be processed to update
the values.

I don't think it will be an issue: because we were relying on the device
ordering, there were no guarantees to have a field stored before an other.
Which is why we introduced .report() in drivers to have those values
updated.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 45 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b38528118642..db925794fbe6 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1657,17 +1657,48 @@ static void hid_process_report(struct hid_device *hid,
 			       int interrupt)
 {
 	unsigned int a;
+	struct hid_field_entry *entry;
 	struct hid_field *field;
 
-	for (a = 0; a < report->maxfield; a++) {
-		field = report->field[a];
+	/* first retrieve all incoming values in data */
+	for (a = 0; a < report->maxfield; a++)
+		hid_input_fetch_field(hid, field = report->field[a], data);
+
+	if (!list_empty(&report->field_entry_list)) {
+		/* INPUT_REPORT, we have a priority list of fields */
+		list_for_each_entry(entry,
+				    &report->field_entry_list,
+				    list) {
+			field = entry->field;
+
+			if (field->flags & HID_MAIN_ITEM_VARIABLE)
+				hid_process_event(hid,
+						  field,
+						  &field->usage[entry->index],
+						  field->new_value[entry->index],
+						  interrupt);
+			else
+				hid_input_array_field(hid, field, interrupt);
+		}
 
-		hid_input_fetch_field(hid, field, data);
+		/* we need to do the memcpy at the end for var items */
+		for (a = 0; a < report->maxfield; a++) {
+			field = report->field[a];
 
-		if (field->flags & HID_MAIN_ITEM_VARIABLE)
-			hid_input_var_field(hid, field, interrupt);
-		else
-			hid_input_array_field(hid, field, interrupt);
+			if (field->flags & HID_MAIN_ITEM_VARIABLE)
+				memcpy(field->value, field->new_value,
+				       field->report_count * sizeof(__s32));
+		}
+	} else {
+		/* FEATURE_REPORT, regular processing */
+		for (a = 0; a < report->maxfield; a++) {
+			field = report->field[a];
+
+			if (field->flags & HID_MAIN_ITEM_VARIABLE)
+				hid_input_var_field(hid, field, interrupt);
+			else
+				hid_input_array_field(hid, field, interrupt);
+		}
 	}
 }
 
-- 
2.33.1

[PATCH 09/12] HID: input: enforce Invert usage to be processed before InRange

[Benjamin Tissoires]

When a device exposes both Invert and InRange, Invert must be processed
before InRange. If we keep the order of the device and we process them
out of order, InRange will first set BTN_TOOL_PEN, and then Invert will
set BTN_TOOL_RUBBER. Userspace knows how to deal with that situation,
but fixing it in the kernel is now easier.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 34 ++++++++++++++++++++++++++++++++--
 include/linux/hid.h     |  4 +++-
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 8770d9a2b2af..61d91117f4ae 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -48,6 +48,25 @@ static const struct {
 	__s32 y;
 }  hid_hat_to_axis[] = {{ 0, 0}, { 0,-1}, { 1,-1}, { 1, 0}, { 1, 1}, { 0, 1}, {-1, 1}, {-1, 0}, {-1,-1}};
 
+/*
+ * hid-input will convert this list into priorities:
+ * the first element will have the highest priority
+ * (the length of the following array) and the last
+ * element the lowest (1).
+ *
+ * hid-input will then shift the priority by 8 bits to leave some space
+ * in case drivers want to interleave other fields.
+ *
+ * If drivers want to add fields before those, hid-input will
+ * leave out the first 8 bits of the priority value.
+ *
+ * This still leaves us 65535 individual priority values.
+ */
+static const __u32 hidinput_usages_priorities[] = {
+	HID_DG_INVERT,		/* Invert must always come before In Range */
+	HID_DG_INRANGE,
+};
+
 #define map_abs(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_ABS, (c))
 #define map_rel(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_REL, (c))
 #define map_key(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_KEY, (c))
@@ -586,11 +605,12 @@ static bool hidinput_field_in_collection(struct hid_device *device, struct hid_f
 }
 
 static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_field *field,
-				     struct hid_usage *usage)
+				     struct hid_usage *usage, unsigned int usage_index)
 {
 	struct input_dev *input = hidinput->input;
 	struct hid_device *device = input_get_drvdata(input);
 	int max = 0, code;
+	unsigned int i = 0;
 	unsigned long *bit = NULL;
 
 	field->hidinput = hidinput;
@@ -608,6 +628,15 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		goto ignore;
 	}
 
+	/* assign a priority based on the static list declared here */
+	for (i = 0; i < ARRAY_SIZE(hidinput_usages_priorities); i++) {
+		if (usage->hid == hidinput_usages_priorities[i]) {
+			field->usages_priorities[usage_index] =
+				(ARRAY_SIZE(hidinput_usages_priorities) - i) << 8;
+			break;
+		}
+	}
+
 	if (device->driver->input_mapping) {
 		int ret = device->driver->input_mapping(device, hidinput, field,
 				usage, &bit, &max);
@@ -1962,7 +1991,8 @@ static inline void hidinput_configure_usages(struct hid_input *hidinput,
 	for (i = 0; i < report->maxfield; i++)
 		for (j = 0; j < report->field[i]->maxusage; j++)
 			hidinput_configure_usage(hidinput, report->field[i],
-						 report->field[i]->usage + j);
+						 report->field[i]->usage + j,
+						 j);
 }
 
 /*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f25020c0d6b8..eaad0655b05c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -477,7 +477,9 @@ struct hid_field {
 	unsigned  report_type;		/* (input,output,feature) */
 	__s32    *value;		/* last known value(s) */
 	__s32    *new_value;		/* newly read value(s) */
-	__s32    *usages_priorities;	/* priority of each usage when reading the report */
+	__s32    *usages_priorities;	/* priority of each usage when reading the report
+					 * bits 8-16 are reserved for hid-input usage
+					 */
 	__s32     logical_minimum;
 	__s32     logical_maximum;
 	__s32     physical_minimum;
-- 
2.33.1

[PATCH 10/12] HID: input: remove the need for HID_QUIRK_INVERT

[Benjamin Tissoires]

HID_QUIRK_INVERT is kind of complex to deal with and was bogus.

Furthermore, it didn't make sense to use a global per struct hid_device
quirk for something dynamic as the current state.

Store the current tool information in the report itself, and re-order
the processing of the fields to enforce having all the tablet "state"
fields before getting to In Range and other input fields.

This way, we now have all the information whether a tool is present
or not while processing In Range.

This new behavior enforces that only one tool gets forwarded to userspace
at the same time, and that if either eraser or invert is set, we enforce
BTN_TOOL_RUBBER.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 66 +++++++++++++++++++++++++++++++++++------
 include/linux/hid.h     |  6 +++-
 2 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 61d91117f4ae..2d13d3ad9d3c 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -63,8 +63,11 @@ static const struct {
  * This still leaves us 65535 individual priority values.
  */
 static const __u32 hidinput_usages_priorities[] = {
+	HID_DG_ERASER,		/* Eraser (eraser touching) must always come before tipswitch */
 	HID_DG_INVERT,		/* Invert must always come before In Range */
-	HID_DG_INRANGE,
+	HID_DG_TIPSWITCH,	/* Is the tip of the tool touching? */
+	HID_DG_TIPPRESSURE,	/* Tip Pressure might emulate tip switch */
+	HID_DG_INRANGE,		/* In Range needs to come after the other tool states */
 };
 
 #define map_abs(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_ABS, (c))
@@ -1368,6 +1371,7 @@ static void hidinput_handle_scroll(struct hid_usage *usage,
 void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, __s32 value)
 {
 	struct input_dev *input;
+	struct hid_report *report = field->report;
 	unsigned *quirks = &hid->quirks;
 
 	if (!usage->type)
@@ -1418,25 +1422,69 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 	}
 
 	switch (usage->hid) {
+	case HID_DG_ERASER:
+		report->tool_active |= !!value;
+
+		/*
+		 * if eraser is set, we must enforce BTN_TOOL_RUBBER
+		 * to accommodate for devices not following the spec.
+		 */
+		if (value)
+			report->tool = BTN_TOOL_RUBBER;
+
+		/* let hid-input set BTN_TOUCH */
+		break;
+
 	case HID_DG_INVERT:
-		*quirks = value ? (*quirks | HID_QUIRK_INVERT) : (*quirks & ~HID_QUIRK_INVERT);
+		report->tool_active |= !!value;
+
+		/*
+		 * If invert is set, we store BTN_TOOL_RUBBER.
+		 */
+		if (value)
+			report->tool = BTN_TOOL_RUBBER;
+
+		/* no further processing */
 		return;
 
 	case HID_DG_INRANGE:
-		if (value) {
-			input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1);
-			return;
-		}
-		input_event(input, usage->type, usage->code, 0);
-		input_event(input, usage->type, BTN_TOOL_RUBBER, 0);
+		report->tool_active |= !!value;
+
+		/*
+		 * If the tool is in used (any of TipSwitch, Erase, Invert,
+		 * InRange), and if tool is not set, store our mapping
+		 */
+		if (report->tool_active && !report->tool)
+			report->tool = usage->code;
+
+		input_event(input, EV_KEY, usage->code, report->tool == usage->code);
+		input_event(input, EV_KEY, BTN_TOOL_RUBBER, report->tool == BTN_TOOL_RUBBER);
+
+		/* reset tool and tool_active for the next event */
+		report->tool = 0;
+		report->tool_active = false;
+
+		/* no further processing */
 		return;
 
+	case HID_DG_TIPSWITCH:
+		report->tool_active |= !!value;
+
+		/* if tool is set we should ignore the current value */
+		if (report->tool)
+			return;
+
+		break;
+
 	case HID_DG_TIPPRESSURE:
 		if (*quirks & HID_QUIRK_NOTOUCH) {
 			int a = field->logical_minimum;
 			int b = field->logical_maximum;
 
-			input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
+			if (value > a + ((b - a) >> 3)) {
+				input_event(input, EV_KEY, BTN_TOUCH, 1);
+				report->tool_active = true;
+			}
 		}
 		break;
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index eaad0655b05c..feb8df61168f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -347,7 +347,7 @@ struct hid_item {
  */
 #define MAX_USBHID_BOOT_QUIRKS 4
 
-#define HID_QUIRK_INVERT			BIT(0)
+/* BIT(0) reserved for backward compatibility, was HID_QUIRK_INVERT */
 #define HID_QUIRK_NOTOUCH			BIT(1)
 #define HID_QUIRK_IGNORE			BIT(2)
 #define HID_QUIRK_NOGET				BIT(3)
@@ -515,6 +515,10 @@ struct hid_report {
 	unsigned maxfield;				/* maximum valid field index */
 	unsigned size;					/* size of the report (bits) */
 	struct hid_device *device;			/* associated device */
+
+	/* tool related state */
+	bool tool_active;				/* whether the current tool is active */
+	unsigned int tool;				/* BTN_TOOL_* */
 };
 
 #define HID_MAX_IDS 256
-- 
2.33.1

Re: [PATCH 10/12] HID: input: remove the need for HID_QUIRK_INVERT

[Benjamin Tissoires]

Hi Ping,

On Wed, Feb 2, 2022 at 6:43 AM Ping Cheng <pinglinux@gmail.com> wrote:
>
> Hi Benjamin,
>
> Thank you for taking care of the issue. The whole set looks good to me, except this one. xf86-input-wacom would not process the events properly if pen and eraser events are reported through the same EV_SYN frame. That's mainly because the tool out of prox will reset all values, while some values the next tool may rely on. Please see my detailed comments inline.
>
> On Wed, Jan 26, 2022 at 8:19 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>>
>> HID_QUIRK_INVERT is kind of complex to deal with and was bogus.
>>
>> Furthermore, it didn't make sense to use a global per struct hid_device
>> quirk for something dynamic as the current state.
>>
>> Store the current tool information in the report itself, and re-order
>> the processing of the fields to enforce having all the tablet "state"
>> fields before getting to In Range and other input fields.
>>
>> This way, we now have all the information whether a tool is present
>> or not while processing In Range.
>>
>> This new behavior enforces that only one tool gets forwarded to userspace
>> at the same time, and that if either eraser or invert is set, we enforce
>>
>> BTN_TOOL_RUBBER.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/hid-input.c | 66 +++++++++++++++++++++++++++++++++++------
>>  include/linux/hid.h     |  6 +++-
>>  2 files changed, 62 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 61d91117f4ae..2d13d3ad9d3c 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -63,8 +63,11 @@ static const struct {
>>   * This still leaves us 65535 individual priority values.
>>   */
>>  static const __u32 hidinput_usages_priorities[] = {
>> +       HID_DG_ERASER,          /* Eraser (eraser touching) must always come before tipswitch */
>>         HID_DG_INVERT,          /* Invert must always come before In Range */
>> -       HID_DG_INRANGE,
>> +       HID_DG_TIPSWITCH,       /* Is the tip of the tool touching? */
>> +       HID_DG_TIPPRESSURE,     /* Tip Pressure might emulate tip switch */
>> +       HID_DG_INRANGE,         /* In Range needs to come after the other tool states */
>>  };
>>
>>  #define map_abs(c)     hid_map_usage(hidinput, usage, &bit, &max, EV_ABS, (c))
>> @@ -1368,6 +1371,7 @@ static void hidinput_handle_scroll(struct hid_usage *usage,
>>  void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, __s32 value)
>>  {
>>         struct input_dev *input;
>> +       struct hid_report *report = field->report;
>>         unsigned *quirks = &hid->quirks;
>>
>>         if (!usage->type)
>> @@ -1418,25 +1422,69 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>         }
>>
>>         switch (usage->hid) {
>> +       case HID_DG_ERASER:
>> +               report->tool_active |= !!value;
>> +
>> +               /*
>> +                * if eraser is set, we must enforce BTN_TOOL_RUBBER
>> +                * to accommodate for devices not following the spec.
>> +                */
>> +               if (value)
>> +                       report->tool = BTN_TOOL_RUBBER;
>> +
>> +               /* let hid-input set BTN_TOUCH */
>> +               break;
>> +
>>         case HID_DG_INVERT:
>> -               *quirks = value ? (*quirks | HID_QUIRK_INVERT) : (*quirks & ~HID_QUIRK_INVERT);
>> +               report->tool_active |= !!value;
>> +
>> +               /*
>> +                * If invert is set, we store BTN_TOOL_RUBBER.
>> +                */
>> +               if (value)
>> +                       report->tool = BTN_TOOL_RUBBER;
>> +
>> +               /* no further processing */
>>                 return;
>>
>>         case HID_DG_INRANGE:
>> -               if (value) {
>> -                       input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1);
>> -                       return;
>> -               }
>> -               input_event(input, usage->type, usage->code, 0);
>> -               input_event(input, usage->type, BTN_TOOL_RUBBER, 0);
>> +               report->tool_active |= !!value;
>> +
>> +               /*
>> +                * If the tool is in used (any of TipSwitch, Erase, Invert,
>> +                * InRange), and if tool is not set, store our mapping
>> +                */
>> +               if (report->tool_active && !report->tool)
>> +                       report->tool = usage->code;
>> +
>> +               input_event(input, EV_KEY, usage->code, report->tool == usage->code);
>> +               input_event(input, EV_KEY, BTN_TOOL_RUBBER, report->tool == BTN_TOOL_RUBBER);
>
>
> When usage->code changes value, for userspace clients, such as X driver, one of the tools in the frame would not have its own input events since there is only one set of input event values in the frame. X driver stores the values in different arrays/channels for each tool to post the events for each tool after all the values are processed.
>
> If we can always post the tool that goes out of prox with an EV_SYN first, the other tool would take the coming input events without problem. The tool that goes out of prox doesn't care about any of the other input events since its value will be reset.

OK. Right now, this series ensures we get
ERASER/TIPSWITCH/INVERT/INRANGE first before processing any other
events.

Given that ERASER/TIPSWITCH are responsible for BTN_TOUCH, I'd like to
get some clarifications (sorry dumping my brain here):

In the example of the pen moving from erasing to touching without the
intent to erase:

The HID event sequence would be:
Eraser: 1, Invert: 1, Tipswitch: 0, InRange: 1, X, Y, etc...
then
Eraser: 0, Invert: 0, Tipswitch: 1, InRange: 1, X, Y, etc...

In this current series we would get:
BTN_TOUCH 1, BTN_TOOL_RUBBER 1, X, Y, etc...
then
BTN_TOUCH 0, BTN_TOUCH 1, BTN_TOOL_PEN 1, BTN_TOOL_RUBBER 0, X, Y, etc...

(yeah, oops, there are 2 BTN_TOUCH in this sequence)

A naive change would do (instead of the last evdev event):
BTN_TOUCH 0, BTN_TOUCH 1, BTN_TOOL_RUBBER 0, *EV_SYN*, BTN_TOOL_PEN 1,
X, Y, etc...

However, I wonder if we should not have instead:
BTN_TOUCH 0, BTN_TOOL_RUBBER 0, *EV_SYN*, BTN_TOUCH 1, BTN_TOOL_PEN 1,
X, Y, etc...

That change should be easy to handle actually.

I am a little bit more concerned the other way around:
touching without the intent to erase to erase:
HID events:
Eraser: 0, Invert: 0, Tipswitch: 1, InRange: 1, X, Y, etc...
Eraser: 1, Invert: 1, Tipswitch: 0, InRange: 1, X, Y, etc...

The current series gives:
BTN_TOUCH 1, BTN_TOOL_PEN 1, X, Y, etc...
then
BTN_TOOL_PEN 1, BTN_TOOL_RUBBER 1, X, Y, etc...

(no BTN_TOUCH events)

And I think to get to the "correct" sequence I would have to store more states:
BTN_TOUCH 0, BTN_TOOL_PEN 0, *EV_SYN*, BTN_TOUCH 1, BTN_TOOL_RUBBER 1,
X, Y, etc...

I think the main problem is how to handle BTN_TOUCH correctly (and
PRESSURE is also in the middle). When this event is not set, it should
be easier to add the EV_SYN between the BTN_TOOL_*.

>
> We could update X driver, if libinput is capable of processing two tools in the same EV_SYN frame. This is more like a special multi-pen situation where two styli share the same set of input event values. The regular multi-pen logic would not process it right, I think.

Well, there is already the problem of that BTN_TOUCH 0/1 in one frame
that is not very clean. And if we can not break userspace that would
be best (note that the current kernel is completely sending random
events for those transitions, so in a way it's still slightly better).

Cheers,
Benjamin

>
> Cheers,
> Ping
>
>> +
>> +               /* reset tool and tool_active for the next event */
>> +               report->tool = 0;
>> +               report->tool_active = false;
>> +
>> +               /* no further processing */
>>                 return;
>>
>> +       case HID_DG_TIPSWITCH:
>> +               report->tool_active |= !!value;
>> +
>> +               /* if tool is set we should ignore the current value */
>> +               if (report->tool)
>> +                       return;
>> +
>> +               break;
>> +
>>         case HID_DG_TIPPRESSURE:
>>                 if (*quirks & HID_QUIRK_NOTOUCH) {
>>                         int a = field->logical_minimum;
>>                         int b = field->logical_maximum;
>>
>> -                       input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
>> +                       if (value > a + ((b - a) >> 3)) {
>> +                               input_event(input, EV_KEY, BTN_TOUCH, 1);
>> +                               report->tool_active = true;
>> +                       }
>>                 }
>>                 break;
>>
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index eaad0655b05c..feb8df61168f 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -347,7 +347,7 @@ struct hid_item {
>>   */
>>  #define MAX_USBHID_BOOT_QUIRKS 4
>>
>> -#define HID_QUIRK_INVERT                       BIT(0)
>> +/* BIT(0) reserved for backward compatibility, was HID_QUIRK_INVERT */
>>  #define HID_QUIRK_NOTOUCH                      BIT(1)
>>  #define HID_QUIRK_IGNORE                       BIT(2)
>>  #define HID_QUIRK_NOGET                                BIT(3)
>> @@ -515,6 +515,10 @@ struct hid_report {
>>         unsigned maxfield;                              /* maximum valid field index */
>>         unsigned size;                                  /* size of the report (bits) */
>>         struct hid_device *device;                      /* associated device */
>> +
>> +       /* tool related state */
>> +       bool tool_active;                               /* whether the current tool is active */
>> +       unsigned int tool;                              /* BTN_TOOL_* */
>>  };
>>
>>  #define HID_MAX_IDS 256
>> --
>> 2.33.1
>>

[PATCH 11/12] HID: input: accommodate priorities for slotted devices

[Benjamin Tissoires]

Multitouch devices in hybrid mode are reporting multiple times the
same collection. We should accommodate for this in our handling
of priorities by defining the slots they belong to.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 103 ++++++++++++++++++++++++++++++++++++----
 include/linux/hid.h     |   1 +
 2 files changed, 96 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 2d13d3ad9d3c..1b00fa363668 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -48,6 +48,16 @@ static const struct {
 	__s32 y;
 }  hid_hat_to_axis[] = {{ 0, 0}, { 0,-1}, { 1,-1}, { 1, 0}, { 1, 1}, { 0, 1}, {-1, 1}, {-1, 0}, {-1,-1}};
 
+struct usage_priority {
+	__u32 usage;			/* the HID usage associated */
+	bool global;			/* we assume all usages to be slotted,
+					 * unless global
+					 */
+	unsigned int slot_overwrite;	/* for globals: allows to set the usage
+					 * before or after the slots
+					 */
+};
+
 /*
  * hid-input will convert this list into priorities:
  * the first element will have the highest priority
@@ -57,17 +67,30 @@ static const struct {
  * hid-input will then shift the priority by 8 bits to leave some space
  * in case drivers want to interleave other fields.
  *
+ * To accommodate slotted devices, the slot priority is
+ * defined in the next 8 bits (defined by 0xff - slot).
+ *
  * If drivers want to add fields before those, hid-input will
  * leave out the first 8 bits of the priority value.
  *
  * This still leaves us 65535 individual priority values.
  */
-static const __u32 hidinput_usages_priorities[] = {
-	HID_DG_ERASER,		/* Eraser (eraser touching) must always come before tipswitch */
-	HID_DG_INVERT,		/* Invert must always come before In Range */
-	HID_DG_TIPSWITCH,	/* Is the tip of the tool touching? */
-	HID_DG_TIPPRESSURE,	/* Tip Pressure might emulate tip switch */
-	HID_DG_INRANGE,		/* In Range needs to come after the other tool states */
+static const struct usage_priority hidinput_usages_priorities[] = {
+	{ /* Eraser (eraser touching) must always come before tipswitch */
+	  .usage = HID_DG_ERASER,
+	},
+	{ /* Invert must always come before In Range */
+	  .usage = HID_DG_INVERT,
+	},
+	{ /* Is the tip of the tool touching? */
+	  .usage = HID_DG_TIPSWITCH,
+	},
+	{ /* Tip Pressure might emulate tip switch */
+	  .usage = HID_DG_TIPPRESSURE,
+	},
+	{ /* In Range needs to come after the other tool states */
+	  .usage = HID_DG_INRANGE,
+	},
 };
 
 #define map_abs(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_ABS, (c))
@@ -612,6 +635,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 {
 	struct input_dev *input = hidinput->input;
 	struct hid_device *device = input_get_drvdata(input);
+	const struct usage_priority *usage_priority = NULL;
 	int max = 0, code;
 	unsigned int i = 0;
 	unsigned long *bit = NULL;
@@ -633,13 +657,26 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 
 	/* assign a priority based on the static list declared here */
 	for (i = 0; i < ARRAY_SIZE(hidinput_usages_priorities); i++) {
-		if (usage->hid == hidinput_usages_priorities[i]) {
+		if (usage->hid == hidinput_usages_priorities[i].usage) {
+			usage_priority = &hidinput_usages_priorities[i];
+
 			field->usages_priorities[usage_index] =
 				(ARRAY_SIZE(hidinput_usages_priorities) - i) << 8;
 			break;
 		}
 	}
 
+	/*
+	 * For slotted devices, we need to also add the slot index
+	 * in the priority.
+	 */
+	if (usage_priority && usage_priority->global)
+		field->usages_priorities[usage_index] |=
+			usage_priority->slot_overwrite;
+	else
+		field->usages_priorities[usage_index] |=
+			(0xff - field->slot_idx) << 16;
+
 	if (device->driver->input_mapping) {
 		int ret = device->driver->input_mapping(device, hidinput, field,
 				usage, &bit, &max);
@@ -2034,7 +2071,57 @@ static struct hid_input *hidinput_match_application(struct hid_report *report)
 static inline void hidinput_configure_usages(struct hid_input *hidinput,
 					     struct hid_report *report)
 {
-	int i, j;
+	int i, j, k;
+	int first_field_index = 0;
+	int slot_collection_index = -1;
+	int prev_collection_index = -1;
+	unsigned int slot_idx = 0;
+	struct hid_field *field;
+
+	/*
+	 * First tag all the fields that are part of a slot,
+	 * a slot needs to have one Contact ID in the collection
+	 */
+	for (i = 0; i < report->maxfield; i++) {
+		field = report->field[i];
+
+		/* ignore fields without usage */
+		if (field->maxusage < 1)
+			continue;
+
+		/*
+		 * janitoring when collection_index changes
+		 */
+		if (prev_collection_index != field->usage->collection_index) {
+			prev_collection_index = field->usage->collection_index;
+			first_field_index = i;
+		}
+
+		/*
+		 * if we already found a Contact ID in the collection,
+		 * tag and continue to the next.
+		 */
+		if (slot_collection_index == field->usage->collection_index) {
+			field->slot_idx = slot_idx;
+			continue;
+		}
+
+		/* check if the current field has Contact ID */
+		for (j = 0; j < field->maxusage; j++) {
+			if (field->usage[j].hid == HID_DG_CONTACTID) {
+				slot_collection_index = field->usage->collection_index;
+				slot_idx++;
+
+				/*
+				 * mark all previous fields and this one in the
+				 * current collection to be slotted.
+				 */
+				for (k = first_field_index; k <= i; k++)
+					report->field[k]->slot_idx = slot_idx;
+				break;
+			}
+		}
+	}
 
 	for (i = 0; i < report->maxfield; i++)
 		for (j = 0; j < report->field[i]->maxusage; j++)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index feb8df61168f..4363a63b9775 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -492,6 +492,7 @@ struct hid_field {
 	/* hidinput data */
 	struct hid_input *hidinput;	/* associated input structure */
 	__u16 dpad;			/* dpad input code */
+	unsigned int slot_idx;		/* slot index in a report */
 };
 
 #define HID_MAX_FIELDS 256
-- 
2.33.1

[PATCH 12/12] Input: docs: add more details on the use of BTN_TOOL

[Benjamin Tissoires]

The HID core stack used to be very relaxed considering the BTN_TOOL_*
usage. With the recent commits, we should now enforce to have only one
tool at a time, meaning that we can now express that requirement in the
docs.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 Documentation/input/event-codes.rst | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
index b24ae7d292cc..41b1fa647dab 100644
--- a/Documentation/input/event-codes.rst
+++ b/Documentation/input/event-codes.rst
@@ -137,7 +137,10 @@ A few EV_KEY codes have special meanings:
     code should be set to a value of 1. When the tool is no longer interacting
     with the input device, the BTN_TOOL_<name> code should be reset to 0. All
     trackpads, tablets, and touchscreens should use at least one BTN_TOOL_<name>
-    code when events are generated.
+    code when events are generated. Likewise all trackpads, tablets, and
+    touchscreens should export only one BTN_TOOL_<name> at a time. It is
+    however accepted to switch tool in one EV_SYN frame by setting the old
+    BTN_TOOL_<name> at a value of 0 and the new one at 1.
 
 * BTN_TOUCH:
 
-- 
2.33.1

Re: [PATCH 12/12] Input: docs: add more details on the use of BTN_TOOL

[Peter Hutterer]

On Wed, Jan 26, 2022 at 05:18:32PM +0100, Benjamin Tissoires wrote:
> The HID core stack used to be very relaxed considering the BTN_TOOL_*
> usage. With the recent commits, we should now enforce to have only one
> tool at a time, meaning that we can now express that requirement in the
> docs.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  Documentation/input/event-codes.rst | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
> index b24ae7d292cc..41b1fa647dab 100644
> --- a/Documentation/input/event-codes.rst
> +++ b/Documentation/input/event-codes.rst
> @@ -137,7 +137,10 @@ A few EV_KEY codes have special meanings:
>      code should be set to a value of 1. When the tool is no longer interacting
>      with the input device, the BTN_TOOL_<name> code should be reset to 0. All
>      trackpads, tablets, and touchscreens should use at least one BTN_TOOL_<name>
> -    code when events are generated.
> +    code when events are generated. Likewise all trackpads, tablets, and
> +    touchscreens should export only one BTN_TOOL_<name> at a time. It is

s/export/set to nonzero/ to avoid any ambiguity with setting the evbit on the
device vs setting the value to nonzero.

> +    however accepted to switch tool in one EV_SYN frame by setting the old
> +    BTN_TOOL_<name> at a value of 0 and the new one at 1.

I would even s/accepted/recommended/ but this LGTM to me, thanks.

Acked-by: Peter Hutterer <peter.hutterer@who-t.net>

Cheers,
  Peter

Re: [PATCH 12/12] Input: docs: add more details on the use of BTN_TOOL

[Benjamin Tissoires]

Hi Peter,


Looks like you commented on v1, and I made some changes in the v2 :)

On Wed, Feb 9, 2022 at 7:22 AM Peter Hutterer <peter.hutterer@who-t.net> wrote:
>
> On Wed, Jan 26, 2022 at 05:18:32PM +0100, Benjamin Tissoires wrote:
> > The HID core stack used to be very relaxed considering the BTN_TOOL_*
> > usage. With the recent commits, we should now enforce to have only one
> > tool at a time, meaning that we can now express that requirement in the
> > docs.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  Documentation/input/event-codes.rst | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
> > index b24ae7d292cc..41b1fa647dab 100644
> > --- a/Documentation/input/event-codes.rst
> > +++ b/Documentation/input/event-codes.rst
> > @@ -137,7 +137,10 @@ A few EV_KEY codes have special meanings:
> >      code should be set to a value of 1. When the tool is no longer interacting
> >      with the input device, the BTN_TOOL_<name> code should be reset to 0. All
> >      trackpads, tablets, and touchscreens should use at least one BTN_TOOL_<name>
> > -    code when events are generated.
> > +    code when events are generated. Likewise all trackpads, tablets, and
> > +    touchscreens should export only one BTN_TOOL_<name> at a time. It is
>
> s/export/set to nonzero/ to avoid any ambiguity with setting the evbit on the
> device vs setting the value to nonzero.

I can carry this change in the v3.

>
> > +    however accepted to switch tool in one EV_SYN frame by setting the old
> > +    BTN_TOOL_<name> at a value of 0 and the new one at 1.
>
> I would even s/accepted/recommended/ but this LGTM to me, thanks.

In v2 I used recommended, but I turned the meaning the other way
around. In the v2 it reads:
"To not break existing userspace, it is recommended to not switch tool
in one EV_SYN frame but first emitting the old BTN_TOOL_<name> at 0,
then emit one SYN_REPORT and then set the new BTN_TOOL_<name> at 1."

So maybe I should use "accepted" in v3?

>
> Acked-by: Peter Hutterer <peter.hutterer@who-t.net>

Do I keep your acked-by in the v2/3?

Cheers,
Benjamin

Re: [PATCH 00/12] HID: fix for generic input processing

[Angela Czubak]

Hi Benjamin,

On Wed, Jan 26, 2022 at 5:18 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi,
>
> This is a followup of the discussion we had between Wacom and
> the maintainers, and a followup of those 2 patch series:
>
> https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
> https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/
>
> It took me a while to get it right, but I finally can submit the
> series:
>
> - the first 8 patches are some cleanup in the hid-input.c and
>   hid-core.c code. They also create a list of input fields that
>   is then used to process the event, in the priority we think
>   is good.
>
>   For instance, on multitouch devices, it is better to have
>   Contact Count before processing all touches, and in each
>   touch, having Contact ID first is better. This series doesn't
>   cover hid-multitouch, but I have a series on top of this one that
>   does cover it.
>
>   Anyway, in our case, here, we need to process Invert before
>   In Range for tablets so we can make a decision whether the user
>   has the intend to erase or not.
>
> - patch 9 enforces the invert usage before In Range as mentioned
>   above
>
> - patch 10 is the actual bulk of processing that should fix the
>   generic tablet handling. Now that we have a reliable ordering
>   of the fields, we can compute the state of the tool in a reliable
>   way, and be kinder to userspace by not sending to it 2 tools at
>   the same time.
>
>   This patch has been extensively tested by hid-tools with the new
>   MR I submitted that add tests for tablets [0].
>
> - patch 11 is a nice to have that I need for my second series regarding
>   hid-multitouch. It is not mandatory with that series, but given
>   that it changes the format of the priorities in hid-input.c I thought
>   it would be best to send it as part of this series.
>
>   Note that now we are tagging the *reports* and the individual fields
>   when they are part of a multitouch collection, which should help
>   the drivers that implement this processing (hid-multitouch and wacom).
>
> - last, patch 12 is an attempt at fixing the documentation regarding
>   BTN_TOOL_* (requested by Peter).
>
>   Dmitry, feel free to take this one through your tree if you prefer
>   to do so (and if you are happy with it), otherwise we can take it
>   through the hid tree.
>
> As mentioned above, I have a followup series not entirely tidied up
> that implements the processing of Win8 mutltiouch devices in
> hid-input.c.
> There are several benefits for that: we should be able to drop the
> multitouch code in wacom.ko, we can simplify part of hid-multitouch,
> and we will be able to quirk a particular device in a separate module,
> without touching at the generic code (in hid-multitouch or hid-input).
>
> Anyway, I am missing a few bits for that so that's coming in later.
>

Is there any timeline for the followup series? I am wondering how that
would affect haptic support implementation.

> Cheers,
> Benjamin
>
>
> [0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
>
> Benjamin Tissoires (12):
>   HID: core: statically allocate read buffers
>   HID: core: de-duplicate some code in hid_input_field()
>   HID: core: split data fetching from processing in hid_input_field()
>   HID: input: tag touchscreens as such if the physical is not there
>   HID: input: rework spaghetti code with switch statements
>   HID: input: move up out-of-range processing of input values
>   HID: compute an ordered list of input fields to process
>   HID: core: for input reports, process the usages by priority list
>   HID: input: enforce Invert usage to be processed before InRange
>   HID: input: remove the need for HID_QUIRK_INVERT
>   HID: input: accommodate priorities for slotted devices
>   Input: docs: add more details on the use of BTN_TOOL
>
>  Documentation/input/event-codes.rst |   5 +-
>  drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
>  drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
>  include/linux/hid.h                 |  23 +-
>  4 files changed, 533 insertions(+), 105 deletions(-)
>
> --
> 2.33.1
>

Does this patch series introduce the leaf driver support you mentioned
in the haptic review?

Re: [PATCH 00/12] HID: fix for generic input processing

[Angela Czubak]

On Tue, Feb 8, 2022 at 8:19 PM Angela Czubak <acz@semihalf.com> wrote:
>
> Hi Benjamin,
>
> On Wed, Jan 26, 2022 at 5:18 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Hi,
> >
> > This is a followup of the discussion we had between Wacom and
> > the maintainers, and a followup of those 2 patch series:
> >
> > https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
> > https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/
> >
> > It took me a while to get it right, but I finally can submit the
> > series:
> >
> > - the first 8 patches are some cleanup in the hid-input.c and
> >   hid-core.c code. They also create a list of input fields that
> >   is then used to process the event, in the priority we think
> >   is good.
> >
> >   For instance, on multitouch devices, it is better to have
> >   Contact Count before processing all touches, and in each
> >   touch, having Contact ID first is better. This series doesn't
> >   cover hid-multitouch, but I have a series on top of this one that
> >   does cover it.
> >
> >   Anyway, in our case, here, we need to process Invert before
> >   In Range for tablets so we can make a decision whether the user
> >   has the intend to erase or not.
> >
> > - patch 9 enforces the invert usage before In Range as mentioned
> >   above
> >
> > - patch 10 is the actual bulk of processing that should fix the
> >   generic tablet handling. Now that we have a reliable ordering
> >   of the fields, we can compute the state of the tool in a reliable
> >   way, and be kinder to userspace by not sending to it 2 tools at
> >   the same time.
> >
> >   This patch has been extensively tested by hid-tools with the new
> >   MR I submitted that add tests for tablets [0].
> >
> > - patch 11 is a nice to have that I need for my second series regarding
> >   hid-multitouch. It is not mandatory with that series, but given
> >   that it changes the format of the priorities in hid-input.c I thought
> >   it would be best to send it as part of this series.
> >
> >   Note that now we are tagging the *reports* and the individual fields
> >   when they are part of a multitouch collection, which should help
> >   the drivers that implement this processing (hid-multitouch and wacom).
> >
> > - last, patch 12 is an attempt at fixing the documentation regarding
> >   BTN_TOOL_* (requested by Peter).
> >
> >   Dmitry, feel free to take this one through your tree if you prefer
> >   to do so (and if you are happy with it), otherwise we can take it
> >   through the hid tree.
> >
> > As mentioned above, I have a followup series not entirely tidied up
> > that implements the processing of Win8 mutltiouch devices in
> > hid-input.c.
> > There are several benefits for that: we should be able to drop the
> > multitouch code in wacom.ko, we can simplify part of hid-multitouch,
> > and we will be able to quirk a particular device in a separate module,
> > without touching at the generic code (in hid-multitouch or hid-input).
> >
> > Anyway, I am missing a few bits for that so that's coming in later.
> >
>
> Is there any timeline for the followup series? I am wondering how that
> would affect haptic support implementation.

Hi Benjamin,

just pinging in hope of receiving some answer :)
I am thinking of preparing another version of haptic support patches
(https://lore.kernel.org/all/20220114183152.1691659-1-acz@semihalf.com/T/)
and if I could already start remodelling them based on your changes so that
it is actually a haptic hid driver and not and API that would be great :)
I am simply wondering when multitouch driver is going to be expressed simply
by your changes.

>
> > Cheers,
> > Benjamin
> >
> >
> > [0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
> >
> > Benjamin Tissoires (12):
> >   HID: core: statically allocate read buffers
> >   HID: core: de-duplicate some code in hid_input_field()
> >   HID: core: split data fetching from processing in hid_input_field()
> >   HID: input: tag touchscreens as such if the physical is not there
> >   HID: input: rework spaghetti code with switch statements
> >   HID: input: move up out-of-range processing of input values
> >   HID: compute an ordered list of input fields to process
> >   HID: core: for input reports, process the usages by priority list
> >   HID: input: enforce Invert usage to be processed before InRange
> >   HID: input: remove the need for HID_QUIRK_INVERT
> >   HID: input: accommodate priorities for slotted devices
> >   Input: docs: add more details on the use of BTN_TOOL
> >
> >  Documentation/input/event-codes.rst |   5 +-
> >  drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
> >  drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
> >  include/linux/hid.h                 |  23 +-
> >  4 files changed, 533 insertions(+), 105 deletions(-)
> >
> > --
> > 2.33.1
> >
>
> Does this patch series introduce the leaf driver support you mentioned
> in the haptic review?

Re: [PATCH 00/12] HID: fix for generic input processing

[Benjamin Tissoires]

On Mon, Feb 14, 2022 at 11:51 AM Angela Czubak <acz@semihalf.com> wrote:
>
> On Tue, Feb 8, 2022 at 8:19 PM Angela Czubak <acz@semihalf.com> wrote:
> >
> > Hi Benjamin,
> >
> > On Wed, Jan 26, 2022 at 5:18 PM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > This is a followup of the discussion we had between Wacom and
> > > the maintainers, and a followup of those 2 patch series:
> > >
> > > https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
> > > https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/
> > >
> > > It took me a while to get it right, but I finally can submit the
> > > series:
> > >
> > > - the first 8 patches are some cleanup in the hid-input.c and
> > >   hid-core.c code. They also create a list of input fields that
> > >   is then used to process the event, in the priority we think
> > >   is good.
> > >
> > >   For instance, on multitouch devices, it is better to have
> > >   Contact Count before processing all touches, and in each
> > >   touch, having Contact ID first is better. This series doesn't
> > >   cover hid-multitouch, but I have a series on top of this one that
> > >   does cover it.
> > >
> > >   Anyway, in our case, here, we need to process Invert before
> > >   In Range for tablets so we can make a decision whether the user
> > >   has the intend to erase or not.
> > >
> > > - patch 9 enforces the invert usage before In Range as mentioned
> > >   above
> > >
> > > - patch 10 is the actual bulk of processing that should fix the
> > >   generic tablet handling. Now that we have a reliable ordering
> > >   of the fields, we can compute the state of the tool in a reliable
> > >   way, and be kinder to userspace by not sending to it 2 tools at
> > >   the same time.
> > >
> > >   This patch has been extensively tested by hid-tools with the new
> > >   MR I submitted that add tests for tablets [0].
> > >
> > > - patch 11 is a nice to have that I need for my second series regarding
> > >   hid-multitouch. It is not mandatory with that series, but given
> > >   that it changes the format of the priorities in hid-input.c I thought
> > >   it would be best to send it as part of this series.
> > >
> > >   Note that now we are tagging the *reports* and the individual fields
> > >   when they are part of a multitouch collection, which should help
> > >   the drivers that implement this processing (hid-multitouch and wacom).
> > >
> > > - last, patch 12 is an attempt at fixing the documentation regarding
> > >   BTN_TOOL_* (requested by Peter).
> > >
> > >   Dmitry, feel free to take this one through your tree if you prefer
> > >   to do so (and if you are happy with it), otherwise we can take it
> > >   through the hid tree.
> > >
> > > As mentioned above, I have a followup series not entirely tidied up
> > > that implements the processing of Win8 mutltiouch devices in
> > > hid-input.c.
> > > There are several benefits for that: we should be able to drop the
> > > multitouch code in wacom.ko, we can simplify part of hid-multitouch,
> > > and we will be able to quirk a particular device in a separate module,
> > > without touching at the generic code (in hid-multitouch or hid-input).
> > >
> > > Anyway, I am missing a few bits for that so that's coming in later.
> > >
> >
> > Is there any timeline for the followup series? I am wondering how that
> > would affect haptic support implementation.
>
> Hi Benjamin,
>
> just pinging in hope of receiving some answer :)
> I am thinking of preparing another version of haptic support patches
> (https://lore.kernel.org/all/20220114183152.1691659-1-acz@semihalf.com/T/)
> and if I could already start remodelling them based on your changes so that
> it is actually a haptic hid driver and not and API that would be great :)
> I am simply wondering when multitouch driver is going to be expressed simply
> by your changes.

Hi Angela,

FWIW, I got a public branch that has the multitouch changes at
https://gitlab.freedesktop.org/bentiss/hid/-/commits/wip/input-mt-v5

The logic in the multitouch processing is correct but it is missing a
few bits IIRC:
- suspend/resume doesn't unset/set the multitouch parameters (doesn't
seem to be an issue on my devel laptop though)
- scantime is not properly handled
- width/height is not using the same path than hid-multitouch (and
probably not reported at all)
- hid-multitouch needs to be cleaned up to use the new core changes
instead of re-doing stuffs itself

I think that you should be able to experiment your hid-haptic changes
already, and see if that is indeed easier to use than creating an API
driver.

Cheers,
Benjamin

>
> >
> > > Cheers,
> > > Benjamin
> > >
> > >
> > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
> > >
> > > Benjamin Tissoires (12):
> > >   HID: core: statically allocate read buffers
> > >   HID: core: de-duplicate some code in hid_input_field()
> > >   HID: core: split data fetching from processing in hid_input_field()
> > >   HID: input: tag touchscreens as such if the physical is not there
> > >   HID: input: rework spaghetti code with switch statements
> > >   HID: input: move up out-of-range processing of input values
> > >   HID: compute an ordered list of input fields to process
> > >   HID: core: for input reports, process the usages by priority list
> > >   HID: input: enforce Invert usage to be processed before InRange
> > >   HID: input: remove the need for HID_QUIRK_INVERT
> > >   HID: input: accommodate priorities for slotted devices
> > >   Input: docs: add more details on the use of BTN_TOOL
> > >
> > >  Documentation/input/event-codes.rst |   5 +-
> > >  drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
> > >  drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
> > >  include/linux/hid.h                 |  23 +-
> > >  4 files changed, 533 insertions(+), 105 deletions(-)
> > >
> > > --
> > > 2.33.1
> > >
> >
> > Does this patch series introduce the leaf driver support you mentioned
> > in the haptic review?
>

Re: [PATCH 00/12] HID: fix for generic input processing

[Benjamin Tissoires]

On Tue, Feb 8, 2022 at 8:20 PM Angela Czubak <acz@semihalf.com> wrote:
>
> Hi Benjamin,
>
> On Wed, Jan 26, 2022 at 5:18 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Hi,
> >
> > This is a followup of the discussion we had between Wacom and
> > the maintainers, and a followup of those 2 patch series:
> >
> > https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
> > https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/
> >
> > It took me a while to get it right, but I finally can submit the
> > series:
> >
> > - the first 8 patches are some cleanup in the hid-input.c and
> >   hid-core.c code. They also create a list of input fields that
> >   is then used to process the event, in the priority we think
> >   is good.
> >
> >   For instance, on multitouch devices, it is better to have
> >   Contact Count before processing all touches, and in each
> >   touch, having Contact ID first is better. This series doesn't
> >   cover hid-multitouch, but I have a series on top of this one that
> >   does cover it.
> >
> >   Anyway, in our case, here, we need to process Invert before
> >   In Range for tablets so we can make a decision whether the user
> >   has the intend to erase or not.
> >
> > - patch 9 enforces the invert usage before In Range as mentioned
> >   above
> >
> > - patch 10 is the actual bulk of processing that should fix the
> >   generic tablet handling. Now that we have a reliable ordering
> >   of the fields, we can compute the state of the tool in a reliable
> >   way, and be kinder to userspace by not sending to it 2 tools at
> >   the same time.
> >
> >   This patch has been extensively tested by hid-tools with the new
> >   MR I submitted that add tests for tablets [0].
> >
> > - patch 11 is a nice to have that I need for my second series regarding
> >   hid-multitouch. It is not mandatory with that series, but given
> >   that it changes the format of the priorities in hid-input.c I thought
> >   it would be best to send it as part of this series.
> >
> >   Note that now we are tagging the *reports* and the individual fields
> >   when they are part of a multitouch collection, which should help
> >   the drivers that implement this processing (hid-multitouch and wacom).
> >
> > - last, patch 12 is an attempt at fixing the documentation regarding
> >   BTN_TOOL_* (requested by Peter).
> >
> >   Dmitry, feel free to take this one through your tree if you prefer
> >   to do so (and if you are happy with it), otherwise we can take it
> >   through the hid tree.
> >
> > As mentioned above, I have a followup series not entirely tidied up
> > that implements the processing of Win8 mutltiouch devices in
> > hid-input.c.
> > There are several benefits for that: we should be able to drop the
> > multitouch code in wacom.ko, we can simplify part of hid-multitouch,
> > and we will be able to quirk a particular device in a separate module,
> > without touching at the generic code (in hid-multitouch or hid-input).
> >
> > Anyway, I am missing a few bits for that so that's coming in later.
> >
>
> Is there any timeline for the followup series? I am wondering how that
> would affect haptic support implementation.

Sorry, no timeline on when we will be able to merge this. I was
enjoying some time off last week, but now I need to check whether the
comments I received need a v3 or not.

>
> > Cheers,
> > Benjamin
> >
> >
> > [0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
> >
> > Benjamin Tissoires (12):
> >   HID: core: statically allocate read buffers
> >   HID: core: de-duplicate some code in hid_input_field()
> >   HID: core: split data fetching from processing in hid_input_field()
> >   HID: input: tag touchscreens as such if the physical is not there
> >   HID: input: rework spaghetti code with switch statements
> >   HID: input: move up out-of-range processing of input values
> >   HID: compute an ordered list of input fields to process
> >   HID: core: for input reports, process the usages by priority list
> >   HID: input: enforce Invert usage to be processed before InRange
> >   HID: input: remove the need for HID_QUIRK_INVERT
> >   HID: input: accommodate priorities for slotted devices
> >   Input: docs: add more details on the use of BTN_TOOL
> >
> >  Documentation/input/event-codes.rst |   5 +-
> >  drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
> >  drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
> >  include/linux/hid.h                 |  23 +-
> >  4 files changed, 533 insertions(+), 105 deletions(-)
> >
> > --
> > 2.33.1
> >
>
> Does this patch series introduce the leaf driver support you mentioned
> in the haptic review?
>

As mentioned above, no, this patch series does not have the multitouch
support in it.

Let me answer your other email you sent while I am typing this one :)

Cheers,
Benjamin