[PATCH v2 0/7] HID: lenovo: Mute LED handling fixes and improvements

[PATCH v2 0/7] HID: lenovo: Mute LED handling fixes and improvements

[Hans de Goede]

Hi All,

Here is v2 of my series with mute LED handling fixes and improvements
for the hid-lenovo driver.

This time I've added the LED folks to the Cc in case they have any input,
but there is nothing controversial in here wrt use of the LED API.

The following patches were changed or are new in version 2 of the
series, see the individual patches for detaisl:

[PATCH v2 2/7] HID: lenovo: Fix lenovo_led_set_tp10ubkbd() error handling
[PATCH v2 4/7] HID: lenovo: Remove lenovo_led_brightness_get()
[PATCH v2 5/7] HID: lenovo: Set LEDs max_brightness value

Regards,

Hans


Hans de Goede (7):
  HID: lenovo: Use brightness_set_blocking callback for setting LEDs
    brightness
  HID: lenovo: Fix lenovo_led_set_tp10ubkbd() error handling
  HID: lenovo: Check hid_get_drvdata() returns non NULL in
    lenovo_event()
  HID: lenovo: Remove lenovo_led_brightness_get()
  HID: lenovo: Set LEDs max_brightness value
  HID: lenovo: Map mic-mute button to KEY_F20 instead of KEY_MICMUTE
  HID: lenovo: Set default_trigger-s for the mute and micmute LEDs

 drivers/hid/hid-lenovo.c | 61 ++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

-- 
2.30.1

[PATCH v2 1/7] HID: lenovo: Use brightness_set_blocking callback for setting LEDs brightness

[Hans de Goede]

The lenovo_led_brightness_set function may sleep, so we should have the
the led_class_dev's brightness_set_blocking callback point to it, rather
then the regular brightness_set callback.

When toggle through sysfs this is not a problem, but the brightness_set
callback may be called from atomic context when using LED-triggers.

Fixes: bc04b37ea0ec ("HID: lenovo: Add ThinkPad 10 Ultrabook Keyboard support")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-lenovo.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index c6c8e20f3e8d..4dc5e5f932ed 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -777,7 +777,7 @@ static enum led_brightness lenovo_led_brightness_get(
 				: LED_OFF;
 }
 
-static void lenovo_led_brightness_set(struct led_classdev *led_cdev,
+static int lenovo_led_brightness_set(struct led_classdev *led_cdev,
 			enum led_brightness value)
 {
 	struct device *dev = led_cdev->dev->parent;
@@ -802,6 +802,8 @@ static void lenovo_led_brightness_set(struct led_classdev *led_cdev,
 		lenovo_led_set_tp10ubkbd(hdev, tp10ubkbd_led[led_nr], value);
 		break;
 	}
+
+	return 0;
 }
 
 static int lenovo_register_leds(struct hid_device *hdev)
@@ -822,7 +824,7 @@ static int lenovo_register_leds(struct hid_device *hdev)
 
 	data->led_mute.name = name_mute;
 	data->led_mute.brightness_get = lenovo_led_brightness_get;
-	data->led_mute.brightness_set = lenovo_led_brightness_set;
+	data->led_mute.brightness_set_blocking = lenovo_led_brightness_set;
 	data->led_mute.dev = &hdev->dev;
 	ret = led_classdev_register(&hdev->dev, &data->led_mute);
 	if (ret < 0)
@@ -830,7 +832,7 @@ static int lenovo_register_leds(struct hid_device *hdev)
 
 	data->led_micmute.name = name_micm;
 	data->led_micmute.brightness_get = lenovo_led_brightness_get;
-	data->led_micmute.brightness_set = lenovo_led_brightness_set;
+	data->led_micmute.brightness_set_blocking = lenovo_led_brightness_set;
 	data->led_micmute.dev = &hdev->dev;
 	ret = led_classdev_register(&hdev->dev, &data->led_micmute);
 	if (ret < 0) {
-- 
2.30.1

[PATCH v2 2/7] HID: lenovo: Fix lenovo_led_set_tp10ubkbd() error handling

[Hans de Goede]

Fix the following issues with lenovo_led_set_tp10ubkbd() error handling:

1. On success hid_hw_raw_request() returns the number of bytes send.
   So we should check for (ret != 3) rather then for (ret != 0).

2. Actually propagate errors to the caller.

3. Since the LEDs are part of an USB keyboard-dock the mute LEDs can go
   away at any time. Don't log an error when ret == -ENODEV and set the
   LED_HW_PLUGGABLE flag to avoid errors getting logged when the USB gets
   disconnected.

Fixes: bc04b37ea0ec ("HID: lenovo: Add ThinkPad 10 Ultrabook Keyboard support")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Rewrite to fix a bunch of other error-handling issues too
---
 drivers/hid/hid-lenovo.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 4dc5e5f932ed..49b8767a8d30 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -62,8 +62,8 @@ struct lenovo_drvdata {
 #define TP10UBKBD_LED_OFF		1
 #define TP10UBKBD_LED_ON		2
 
-static void lenovo_led_set_tp10ubkbd(struct hid_device *hdev, u8 led_code,
-				     enum led_brightness value)
+static int lenovo_led_set_tp10ubkbd(struct hid_device *hdev, u8 led_code,
+				    enum led_brightness value)
 {
 	struct lenovo_drvdata *data = hid_get_drvdata(hdev);
 	int ret;
@@ -75,10 +75,12 @@ static void lenovo_led_set_tp10ubkbd(struct hid_device *hdev, u8 led_code,
 	data->led_report[2] = value ? TP10UBKBD_LED_ON : TP10UBKBD_LED_OFF;
 	ret = hid_hw_raw_request(hdev, data->led_report[0], data->led_report, 3,
 				 HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
-	if (ret)
+	if (ret != 3 && ret != -ENODEV)
 		hid_err(hdev, "Set LED output report error: %d\n", ret);
 
 	mutex_unlock(&data->led_report_mutex);
+
+	return ret < 0 ? ret : 0;
 }
 
 static void lenovo_tp10ubkbd_sync_fn_lock(struct work_struct *work)
@@ -349,7 +351,7 @@ static ssize_t attr_fn_lock_store(struct device *dev,
 {
 	struct hid_device *hdev = to_hid_device(dev);
 	struct lenovo_drvdata *data = hid_get_drvdata(hdev);
-	int value;
+	int value, ret;
 
 	if (kstrtoint(buf, 10, &value))
 		return -EINVAL;
@@ -364,7 +366,9 @@ static ssize_t attr_fn_lock_store(struct device *dev,
 		lenovo_features_set_cptkbd(hdev);
 		break;
 	case USB_DEVICE_ID_LENOVO_TP10UBKBD:
-		lenovo_led_set_tp10ubkbd(hdev, TP10UBKBD_FN_LOCK_LED, value);
+		ret = lenovo_led_set_tp10ubkbd(hdev, TP10UBKBD_FN_LOCK_LED, value);
+		if (ret)
+			return ret;
 		break;
 	}
 
@@ -785,6 +789,7 @@ static int lenovo_led_brightness_set(struct led_classdev *led_cdev,
 	struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
 	u8 tp10ubkbd_led[] = { TP10UBKBD_MUTE_LED, TP10UBKBD_MICMUTE_LED };
 	int led_nr = 0;
+	int ret = 0;
 
 	if (led_cdev == &data_pointer->led_micmute)
 		led_nr = 1;
@@ -799,11 +804,11 @@ static int lenovo_led_brightness_set(struct led_classdev *led_cdev,
 		lenovo_led_set_tpkbd(hdev);
 		break;
 	case USB_DEVICE_ID_LENOVO_TP10UBKBD:
-		lenovo_led_set_tp10ubkbd(hdev, tp10ubkbd_led[led_nr], value);
+		ret = lenovo_led_set_tp10ubkbd(hdev, tp10ubkbd_led[led_nr], value);
 		break;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int lenovo_register_leds(struct hid_device *hdev)
@@ -825,6 +830,7 @@ static int lenovo_register_leds(struct hid_device *hdev)
 	data->led_mute.name = name_mute;
 	data->led_mute.brightness_get = lenovo_led_brightness_get;
 	data->led_mute.brightness_set_blocking = lenovo_led_brightness_set;
+	data->led_mute.flags = LED_HW_PLUGGABLE;
 	data->led_mute.dev = &hdev->dev;
 	ret = led_classdev_register(&hdev->dev, &data->led_mute);
 	if (ret < 0)
@@ -833,6 +839,7 @@ static int lenovo_register_leds(struct hid_device *hdev)
 	data->led_micmute.name = name_micm;
 	data->led_micmute.brightness_get = lenovo_led_brightness_get;
 	data->led_micmute.brightness_set_blocking = lenovo_led_brightness_set;
+	data->led_micmute.flags = LED_HW_PLUGGABLE;
 	data->led_micmute.dev = &hdev->dev;
 	ret = led_classdev_register(&hdev->dev, &data->led_micmute);
 	if (ret < 0) {
-- 
2.30.1

[PATCH v2 3/7] HID: lenovo: Check hid_get_drvdata() returns non NULL in lenovo_event()

[Hans de Goede]

The HID lenovo probe function only attaches drvdata to one of the
USB interfaces, but lenovo_event() will get called for all USB interfaces
to which hid-lenovo is bound.

This allows a malicious device to fake being a device handled by
hid-lenovo, which generates events for which lenovo_event() has
special handling (and thus dereferences hid_get_drvdata()) on another
interface triggering a NULL pointer exception.

Add a check for hid_get_drvdata() returning NULL, avoiding this
possible NULL pointer exception.

Fixes: bc04b37ea0ec ("HID: lenovo: Add ThinkPad 10 Ultrabook Keyboard support")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-lenovo.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 49b8767a8d30..f1868b5d9b58 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -502,6 +502,9 @@ static int lenovo_event_cptkbd(struct hid_device *hdev,
 static int lenovo_event(struct hid_device *hdev, struct hid_field *field,
 		struct hid_usage *usage, __s32 value)
 {
+	if (!hid_get_drvdata(hdev))
+		return 0;
+
 	switch (hdev->product) {
 	case USB_DEVICE_ID_LENOVO_CUSBKBD:
 	case USB_DEVICE_ID_LENOVO_CBTKBD:
-- 
2.30.1

[PATCH v2 4/7] HID: lenovo: Remove lenovo_led_brightness_get()

[Hans de Goede]

The led_classdev already contains a cached value of the last set
brightness, the brightness_get callback is only meant for LED drivers
which can read back the actual / current brightness from the hardware.

Since lenovo_led_brightness_get() just returns the last set value
it does not add any functionality, so we can just remove it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- New patch in v2 of this patch-set
---
 drivers/hid/hid-lenovo.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index f1868b5d9b58..bb8e5ebdaabe 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -768,22 +768,6 @@ static void lenovo_led_set_tpkbd(struct hid_device *hdev)
 	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
 }
 
-static enum led_brightness lenovo_led_brightness_get(
-			struct led_classdev *led_cdev)
-{
-	struct device *dev = led_cdev->dev->parent;
-	struct hid_device *hdev = to_hid_device(dev);
-	struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
-	int led_nr = 0;
-
-	if (led_cdev == &data_pointer->led_micmute)
-		led_nr = 1;
-
-	return data_pointer->led_state & (1 << led_nr)
-				? LED_FULL
-				: LED_OFF;
-}
-
 static int lenovo_led_brightness_set(struct led_classdev *led_cdev,
 			enum led_brightness value)
 {
@@ -831,7 +815,6 @@ static int lenovo_register_leds(struct hid_device *hdev)
 	snprintf(name_micm, name_sz, "%s:amber:micmute", dev_name(&hdev->dev));
 
 	data->led_mute.name = name_mute;
-	data->led_mute.brightness_get = lenovo_led_brightness_get;
 	data->led_mute.brightness_set_blocking = lenovo_led_brightness_set;
 	data->led_mute.flags = LED_HW_PLUGGABLE;
 	data->led_mute.dev = &hdev->dev;
@@ -840,7 +823,6 @@ static int lenovo_register_leds(struct hid_device *hdev)
 		return ret;
 
 	data->led_micmute.name = name_micm;
-	data->led_micmute.brightness_get = lenovo_led_brightness_get;
 	data->led_micmute.brightness_set_blocking = lenovo_led_brightness_set;
 	data->led_micmute.flags = LED_HW_PLUGGABLE;
 	data->led_micmute.dev = &hdev->dev;
-- 
2.30.1

[PATCH v2 5/7] HID: lenovo: Set LEDs max_brightness value

[Hans de Goede]

The LEDs can only by turned on/off, so max_brightness should be set to 1
(aka LED_ON). Without this the max_brightness sysfs-attribute will report
255 which is wrong.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- New patch in v2 of this patch-set
---
 drivers/hid/hid-lenovo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index bb8e5ebdaabe..d936edb88f07 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -816,6 +816,7 @@ static int lenovo_register_leds(struct hid_device *hdev)
 
 	data->led_mute.name = name_mute;
 	data->led_mute.brightness_set_blocking = lenovo_led_brightness_set;
+	data->led_mute.max_brightness = LED_ON;
 	data->led_mute.flags = LED_HW_PLUGGABLE;
 	data->led_mute.dev = &hdev->dev;
 	ret = led_classdev_register(&hdev->dev, &data->led_mute);
@@ -824,6 +825,7 @@ static int lenovo_register_leds(struct hid_device *hdev)
 
 	data->led_micmute.name = name_micm;
 	data->led_micmute.brightness_set_blocking = lenovo_led_brightness_set;
+	data->led_micmute.max_brightness = LED_ON;
 	data->led_micmute.flags = LED_HW_PLUGGABLE;
 	data->led_micmute.dev = &hdev->dev;
 	ret = led_classdev_register(&hdev->dev, &data->led_micmute);
-- 
2.30.1

[PATCH v2 6/7] HID: lenovo: Map mic-mute button to KEY_F20 instead of KEY_MICMUTE

[Hans de Goede]

Mapping the mic-mute button to KEY_MICMUTE is technically correct but
KEY_MICMUTE translates to a scancode of 256 (248 + 8) under X,
which does not fit in 8 bits, so it does not work.

Because of this userspace is expecting KEY_F20 instead,
theoretically KEY_MICMUTE should work under Wayland but even
there it does not work, because the desktop-environment is
listening only for KEY_F20 and not for KEY_MICMUTE.

Fixes: bc04b37ea0ec ("HID: lenovo: Add ThinkPad 10 Ultrabook Keyboard support")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-lenovo.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index d936edb88f07..041bfa1937a8 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -33,6 +33,9 @@
 
 #include "hid-ids.h"
 
+/* Userspace expects F20 for mic-mute KEY_MICMUTE does not work */
+#define LENOVO_KEY_MICMUTE KEY_F20
+
 struct lenovo_drvdata {
 	u8 led_report[3]; /* Must be first for proper alignment */
 	int led_state;
@@ -128,7 +131,7 @@ static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
 	if (usage->hid == (HID_UP_BUTTON | 0x0010)) {
 		/* This sub-device contains trackpoint, mark it */
 		hid_set_drvdata(hdev, (void *)1);
-		map_key_clear(KEY_MICMUTE);
+		map_key_clear(LENOVO_KEY_MICMUTE);
 		return 1;
 	}
 	return 0;
@@ -143,7 +146,7 @@ static int lenovo_input_mapping_cptkbd(struct hid_device *hdev,
 	    (usage->hid & HID_USAGE_PAGE) == HID_UP_LNVENDOR) {
 		switch (usage->hid & HID_USAGE) {
 		case 0x00f1: /* Fn-F4: Mic mute */
-			map_key_clear(KEY_MICMUTE);
+			map_key_clear(LENOVO_KEY_MICMUTE);
 			return 1;
 		case 0x00f2: /* Fn-F5: Brightness down */
 			map_key_clear(KEY_BRIGHTNESSDOWN);
@@ -233,7 +236,7 @@ static int lenovo_input_mapping_tp10_ultrabook_kbd(struct hid_device *hdev,
 			map_key_clear(KEY_FN_ESC);
 			return 1;
 		case 9: /* Fn-F4: Mic mute */
-			map_key_clear(KEY_MICMUTE);
+			map_key_clear(LENOVO_KEY_MICMUTE);
 			return 1;
 		case 10: /* Fn-F7: Control panel */
 			map_key_clear(KEY_CONFIG);
-- 
2.30.1

[PATCH v2 7/7] HID: lenovo: Set default_trigger-s for the mute and micmute LEDs

[Hans de Goede]

The mute and mic-mute LEDs should be automatically turned on/off based
on the audio-cards mixer settings.

Add the standardized default-trigger names for this, so that the alsa
code can turn the LEDs on/off as appropriate (on supported audio cards).

This brings the mute/mic-mute LED support inline with the thinkpad_acpi
support for the same LEDs in keyboards directly connected to the
laptop's embedded-controller.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-lenovo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 041bfa1937a8..18e9ffcf2ace 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -818,6 +818,7 @@ static int lenovo_register_leds(struct hid_device *hdev)
 	snprintf(name_micm, name_sz, "%s:amber:micmute", dev_name(&hdev->dev));
 
 	data->led_mute.name = name_mute;
+	data->led_mute.default_trigger = "audio-mute";
 	data->led_mute.brightness_set_blocking = lenovo_led_brightness_set;
 	data->led_mute.max_brightness = LED_ON;
 	data->led_mute.flags = LED_HW_PLUGGABLE;
@@ -827,6 +828,7 @@ static int lenovo_register_leds(struct hid_device *hdev)
 		return ret;
 
 	data->led_micmute.name = name_micm;
+	data->led_micmute.default_trigger = "audio-micmute";
 	data->led_micmute.brightness_set_blocking = lenovo_led_brightness_set;
 	data->led_micmute.max_brightness = LED_ON;
 	data->led_micmute.flags = LED_HW_PLUGGABLE;
-- 
2.30.1

Re: [PATCH v2 5/7] HID: lenovo: Set LEDs max_brightness value

[Marek Behun]

On Sat, 20 Feb 2021 13:24:36 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> +	data->led_mute.max_brightness = LED_ON;

These constants are obsolete since we now support variable maximum
brightness. Just put 1 there.

Marek

Re: [PATCH v2 5/7] HID: lenovo: Set LEDs max_brightness value

[Hans de Goede]

Hi,

On 2/20/21 4:16 PM, Marek Behun wrote:
> On Sat, 20 Feb 2021 13:24:36 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> +	data->led_mute.max_brightness = LED_ON;
> 
> These constants are obsolete since we now support variable maximum
> brightness. Just put 1 there.

Ok, any other remarks wrt the LED bits before I send out a v3 ?

Regards,

Hans

Re: [PATCH v2 5/7] HID: lenovo: Set LEDs max_brightness value

[Marek Behun]

On Sat, 20 Feb 2021 17:34:12 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/20/21 4:16 PM, Marek Behun wrote:
> > On Sat, 20 Feb 2021 13:24:36 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> +	data->led_mute.max_brightness = LED_ON;  
> > 
> > These constants are obsolete since we now support variable maximum
> > brightness. Just put 1 there.  
> 
> Ok, any other remarks wrt the LED bits before I send out a v3 ?

I will review the remaining patches tonight.

Re: [PATCH v2 1/7] HID: lenovo: Use brightness_set_blocking callback for setting LEDs brightness

[Marek Behún]

Reviewed-by: Marek Behún <kabel@kernel.org>

Re: [PATCH v2 2/7] HID: lenovo: Fix lenovo_led_set_tp10ubkbd() error handling

[Marek Behún]

On Sat, 20 Feb 2021 13:24:33 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Fix the following issues with lenovo_led_set_tp10ubkbd() error handling:
> 
> 1. On success hid_hw_raw_request() returns the number of bytes send.

bytes sent

> @@ -75,10 +75,12 @@ static void lenovo_led_set_tp10ubkbd(struct hid_device *hdev, u8 led_code,
>  	data->led_report[2] = value ? TP10UBKBD_LED_ON : TP10UBKBD_LED_OFF;
>  	ret = hid_hw_raw_request(hdev, data->led_report[0], data->led_report, 3,
>  				 HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> -	if (ret)
> +	if (ret != 3 && ret != -ENODEV)
>  		hid_err(hdev, "Set LED output report error: %d\n", ret);
>  
>  	mutex_unlock(&data->led_report_mutex);
> +
> +	return ret < 0 ? ret : 0;

So if ret == 2, it will log error but return 0. I think in that case it
should return -EIO.

Re: [PATCH v2 3/7] HID: lenovo: Check hid_get_drvdata() returns non NULL in lenovo_event()

[Marek Behún]

Reviewed-by: Marek Behún <kabel@kernel.org>

Re: [PATCH v2 4/7] HID: lenovo: Remove lenovo_led_brightness_get()

[Marek Behún]

Reviewed-by: Marek Behún <kabel@kernel.org>

Re: [PATCH v2 6/7] HID: lenovo: Map mic-mute button to KEY_F20 instead of KEY_MICMUTE

[Marek Behun]

On Sat, 20 Feb 2021 13:24:37 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Mapping the mic-mute button to KEY_MICMUTE is technically correct but
> KEY_MICMUTE translates to a scancode of 256 (248 + 8) under X,
> which does not fit in 8 bits, so it does not work.

Why does it need to fit 8 bits? Where is the problem?

Marek

Re: [PATCH v2 7/7] HID: lenovo: Set default_trigger-s for the mute and micmute LEDs

[Marek Behun]

I would use
  ... Set default_triggers for ...
instead of
  ... Set default_trigger-s for ...
in commit title.

Reviewed-by: Marek Behún <kabel@kernel.org>

Re: [PATCH v2 6/7] HID: lenovo: Map mic-mute button to KEY_F20 instead of KEY_MICMUTE

[Hans de Goede]

Hi,

On 2/21/21 2:42 AM, Marek Behun wrote:
> On Sat, 20 Feb 2021 13:24:37 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Mapping the mic-mute button to KEY_MICMUTE is technically correct but
>> KEY_MICMUTE translates to a scancode of 256 (248 + 8) under X,
>> which does not fit in 8 bits, so it does not work.
> 
> Why does it need to fit 8 bits? Where is the problem?

As the commit message says, "under X" aka X11 / Xorg. This is a well known
limitation of the X11 input stack / of XKB *as implemented in X11*
the Wayland input stack does not have this limitations and does allow
using raw key-codes >= 248.

If you look at e.g. :
https://github.com/systemd/systemd/blob/main/hwdb.d/60-keyboard.hwdb

Which (mostly) maps custom PS/2 scancodes used for some "media" keys
on laptops to linux evdev KEY_FOO codes, then you will see that there
are no lines there which end with "=micmute" instead there are quite
a few lines like this:

 KEYBOARD_KEY_8a=f20                                    # Microphone mute button; should be micmute

Arguably it would be more correct to have the kernel still send
KEY_MICMUTE and do the remapping to KEY_F20 in userspace in e.g. hwdb.

But that will not work here, the remapping is done based on mapping
the HID usage-code to a new evdev KEY_FOO code, basically overriding
lenovo_input_mapping_tp10_ultrabook_kbd() mapping.

But the "Lenovo ThinkPad 10 Ultrabook Keyboard" uses the same 0x000c0001
usage code for all of its custom Fn+F# media keys, so instead of doing
the mapping purely on usage-code it is done on a combination of usage-code +
the index of the key in the input-report (since the usage-code is not unique
for a single key):

        /*
         * The ThinkPad 10 Ultrabook Keyboard uses 0x000c0001 usage for
         * a bunch of keys which have no standard consumer page code.
         */
        if (usage->hid == 0x000c0001) {
                switch (usage->usage_index) {
                case 8: /* Fn-Esc: Fn-lock toggle */
                        map_key_clear(KEY_FN_ESC);
                        return 1;
                case 9: /* Fn-F4: Mic mute */
                        map_key_clear(LENOVO_KEY_MICMUTE);
                        return 1;
		...


So in this case we cannot fixup the mapping from userspace, as userspace
remapping is purely done based on the "scancode" which in case of HID devices
is the HID usage-code.

I don't even know what will happen if we were to try. I guess that either the
first key with a matching usage-code is remapped, or all of them are remapped,
both of which are wrong.

Regards,

Hans

Re: [PATCH v2 6/7] HID: lenovo: Map mic-mute button to KEY_F20 instead of KEY_MICMUTE

[Marek Behún]

On Sun, 21 Feb 2021 11:42:16 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/21/21 2:42 AM, Marek Behun wrote:
> > On Sat, 20 Feb 2021 13:24:37 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> Mapping the mic-mute button to KEY_MICMUTE is technically correct but
> >> KEY_MICMUTE translates to a scancode of 256 (248 + 8) under X,
> >> which does not fit in 8 bits, so it does not work.  
> > 
> > Why does it need to fit 8 bits? Where is the problem?  
> 
> As the commit message says, "under X" aka X11 / Xorg. This is a well known
> limitation of the X11 input stack / of XKB *as implemented in X11*
> the Wayland input stack does not have this limitations and does allow
> using raw key-codes >= 248.
> 
> If you look at e.g. :
> https://github.com/systemd/systemd/blob/main/hwdb.d/60-keyboard.hwdb
> 
> Which (mostly) maps custom PS/2 scancodes used for some "media" keys
> on laptops to linux evdev KEY_FOO codes, then you will see that there
> are no lines there which end with "=micmute" instead there are quite
> a few lines like this:
> 
>  KEYBOARD_KEY_8a=f20                                    # Microphone mute button; should be micmute
> 
> Arguably it would be more correct to have the kernel still send
> KEY_MICMUTE and do the remapping to KEY_F20 in userspace in e.g. hwdb.
> 
> But that will not work here, the remapping is done based on mapping
> the HID usage-code to a new evdev KEY_FOO code, basically overriding
> lenovo_input_mapping_tp10_ultrabook_kbd() mapping.
> 
> But the "Lenovo ThinkPad 10 Ultrabook Keyboard" uses the same 0x000c0001
> usage code for all of its custom Fn+F# media keys, so instead of doing
> the mapping purely on usage-code it is done on a combination of usage-code +
> the index of the key in the input-report (since the usage-code is not unique
> for a single key):
> 
>         /*
>          * The ThinkPad 10 Ultrabook Keyboard uses 0x000c0001 usage for
>          * a bunch of keys which have no standard consumer page code.
>          */
>         if (usage->hid == 0x000c0001) {
>                 switch (usage->usage_index) {
>                 case 8: /* Fn-Esc: Fn-lock toggle */
>                         map_key_clear(KEY_FN_ESC);
>                         return 1;
>                 case 9: /* Fn-F4: Mic mute */
>                         map_key_clear(LENOVO_KEY_MICMUTE);
>                         return 1;
> 		...
> 
> 
> So in this case we cannot fixup the mapping from userspace, as userspace
> remapping is purely done based on the "scancode" which in case of HID devices
> is the HID usage-code.
> 
> I don't even know what will happen if we were to try. I guess that either the
> first key with a matching usage-code is remapped, or all of them are remapped,
> both of which are wrong.
> 
> Regards,
> 
> Hans
> 

And no one ever solved this for X? OMFG :(

Very well then.

Re: [PATCH v2 6/7] HID: lenovo: Map mic-mute button to KEY_F20 instead of KEY_MICMUTE

[Hans de Goede]

Hi,

On 2/21/21 12:37 PM, Marek Behún wrote:
> On Sun, 21 Feb 2021 11:42:16 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 2/21/21 2:42 AM, Marek Behun wrote:
>>> On Sat, 20 Feb 2021 13:24:37 +0100
>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>   
>>>> Mapping the mic-mute button to KEY_MICMUTE is technically correct but
>>>> KEY_MICMUTE translates to a scancode of 256 (248 + 8) under X,
>>>> which does not fit in 8 bits, so it does not work.  
>>>
>>> Why does it need to fit 8 bits? Where is the problem?  
>>
>> As the commit message says, "under X" aka X11 / Xorg. This is a well known
>> limitation of the X11 input stack / of XKB *as implemented in X11*
>> the Wayland input stack does not have this limitations and does allow
>> using raw key-codes >= 248.
>>
>> If you look at e.g. :
>> https://github.com/systemd/systemd/blob/main/hwdb.d/60-keyboard.hwdb
>>
>> Which (mostly) maps custom PS/2 scancodes used for some "media" keys
>> on laptops to linux evdev KEY_FOO codes, then you will see that there
>> are no lines there which end with "=micmute" instead there are quite
>> a few lines like this:
>>
>>  KEYBOARD_KEY_8a=f20                                    # Microphone mute button; should be micmute
>>
>> Arguably it would be more correct to have the kernel still send
>> KEY_MICMUTE and do the remapping to KEY_F20 in userspace in e.g. hwdb.
>>
>> But that will not work here, the remapping is done based on mapping
>> the HID usage-code to a new evdev KEY_FOO code, basically overriding
>> lenovo_input_mapping_tp10_ultrabook_kbd() mapping.
>>
>> But the "Lenovo ThinkPad 10 Ultrabook Keyboard" uses the same 0x000c0001
>> usage code for all of its custom Fn+F# media keys, so instead of doing
>> the mapping purely on usage-code it is done on a combination of usage-code +
>> the index of the key in the input-report (since the usage-code is not unique
>> for a single key):
>>
>>         /*
>>          * The ThinkPad 10 Ultrabook Keyboard uses 0x000c0001 usage for
>>          * a bunch of keys which have no standard consumer page code.
>>          */
>>         if (usage->hid == 0x000c0001) {
>>                 switch (usage->usage_index) {
>>                 case 8: /* Fn-Esc: Fn-lock toggle */
>>                         map_key_clear(KEY_FN_ESC);
>>                         return 1;
>>                 case 9: /* Fn-F4: Mic mute */
>>                         map_key_clear(LENOVO_KEY_MICMUTE);
>>                         return 1;
>> 		...
>>
>>
>> So in this case we cannot fixup the mapping from userspace, as userspace
>> remapping is purely done based on the "scancode" which in case of HID devices
>> is the HID usage-code.
>>
>> I don't even know what will happen if we were to try. I guess that either the
>> first key with a matching usage-code is remapped, or all of them are remapped,
>> both of which are wrong.
>>
>> Regards,
>>
>> Hans
>>
> 
> And no one ever solved this for X? OMFG :(

Many people have looked into fixing this, but X11 is a network protocol, and
the other side could be a many many years old X-terminal (one of those devices
which don't have their own OS but connect over xdmcp to show a desktop
running on some big machine somewhere else on the LAN).

XKB in X is layered on top of the original X input protocol, and the data
gets passed around multiple times in multiple different structs and it is
limited to a 8 bit wide int everywhere.

Note it is not just micmute. The F13 - F24 F-key range has been used to
work around this for a while now.

I'm aware of the following "mappings" being used for this:

evdev	->	interpreted by userspace as

KEY_F20 ->	mic-mute toggle
KEY_F21 ->	touchpad on/off toggle
KEY_F22 ->	touchpad on
KEY_F23 ->	touchpad off

This is not pretty, I know.

Regards,

Hans

Re: [PATCH v2 1/7] HID: lenovo: Use brightness_set_blocking callback for setting LEDs brightness

[Pavel Machek]

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

On Sat 2021-02-20 13:24:32, Hans de Goede wrote:
> The lenovo_led_brightness_set function may sleep, so we should have the
> the led_class_dev's brightness_set_blocking callback point to it, rather
> then the regular brightness_set callback.
> 
> When toggle through sysfs this is not a problem, but the brightness_set
> callback may be called from atomic context when using LED-triggers.
> 
> Fixes: bc04b37ea0ec ("HID: lenovo: Add ThinkPad 10 Ultrabook Keyboard support")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Pavel Machek <pavel@ucw.cz>				Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]