[RFCv2 0/8] USI stylus support series

[RFCv2 0/8] USI stylus support series

[Tero Kristo]

Hi,

This series is an update based on comments from Benjamin. What is done
is this series is to ditch the separate hid-driver for USI, and add the
generic support to core layers. This part basically brings the support
for providing USI events, without programmability (patches 1-6).

Additionally, a HID-BPF based sample is provided which can be used to
program / query pen parameters in comparison to the old driver level
implementation (patches 7-8, patch #8 is an incremental change on top of
patch #7 which just converts the fifo to socket so that the client can
also get results back from the server.)

The whole series is based on top of Benjamin's hid-bpf support work, and
I've pushed a branch at [1] with a series that works and brings in
the dependency. There are also a few separate patches in this series to
fix the problems I found from Benjamin's initial work for hid-bpf; I
wasn't able to get things working without those. The branch is also
based on top of 5.16-rc2 which required some extra changes to the
patches from Benjamin.

-Tero

[1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf

[RFCv2 1/8] HID: Add map_msc() to avoid boilerplate code

[Tero Kristo]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Since we are going to have more MSC events too, add map_msc() that can
be used to fill in necessary fields and avoid boilerplate code.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/hid/hid-input.c | 6 ++----
 include/linux/hid.h     | 4 ++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 2c72ce4147b1..39ebedb2323b 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -52,6 +52,7 @@ static const struct {
 #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))
 #define map_led(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_LED, (c))
+#define map_msc(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_MSC, (c))
 
 #define map_abs_clear(c)	hid_map_usage_clear(hidinput, usage, &bit, \
 		&max, EV_ABS, (c))
@@ -872,10 +873,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 
 		case 0x5b: /* TransducerSerialNumber */
 		case 0x6e: /* TransducerSerialNumber2 */
-			usage->type = EV_MSC;
-			usage->code = MSC_SERIAL;
-			bit = input->mscbit;
-			max = MSC_MAX;
+			map_msc(MSC_SERIAL);
 			break;
 
 		default:  goto unknown;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index e95800bab56a..96eaca0d5322 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -766,6 +766,10 @@ static inline void hid_map_usage(struct hid_input *hidinput,
 		bmap = input->ledbit;
 		limit = LED_MAX;
 		break;
+	case EV_MSC:
+		bmap = input->mscbit;
+		limit = MSC_MAX;
+		break;
 	}
 
 	if (unlikely(c > limit || !bmap)) {
-- 
2.25.1

[RFCv2 2/8] HID: hid-input: Add suffix also for HID_DG_PEN

[Tero Kristo]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

This and HID_DG_STYLUS are pretty much the same thing so add suffix for
HID_DG_PEN too. This makes the input device name look better.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/hid/hid-input.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 39ebedb2323b..73c2edda742e 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1737,6 +1737,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid,
 		case HID_GD_MOUSE:
 			suffix = "Mouse";
 			break;
+		case HID_DG_PEN:
 		case HID_DG_STYLUS:
 			suffix = "Pen";
 			break;
-- 
2.25.1

[RFCv2 3/8] HID: core: Add support for USI style events

[Tero Kristo]

Add support for Universal Stylus Interface (USI) style events to the HID
core and input layers.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-input.c                | 18 ++++++++++++++++++
 include/linux/mod_devicetable.h        |  2 +-
 include/uapi/linux/hid.h               | 10 ++++++++++
 include/uapi/linux/input-event-codes.h | 22 ++++++++++++++--------
 4 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 73c2edda742e..b428ee9b4d9b 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -829,6 +829,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			}
 			break;
 
+		case 0x38: /* Transducer Index */
+			map_msc(MSC_PEN_ID);
+			break;
+
 		case 0x3b: /* Battery Strength */
 			hidinput_setup_battery(device, HID_INPUT_REPORT, field, false);
 			usage->type = EV_PWR;
@@ -876,6 +880,20 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			map_msc(MSC_SERIAL);
 			break;
 
+		case 0x5c: map_msc(MSC_PEN_COLOR);		break;
+		case 0x5e: map_msc(MSC_PEN_LINE_WIDTH);		break;
+
+		case 0x70:
+		case 0x71:
+		case 0x72:
+		case 0x73:
+		case 0x74:
+		case 0x75:
+		case 0x76:
+		case 0x77:
+			map_msc(MSC_PEN_LINE_STYLE);
+			break;
+
 		default:  goto unknown;
 		}
 		break;
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index ae2e75d15b21..4ff40be7676b 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -322,7 +322,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_KEY_MAX		0x2ff
 #define INPUT_DEVICE_ID_REL_MAX		0x0f
 #define INPUT_DEVICE_ID_ABS_MAX		0x3f
-#define INPUT_DEVICE_ID_MSC_MAX		0x07
+#define INPUT_DEVICE_ID_MSC_MAX		0x09
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
 #define INPUT_DEVICE_ID_FF_MAX		0x7f
diff --git a/include/uapi/linux/hid.h b/include/uapi/linux/hid.h
index 861bfbbfc565..60ef9b615a1a 100644
--- a/include/uapi/linux/hid.h
+++ b/include/uapi/linux/hid.h
@@ -255,6 +255,7 @@
 #define HID_DG_TOUCH				0x000d0033
 #define HID_DG_UNTOUCH				0x000d0034
 #define HID_DG_TAP				0x000d0035
+#define HID_DG_TRANSDUCER_INDEX			0x000d0038
 #define HID_DG_TABLETFUNCTIONKEY		0x000d0039
 #define HID_DG_PROGRAMCHANGEKEY			0x000d003a
 #define HID_DG_BATTERYSTRENGTH			0x000d003b
@@ -267,6 +268,15 @@
 #define HID_DG_BARRELSWITCH			0x000d0044
 #define HID_DG_ERASER				0x000d0045
 #define HID_DG_TABLETPICK			0x000d0046
+#define HID_DG_PEN_COLOR			0x000d005c
+#define HID_DG_PEN_LINE_WIDTH			0x000d005e
+#define HID_DG_PEN_LINE_STYLE			0x000d0070
+#define HID_DG_PEN_LINE_STYLE_INK		0x000d0072
+#define HID_DG_PEN_LINE_STYLE_PENCIL		0x000d0073
+#define HID_DG_PEN_LINE_STYLE_HIGHLIGHTER	0x000d0074
+#define HID_DG_PEN_LINE_STYLE_CHISEL_MARKER	0x000d0075
+#define HID_DG_PEN_LINE_STYLE_BRUSH		0x000d0076
+#define HID_DG_PEN_LINE_STYLE_NO_PREFERENCE	0x000d0077
 
 #define HID_CP_CONSUMERCONTROL			0x000c0001
 #define HID_CP_NUMERICKEYPAD			0x000c0002
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 225ec87d4f22..98295f71941a 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -901,14 +901,20 @@
  * Misc events
  */
 
-#define MSC_SERIAL		0x00
-#define MSC_PULSELED		0x01
-#define MSC_GESTURE		0x02
-#define MSC_RAW			0x03
-#define MSC_SCAN		0x04
-#define MSC_TIMESTAMP		0x05
-#define MSC_MAX			0x07
-#define MSC_CNT			(MSC_MAX+1)
+#define MSC_SERIAL			0x00
+#define MSC_PULSELED			0x01
+#define MSC_GESTURE			0x02
+#define MSC_RAW				0x03
+#define MSC_SCAN			0x04
+#define MSC_TIMESTAMP			0x05
+/* USI Pen events */
+#define MSC_PEN_ID			0x06
+#define MSC_PEN_COLOR			0x07
+#define MSC_PEN_LINE_WIDTH		0x08
+#define MSC_PEN_LINE_STYLE		0x09
+/* TODO: Add USI diagnostic & battery events too */
+#define MSC_MAX				0x09
+#define MSC_CNT				(MSC_MAX + 1)
 
 /*
  * LEDs
-- 
2.25.1

[RFCv2 4/8] HID: input: Make hidinput_find_field() static

[Tero Kristo]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

This function is not called outside of hid-input.c so we can make it
static.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/hid/hid-input.c | 4 ++--
 include/linux/hid.h     | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index b428ee9b4d9b..f6332d269d49 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1477,7 +1477,8 @@ void hidinput_report_event(struct hid_device *hid, struct hid_report *report)
 }
 EXPORT_SYMBOL_GPL(hidinput_report_event);
 
-int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field)
+static int hidinput_find_field(struct hid_device *hid, unsigned int type,
+			       unsigned int code, struct hid_field **field)
 {
 	struct hid_report *report;
 	int i, j;
@@ -1492,7 +1493,6 @@ int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int
 	}
 	return -1;
 }
-EXPORT_SYMBOL_GPL(hidinput_find_field);
 
 struct hid_field *hidinput_get_led_field(struct hid_device *hid)
 {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 96eaca0d5322..3f1fd4fcf1a9 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -636,7 +636,6 @@ extern void hidinput_disconnect(struct hid_device *);
 
 int hid_set_field(struct hid_field *, unsigned, __s32);
 int hid_input_report(struct hid_device *, int type, u8 *, u32, int);
-int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
 struct hid_field *hidinput_get_led_field(struct hid_device *hid);
 unsigned int hidinput_count_leds(struct hid_device *hid);
 __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
-- 
2.25.1

[RFCv2 5/8] HID: core: map USI pen style reports directly

[Tero Kristo]

USI pen style reports have funky layout, so don't try to parse them.
Instead, handle them identically to variable report items.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 525dbc59633f..315dc2b8ecbb 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1567,8 +1567,8 @@ 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) {
+		if (HID_MAIN_ITEM_VARIABLE & field->flags ||
+		    field->logical == HID_DG_PEN_LINE_STYLE) {
 			hid_process_event(hid, field, &field->usage[n], value[n], interrupt);
 			continue;
 		}
-- 
2.25.1

[RFCv2 6/8] HID: debug: Add USI usages

[Tero Kristo]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Add USI defined usages to the HID debug code.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
[tero.kristo: squashed in EV_MSC definitions]
Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-debug.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 7a92e2a04a09..cec0470e5485 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -141,8 +141,10 @@ static const struct hid_usage_entry hid_usage_table[] = {
     {0, 0x33, "Touch"},
     {0, 0x34, "UnTouch"},
     {0, 0x35, "Tap"},
+    {0, 0x38, "Transducer Index"},
     {0, 0x39, "TabletFunctionKey"},
     {0, 0x3a, "ProgramChangeKey"},
+    {0, 0x3B, "Battery Strength"},
     {0, 0x3c, "Invert"},
     {0, 0x42, "TipSwitch"},
     {0, 0x43, "SecondaryTipSwitch"},
@@ -160,7 +162,40 @@ static const struct hid_usage_entry hid_usage_table[] = {
     {0, 0x59, "ButtonType"},
     {0, 0x5A, "SecondaryBarrelSwitch"},
     {0, 0x5B, "TransducerSerialNumber"},
+    {0, 0x5C, "Preferred Color"},
+    {0, 0x5D, "Preferred Color is Locked"},
+    {0, 0x5E, "Preferred Line Width"},
+    {0, 0x5F, "Preferred Line Width is Locked"},
     {0, 0x6e, "TransducerSerialNumber2"},
+    {0, 0x70, "Preferred Line Style"},
+      {0, 0x71, "Preferred Line Style is Locked"},
+      {0, 0x72, "Ink"},
+      {0, 0x73, "Pencil"},
+      {0, 0x74, "Highlighter"},
+      {0, 0x75, "Chisel Marker"},
+      {0, 0x76, "Brush"},
+      {0, 0x77, "No Preference"},
+    {0, 0x80, "Digitizer Diagnostic"},
+    {0, 0x81, "Digitizer Error"},
+      {0, 0x82, "Err Normal Status"},
+      {0, 0x83, "Err Transducers Exceeded"},
+      {0, 0x84, "Err Full Trans Features Unavailable"},
+      {0, 0x85, "Err Charge Low"},
+    {0, 0x90, "Transducer Software Info"},
+      {0, 0x91, "Transducer Vendor Id"},
+      {0, 0x92, "Transducer Product Id"},
+    {0, 0x93, "Device Supported Protocols"},
+    {0, 0x94, "Transducer Supported Protocols"},
+      {0, 0x95, "No Protocol"},
+      {0, 0x96, "Wacom AES Protocol"},
+      {0, 0x97, "USI Protocol"},
+      {0, 0x98, "Microsoft Pen Protocol"},
+    {0, 0xA0, "Supported Report Rates"},
+      {0, 0xA1, "Report Rate"},
+      {0, 0xA2, "Transducer Connected"},
+      {0, 0xA3, "Switch Disabled"},
+      {0, 0xA4, "Switch Unimplemented"},
+      {0, 0xA5, "Transducer Switches"},
   { 15, 0, "PhysicalInterfaceDevice" },
     {0, 0x00, "Undefined"},
     {0, 0x01, "Physical_Interface_Device"},
@@ -990,7 +1025,10 @@ static const char *absolutes[ABS_CNT] = {
 
 static const char *misc[MSC_MAX + 1] = {
 	[MSC_SERIAL] = "Serial",	[MSC_PULSELED] = "Pulseled",
-	[MSC_GESTURE] = "Gesture",	[MSC_RAW] = "RawData"
+	[MSC_GESTURE] = "Gesture",	[MSC_RAW] = "RawData",
+	[MSC_PEN_ID] = "PenID",		[MSC_PEN_COLOR] "PenColor",
+	[MSC_PEN_LINE_WIDTH] = "PenLineWidth",
+	[MSC_PEN_LINE_STYLE] = "PenLineStyle",
 };
 
 static const char *leds[LED_MAX + 1] = {
-- 
2.25.1

[RFCv2 7/8] samples: hid: add new hid-usi sample

[Tero Kristo]

HID-usi sample adds support for writing USI pen variables. When the
sample is run, it creates a unix fifo under /tmp/usi, which can be
written with "<param> set <val>", where param is in [color,width,style],
and val is an integer value for the parameter.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 samples/bpf/Makefile       |   3 +
 samples/bpf/hid_usi.h      |  21 ++++
 samples/bpf/hid_usi_kern.c | 225 +++++++++++++++++++++++++++++++++
 samples/bpf/hid_usi_user.c | 247 +++++++++++++++++++++++++++++++++++++
 4 files changed, 496 insertions(+)
 create mode 100644 samples/bpf/hid_usi.h
 create mode 100644 samples/bpf/hid_usi_kern.c
 create mode 100644 samples/bpf/hid_usi_user.c

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 11f6f0cfab9e..6f45f6048979 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -62,6 +62,7 @@ tprogs-y += xdp_monitor
 
 tprogs-y += hid_mouse
 tprogs-y += hid_surface_dial
+tprogs-y += hid_usi
 
 # Libbpf dependencies
 LIBBPF_SRC = $(TOOLS_PATH)/lib/bpf
@@ -129,6 +130,7 @@ xdp_redirect-objs := xdp_redirect_user.o $(XDP_SAMPLE)
 xdp_monitor-objs := xdp_monitor_user.o $(XDP_SAMPLE)
 hid_mouse-objs := hid_mouse_user.o
 hid_surface_dial-objs := hid_surface_dial_user.o
+hid_usi-objs := hid_usi_user.o
 
 # Tell kbuild to always build the programs
 always-y := $(tprogs-y)
@@ -188,6 +190,7 @@ always-y += hbm_edt_kern.o
 always-y += xdpsock_kern.o
 always-y += hid_mouse_kern.o
 always-y += hid_surface_dial_kern.o
+always-y += hid_usi_kern.o
 
 ifeq ($(ARCH), arm)
 # Strip all except -D__LINUX_ARM_ARCH__ option needed to handle linux
diff --git a/samples/bpf/hid_usi.h b/samples/bpf/hid_usi.h
new file mode 100644
index 000000000000..90803375d9c1
--- /dev/null
+++ b/samples/bpf/hid_usi.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright(c) 2021 Intel Corporation.
+ */
+
+#ifndef HID_USI_H_
+#define HID_USI_H_
+
+#include <linux/bits.h>
+
+enum {
+	USI_PEN_ID = 0,
+	USI_PEN_IN_RANGE,
+	USI_PEN_TOUCHING,
+	USI_PEN_COLOR,
+	USI_PEN_LINE_WIDTH,
+	USI_PEN_LINE_STYLE,
+	USI_NUM_PARAMS
+};
+
+#endif /* HID_USI_H */
diff --git a/samples/bpf/hid_usi_kern.c b/samples/bpf/hid_usi_kern.c
new file mode 100644
index 000000000000..8b3c300f7bb2
--- /dev/null
+++ b/samples/bpf/hid_usi_kern.c
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2021 Intel Corporation. */
+#include <linux/version.h>
+#include <uapi/linux/bpf.h>
+#include <uapi/linux/hid.h>
+#include <uapi/linux/bpf_hid.h>
+#include <bpf/bpf_helpers.h>
+
+#include "hid_usi.h"
+
+static const char param_names[USI_NUM_PARAMS][6] = {
+	"id",
+	"flags",
+	"color",
+	"width",
+	"style",
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_RINGBUF);
+	__uint(max_entries, 4096);
+} ringbuf SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, USI_NUM_PARAMS);
+} cache SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, USI_NUM_PARAMS);
+} wr_cache SEC(".maps");
+
+struct rep_data {
+	int offset;
+	int size;
+	int idx;
+};
+
+static struct rep_data inputs[USI_NUM_PARAMS];
+static struct rep_data features[USI_NUM_PARAMS];
+
+SEC("hid/raw_event")
+int hid_raw_event(struct hid_bpf_ctx *ctx)
+{
+	u32 i;
+	u32 tmp;
+	int val;
+	int *wrc, *c;
+	u8 *buf;
+	u32 flags = 0;
+	int offset;
+	int size;
+	int in_range;
+	int touching;
+	bool cache_valid;
+
+	if (ctx->event.data[0] != inputs[USI_PEN_IN_RANGE].idx)
+		return 0;
+
+	in_range = bpf_hid_get_data(ctx, inputs[USI_PEN_IN_RANGE].offset,
+				    inputs[USI_PEN_IN_RANGE].size);
+	touching = bpf_hid_get_data(ctx, inputs[USI_PEN_TOUCHING].offset,
+				    inputs[USI_PEN_TOUCHING].size);
+
+	bpf_printk("flags: in_range=%d, touching=%d", in_range, touching);
+
+	if (!touching && cache_valid) {
+		for (i = USI_PEN_COLOR; i < USI_NUM_PARAMS; i++) {
+			val = 0;
+
+			/*
+			 * Make a local copy of i so that we can get a
+			 * pointer reference to it. i itself must remain
+			 * inside a register so that BPF verifier can
+			 * calculate its value properly.
+			 */
+			tmp = i;
+			bpf_map_update_elem(&wr_cache, &tmp, &val, 0);
+			bpf_map_update_elem(&cache, &tmp, &val, 0);
+		}
+		cache_valid = false;
+	}
+
+	if (!touching)
+		return 0;
+
+	for (i = USI_PEN_COLOR; i < USI_NUM_PARAMS; i++) {
+		offset = inputs[i].offset;
+		size = inputs[i].size;
+		val = bpf_hid_get_data(ctx, offset, size);
+
+		/*
+		 * Again, make a local copy of i which we can refer via a
+		 * pointer to satisfy BPF verifier.
+		 */
+		tmp = i;
+
+		wrc = bpf_map_lookup_elem(&wr_cache, &tmp);
+		c = bpf_map_lookup_elem(&cache, &tmp);
+		if (!wrc || !c)
+			continue;
+
+		bpf_printk("raw[%d]: %s = %02x", i, param_names[i], val);
+		bpf_printk(" (c=%02x, wr=%02x)", *c, *wrc);
+
+		if (*wrc != *c) {
+			val = *wrc;
+
+			buf = bpf_ringbuf_reserve(&ringbuf, 16, 0);
+			if (!buf)
+				continue;
+
+			buf[0] = features[i].idx;
+			buf[1] = 1;
+			buf[2] = val;
+
+			bpf_hid_raw_request(ctx, buf, 3,
+					    HID_FEATURE_REPORT,
+					    HID_REQ_SET_REPORT);
+
+			bpf_ringbuf_discard(buf, 0);
+		}
+
+		if (val != *c && !cache_valid) {
+			bpf_map_update_elem(&cache, &tmp, &val, 0);
+			bpf_map_update_elem(&wr_cache, &tmp, &val, 0);
+			cache_valid = true;
+		} else {
+			bpf_hid_set_data(ctx, offset, size, *c);
+		}
+	}
+
+	return 0;
+}
+
+SEC("hid/event")
+int set_haptic(struct hid_bpf_ctx *ctx)
+{
+	return 0;
+}
+
+static void process_tag(struct hid_bpf_ctx *ctx, struct rep_data *data,
+			struct hid_bpf_parser *parser, u64 *idx)
+{
+	u32 i;
+	int id;
+	u32 offset;
+
+	for (i = 0; i < parser->local.usage_index && i < 16; i++) {
+		offset = parser->report.current_offset +
+			i * parser->global.report_size;
+
+		switch (parser->local.usage[i]) {
+		case HID_DG_PEN_COLOR:
+			id = USI_PEN_COLOR;
+			break;
+		case HID_DG_PEN_LINE_WIDTH:
+			id = USI_PEN_LINE_WIDTH;
+			break;
+		case HID_DG_PEN_LINE_STYLE_INK:
+			id = USI_PEN_LINE_STYLE;
+			break;
+		case HID_DG_INRANGE:
+			if (parser->local.usage_index == 1)
+				continue;
+
+			id = USI_PEN_IN_RANGE;
+			break;
+		case HID_DG_TIPSWITCH:
+			if (parser->local.usage_index == 1)
+				continue;
+
+			id = USI_PEN_TOUCHING;
+			break;
+		default:
+			continue;
+		}
+
+		data[id].offset = offset + 8;
+		data[id].size = parser->global.report_size;
+		data[id].idx = parser->report.id;
+
+		bpf_printk("parsed id=%d, offset=%d, idx=%d",
+			   id, data[id].offset, data[id].idx);
+	}
+}
+
+static u64 process_hid_rdesc_item(struct hid_bpf_ctx *ctx,
+				  struct hid_bpf_parser *parser, u64 *idx,
+				  void *data)
+{
+	struct hid_bpf_item *item = &parser->item;
+
+	switch (item->type) {
+	case HID_ITEM_TYPE_MAIN:
+		if (item->tag == HID_MAIN_ITEM_TAG_INPUT)
+			process_tag(ctx, inputs, parser, idx);
+		if (item->tag == HID_MAIN_ITEM_TAG_FEATURE)
+			process_tag(ctx, features, parser, idx);
+	}
+
+	return 0;
+}
+
+SEC("hid/rdesc_fixup")
+int hid_rdesc_fixup(struct hid_bpf_ctx *ctx)
+{
+	int ret;
+
+	if (ctx->type != HID_BPF_RDESC_FIXUP)
+		return 0;
+
+	ret = bpf_hid_foreach_rdesc_item(ctx, process_hid_rdesc_item, (void *)0, 0);
+	bpf_printk("ret: %d", ret);
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
+u32 _version SEC("version") = LINUX_VERSION_CODE;
diff --git a/samples/bpf/hid_usi_user.c b/samples/bpf/hid_usi_user.c
new file mode 100644
index 000000000000..b05a3f768835
--- /dev/null
+++ b/samples/bpf/hid_usi_user.c
@@ -0,0 +1,247 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021, Intel Corporation
+ */
+#include <linux/bpf.h>
+#include <linux/if_link.h>
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <libgen.h>
+#include <sys/resource.h>
+#include <getopt.h>
+
+#include "bpf_util.h"
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include <sys/stat.h>
+
+#include "hid_usi.h"
+
+static char *sysfs_path;
+static char *fifoname = "/tmp/usi";
+static int sysfs_fd;
+static int fifo_fd;
+static int prog_count;
+static int cache, wr_cache;
+
+static const struct option long_options[] = {
+	{ "help", no_argument, NULL, 'h' },
+	{ "fifo", required_argument, NULL, 'f' },
+};
+
+struct prog {
+	int fd;
+	enum bpf_attach_type type;
+};
+
+static struct prog progs[10];
+
+static void int_exit(int sig)
+{
+	int ret;
+
+	for (prog_count--; prog_count >= 0 ; prog_count--) {
+		ret = bpf_prog_detach2(progs[prog_count].fd, sysfs_fd, progs[prog_count].type);
+		if (ret)
+			fprintf(stderr, "bpf_prog_detach2: returned %m\n");
+	}
+
+	close(sysfs_fd);
+	close(fifo_fd);
+	remove(fifoname);
+	exit(0);
+}
+
+static void usage(const char *prog)
+{
+	fprintf(stderr,
+		"usage: %s [-f <fifoname>] /dev/HIDRAW\n\n",
+		prog);
+}
+
+static int param_to_idx(const char *param)
+{
+	if (!strcmp(param, "color"))
+		return USI_PEN_COLOR;
+	if (!strcmp(param, "width"))
+		return USI_PEN_LINE_WIDTH;
+	if (!strcmp(param, "style"))
+		return USI_PEN_LINE_STYLE;
+
+	return -EINVAL;
+}
+
+static int write_value(const char *param, int value)
+{
+	int err;
+	int idx = param_to_idx(param);
+
+	printf("%s: param=%s (%d), value=%d\n", __func__, param, idx, value);
+	err = bpf_map_update_elem(wr_cache, &idx, &value, BPF_ANY);
+	if (err)
+		printf("Update failed for %d, err=%d\n", idx, err);
+
+	return 0;
+}
+
+static int read_value(const char *param)
+{
+	int value;
+	int idx = param_to_idx(param);
+
+	printf("%s: param=%s (%d)\n", __func__, param, idx);
+
+	if (bpf_map_lookup_elem(cache, &idx, &value))
+		printf("Value missing for %d\n", idx);
+	else
+		printf("Value for %d = %d\n", idx, value);
+
+	return 0;
+}
+
+static int attach_progs(int argc, char **argv)
+{
+	char filename[256];
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+	struct bpf_object *obj;
+	struct bpf_program *prog;
+	int err = 0;
+	char buf[BUFSIZ];
+	char param[16];
+	char op[8];
+	int value;
+	int m, n;
+
+	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+	obj = bpf_object__open_file(filename, NULL);
+	err = libbpf_get_error(obj);
+	if (err) {
+		fprintf(stderr, "ERROR: opening BPF object file failed\n");
+		obj = NULL;
+		err = 1;
+		goto cleanup;
+	}
+
+	/* load BPF program */
+	err = bpf_object__load(obj);
+	if (err) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	sysfs_fd = open(sysfs_path, O_RDONLY);
+
+	bpf_object__for_each_program(prog, obj) {
+		progs[prog_count].fd = bpf_program__fd(prog);
+		progs[prog_count].type = bpf_program__get_expected_attach_type(prog);
+
+		err = bpf_prog_attach(progs[prog_count].fd,
+				      sysfs_fd,
+				      progs[prog_count].type,
+				      0);
+		if (err) {
+			fprintf(stderr, "bpf_prog_attach: err=%m\n");
+			progs[prog_count].fd = 0;
+			goto cleanup;
+		}
+		printf("attached BPF program with FD=%d, type=%d\n",
+		       progs[prog_count].fd, progs[prog_count].type);
+		prog_count++;
+	}
+
+	signal(SIGINT, int_exit);
+	signal(SIGTERM, int_exit);
+
+	err = bpf_obj_get_info_by_fd(progs[0].fd, &info, &info_len);
+	if (err) {
+		printf("can't get prog info - %s\n", strerror(errno));
+		goto cleanup;
+	}
+
+	cache = bpf_object__find_map_fd_by_name(obj, "cache");
+	if (cache < 0) {
+		printf("can't get 'cache' shared mem from object - %m\n");
+		goto cleanup;
+	}
+
+	wr_cache = bpf_object__find_map_fd_by_name(obj, "wr_cache");
+	if (wr_cache < 0) {
+		printf("can't get 'wr_cache' shared mem from object - %m\n");
+		goto cleanup;
+	}
+
+	mkfifo(fifoname, 0666);
+
+	fifo_fd = open(fifoname, O_RDWR);
+	if (fifo_fd < 0) {
+		perror("Fifo open error.\n");
+		err = fifo_fd;
+		goto cleanup;
+	}
+
+	while (1) {
+		n = read(fifo_fd, buf, BUFSIZ);
+		if (n < 0)
+			break;
+		buf[n] = 0;
+
+		printf("%s: received '%s'\n", __func__, buf);
+
+		m = sscanf(buf, "%16s %8s %d", param, op, &value);
+		if (m == 2 && strcmp(op, "get") == 0)
+			read_value(param);
+		else if (m == 3 && strcmp(op, "set") == 0)
+			write_value(param, value);
+	}
+
+	return 0;
+
+ cleanup:
+	for (prog_count--; prog_count >= 0; prog_count--) {
+		if (bpf_prog_detach2(progs[prog_count].fd, sysfs_fd, progs[prog_count].type))
+			fprintf(stderr, "bpf_prog_detach2: returned %m\n");
+	}
+
+	bpf_object__close(obj);
+	return err;
+}
+
+int main(int argc, char **argv)
+{
+	int opt;
+
+	while ((opt = getopt_long(argc, argv, "f:", long_options,
+				  NULL)) != -1) {
+		switch (opt) {
+		case 'f':
+			fifoname = optarg;
+			break;
+		default:
+			usage(basename(argv[0]));
+			return 1;
+		}
+	}
+
+	if (optind == argc) {
+		usage(basename(argv[0]));
+		return 1;
+	}
+
+	sysfs_path = argv[optind];
+	if (!sysfs_path) {
+		perror("hidraw");
+		return 1;
+	}
+
+	printf("fifoname: %s\n", fifoname);
+	printf("sysfs_path: %s\n", sysfs_path);
+
+	return attach_progs(argc, argv);
+}
-- 
2.25.1

[RFCv2 8/8] samples: hid: convert USI sample to use unix socket for comms

[Tero Kristo]

Convert USI hid sample userspace interface from using unix fifo to unix
socket. This allows sending results back via the communication channel
also.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 samples/bpf/hid_usi_user.c | 73 ++++++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 23 deletions(-)

diff --git a/samples/bpf/hid_usi_user.c b/samples/bpf/hid_usi_user.c
index b05a3f768835..5bf5ebc21a30 100644
--- a/samples/bpf/hid_usi_user.c
+++ b/samples/bpf/hid_usi_user.c
@@ -14,6 +14,8 @@
 #include <libgen.h>
 #include <sys/resource.h>
 #include <getopt.h>
+#include <sys/socket.h>
+#include <sys/un.h>
 
 #include "bpf_util.h"
 #include <bpf/bpf.h>
@@ -24,15 +26,15 @@
 #include "hid_usi.h"
 
 static char *sysfs_path;
-static char *fifoname = "/tmp/usi";
+static char *sockname = "/tmp/usi";
 static int sysfs_fd;
-static int fifo_fd;
+static int sock_fd;
 static int prog_count;
 static int cache, wr_cache;
 
 static const struct option long_options[] = {
 	{ "help", no_argument, NULL, 'h' },
-	{ "fifo", required_argument, NULL, 'f' },
+	{ "sock", required_argument, NULL, 's' },
 };
 
 struct prog {
@@ -53,15 +55,15 @@ static void int_exit(int sig)
 	}
 
 	close(sysfs_fd);
-	close(fifo_fd);
-	remove(fifoname);
+	close(sock_fd);
+	remove(sockname);
 	exit(0);
 }
 
 static void usage(const char *prog)
 {
 	fprintf(stderr,
-		"usage: %s [-f <fifoname>] /dev/HIDRAW\n\n",
+		"usage: %s [-s <sockname>] /dev/HIDRAW\n\n",
 		prog);
 }
 
@@ -84,15 +86,17 @@ static int write_value(const char *param, int value)
 
 	printf("%s: param=%s (%d), value=%d\n", __func__, param, idx, value);
 	err = bpf_map_update_elem(wr_cache, &idx, &value, BPF_ANY);
-	if (err)
+	if (err) {
 		printf("Update failed for %d, err=%d\n", idx, err);
+		return err;
+	}
 
 	return 0;
 }
 
 static int read_value(const char *param)
 {
-	int value;
+	int value = -ENOENT;
 	int idx = param_to_idx(param);
 
 	printf("%s: param=%s (%d)\n", __func__, param, idx);
@@ -102,7 +106,7 @@ static int read_value(const char *param)
 	else
 		printf("Value for %d = %d\n", idx, value);
 
-	return 0;
+	return value;
 }
 
 static int attach_progs(int argc, char **argv)
@@ -118,6 +122,9 @@ static int attach_progs(int argc, char **argv)
 	char op[8];
 	int value;
 	int m, n;
+	struct sockaddr_un addr;
+	struct sockaddr_un from;
+	socklen_t from_len = sizeof(from);
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 	obj = bpf_object__open_file(filename, NULL);
@@ -177,28 +184,48 @@ static int attach_progs(int argc, char **argv)
 		goto cleanup;
 	}
 
-	mkfifo(fifoname, 0666);
+	sock_fd = socket(PF_UNIX, SOCK_DGRAM, 0);
+	if (sock_fd < 0) {
+		perror("socket open error.\n");
+		err = sock_fd;
+		goto cleanup;
+	}
+
+	addr.sun_family = AF_UNIX;
+	strcpy(addr.sun_path, sockname);
+	unlink(sockname);
 
-	fifo_fd = open(fifoname, O_RDWR);
-	if (fifo_fd < 0) {
-		perror("Fifo open error.\n");
-		err = fifo_fd;
+	if (bind(sock_fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
+		perror("bind");
 		goto cleanup;
 	}
 
 	while (1) {
-		n = read(fifo_fd, buf, BUFSIZ);
+		n = recvfrom(sock_fd, buf, BUFSIZ, 0, (struct sockaddr *)&from,
+			     &from_len);
 		if (n < 0)
 			break;
 		buf[n] = 0;
 
 		printf("%s: received '%s'\n", __func__, buf);
 
+		printf("%s: from_len=%d, from=%s\n", __func__, from_len,
+		       from.sun_path);
+
 		m = sscanf(buf, "%16s %8s %d", param, op, &value);
-		if (m == 2 && strcmp(op, "get") == 0)
-			read_value(param);
-		else if (m == 3 && strcmp(op, "set") == 0)
-			write_value(param, value);
+		if (m == 2 && strcmp(op, "get") == 0) {
+			value = read_value(param);
+			sprintf(buf, "%s: %d\n", param, value);
+			printf("%s: sending '%s'\n", __func__, buf);
+			sendto(sock_fd, buf, strlen(buf) + 1, 0,
+			       (struct sockaddr *)&from, from_len);
+		} else if (m == 3 && strcmp(op, "set") == 0) {
+			err = write_value(param, value);
+			sprintf(buf, "%s: %d, err=%d\n", param, value, err);
+			printf("%s: sending '%s'\n", __func__, buf);
+			sendto(sock_fd, buf, strlen(buf) + 1, 0,
+			       (struct sockaddr *)&from, from_len);
+		}
 	}
 
 	return 0;
@@ -217,11 +244,11 @@ int main(int argc, char **argv)
 {
 	int opt;
 
-	while ((opt = getopt_long(argc, argv, "f:", long_options,
+	while ((opt = getopt_long(argc, argv, "s:", long_options,
 				  NULL)) != -1) {
 		switch (opt) {
-		case 'f':
-			fifoname = optarg;
+		case 's':
+			sockname = optarg;
 			break;
 		default:
 			usage(basename(argv[0]));
@@ -240,7 +267,7 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	printf("fifoname: %s\n", fifoname);
+	printf("sockname: %s\n", sockname);
 	printf("sysfs_path: %s\n", sysfs_path);
 
 	return attach_progs(argc, argv);
-- 
2.25.1

Re: [RFCv2 0/8] USI stylus support series

[Hyungwoo Yang]

Hi Tero,


I have a question. As you know, USI provides a room for vendors to 
differentiate their stylus. If a vendor wants to add reach features to 
differentiate their stylus. Do you think the vendor needs to come up 
with like HID-USI-<vendor>.c to configure the corresponding 
usages(vendor-defined data) ? or we should use other approach ? like 
register callbacks via HID-core ?

-Hyungwoo

On 11/26/21 5:01 AM, Tero Kristo wrote:
> Hi,
>
> This series is an update based on comments from Benjamin. What is done
> is this series is to ditch the separate hid-driver for USI, and add the
> generic support to core layers. This part basically brings the support
> for providing USI events, without programmability (patches 1-6).
>
> Additionally, a HID-BPF based sample is provided which can be used to
> program / query pen parameters in comparison to the old driver level
> implementation (patches 7-8, patch #8 is an incremental change on top of
> patch #7 which just converts the fifo to socket so that the client can
> also get results back from the server.)
>
> The whole series is based on top of Benjamin's hid-bpf support work, and
> I've pushed a branch at [1] with a series that works and brings in
> the dependency. There are also a few separate patches in this series to
> fix the problems I found from Benjamin's initial work for hid-bpf; I
> wasn't able to get things working without those. The branch is also
> based on top of 5.16-rc2 which required some extra changes to the
> patches from Benjamin.
>
> -Tero
>
> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
>
>
>
>

Re: [RFCv2 0/8] USI stylus support series

[Tero Kristo]

Hi Hyungwoo,

On 30/11/2021 08:36, Hyungwoo Yang wrote:
> Hi Tero,
>
>
> I have a question. As you know, USI provides a room for vendors to 
> differentiate their stylus. If a vendor wants to add reach features to 
> differentiate their stylus. Do you think the vendor needs to come up 
> with like HID-USI-<vendor>.c to configure the corresponding 
> usages(vendor-defined data) ? or we should use other approach ? like 
> register callbacks via HID-core ?

I think this depends quite a bit on what kind of features we are talking 
about. Looking at this series, we have:

- hid-core changes to add new event codes / add support for these

- bfp modifications to support caching and write feature for the new 
event codes (technically they can be written already via /dev/hidraw 
directly, but the interface is bit clumsy, so we improve it with BFP)

So, depending on the feature you want to add, you can take either way, 
but probably hacking with BPF is going to be easiest. Vendor could even 
write their own BPF tool. Also, please note that the location of 
samples/bpf/hid_usi* is not going to remain, there will most likely be 
an external repository where the new HID related BPF tools are going to 
maintained, Benjamin had some thoughts about that already.

-Tero


>
> -Hyungwoo
>
> On 11/26/21 5:01 AM, Tero Kristo wrote:
>> Hi,
>>
>> This series is an update based on comments from Benjamin. What is done
>> is this series is to ditch the separate hid-driver for USI, and add the
>> generic support to core layers. This part basically brings the support
>> for providing USI events, without programmability (patches 1-6).
>>
>> Additionally, a HID-BPF based sample is provided which can be used to
>> program / query pen parameters in comparison to the old driver level
>> implementation (patches 7-8, patch #8 is an incremental change on top of
>> patch #7 which just converts the fifo to socket so that the client can
>> also get results back from the server.)
>>
>> The whole series is based on top of Benjamin's hid-bpf support work, and
>> I've pushed a branch at [1] with a series that works and brings in
>> the dependency. There are also a few separate patches in this series to
>> fix the problems I found from Benjamin's initial work for hid-bpf; I
>> wasn't able to get things working without those. The branch is also
>> based on top of 5.16-rc2 which required some extra changes to the
>> patches from Benjamin.
>>
>> -Tero
>>
>> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
>>
>>
>>
>>

Re: [RFCv2 0/8] USI stylus support series

[Benjamin Tissoires]

Hi Tero,

On Fri, Nov 26, 2021 at 2:02 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi,
>
> This series is an update based on comments from Benjamin. What is done
> is this series is to ditch the separate hid-driver for USI, and add the
> generic support to core layers. This part basically brings the support
> for providing USI events, without programmability (patches 1-6).

That part seems to be almost good for now. I have a few things to check:
- patch2: "HID: hid-input: Add suffix also for HID_DG_PEN" I need to
ensure there are no touchscreens affected by this (there used to be a
mess with some vendors where they would not declare things properly)
- patch5: "HID: core: map USI pen style reports directly" this one
feels plain wrong. I would need to have a look at the report
descriptor but this is too specific in a very generic code

>
> Additionally, a HID-BPF based sample is provided which can be used to
> program / query pen parameters in comparison to the old driver level
> implementation (patches 7-8, patch #8 is an incremental change on top of
> patch #7 which just converts the fifo to socket so that the client can
> also get results back from the server.)

After a few more thoughts, I wondered what your input is on this. We
should be able to do the very same with plain hidraw... However, you
added a `hid/raw_event` processing that will still be kept in the
kernel, so maybe bpf would be useful for that at least.

>
> The whole series is based on top of Benjamin's hid-bpf support work, and
> I've pushed a branch at [1] with a series that works and brings in
> the dependency. There are also a few separate patches in this series to
> fix the problems I found from Benjamin's initial work for hid-bpf; I
> wasn't able to get things working without those. The branch is also
> based on top of 5.16-rc2 which required some extra changes to the
> patches from Benjamin.

Yeah, I also rebased on top of 5.16 shortly after sharing that branch
and got roughly the same last fix (HID: bpf: compile fix for
bpf_hid_foreach_rdesc_item). I am *very* interested in your "HID: bpf:
execute BPF programs in proper context" because that is something that
was bothering me a lot :)

"HID: bpf: add expected_attach_type to bpf prog during detach" is
something I'll need to bring in too

but "HID: bpf: fix file mapping" is actually wrong. I initially wanted
to attach BPF programs to hidraw, but shortly realized that this is
not working because the `hid/rdesc_fixup` kills the hidraw node and so
releases the BPF programs. The way I am now attaching it is to use the
fd associated with the modalias in the sysfs file (for instance: `sudo
./hid_surface_dial /sys/bus/hid/devices/0005:045E:091B.*/modalias`).
This way, the reference to the struct hid_device is kept even if we
disconnect the device and reprobe it.

Thanks again for your work, and I'd be curious to have your thoughts
on hid-bpf and if you think it is better than hidraw/evdev write/new
ioctls for your use case.

Cheers,
Benjamin

>
> -Tero
>
> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
>
>

Re: [RFCv2 0/8] USI stylus support series

[Tero Kristo]

Hi Benjamin,

On 30/11/2021 16:44, Benjamin Tissoires wrote:
> Hi Tero,
>
> On Fri, Nov 26, 2021 at 2:02 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>> Hi,
>>
>> This series is an update based on comments from Benjamin. What is done
>> is this series is to ditch the separate hid-driver for USI, and add the
>> generic support to core layers. This part basically brings the support
>> for providing USI events, without programmability (patches 1-6).
> That part seems to be almost good for now. I have a few things to check:
> - patch2: "HID: hid-input: Add suffix also for HID_DG_PEN" I need to
> ensure there are no touchscreens affected by this (there used to be a
> mess with some vendors where they would not declare things properly)
> - patch5: "HID: core: map USI pen style reports directly" this one
> feels plain wrong. I would need to have a look at the report
> descriptor but this is too specific in a very generic code

Relevant part of the report descriptor is here:

     Field(8)
       Physical(Digitizers.Stylus)
       Logical(Digitizers.Preferred Line Style)
       Application(Digitizers.Pen)
       Usage(6)
         Digitizers.Ink
         Digitizers.Pencil
         Digitizers.Highlighter
         Digitizers.Chisel Marker
         Digitizers.Brush
         Digitizers.No Preference
       Logical Minimum(1)
       Logical Maximum(6)
       Physical Minimum(0)
       Physical Maximum(255)
       Unit Exponent(-1)
       Unit(SI Linear : Centimeter)
       Report Size(8)
       Report Count(1)
       Report Offset(88)
       Flags( Variable Absolute NoPreferredState )

To me, it looks almost like it is a bug in the report descriptor itself; 
as you see there are 6 usage values but the report size / count is 1 
byte. The fact that there are 6 usage values in the field confuses 
hid-core. Basically the usage values are used as encoded content for the 
field.

Alternatively I think this could be patched up in the BPF program, as I 
am modifying the content of the raw hid report already; I could just as 
well modify this one also. Or, maybe I could fix the report descriptor 
itself to act as a sane variable, as I am parsing the report descriptor 
already?

>
>> Additionally, a HID-BPF based sample is provided which can be used to
>> program / query pen parameters in comparison to the old driver level
>> implementation (patches 7-8, patch #8 is an incremental change on top of
>> patch #7 which just converts the fifo to socket so that the client can
>> also get results back from the server.)
> After a few more thoughts, I wondered what your input is on this. We
> should be able to do the very same with plain hidraw... However, you
> added a `hid/raw_event` processing that will still be kept in the
> kernel, so maybe bpf would be useful for that at least.

Yes, plain hidraw can be sort of used to program the values, however the 
interface is kind of annoying to use for the USI pens. You need to be 
touching the display with the pen before anything is accepted. Maybe 
writing some support code to the libevdev would help.

The hidraw hook is needed for processing the cached values also, USI 
pens report their parameters with a delay of some few hundred ms 
depending on controller vendor. And in some cases they don't report 
anything back before forcibly querying the value from the controller, 
and also the write mechanism acts differently; some controllers report 
the programmed value back, others keep reporting the old value until the 
pen leaves the screen and touches it again.


>
>> The whole series is based on top of Benjamin's hid-bpf support work, and
>> I've pushed a branch at [1] with a series that works and brings in
>> the dependency. There are also a few separate patches in this series to
>> fix the problems I found from Benjamin's initial work for hid-bpf; I
>> wasn't able to get things working without those. The branch is also
>> based on top of 5.16-rc2 which required some extra changes to the
>> patches from Benjamin.
> Yeah, I also rebased on top of 5.16 shortly after sharing that branch
> and got roughly the same last fix (HID: bpf: compile fix for
> bpf_hid_foreach_rdesc_item). I am *very* interested in your "HID: bpf:
> execute BPF programs in proper context" because that is something that
> was bothering me a lot :)

Right, I think I have plenty of lockdep / scheduler checks enabled in my 
kernel. They generate plenty of spam with i2c-hid without that patch. 
The same issue may not be visible with some other low level hid devices 
though, I don't have testing capability for anything but the i2c-hid 
right now. I2C is quite notorious for the locking aspects as it is slow 
and is used to control some pretty low level stuff like power management 
etc.

>
> "HID: bpf: add expected_attach_type to bpf prog during detach" is
> something I'll need to bring in too
>
> but "HID: bpf: fix file mapping" is actually wrong. I initially wanted
> to attach BPF programs to hidraw, but shortly realized that this is
> not working because the `hid/rdesc_fixup` kills the hidraw node and so
> releases the BPF programs. The way I am now attaching it is to use the
> fd associated with the modalias in the sysfs file (for instance: `sudo
> ./hid_surface_dial /sys/bus/hid/devices/0005:045E:091B.*/modalias`).
> This way, the reference to the struct hid_device is kept even if we
> disconnect the device and reprobe it.
Ok I can check this out if it works me also. The samples lead me to 
/dev/hidraw usage.
>
> Thanks again for your work, and I'd be curious to have your thoughts
> on hid-bpf and if you think it is better than hidraw/evdev write/new
> ioctls for your use case.

The new driver was 777 lines diff, the BPF one is 496 lines so it 
appears smaller. The driver did support two different vendors though 
(ELAN+Goodix, with their specific quirks in place), the BPF only a 
single one right now (ELAN).

The vendor specific quirks are a question, do we want to support that 
somehow in a single BPF binary, or should we attach vendor specific BPF 
programs?

Chromium-os devices are one of the main customers for USI pens right 
now, and I am not sure how well they will take the BPF concept. :) I did 
ask their feedback though, and I'll come back on this once I have something.

Personally, I don't have much preference either way at this moment, both 
seem like feasible options. I might lean a bit towards evdev/ioctl as it 
seems a cleaner implementation as of now. The write mechanism I 
implemented for the USI-BPF is a bit hacky, as it just directly writes 
to a shared memory buffer and the buffer gets parsed by the kernel part 
when it processes hidraw event. Anyways, do you have any feedback on 
that part? BPF is completely new to me again so would love to get some 
feedback.

One option is of course to push the write portion of the code to 
userspace and just use hidraw, but we still need to filter out the bogus 
events somehow, and do that in vendor specific manner. I don't think 
this can be done on userspace, as plenty of information that would be 
needed to do this properly has been lost at the input-event level.

-Tero

>
> Cheers,
> Benjamin
>
>> -Tero
>>
>> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
>>
>>

Re: [RFCv2 0/8] USI stylus support series

[Benjamin Tissoires]

Hi Tero,

On Tue, Nov 30, 2021 at 5:13 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi Benjamin,
>
> On 30/11/2021 16:44, Benjamin Tissoires wrote:
> > Hi Tero,
> >
> > On Fri, Nov 26, 2021 at 2:02 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
> >> Hi,
> >>
> >> This series is an update based on comments from Benjamin. What is done
> >> is this series is to ditch the separate hid-driver for USI, and add the
> >> generic support to core layers. This part basically brings the support
> >> for providing USI events, without programmability (patches 1-6).
> > That part seems to be almost good for now. I have a few things to check:
> > - patch2: "HID: hid-input: Add suffix also for HID_DG_PEN" I need to
> > ensure there are no touchscreens affected by this (there used to be a
> > mess with some vendors where they would not declare things properly)
> > - patch5: "HID: core: map USI pen style reports directly" this one
> > feels plain wrong. I would need to have a look at the report
> > descriptor but this is too specific in a very generic code
>
> Relevant part of the report descriptor is here:
>
>      Field(8)
>        Physical(Digitizers.Stylus)
>        Logical(Digitizers.Preferred Line Style)
>        Application(Digitizers.Pen)
>        Usage(6)
>          Digitizers.Ink
>          Digitizers.Pencil
>          Digitizers.Highlighter
>          Digitizers.Chisel Marker
>          Digitizers.Brush
>          Digitizers.No Preference
>        Logical Minimum(1)
>        Logical Maximum(6)
>        Physical Minimum(0)
>        Physical Maximum(255)
>        Unit Exponent(-1)
>        Unit(SI Linear : Centimeter)
>        Report Size(8)
>        Report Count(1)
>        Report Offset(88)
>        Flags( Variable Absolute NoPreferredState )
>
> To me, it looks almost like it is a bug in the report descriptor itself;
> as you see there are 6 usage values but the report size / count is 1
> byte. The fact that there are 6 usage values in the field confuses
> hid-core. Basically the usage values are used as encoded content for the
> field.

It took me a few days but I finally understand that this report
descriptor is actually correct.

The descriptor gives an array of 1 element of size 8, which is enough
to give an index within the available values being [Digitizers.Ink,
Digitizers.Pencil, Digitizers.Highlighter, Digitizers.Chisel Marker,
Digitizers.Brush, Digitizers.No Preference]

Given that logical min is 1, this index is 1-based.

So the job of the kernel is to provide the event
Digitizers.Highlighter whenever the value here is 3. The mapping 3 <->
Digitizers.Highlighter is specific to this report descriptor and
should not be forwarded to user space.

>
> Alternatively I think this could be patched up in the BPF program, as I
> am modifying the content of the raw hid report already; I could just as
> well modify this one also. Or, maybe I could fix the report descriptor
> itself to act as a sane variable, as I am parsing the report descriptor
> already?

I couldn't understand the fix you did in the BPF program. Can you
explain it by also giving me an example of raw event from the device
and the outputs (fixed and not fixed) of the kernel?


Talking about that, I realized that you gave me the report descriptor
of the Acer panel in an other version of this RFC. Could you give me:
- the bus used (USB or I2C)?
- the vendor ID?
- the product ID?
- and the same for the other panel, with its report descriptor?

This way I can add them in the testsuite, and start playing with them.

>
> >
> >> Additionally, a HID-BPF based sample is provided which can be used to
> >> program / query pen parameters in comparison to the old driver level
> >> implementation (patches 7-8, patch #8 is an incremental change on top of
> >> patch #7 which just converts the fifo to socket so that the client can
> >> also get results back from the server.)
> > After a few more thoughts, I wondered what your input is on this. We
> > should be able to do the very same with plain hidraw... However, you
> > added a `hid/raw_event` processing that will still be kept in the
> > kernel, so maybe bpf would be useful for that at least.
>
> Yes, plain hidraw can be sort of used to program the values, however the
> interface is kind of annoying to use for the USI pens. You need to be
> touching the display with the pen before anything is accepted. Maybe
> writing some support code to the libevdev would help.
>
> The hidraw hook is needed for processing the cached values also, USI
> pens report their parameters with a delay of some few hundred ms
> depending on controller vendor. And in some cases they don't report
> anything back before forcibly querying the value from the controller,
> and also the write mechanism acts differently; some controllers report
> the programmed value back, others keep reporting the old value until the
> pen leaves the screen and touches it again.

Hmm, not sure I follow this entirely. I guess I would need to have one
of such devices in my hands :(

>
>
> >
> >> The whole series is based on top of Benjamin's hid-bpf support work, and
> >> I've pushed a branch at [1] with a series that works and brings in
> >> the dependency. There are also a few separate patches in this series to
> >> fix the problems I found from Benjamin's initial work for hid-bpf; I
> >> wasn't able to get things working without those. The branch is also
> >> based on top of 5.16-rc2 which required some extra changes to the
> >> patches from Benjamin.
> > Yeah, I also rebased on top of 5.16 shortly after sharing that branch
> > and got roughly the same last fix (HID: bpf: compile fix for
> > bpf_hid_foreach_rdesc_item). I am *very* interested in your "HID: bpf:
> > execute BPF programs in proper context" because that is something that
> > was bothering me a lot :)
>
> Right, I think I have plenty of lockdep / scheduler checks enabled in my
> kernel. They generate plenty of spam with i2c-hid without that patch.
> The same issue may not be visible with some other low level hid devices
> though, I don't have testing capability for anything but the i2c-hid
> right now. I2C is quite notorious for the locking aspects as it is slow
> and is used to control some pretty low level stuff like power management
> etc.

As a rule of thumb, hid_hw_raw_request() can not and should not be
called in IRQ.
I tested your patch with a USB device, and got plenty of complaints too.

I know bpf now has the ability to defer a function call with timers,
so maybe that's what we need here.

>
> >
> > "HID: bpf: add expected_attach_type to bpf prog during detach" is
> > something I'll need to bring in too
> >
> > but "HID: bpf: fix file mapping" is actually wrong. I initially wanted
> > to attach BPF programs to hidraw, but shortly realized that this is
> > not working because the `hid/rdesc_fixup` kills the hidraw node and so
> > releases the BPF programs. The way I am now attaching it is to use the
> > fd associated with the modalias in the sysfs file (for instance: `sudo
> > ./hid_surface_dial /sys/bus/hid/devices/0005:045E:091B.*/modalias`).
> > This way, the reference to the struct hid_device is kept even if we
> > disconnect the device and reprobe it.
> Ok I can check this out if it works me also. The samples lead me to
> /dev/hidraw usage.
> >
> > Thanks again for your work, and I'd be curious to have your thoughts
> > on hid-bpf and if you think it is better than hidraw/evdev write/new
> > ioctls for your use case.
>
> The new driver was 777 lines diff, the BPF one is 496 lines so it
> appears smaller. The driver did support two different vendors though
> (ELAN+Goodix, with their specific quirks in place), the BPF only a
> single one right now (ELAN).
>
> The vendor specific quirks are a question, do we want to support that
> somehow in a single BPF binary, or should we attach vendor specific BPF
> programs?

Good question.
The plan I had was to basically pre-compile BPF programs for the
various devices, but having them separated into generic + vendor
specifics seems interesting too.

I don't have a good answer right now.

>
> Chromium-os devices are one of the main customers for USI pens right
> now, and I am not sure how well they will take the BPF concept. :) I did
> ask their feedback though, and I'll come back on this once I have something.

Cool thanks.

>
> Personally, I don't have much preference either way at this moment, both
> seem like feasible options. I might lean a bit towards evdev/ioctl as it
> seems a cleaner implementation as of now. The write mechanism I
> implemented for the USI-BPF is a bit hacky, as it just directly writes
> to a shared memory buffer and the buffer gets parsed by the kernel part
> when it processes hidraw event. Anyways, do you have any feedback on
> that part? BPF is completely new to me again so would love to get some
> feedback.

Yeah, this feels wrong to me too.
I guess what we want is to run a BPF call initiated from the
userspace. I am not sure if this is doable. I'll need to dig further
too (I am relatively new to BPF too as a matter of facts).

Cheers,
Benjamin

>
> One option is of course to push the write portion of the code to
> userspace and just use hidraw, but we still need to filter out the bogus
> events somehow, and do that in vendor specific manner. I don't think
> this can be done on userspace, as plenty of information that would be
> needed to do this properly has been lost at the input-event level.
>
> -Tero
>
> >
> > Cheers,
> > Benjamin
> >
> >> -Tero
> >>
> >> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
> >>
> >>
>

Re: [RFCv2 0/8] USI stylus support series

[Tero Kristo]

[-- Attachment #1: Type: text/plain, Size: 12970 bytes --]

Hi Benjamin,

On 08/12/2021 16:56, Benjamin Tissoires wrote:
> Hi Tero,
>
> On Tue, Nov 30, 2021 at 5:13 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>> Hi Benjamin,
>>
>> On 30/11/2021 16:44, Benjamin Tissoires wrote:
>>> Hi Tero,
>>>
>>> On Fri, Nov 26, 2021 at 2:02 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>>>> Hi,
>>>>
>>>> This series is an update based on comments from Benjamin. What is done
>>>> is this series is to ditch the separate hid-driver for USI, and add the
>>>> generic support to core layers. This part basically brings the support
>>>> for providing USI events, without programmability (patches 1-6).
>>> That part seems to be almost good for now. I have a few things to check:
>>> - patch2: "HID: hid-input: Add suffix also for HID_DG_PEN" I need to
>>> ensure there are no touchscreens affected by this (there used to be a
>>> mess with some vendors where they would not declare things properly)
>>> - patch5: "HID: core: map USI pen style reports directly" this one
>>> feels plain wrong. I would need to have a look at the report
>>> descriptor but this is too specific in a very generic code
>> Relevant part of the report descriptor is here:
>>
>>       Field(8)
>>         Physical(Digitizers.Stylus)
>>         Logical(Digitizers.Preferred Line Style)
>>         Application(Digitizers.Pen)
>>         Usage(6)
>>           Digitizers.Ink
>>           Digitizers.Pencil
>>           Digitizers.Highlighter
>>           Digitizers.Chisel Marker
>>           Digitizers.Brush
>>           Digitizers.No Preference
>>         Logical Minimum(1)
>>         Logical Maximum(6)
>>         Physical Minimum(0)
>>         Physical Maximum(255)
>>         Unit Exponent(-1)
>>         Unit(SI Linear : Centimeter)
>>         Report Size(8)
>>         Report Count(1)
>>         Report Offset(88)
>>         Flags( Variable Absolute NoPreferredState )
>>
>> To me, it looks almost like it is a bug in the report descriptor itself;
>> as you see there are 6 usage values but the report size / count is 1
>> byte. The fact that there are 6 usage values in the field confuses
>> hid-core. Basically the usage values are used as encoded content for the
>> field.
> It took me a few days but I finally understand that this report
> descriptor is actually correct.
>
> The descriptor gives an array of 1 element of size 8, which is enough
> to give an index within the available values being [Digitizers.Ink,
> Digitizers.Pencil, Digitizers.Highlighter, Digitizers.Chisel Marker,
> Digitizers.Brush, Digitizers.No Preference]
>
> Given that logical min is 1, this index is 1-based.
>
> So the job of the kernel is to provide the event
> Digitizers.Highlighter whenever the value here is 3. The mapping 3 <->
> Digitizers.Highlighter is specific to this report descriptor and
> should not be forwarded to user space.

Yes, all this is true. I also see you re-wrote this part a bit in the 
series to add individual events for all the different line styles. I'll 
give this a shot and see how it works out. A problem I see is that we 
need to be able to program the pen line style also somehow, do we just 
set a single pen style to "enabled" and all the rest get set to 
"disabled" under the hood?


>
>> Alternatively I think this could be patched up in the BPF program, as I
>> am modifying the content of the raw hid report already; I could just as
>> well modify this one also. Or, maybe I could fix the report descriptor
>> itself to act as a sane variable, as I am parsing the report descriptor
>> already?
> I couldn't understand the fix you did in the BPF program. Can you
> explain it by also giving me an example of raw event from the device
> and the outputs (fixed and not fixed) of the kernel?

The fix in the BPF code is this (under process_tag()):

                         /*
                          * Force flags for line style. This makes it act
                          * as a simple variable from HID core point of 
view.
                          */
                         bpf_hid_set_data(ctx, (*idx + 1) << 3, 8, 0x2);

After that, the pen line style gets forwarded as a simple integer value 
to input-core / userspace also. raw events did not need modification 
after all, I just modified the report descriptor.

>
>
> Talking about that, I realized that you gave me the report descriptor
> of the Acer panel in an other version of this RFC. Could you give me:
> - the bus used (USB or I2C)?
I have been using I2C in all my testing, the controllers I have access 
to are behind I2C only.
> - the vendor ID?
> - the product ID?
> - and the same for the other panel, with its report descriptor?
>
> This way I can add them in the testsuite, and start playing with them.
Attached a tarball with both descriptors and their corresponding IDs 
(copied the R+N+I data from hid-recorder.)
>
>>>> Additionally, a HID-BPF based sample is provided which can be used to
>>>> program / query pen parameters in comparison to the old driver level
>>>> implementation (patches 7-8, patch #8 is an incremental change on top of
>>>> patch #7 which just converts the fifo to socket so that the client can
>>>> also get results back from the server.)
>>> After a few more thoughts, I wondered what your input is on this. We
>>> should be able to do the very same with plain hidraw... However, you
>>> added a `hid/raw_event` processing that will still be kept in the
>>> kernel, so maybe bpf would be useful for that at least.
>> Yes, plain hidraw can be sort of used to program the values, however the
>> interface is kind of annoying to use for the USI pens. You need to be
>> touching the display with the pen before anything is accepted. Maybe
>> writing some support code to the libevdev would help.
>>
>> The hidraw hook is needed for processing the cached values also, USI
>> pens report their parameters with a delay of some few hundred ms
>> depending on controller vendor. And in some cases they don't report
>> anything back before forcibly querying the value from the controller,
>> and also the write mechanism acts differently; some controllers report
>> the programmed value back, others keep reporting the old value until the
>> pen leaves the screen and touches it again.
> Hmm, not sure I follow this entirely. I guess I would need to have one
> of such devices in my hands :(

Yes, it is kind of confusing, I was also trying to figure out the 
details with a remote proxy (someone telling me how things behave) until 
I decided to order a second chromebook that had the same controller. I 
can try to provide logs of the different cases if you want though. The 
quirks I know of at the moment:

1) controller does not immediately report "correct" values when pen 
touches screen (ELAN)

2) controller does never report "correct" values when pen touches screen 
(must do a force GET_REPORT) (GOODIX)

3) controller does not report "correct" values after SET_REPORT 
(reporting old value) (ELAN)

4) controller responds with bogus data in GET_REPORT (does not know the 
correct value yet) (ELAN + GOODIX)

I believe other vendors have different behavior with their controllers 
also, as the specs are not 100% clear on multiple things.

>
>>
>>>> The whole series is based on top of Benjamin's hid-bpf support work, and
>>>> I've pushed a branch at [1] with a series that works and brings in
>>>> the dependency. There are also a few separate patches in this series to
>>>> fix the problems I found from Benjamin's initial work for hid-bpf; I
>>>> wasn't able to get things working without those. The branch is also
>>>> based on top of 5.16-rc2 which required some extra changes to the
>>>> patches from Benjamin.
>>> Yeah, I also rebased on top of 5.16 shortly after sharing that branch
>>> and got roughly the same last fix (HID: bpf: compile fix for
>>> bpf_hid_foreach_rdesc_item). I am *very* interested in your "HID: bpf:
>>> execute BPF programs in proper context" because that is something that
>>> was bothering me a lot :)
>> Right, I think I have plenty of lockdep / scheduler checks enabled in my
>> kernel. They generate plenty of spam with i2c-hid without that patch.
>> The same issue may not be visible with some other low level hid devices
>> though, I don't have testing capability for anything but the i2c-hid
>> right now. I2C is quite notorious for the locking aspects as it is slow
>> and is used to control some pretty low level stuff like power management
>> etc.
> As a rule of thumb, hid_hw_raw_request() can not and should not be
> called in IRQ.
> I tested your patch with a USB device, and got plenty of complaints too.
>
> I know bpf now has the ability to defer a function call with timers,
> so maybe that's what we need here.
That sounds like something that would work yes, I did use workqueue 
before when this was a separate driver instead of a BPF program.
>
>>> "HID: bpf: add expected_attach_type to bpf prog during detach" is
>>> something I'll need to bring in too
>>>
>>> but "HID: bpf: fix file mapping" is actually wrong. I initially wanted
>>> to attach BPF programs to hidraw, but shortly realized that this is
>>> not working because the `hid/rdesc_fixup` kills the hidraw node and so
>>> releases the BPF programs. The way I am now attaching it is to use the
>>> fd associated with the modalias in the sysfs file (for instance: `sudo
>>> ./hid_surface_dial /sys/bus/hid/devices/0005:045E:091B.*/modalias`).
>>> This way, the reference to the struct hid_device is kept even if we
>>> disconnect the device and reprobe it.
>> Ok I can check this out if it works me also. The samples lead me to
>> /dev/hidraw usage.
>>> Thanks again for your work, and I'd be curious to have your thoughts
>>> on hid-bpf and if you think it is better than hidraw/evdev write/new
>>> ioctls for your use case.
>> The new driver was 777 lines diff, the BPF one is 496 lines so it
>> appears smaller. The driver did support two different vendors though
>> (ELAN+Goodix, with their specific quirks in place), the BPF only a
>> single one right now (ELAN).
>>
>> The vendor specific quirks are a question, do we want to support that
>> somehow in a single BPF binary, or should we attach vendor specific BPF
>> programs?
> Good question.
> The plan I had was to basically pre-compile BPF programs for the
> various devices, but having them separated into generic + vendor
> specifics seems interesting too.
>
> I don't have a good answer right now.
At least for USI purposes, ELAN+GOODIX controllers have pretty different 
quirks for them and it seems like having separate BPF programs might be 
better; trying to get the same BPF program to run for both sounds 
painful (it was rather painful to get this to work for single vendor.)
>
>> Chromium-os devices are one of the main customers for USI pens right
>> now, and I am not sure how well they will take the BPF concept. :) I did
>> ask their feedback though, and I'll come back on this once I have something.
> Cool thanks.
>
>> Personally, I don't have much preference either way at this moment, both
>> seem like feasible options. I might lean a bit towards evdev/ioctl as it
>> seems a cleaner implementation as of now. The write mechanism I
>> implemented for the USI-BPF is a bit hacky, as it just directly writes
>> to a shared memory buffer and the buffer gets parsed by the kernel part
>> when it processes hidraw event. Anyways, do you have any feedback on
>> that part? BPF is completely new to me again so would love to get some
>> feedback.
> Yeah, this feels wrong to me too.
> I guess what we want is to run a BPF call initiated from the
> userspace. I am not sure if this is doable. I'll need to dig further
> too (I am relatively new to BPF too as a matter of facts).

I could not find a way to initiate BPF call from userspace, thats the 
reason I implemented it this way. That said, I don't see any case where 
this would fail though; we only ever write the values from single source 
(userspace) and read them from kernel. If we miss a write, we just get 
the old value and report the change later on.

To initiate a BPF call from userspace we would need some sort of hid-bpf 
callback to a BPF program, which gets triggered by an ioctl or evdev 
write or something coming from userspace. Which brings us back to the 
chicken-egg problem we have with USI right now. :)

-Tero


> Cheers,
> Benjamin
>
>> One option is of course to push the write portion of the code to
>> userspace and just use hidraw, but we still need to filter out the bogus
>> events somehow, and do that in vendor specific manner. I don't think
>> this can be done on userspace, as plenty of information that would be
>> needed to do this properly has been lost at the input-event level.
>>
>> -Tero
>>
>>> Cheers,
>>> Benjamin
>>>
>>>> -Tero
>>>>
>>>> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
>>>>
>>>>

[-- Attachment #2: usi-rdescs.tar.gz --]
[-- Type: application/gzip, Size: 1301 bytes --]

Re: [RFCv2 0/8] USI stylus support series

[Benjamin Tissoires]

Hi Tero,

On Thu, Dec 9, 2021 at 9:56 AM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi Benjamin,
>
> On 08/12/2021 16:56, Benjamin Tissoires wrote:
> > Hi Tero,
> >
> > On Tue, Nov 30, 2021 at 5:13 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
> >> Hi Benjamin,
> >>
> >> On 30/11/2021 16:44, Benjamin Tissoires wrote:
> >>> Hi Tero,
> >>>
> >>> On Fri, Nov 26, 2021 at 2:02 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
> >>>> Hi,
> >>>>
> >>>> This series is an update based on comments from Benjamin. What is done
> >>>> is this series is to ditch the separate hid-driver for USI, and add the
> >>>> generic support to core layers. This part basically brings the support
> >>>> for providing USI events, without programmability (patches 1-6).
> >>> That part seems to be almost good for now. I have a few things to check:
> >>> - patch2: "HID: hid-input: Add suffix also for HID_DG_PEN" I need to
> >>> ensure there are no touchscreens affected by this (there used to be a
> >>> mess with some vendors where they would not declare things properly)
> >>> - patch5: "HID: core: map USI pen style reports directly" this one
> >>> feels plain wrong. I would need to have a look at the report
> >>> descriptor but this is too specific in a very generic code
> >> Relevant part of the report descriptor is here:
> >>
> >>       Field(8)
> >>         Physical(Digitizers.Stylus)
> >>         Logical(Digitizers.Preferred Line Style)
> >>         Application(Digitizers.Pen)
> >>         Usage(6)
> >>           Digitizers.Ink
> >>           Digitizers.Pencil
> >>           Digitizers.Highlighter
> >>           Digitizers.Chisel Marker
> >>           Digitizers.Brush
> >>           Digitizers.No Preference
> >>         Logical Minimum(1)
> >>         Logical Maximum(6)
> >>         Physical Minimum(0)
> >>         Physical Maximum(255)
> >>         Unit Exponent(-1)
> >>         Unit(SI Linear : Centimeter)
> >>         Report Size(8)
> >>         Report Count(1)
> >>         Report Offset(88)
> >>         Flags( Variable Absolute NoPreferredState )
> >>
> >> To me, it looks almost like it is a bug in the report descriptor itself;
> >> as you see there are 6 usage values but the report size / count is 1
> >> byte. The fact that there are 6 usage values in the field confuses
> >> hid-core. Basically the usage values are used as encoded content for the
> >> field.
> > It took me a few days but I finally understand that this report
> > descriptor is actually correct.
> >
> > The descriptor gives an array of 1 element of size 8, which is enough
> > to give an index within the available values being [Digitizers.Ink,
> > Digitizers.Pencil, Digitizers.Highlighter, Digitizers.Chisel Marker,
> > Digitizers.Brush, Digitizers.No Preference]
> >
> > Given that logical min is 1, this index is 1-based.
> >
> > So the job of the kernel is to provide the event
> > Digitizers.Highlighter whenever the value here is 3. The mapping 3 <->
> > Digitizers.Highlighter is specific to this report descriptor and
> > should not be forwarded to user space.
>
> Yes, all this is true. I also see you re-wrote this part a bit in the
> series to add individual events for all the different line styles. I'll
> give this a shot and see how it works out. A problem I see is that we
> need to be able to program the pen line style also somehow, do we just
> set a single pen style to "enabled" and all the rest get set to
> "disabled" under the hood?
>

I think we need to have a parameter `PreferredLineStyle` which can
only take the values from the array above.

If your API provides that, the rest is implementation detail.
Assigning a value to it will by definition invalidate the old value.

Of course this means that the evdev approach is not suited for that,
which makes me think that is probably not the best option.

>
> >
> >> Alternatively I think this could be patched up in the BPF program, as I
> >> am modifying the content of the raw hid report already; I could just as
> >> well modify this one also. Or, maybe I could fix the report descriptor
> >> itself to act as a sane variable, as I am parsing the report descriptor
> >> already?
> > I couldn't understand the fix you did in the BPF program. Can you
> > explain it by also giving me an example of raw event from the device
> > and the outputs (fixed and not fixed) of the kernel?
>
> The fix in the BPF code is this (under process_tag()):
>
>                          /*
>                           * Force flags for line style. This makes it act
>                           * as a simple variable from HID core point of
> view.
>                           */
>                          bpf_hid_set_data(ctx, (*idx + 1) << 3, 8, 0x2);
>
> After that, the pen line style gets forwarded as a simple integer value
> to input-core / userspace also. raw events did not need modification
> after all, I just modified the report descriptor.

Right. So you are stripping away the actual meaning, which is report
descriptor dependent.
This is not good because a HW vendor might decide to not order the 6
possible values by their HID usage but put the `No Prefererence` first
for instance. There is also a strong possibility a HW vendor decides
to not rely on the PreferredLineStyleIsLocked and gives a choice of
only one possible value (though that would be mean as this is a per
stylus propriety).

>
> >
> >
> > Talking about that, I realized that you gave me the report descriptor
> > of the Acer panel in an other version of this RFC. Could you give me:
> > - the bus used (USB or I2C)?
> I have been using I2C in all my testing, the controllers I have access
> to are behind I2C only.
> > - the vendor ID?
> > - the product ID?
> > - and the same for the other panel, with its report descriptor?
> >
> > This way I can add them in the testsuite, and start playing with them.
> Attached a tarball with both descriptors and their corresponding IDs
> (copied the R+N+I data from hid-recorder.)

Thanks!

> >
> >>>> Additionally, a HID-BPF based sample is provided which can be used to
> >>>> program / query pen parameters in comparison to the old driver level
> >>>> implementation (patches 7-8, patch #8 is an incremental change on top of
> >>>> patch #7 which just converts the fifo to socket so that the client can
> >>>> also get results back from the server.)
> >>> After a few more thoughts, I wondered what your input is on this. We
> >>> should be able to do the very same with plain hidraw... However, you
> >>> added a `hid/raw_event` processing that will still be kept in the
> >>> kernel, so maybe bpf would be useful for that at least.
> >> Yes, plain hidraw can be sort of used to program the values, however the
> >> interface is kind of annoying to use for the USI pens. You need to be
> >> touching the display with the pen before anything is accepted. Maybe
> >> writing some support code to the libevdev would help.
> >>
> >> The hidraw hook is needed for processing the cached values also, USI
> >> pens report their parameters with a delay of some few hundred ms
> >> depending on controller vendor. And in some cases they don't report
> >> anything back before forcibly querying the value from the controller,
> >> and also the write mechanism acts differently; some controllers report
> >> the programmed value back, others keep reporting the old value until the
> >> pen leaves the screen and touches it again.
> > Hmm, not sure I follow this entirely. I guess I would need to have one
> > of such devices in my hands :(
>
> Yes, it is kind of confusing, I was also trying to figure out the
> details with a remote proxy (someone telling me how things behave) until
> I decided to order a second chromebook that had the same controller. I
> can try to provide logs of the different cases if you want though. The
> quirks I know of at the moment:

I'll need more clarifications (and getting logs might help me
understand better, yes, please):

>
> 1) controller does not immediately report "correct" values when pen
> touches screen (ELAN)

I assume this is in the input reports, not in the feature reports.
What happens in the hovering case (not touching)?
Do we get fake values easily identifiable or are they just as normal
as the correct ones?

Anyway, considering the use case, this might not be an issue (I was
re-reading the HUT and this is only an indication for applications).

>
> 2) controller does never report "correct" values when pen touches screen
> (must do a force GET_REPORT) (GOODIX)

Again, Input reports?
What's in the hovering state reported?
Is the GET_REPORT needed against the feature report or the input report?

>
> 3) controller does not report "correct" values after SET_REPORT
> (reporting old value) (ELAN)

Am I correct?:

- Pen is hovering/touching
- controller is reporting correct current values in the input reports
(following the 2 cases above)
- host sends a SET_REPORT on the feature
- controller is still sending the old values in the input reports

What happens if you issue a GET_REPORT on the Input?
On the Feature?

>
> 4) controller responds with bogus data in GET_REPORT (does not know the
> correct value yet) (ELAN + GOODIX)

I assume that's when the stylus is not in proximity, and when you
issue a GET_REPORT of the feature report, not the input, correct?

If so, this is something I would have expected, given that those
properties are per stylus, not per controller.

>
> I believe other vendors have different behavior with their controllers
> also, as the specs are not 100% clear on multiple things.

Well, depending on your answers above, we might have a common set of
cases we can use, or paper over it through bpf if there is a strong
need.

Also, a few more questions:
- have you tried those cases above with the same stylus, or is it HP
controller - HP stylus /  Acer-Acer?
- do these pens have physical notification of the style/width, or do
they just store the data in their memory?
- what are the chromebook models (if I need to eventually expense one)?
- to me, the Goodix report descriptor is bogus in the feature reports.
The Usage Page is stuck at "vendor defined" when it should have been
reset to "Digitizer" before the report ID 9. Is it just me and my
tools or am I missing something?

>
> >
> >>
> >>>> The whole series is based on top of Benjamin's hid-bpf support work, and
> >>>> I've pushed a branch at [1] with a series that works and brings in
> >>>> the dependency. There are also a few separate patches in this series to
> >>>> fix the problems I found from Benjamin's initial work for hid-bpf; I
> >>>> wasn't able to get things working without those. The branch is also
> >>>> based on top of 5.16-rc2 which required some extra changes to the
> >>>> patches from Benjamin.
> >>> Yeah, I also rebased on top of 5.16 shortly after sharing that branch
> >>> and got roughly the same last fix (HID: bpf: compile fix for
> >>> bpf_hid_foreach_rdesc_item). I am *very* interested in your "HID: bpf:
> >>> execute BPF programs in proper context" because that is something that
> >>> was bothering me a lot :)
> >> Right, I think I have plenty of lockdep / scheduler checks enabled in my
> >> kernel. They generate plenty of spam with i2c-hid without that patch.
> >> The same issue may not be visible with some other low level hid devices
> >> though, I don't have testing capability for anything but the i2c-hid
> >> right now. I2C is quite notorious for the locking aspects as it is slow
> >> and is used to control some pretty low level stuff like power management
> >> etc.
> > As a rule of thumb, hid_hw_raw_request() can not and should not be
> > called in IRQ.
> > I tested your patch with a USB device, and got plenty of complaints too.
> >
> > I know bpf now has the ability to defer a function call with timers,
> > so maybe that's what we need here.
> That sounds like something that would work yes, I did use workqueue
> before when this was a separate driver instead of a BPF program.
> >
> >>> "HID: bpf: add expected_attach_type to bpf prog during detach" is
> >>> something I'll need to bring in too
> >>>
> >>> but "HID: bpf: fix file mapping" is actually wrong. I initially wanted
> >>> to attach BPF programs to hidraw, but shortly realized that this is
> >>> not working because the `hid/rdesc_fixup` kills the hidraw node and so
> >>> releases the BPF programs. The way I am now attaching it is to use the
> >>> fd associated with the modalias in the sysfs file (for instance: `sudo
> >>> ./hid_surface_dial /sys/bus/hid/devices/0005:045E:091B.*/modalias`).
> >>> This way, the reference to the struct hid_device is kept even if we
> >>> disconnect the device and reprobe it.
> >> Ok I can check this out if it works me also. The samples lead me to
> >> /dev/hidraw usage.
> >>> Thanks again for your work, and I'd be curious to have your thoughts
> >>> on hid-bpf and if you think it is better than hidraw/evdev write/new
> >>> ioctls for your use case.
> >> The new driver was 777 lines diff, the BPF one is 496 lines so it
> >> appears smaller. The driver did support two different vendors though
> >> (ELAN+Goodix, with their specific quirks in place), the BPF only a
> >> single one right now (ELAN).
> >>
> >> The vendor specific quirks are a question, do we want to support that
> >> somehow in a single BPF binary, or should we attach vendor specific BPF
> >> programs?
> > Good question.
> > The plan I had was to basically pre-compile BPF programs for the
> > various devices, but having them separated into generic + vendor
> > specifics seems interesting too.
> >
> > I don't have a good answer right now.
> At least for USI purposes, ELAN+GOODIX controllers have pretty different
> quirks for them and it seems like having separate BPF programs might be
> better; trying to get the same BPF program to run for both sounds
> painful (it was rather painful to get this to work for single vendor.)

The more I look at the 2 report descriptors, the more I think that we
should be able to have a common code in hid-input for dealing with
input reports, and then have specifics as bpf programs.
As mentioned earlier, I think Goodix needs a report fixup for the
features, and we might want to change the reported values for Elan
immediately after we issue the config change.

It seems very much that we are in the same situation as Windows 7
multitouch screens. The spec was not restrictive enough that the HW
makers were not very careful, and we added multiple quirks for them.
I would prefer to have a common minimal hid-input handling and defer
the quirks in a BPF program :)

> >
> >> Chromium-os devices are one of the main customers for USI pens right
> >> now, and I am not sure how well they will take the BPF concept. :) I did
> >> ask their feedback though, and I'll come back on this once I have something.
> > Cool thanks.
> >
> >> Personally, I don't have much preference either way at this moment, both
> >> seem like feasible options. I might lean a bit towards evdev/ioctl as it
> >> seems a cleaner implementation as of now. The write mechanism I
> >> implemented for the USI-BPF is a bit hacky, as it just directly writes
> >> to a shared memory buffer and the buffer gets parsed by the kernel part
> >> when it processes hidraw event. Anyways, do you have any feedback on
> >> that part? BPF is completely new to me again so would love to get some
> >> feedback.
> > Yeah, this feels wrong to me too.
> > I guess what we want is to run a BPF call initiated from the
> > userspace. I am not sure if this is doable. I'll need to dig further
> > too (I am relatively new to BPF too as a matter of facts).
>
> I could not find a way to initiate BPF call from userspace, thats the
> reason I implemented it this way. That said, I don't see any case where
> this would fail though; we only ever write the values from single source
> (userspace) and read them from kernel. If we miss a write, we just get
> the old value and report the change later on.

Yeah, I understand it works, it's just that you can not initiate a
bpf_hid_raw_request() call from a raw_event callback. You are in an
IRQ, and we need to run things as fast as possible.
So either we defer, or we find another way of contacting the stylus
outside of the IRQ.

>
> To initiate a BPF call from userspace we would need some sort of hid-bpf
> callback to a BPF program, which gets triggered by an ioctl or evdev
> write or something coming from userspace. Which brings us back to the
> chicken-egg problem we have with USI right now. :)

I am thinking of adding a new syscall hid_bpf_run() that the userspace
program can trigger. Otherwise, it seems from a very rough overview we
could hijack the bpf_test_run() syscall, but that would not be very
nice.
Initiating an event from evdev is not very compatible with the BPF
approach because you'll need to also open the evdev node, which you
don't when you run a BPF program.

Cheers,
Benjamin

>
> -Tero
>
>
> > Cheers,
> > Benjamin
> >
> >> One option is of course to push the write portion of the code to
> >> userspace and just use hidraw, but we still need to filter out the bogus
> >> events somehow, and do that in vendor specific manner. I don't think
> >> this can be done on userspace, as plenty of information that would be
> >> needed to do this properly has been lost at the input-event level.
> >>
> >> -Tero
> >>
> >>> Cheers,
> >>> Benjamin
> >>>
> >>>> -Tero
> >>>>
> >>>> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
> >>>>
> >>>>

Re: [RFCv2 0/8] USI stylus support series

[Tero Kristo]

[-- Attachment #1: Type: text/plain, Size: 20475 bytes --]

Hi Benjamin,

On 09/12/2021 15:53, Benjamin Tissoires wrote:
> Hi Tero,
>
> On Thu, Dec 9, 2021 at 9:56 AM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>> Hi Benjamin,
>>
>> On 08/12/2021 16:56, Benjamin Tissoires wrote:
>>> Hi Tero,
>>>
>>> On Tue, Nov 30, 2021 at 5:13 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>>>> Hi Benjamin,
>>>>
>>>> On 30/11/2021 16:44, Benjamin Tissoires wrote:
>>>>> Hi Tero,
>>>>>
>>>>> On Fri, Nov 26, 2021 at 2:02 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This series is an update based on comments from Benjamin. What is done
>>>>>> is this series is to ditch the separate hid-driver for USI, and add the
>>>>>> generic support to core layers. This part basically brings the support
>>>>>> for providing USI events, without programmability (patches 1-6).
>>>>> That part seems to be almost good for now. I have a few things to check:
>>>>> - patch2: "HID: hid-input: Add suffix also for HID_DG_PEN" I need to
>>>>> ensure there are no touchscreens affected by this (there used to be a
>>>>> mess with some vendors where they would not declare things properly)
>>>>> - patch5: "HID: core: map USI pen style reports directly" this one
>>>>> feels plain wrong. I would need to have a look at the report
>>>>> descriptor but this is too specific in a very generic code
>>>> Relevant part of the report descriptor is here:
>>>>
>>>>        Field(8)
>>>>          Physical(Digitizers.Stylus)
>>>>          Logical(Digitizers.Preferred Line Style)
>>>>          Application(Digitizers.Pen)
>>>>          Usage(6)
>>>>            Digitizers.Ink
>>>>            Digitizers.Pencil
>>>>            Digitizers.Highlighter
>>>>            Digitizers.Chisel Marker
>>>>            Digitizers.Brush
>>>>            Digitizers.No Preference
>>>>          Logical Minimum(1)
>>>>          Logical Maximum(6)
>>>>          Physical Minimum(0)
>>>>          Physical Maximum(255)
>>>>          Unit Exponent(-1)
>>>>          Unit(SI Linear : Centimeter)
>>>>          Report Size(8)
>>>>          Report Count(1)
>>>>          Report Offset(88)
>>>>          Flags( Variable Absolute NoPreferredState )
>>>>
>>>> To me, it looks almost like it is a bug in the report descriptor itself;
>>>> as you see there are 6 usage values but the report size / count is 1
>>>> byte. The fact that there are 6 usage values in the field confuses
>>>> hid-core. Basically the usage values are used as encoded content for the
>>>> field.
>>> It took me a few days but I finally understand that this report
>>> descriptor is actually correct.
>>>
>>> The descriptor gives an array of 1 element of size 8, which is enough
>>> to give an index within the available values being [Digitizers.Ink,
>>> Digitizers.Pencil, Digitizers.Highlighter, Digitizers.Chisel Marker,
>>> Digitizers.Brush, Digitizers.No Preference]
>>>
>>> Given that logical min is 1, this index is 1-based.
>>>
>>> So the job of the kernel is to provide the event
>>> Digitizers.Highlighter whenever the value here is 3. The mapping 3 <->
>>> Digitizers.Highlighter is specific to this report descriptor and
>>> should not be forwarded to user space.
>> Yes, all this is true. I also see you re-wrote this part a bit in the
>> series to add individual events for all the different line styles. I'll
>> give this a shot and see how it works out. A problem I see is that we
>> need to be able to program the pen line style also somehow, do we just
>> set a single pen style to "enabled" and all the rest get set to
>> "disabled" under the hood?
>>
> I think we need to have a parameter `PreferredLineStyle` which can
> only take the values from the array above.
>
> If your API provides that, the rest is implementation detail.
> Assigning a value to it will by definition invalidate the old value.
>
> Of course this means that the evdev approach is not suited for that,
> which makes me think that is probably not the best option.

Ok I will experiment with this.

>>>> Alternatively I think this could be patched up in the BPF program, as I
>>>> am modifying the content of the raw hid report already; I could just as
>>>> well modify this one also. Or, maybe I could fix the report descriptor
>>>> itself to act as a sane variable, as I am parsing the report descriptor
>>>> already?
>>> I couldn't understand the fix you did in the BPF program. Can you
>>> explain it by also giving me an example of raw event from the device
>>> and the outputs (fixed and not fixed) of the kernel?
>> The fix in the BPF code is this (under process_tag()):
>>
>>                           /*
>>                            * Force flags for line style. This makes it act
>>                            * as a simple variable from HID core point of
>> view.
>>                            */
>>                           bpf_hid_set_data(ctx, (*idx + 1) << 3, 8, 0x2);
>>
>> After that, the pen line style gets forwarded as a simple integer value
>> to input-core / userspace also. raw events did not need modification
>> after all, I just modified the report descriptor.
> Right. So you are stripping away the actual meaning, which is report
> descriptor dependent.
> This is not good because a HW vendor might decide to not order the 6
> possible values by their HID usage but put the `No Prefererence` first
> for instance. There is also a strong possibility a HW vendor decides
> to not rely on the PreferredLineStyleIsLocked and gives a choice of
> only one possible value (though that would be mean as this is a per
> stylus propriety).

Ok thanks for explanation, I will experiment with this also and see how 
it works.


>
>>>
>>> Talking about that, I realized that you gave me the report descriptor
>>> of the Acer panel in an other version of this RFC. Could you give me:
>>> - the bus used (USB or I2C)?
>> I have been using I2C in all my testing, the controllers I have access
>> to are behind I2C only.
>>> - the vendor ID?
>>> - the product ID?
>>> - and the same for the other panel, with its report descriptor?
>>>
>>> This way I can add them in the testsuite, and start playing with them.
>> Attached a tarball with both descriptors and their corresponding IDs
>> (copied the R+N+I data from hid-recorder.)
> Thanks!
>
>>>>>> Additionally, a HID-BPF based sample is provided which can be used to
>>>>>> program / query pen parameters in comparison to the old driver level
>>>>>> implementation (patches 7-8, patch #8 is an incremental change on top of
>>>>>> patch #7 which just converts the fifo to socket so that the client can
>>>>>> also get results back from the server.)
>>>>> After a few more thoughts, I wondered what your input is on this. We
>>>>> should be able to do the very same with plain hidraw... However, you
>>>>> added a `hid/raw_event` processing that will still be kept in the
>>>>> kernel, so maybe bpf would be useful for that at least.
>>>> Yes, plain hidraw can be sort of used to program the values, however the
>>>> interface is kind of annoying to use for the USI pens. You need to be
>>>> touching the display with the pen before anything is accepted. Maybe
>>>> writing some support code to the libevdev would help.
>>>>
>>>> The hidraw hook is needed for processing the cached values also, USI
>>>> pens report their parameters with a delay of some few hundred ms
>>>> depending on controller vendor. And in some cases they don't report
>>>> anything back before forcibly querying the value from the controller,
>>>> and also the write mechanism acts differently; some controllers report
>>>> the programmed value back, others keep reporting the old value until the
>>>> pen leaves the screen and touches it again.
>>> Hmm, not sure I follow this entirely. I guess I would need to have one
>>> of such devices in my hands :(
>> Yes, it is kind of confusing, I was also trying to figure out the
>> details with a remote proxy (someone telling me how things behave) until
>> I decided to order a second chromebook that had the same controller. I
>> can try to provide logs of the different cases if you want though. The
>> quirks I know of at the moment:
> I'll need more clarifications (and getting logs might help me
> understand better, yes, please):
>
>> 1) controller does not immediately report "correct" values when pen
>> touches screen (ELAN)
> I assume this is in the input reports, not in the feature reports.
Yes, this is with input reports. Provided a sample in the attached 
tarball (usi-pen-initial-latency-*; there is dmesg + hid-recorder files.)
> What happens in the hovering case (not touching)?
It looks like the controller only queries the pen for actual values when 
it touches the screen, while hovering, it reports the old/incorrect 
values forever.
> Do we get fake values easily identifiable or are they just as normal
> as the correct ones?
They are as normal as correct values, because the controller picks 
whatever it has (apparently in its internal memory), these can be zeroes 
(from boot), or values from previous pen that touched screen.
>
> Anyway, considering the use case, this might not be an issue (I was
> re-reading the HUT and this is only an indication for applications).
>
>> 2) controller does never report "correct" values when pen touches screen
>> (must do a force GET_REPORT) (GOODIX)
> Again, Input reports?
Check attached tarball for usi-pen-goodix*. Added both hid-recorder and 
dmesg outputs, as they provide slightly different data; in dmesg you can 
see where I actually send the GET_REPORT for pen color and it updates in 
the input report also slightly after this.
> What's in the hovering state reported?
Hovering state doesn't alter the report, but with Goodix controller, you 
can actually GET_REPORT and get sane data out of the pen while it is 
only hovering.
> Is the GET_REPORT needed against the feature report or the input report?
Feature report. Once I GET_REPORT for the Preferred Line Color feature 
report, the proper value gets magically updated for the input reports also.
>
>> 3) controller does not report "correct" values after SET_REPORT
>> (reporting old value) (ELAN)
> Am I correct?:
>
> - Pen is hovering/touching
> - controller is reporting correct current values in the input reports
> (following the 2 cases above)
> - host sends a SET_REPORT on the feature
> - controller is still sending the old values in the input reports
I did retry this with the latest code I have and I wasn't able to 
reproduce it anymore. Might have been a glitch earlier with the driver, 
but I am certain I did see this kind of behavior because I had a 
workaround in the driver for it. There is a latency in changing the 
value and before it reaches the input report though.
> What happens if you issue a GET_REPORT on the Input?
> On the Feature?
I'll check this if I can reproduce the issue.
>
>> 4) controller responds with bogus data in GET_REPORT (does not know the
>> correct value yet) (ELAN + GOODIX)
> I assume that's when the stylus is not in proximity, and when you
> issue a GET_REPORT of the feature report, not the input, correct?
Yes, with feature report. This is during the initial latency period 
where the pen has not been probed by the controller yet.
>
> If so, this is something I would have expected, given that those
> properties are per stylus, not per controller.
>
>> I believe other vendors have different behavior with their controllers
>> also, as the specs are not 100% clear on multiple things.
> Well, depending on your answers above, we might have a common set of
> cases we can use, or paper over it through bpf if there is a strong
> need.
>
> Also, a few more questions:
> - have you tried those cases above with the same stylus, or is it HP
> controller - HP stylus /  Acer-Acer?
I have two interchangeable styluses which I can use with both acer/hp 
devices. It is also possible to swap between pen1 and pen2 and get the 
different parameters out of them.
> - do these pens have physical notification of the style/width, or do
> they just store the data in their memory?
No physical notification, just in memory.
> - what are the chromebook models (if I need to eventually expense one)?

Acer one is chromebook spin 713 (CP713-2W series, model #N19Q5)

HP one is chromebook x360 (model #12b-ca0810no)

You need to be careful with what to buy though if you are looking for a 
specific USI controller, these are generally not documented anywhere, 
and there appears to be different versions of the same chromebook model 
even (e.g. spin 713 has multiple different variants.)

> - to me, the Goodix report descriptor is bogus in the feature reports.
> The Usage Page is stuck at "vendor defined" when it should have been
> reset to "Digitizer" before the report ID 9. Is it just me and my
> tools or am I missing something?
Yeah, it has plenty of vendor defined data in it, which should be 
digitizer. I had hardcoded part of these in my earlier driver.
>
>>>>>> The whole series is based on top of Benjamin's hid-bpf support work, and
>>>>>> I've pushed a branch at [1] with a series that works and brings in
>>>>>> the dependency. There are also a few separate patches in this series to
>>>>>> fix the problems I found from Benjamin's initial work for hid-bpf; I
>>>>>> wasn't able to get things working without those. The branch is also
>>>>>> based on top of 5.16-rc2 which required some extra changes to the
>>>>>> patches from Benjamin.
>>>>> Yeah, I also rebased on top of 5.16 shortly after sharing that branch
>>>>> and got roughly the same last fix (HID: bpf: compile fix for
>>>>> bpf_hid_foreach_rdesc_item). I am *very* interested in your "HID: bpf:
>>>>> execute BPF programs in proper context" because that is something that
>>>>> was bothering me a lot :)
>>>> Right, I think I have plenty of lockdep / scheduler checks enabled in my
>>>> kernel. They generate plenty of spam with i2c-hid without that patch.
>>>> The same issue may not be visible with some other low level hid devices
>>>> though, I don't have testing capability for anything but the i2c-hid
>>>> right now. I2C is quite notorious for the locking aspects as it is slow
>>>> and is used to control some pretty low level stuff like power management
>>>> etc.
>>> As a rule of thumb, hid_hw_raw_request() can not and should not be
>>> called in IRQ.
>>> I tested your patch with a USB device, and got plenty of complaints too.
>>>
>>> I know bpf now has the ability to defer a function call with timers,
>>> so maybe that's what we need here.
>> That sounds like something that would work yes, I did use workqueue
>> before when this was a separate driver instead of a BPF program.
>>>>> "HID: bpf: add expected_attach_type to bpf prog during detach" is
>>>>> something I'll need to bring in too
>>>>>
>>>>> but "HID: bpf: fix file mapping" is actually wrong. I initially wanted
>>>>> to attach BPF programs to hidraw, but shortly realized that this is
>>>>> not working because the `hid/rdesc_fixup` kills the hidraw node and so
>>>>> releases the BPF programs. The way I am now attaching it is to use the
>>>>> fd associated with the modalias in the sysfs file (for instance: `sudo
>>>>> ./hid_surface_dial /sys/bus/hid/devices/0005:045E:091B.*/modalias`).
>>>>> This way, the reference to the struct hid_device is kept even if we
>>>>> disconnect the device and reprobe it.
>>>> Ok I can check this out if it works me also. The samples lead me to
>>>> /dev/hidraw usage.
>>>>> Thanks again for your work, and I'd be curious to have your thoughts
>>>>> on hid-bpf and if you think it is better than hidraw/evdev write/new
>>>>> ioctls for your use case.
>>>> The new driver was 777 lines diff, the BPF one is 496 lines so it
>>>> appears smaller. The driver did support two different vendors though
>>>> (ELAN+Goodix, with their specific quirks in place), the BPF only a
>>>> single one right now (ELAN).
>>>>
>>>> The vendor specific quirks are a question, do we want to support that
>>>> somehow in a single BPF binary, or should we attach vendor specific BPF
>>>> programs?
>>> Good question.
>>> The plan I had was to basically pre-compile BPF programs for the
>>> various devices, but having them separated into generic + vendor
>>> specifics seems interesting too.
>>>
>>> I don't have a good answer right now.
>> At least for USI purposes, ELAN+GOODIX controllers have pretty different
>> quirks for them and it seems like having separate BPF programs might be
>> better; trying to get the same BPF program to run for both sounds
>> painful (it was rather painful to get this to work for single vendor.)
> The more I look at the 2 report descriptors, the more I think that we
> should be able to have a common code in hid-input for dealing with
> input reports, and then have specifics as bpf programs.
> As mentioned earlier, I think Goodix needs a report fixup for the
> features, and we might want to change the reported values for Elan
> immediately after we issue the config change.
>
> It seems very much that we are in the same situation as Windows 7
> multitouch screens. The spec was not restrictive enough that the HW
> makers were not very careful, and we added multiple quirks for them.
> I would prefer to have a common minimal hid-input handling and defer
> the quirks in a BPF program :)
Yes, sounds good to me. When are you going to merge the base hid-bpf 
driver? :)
>
>>>> Chromium-os devices are one of the main customers for USI pens right
>>>> now, and I am not sure how well they will take the BPF concept. :) I did
>>>> ask their feedback though, and I'll come back on this once I have something.
>>> Cool thanks.
>>>
>>>> Personally, I don't have much preference either way at this moment, both
>>>> seem like feasible options. I might lean a bit towards evdev/ioctl as it
>>>> seems a cleaner implementation as of now. The write mechanism I
>>>> implemented for the USI-BPF is a bit hacky, as it just directly writes
>>>> to a shared memory buffer and the buffer gets parsed by the kernel part
>>>> when it processes hidraw event. Anyways, do you have any feedback on
>>>> that part? BPF is completely new to me again so would love to get some
>>>> feedback.
>>> Yeah, this feels wrong to me too.
>>> I guess what we want is to run a BPF call initiated from the
>>> userspace. I am not sure if this is doable. I'll need to dig further
>>> too (I am relatively new to BPF too as a matter of facts).
>> I could not find a way to initiate BPF call from userspace, thats the
>> reason I implemented it this way. That said, I don't see any case where
>> this would fail though; we only ever write the values from single source
>> (userspace) and read them from kernel. If we miss a write, we just get
>> the old value and report the change later on.
> Yeah, I understand it works, it's just that you can not initiate a
> bpf_hid_raw_request() call from a raw_event callback. You are in an
> IRQ, and we need to run things as fast as possible.
> So either we defer, or we find another way of contacting the stylus
> outside of the IRQ.
Hmm right, I wonder why lockdep and friends don't complain about this 
though, maybe BPF is switching context somehow. But yes, obviously 
running this in irq context would be wrong.
>
>> To initiate a BPF call from userspace we would need some sort of hid-bpf
>> callback to a BPF program, which gets triggered by an ioctl or evdev
>> write or something coming from userspace. Which brings us back to the
>> chicken-egg problem we have with USI right now. :)
> I am thinking of adding a new syscall hid_bpf_run() that the userspace
> program can trigger. Otherwise, it seems from a very rough overview we
> could hijack the bpf_test_run() syscall, but that would not be very
> nice.
> Initiating an event from evdev is not very compatible with the BPF
> approach because you'll need to also open the evdev node, which you
> don't when you run a BPF program.

This would work for me.

-Tero

>
> Cheers,
> Benjamin
>
>> -Tero
>>
>>
>>> Cheers,
>>> Benjamin
>>>
>>>> One option is of course to push the write portion of the code to
>>>> userspace and just use hidraw, but we still need to filter out the bogus
>>>> events somehow, and do that in vendor specific manner. I don't think
>>>> this can be done on userspace, as plenty of information that would be
>>>> needed to do this properly has been lost at the input-event level.
>>>>
>>>> -Tero
>>>>
>>>>> Cheers,
>>>>> Benjamin
>>>>>
>>>>>> -Tero
>>>>>>
>>>>>> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
>>>>>>
>>>>>>

[-- Attachment #2: usi-logs.tar.gz --]
[-- Type: application/gzip, Size: 53498 bytes --]