| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b2c95cd3-27b3-9248-e689-b8a47a831225@redhat.com>
Date: Wed, 8 Dec 2021 17:33:47 +0100
From: Benjamin Tissoires <benjamin.tissoires@...hat.com>
To: Tero Kristo <tero.kristo@...ux.intel.com>
Cc: "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
Jiri Kosina <jikos@...nel.org>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
lkml <linux-kernel@...r.kernel.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Peter Hutterer <peter.hutterer@...-t.net>
Subject: Re: [RFCv3 3/7] HID: core: Add support for USI style events
On 12/8/21 16:18, Benjamin Tissoires wrote:
> On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@...ux.intel.com> wrote:
>>
>> Add support for Universal Stylus Interface (USI) style events to the HID
>> core and input layers.
>>
>> Signed-off-by: Tero Kristo <tero.kristo@...ux.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);
>
> This is the new slot version for pens, I am not sure we really want to
> blindly introduce that without testing.
>
> Do you have a panel that supports multiple values (at once) for this
> usage (2 pens???). If not, I would simply skip that usage until we get
> an actual hardware that makes use of it.
>
>> + 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;
>
> We already have ABS_TOOL_WIDTH which seems to relate to that very closely.
>
>> +
>> + case 0x70:
>> + case 0x71:
>> + case 0x72:
>> + case 0x73:
>> + case 0x74:
>> + case 0x75:
>> + case 0x76:
>> + case 0x77:
>> + map_msc(MSC_PEN_LINE_STYLE);
>
> Nope, this is wrong. It took me a long time to understand the report
> descriptor (see my last reply to your v2).
> Basically, we need one input event for each value between 0x72 and
> 0x77. HID core should translate the array of 1 element into a set of
> {depressed usage, pressed usage} and from the user-space, we will get
> "MSC_PEN_STYLE_CHISEL_MARKER 1" for instance.
This seems to be working (on top of this RFC):
---
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index cec0470e5485..18035c63c358 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1027,8 +1027,12 @@ static const char *misc[MSC_MAX + 1] = {
[MSC_SERIAL] = "Serial", [MSC_PULSELED] = "Pulseled",
[MSC_GESTURE] = "Gesture", [MSC_RAW] = "RawData",
[MSC_PEN_ID] = "PenID", [MSC_PEN_COLOR] "PenColor",
- [MSC_PEN_LINE_WIDTH] = "PenLineWidth",
- [MSC_PEN_LINE_STYLE] = "PenLineStyle",
+ [MSC_PEN_LINE_STYLE_INK] = "PenLineStyleInk",
+ [MSC_PEN_LINE_STYLE_PENCIL] = "PenLineStylePencil",
+ [MSC_PEN_LINE_STYLE_HIGHLIGHTER] = "PenLineStyleHighlighter",
+ [MSC_PEN_LINE_STYLE_CHISEL_MARKER] = "PenLineStyleChiselMarker",
+ [MSC_PEN_LINE_STYLE_BRUSH] = "PenLineStyleBrush",
+ [MSC_PEN_LINE_STYLE_NO_PREFERENCE] = "PenLineStyleNoPreference",
};
static const char *leds[LED_MAX + 1] = {
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 079e67c5168f..837585f4e673 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -832,8 +832,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
break;
case 0x38: /* Transducer Index */
- map_msc(MSC_PEN_ID);
- break;
+ goto ignore;
case 0x3b: /* Battery Strength */
hidinput_setup_battery(device, HID_INPUT_REPORT, field, false);
@@ -882,18 +881,36 @@ 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);
+ case 0x5c: /* Digitizer Preferred Color */
+ map_msc(MSC_PEN_COLOR);
+ break;
+
+ case 0x5e: /* Digitizer Preferred Line Width */
+ map_abs_clear(ABS_TOOL_WIDTH);
+ break;
+
+
+ case 0x70: /* Preferred Line Style -> not an input usage*/
+ case 0x71: /* Preferred Line Style is Locked */
+ goto ignore;
+
+ case 0x72: /* Ink */
+ map_msc(MSC_PEN_LINE_STYLE_INK);
+ break;
+ case 0x73: /* Pencil */
+ map_msc(MSC_PEN_LINE_STYLE_PENCIL);
+ break;
+ case 0x74: /* Highlighter */
+ map_msc(MSC_PEN_LINE_STYLE_HIGHLIGHTER);
+ break;
+ case 0x75: /* Chisel Marker */
+ map_msc(MSC_PEN_LINE_STYLE_CHISEL_MARKER);
+ break;
+ case 0x76: /* Brush */
+ map_msc(MSC_PEN_LINE_STYLE_BRUSH);
+ break;
+ case 0x77: /* No Preference */
+ map_msc(MSC_PEN_LINE_STYLE_NO_PREFERENCE);
break;
default: goto unknown;
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index a76d37082ab5..bf2b76a6e7e8 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 0x09
+#define INPUT_DEVICE_ID_MSC_MAX 0x0d
#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/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 98295f71941a..ff705245b7ae 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -908,12 +908,16 @@
#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
+#define MSC_PEN_ID 0x06
+#define MSC_PEN_COLOR 0x07
+#define MSC_PEN_LINE_STYLE_INK 0x08
+#define MSC_PEN_LINE_STYLE_PENCIL 0x09
+#define MSC_PEN_LINE_STYLE_HIGHLIGHTER 0x0a
+#define MSC_PEN_LINE_STYLE_CHISEL_MARKER 0x0b
+#define MSC_PEN_LINE_STYLE_BRUSH 0x0c
+#define MSC_PEN_LINE_STYLE_NO_PREFERENCE 0x0d
/* TODO: Add USI diagnostic & battery events too */
-#define MSC_MAX 0x09
+#define MSC_MAX 0x0d
#define MSC_CNT (MSC_MAX + 1)
/*
---
Cheers,
Benjamin
>
> And *maybe* we could even use KEY events there, so we could remap them
> (but that's a very distant maybe).
>
>> + 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
>
> Could you integrate that patch before my patch "HID: export the
> various HID defines from the spec in the uapi"? And also have it as a
> separate patch from the mapping?
> This way I can schedule that part earlier before we settle on the
> actual user space usages.
>
>>
>> #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
>
> PEN_LINE_WIDTH should be dropped.
>
>> +#define MSC_PEN_LINE_STYLE 0x09
>
> Again, PEN_LINE_STYLE is HID only and we should only map the values,
> not the title of the array.
>
> Cheers,
> Benjamin
>
>> +/* TODO: Add USI diagnostic & battery events too */
>> +#define MSC_MAX 0x09
>> +#define MSC_CNT (MSC_MAX + 1)
>>
>> /*
>> * LEDs
>> --
>> 2.25.1
>>
Powered by blists - more mailing lists