Hi,
On 2/9/21 4:16 PM, Alexander Kobel wrote:
> Hi,
>
> On 2/8/21 11:17 AM, Hans de Goede wrote:
>> On 2/7/21 6:55 PM, Alexander Kobel wrote:
>>> <snip>
>>> I'll go off and try to improve.
>>
>> So Nitin has been kind enough to provide us with some docs for this,
>> please see me reply to Nitin's email and lets continue this part of this mail
>> thread there.
>
> Right. I have the other patch ready, thanks to your great help. I'm
> waiting for Nitin's okay whether / how much info I can copy from the
> reference sheet to source code comments. Once I have that confirmation,
> I will post the revised patch.
>
>> <snip>
>>
>>> Finally, I mentioned some open ends already on a post to ibm-acpi-devel
>>> at https://sourceforge.net/p/ibm-acpi/mailman/message/37200082/; this
>>> very question is among them.
>>> I will start tackling the SW_TABLET_MODE event issue first, but if Mark
>>> and Nitin can already hint about the keyboard shortcuts, it'd be highly
>>> appreciated.
>>
>> I think I might be able to help there, a couple of months ago I bought
>> a second-hand thinkpad-10 tablet which also has a USB attached keyboard.
>>
>> In hindsight I guess I could have asked Mark and Nitin for some more info,
>> but I went on autopilot and just ran hexdump -C on the /dev/hidraw node
>> to see which events all the keys send.
>>
>> And I fired up an usb-sniffer under Windows to figure out the audio-leds,
>> since I'm used to just figure these things out without help from the vendor :)
>
> Yeah, why take the boring route if you know how to do all the work on
> your own... ;-)
>
>> See the recent commits here for my work on this:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/hid/hid-lenovo.c
>
> Thanks, very helpful.
>
>> So on the ibm-acpi list message you said that the kbd sends the following:
>>
>> type 4 (EV_MSC), code 4 (MSC_SCAN), value c0001
>> type 1 (EV_KEY), code 240 (KEY_UNKNOWN), value 1
>>
>> For the Fn-keys, does it send the same MSC_SCAN code for *all* the
>> non-working Fn-keys ?
>
> Correct. And I can confirm that /dev/hidraw1 lets me distinguish between
> the keys.
>
>> If so then it seems that this is very much like the thinkpad 10 kbd dock
>> which also does this, see the lenovo_input_mapping_tp10_ultrabook_kbd()
>> function in drivers/hid/hid-lenovo.c .
>>
>> If I have that right, then I think we should be able to get the
>> Fn keys to work without too much trouble. You could try hacking up
>> drivers/hid/hid-lenovo.c a bit:
>
> (Not yet there, but will investigate.)
>
>> 1. Add an entry to the lenovo_devices array like this:
>>
>> /*
>> * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard part,
>> * while letting hid-multitouch.c handle the touchpad and trackpoint.
>> */
>> { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
>> USB_VENDOR_ID_LENOVO,
>> USB_DEVICE_ID_LENOVO_X1_TAB),
>>
>> 2. Add the following entry to the switch-case in lenovo_input_mapping() :
>>
>> case USB_DEVICE_ID_LENOVO_X1_TAB:
>> return lenovo_input_mapping_tp10_ultrabook_kbd(hdev, hi, field,
>> usage, bit, max);
>>
>> And then build hid-lenovo.c and modprobe it.
>>
>> After the modprobe to:
>>
>> ls -l /sys/bus/hid/devices/0003:17EF:60A3.*/driver
>>
>> This should show 2 devices (I guess) with one being bound to hid-lenovo
>> and 1 being bound to hid-multitouch.
>
> So far (without patching hid-lenovo), 2 bound to hid-generic and 1 to
> hid-multitouch.
Ok, so it seems that they kept the thinkpad 10 kbd bits (mostly) with
1 keyboard interface using the usb boot kbd interface (so that it will
also work inside the BIOS) and a second interface for multimedia-keys +
the mouse emulation of the thinkpad 10 touchpad, those are interfaces
1 and 2, except that they removed the mouse emulation as they added a
new proper multi-touch capable touchpad as interface 3; and that one
also handles the pointing stick I believe.
So yes 2 bound to hid-generic, 1 bound to hid-multitouch seems to be
correct.
>> If this works some of your Fn + F# keys will now hopefully start doing
>> something, you can play around with modifying lenovo_input_mapping_tp10_ultrabook_kbd
>> to make it do the right thing for your kbd.
>>z
>> ###
>>
>> About LED support, just enabling the LED support bits for the
>> USB_DEVICE_ID_LENOVO_TP10UBKBD handling for now might work fine,
>> but there is a tiny chance that sending the wrong command somehow puts
>> the kbd in firmware update mode, I had that happen once with a Logitech
>> kbd which did not seem to have any kind of handshake / passcode to avoid
>> accidental fw updates (*).
>>
>> If you can give me a dump of the hid-descriptors for your keyboard,
>> then I can check if that the LEDs might work the same way too (or not).
>>
>> The easiest way to get a dump is to run the following command as root:
>>
>> cat /sys/kernel/debug/hid/0003:17EF:60A3.*/rdesc > rdesc
>>
>> And then attach rdesc to your next email.
>
> Please find this one attached already now.
> In case it helps, the * expands to 0057 0058 0059 on my system.
Ok, so there still is an output-report number 9 on the second interface,
which probably still controls the LEDS but its descriptors are subtly
different. Although different in a good way I guess because the thinkpad
10 dock descriptor describes the 2 bytes in the output report as being
in the range of 0-1 which is not how they are actually used.
So I think that the code for the Thinkpad 10 ultrabook keyboard as
Lenovo calls it, should also work on the X1 tablet thin keyboard.
I've prepared a set of patches which enable the tp10ubkbd code on
the X1 tablet thin keyboard. But beware as mentioned before there is a
tiny chance that sending the wrong command somehow puts the kbd in
firmware update mode. I believe that trying the tp10ubkbd code is safe,
esp. since this is using a 2 byte large output report and using that
for fw-updating would be a bit weird. Still there is a small risk
(there always is when poking hw) so I will leave it up to you if
you are willing to try this.
Here is how I test this (note you will need to adjust the paths a bit) :
Toggle the 2 mute LEDs:
[root@localhost ~]# echo 1 > /sys/class/leds/0003:17EF:6062.000E:amber:micmute/brightness
[root@localhost ~]# echo 0 > /sys/class/leds/0003:17EF:6062.000E:amber:micmute/brightness
[root@localhost ~]# echo 1 > /sys/class/leds/0003:17EF:6062.000E:amber:mute/brightness
[root@localhost ~]# echo 0 > /sys/class/leds/0003:17EF:6062.000E:amber:mute/brightness
Check Fnlock LED state (toggle on kbd by pressing Fn + Esc) :
[root@localhost ~]# cat /sys/bus/hid/devices/0003:17EF:6062.000E/fn_lock
1
[root@localhost ~]# cat /sys/bus/hid/devices/0003:17EF:6062.000E/fn_lock
0
Change Fnlock state from within Linux:
[root@localhost ~]# echo 1 > /sys/bus/hid/devices/0003:17EF:6062.000E/fn_lock
[root@localhost ~]# echo 0 > /sys/bus/hid/devices/0003:17EF:6062.000E/fn_lock
(The Led on the kbd should update; and the F## key behavior should change)
Regards,
Hans
>From 44311c907697180046648736dc82b34e8ab5beaf Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Thu, 11 Feb 2021 23:14:20 +0100
Subject: [PATCH 1/5] HID: lenovo: Fix false positive errors on setting
tp10ubkbd LEDs
Fix the error check in lenovo_led_set_tp10ubkbd(), on success
hid_hw_raw_request() returns the number of bytes send. So we should
check for (ret != 3) rather then for (ret != 0).
Fixes: bc04b37ea0ec ("HID: lenovo: Add ThinkPad 10 Ultrabook Keyboard support")
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/hid/hid-lenovo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index c6c8e20f3e8d..69d709439676 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -75,7 +75,7 @@ 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)
hid_err(hdev, "Set LED output report error: %d\n", ret);
mutex_unlock(&data->led_report_mutex);
--
2.30.1
>From c24649efa10189d49b87a19b6a999ccfa8987801 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Thu, 11 Feb 2021 23:41:53 +0100
Subject: [PATCH 2/5] HID: lenovo: Check hid_get_drvdata() returns non NULL in
lenovo_event()
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.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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 69d709439676..8a040f6040eb 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -498,6 +498,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
>From 0fde88dac366cca64fd38b4303b329adfec643ef Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Thu, 11 Feb 2021 23:21:39 +0100
Subject: [PATCH 3/5] HID: lenovo: Set default_trigger-s for the mute and
micmute LEDs
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 audo 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@xxxxxxxxxx>
---
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 8a040f6040eb..c4cd4f69cdfd 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -824,6 +824,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_get = lenovo_led_brightness_get;
data->led_mute.brightness_set = lenovo_led_brightness_set;
data->led_mute.dev = &hdev->dev;
@@ -832,6 +833,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_get = lenovo_led_brightness_get;
data->led_micmute.brightness_set = lenovo_led_brightness_set;
data->led_micmute.dev = &hdev->dev;
--
2.30.1
>From 7f03da5567b810f1b3790674a0fb34db4866ceca Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Thu, 11 Feb 2021 23:10:27 +0100
Subject: [PATCH 4/5] HID: lenovo: Rework how the tp10ubkbd code decides which
USB interface to use
Instead of looking for a hdev with a type of HID_TYPE_USBMOUSE find
the interface for the mute/mic-mute/fn-lock LEDs by checking for the
output-report which is used to set them.
This is a preparation patch for adding support for the LEDs on the
X1 tablet thin keyboard which uses the same output-report, but has
a separate (third) USB interface for the touchpad/mouse functionality.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/hid/hid-lenovo.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index c4cd4f69cdfd..33033b74b434 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -957,11 +957,24 @@ static const struct attribute_group lenovo_attr_group_tp10ubkbd = {
static int lenovo_probe_tp10ubkbd(struct hid_device *hdev)
{
+ struct hid_report_enum *rep_enum;
struct lenovo_drvdata *data;
+ struct hid_report *rep;
+ bool found;
int ret;
- /* All the custom action happens on the USBMOUSE device for USB */
- if (hdev->type != HID_TYPE_USBMOUSE)
+ /*
+ * The LEDs and the Fn-lock functionality use output report 9,
+ * with an application of 0xffa0001, add the LEDs on the interface
+ * with this output report.
+ */
+ found = false;
+ rep_enum = &hdev->report_enum[HID_OUTPUT_REPORT];
+ list_for_each_entry(rep, &rep_enum->report_list, list) {
+ if (rep->application == 0xffa00001)
+ found = true;
+ }
+ if (!found)
return 0;
data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
--
2.30.1
>From 44a8670e26c6cb57c03c26d0476f813a2856434f Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Thu, 11 Feb 2021 23:40:11 +0100
Subject: [PATCH 5/5] HID: lenovo: WIP: Add support for Thinkpad X1 Tablet Thin
keyboard
WIP: untested and the key mappings in lenovo_input_mapping_x1_tab_kbd()
need work.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/hid/hid-lenovo.c | 46 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 33033b74b434..9a628e7a2382 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -255,6 +255,39 @@ static int lenovo_input_mapping_tp10_ultrabook_kbd(struct hid_device *hdev,
return 0;
}
+static int lenovo_input_mapping_x1_tab_kbd(struct hid_device *hdev,
+ struct hid_input *hi, struct hid_field *field,
+ struct hid_usage *usage, unsigned long **bit, int *max)
+{
+ /*
+ * The ThinkPad X1 Tablet Thin Keyboard uses 0x000c0001 usage for
+ * a bunch of keys which have no standard consumer page code.
+ */
+ // FIXME the below mappings are taken for the tp10_ultrabook_kbd mapping function
+ // these may very well need to be changed for the X1 Tablet Thin Keyboard
+ 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(KEY_MICMUTE);
+ return 1;
+ case 10: /* Fn-F7: Control panel */
+ map_key_clear(KEY_CONFIG);
+ return 1;
+ case 11: /* Fn-F8: Search (magnifier glass) */
+ map_key_clear(KEY_SEARCH);
+ return 1;
+ case 12: /* Fn-F10: Open My computer (6 boxes) */
+ map_key_clear(KEY_FILE);
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
static int lenovo_input_mapping(struct hid_device *hdev,
struct hid_input *hi, struct hid_field *field,
struct hid_usage *usage, unsigned long **bit, int *max)
@@ -278,6 +311,8 @@ static int lenovo_input_mapping(struct hid_device *hdev,
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
return lenovo_input_mapping_tp10_ultrabook_kbd(hdev, hi, field,
usage, bit, max);
+ case USB_DEVICE_ID_LENOVO_X1_TAB:
+ return lenovo_input_mapping_x1_tab_kbd(hdev, hi, field, usage, bit, max);
default:
return 0;
}
@@ -364,6 +399,7 @@ static ssize_t attr_fn_lock_store(struct device *dev,
lenovo_features_set_cptkbd(hdev);
break;
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
+ case USB_DEVICE_ID_LENOVO_X1_TAB:
lenovo_led_set_tp10ubkbd(hdev, TP10UBKBD_FN_LOCK_LED, value);
break;
}
@@ -506,6 +542,7 @@ static int lenovo_event(struct hid_device *hdev, struct hid_field *field,
case USB_DEVICE_ID_LENOVO_CBTKBD:
return lenovo_event_cptkbd(hdev, field, usage, value);
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
+ case USB_DEVICE_ID_LENOVO_X1_TAB:
return lenovo_event_tp10ubkbd(hdev, field, usage, value);
default:
return 0;
@@ -802,6 +839,7 @@ static void lenovo_led_brightness_set(struct led_classdev *led_cdev,
lenovo_led_set_tpkbd(hdev);
break;
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
+ case USB_DEVICE_ID_LENOVO_X1_TAB:
lenovo_led_set_tp10ubkbd(hdev, tp10ubkbd_led[led_nr], value);
break;
}
@@ -1036,6 +1074,7 @@ static int lenovo_probe(struct hid_device *hdev,
ret = lenovo_probe_cptkbd(hdev);
break;
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
+ case USB_DEVICE_ID_LENOVO_X1_TAB:
ret = lenovo_probe_tp10ubkbd(hdev);
break;
default:
@@ -1101,6 +1140,7 @@ static void lenovo_remove(struct hid_device *hdev)
lenovo_remove_cptkbd(hdev);
break;
case USB_DEVICE_ID_LENOVO_TP10UBKBD:
+ case USB_DEVICE_ID_LENOVO_X1_TAB:
lenovo_remove_tp10ubkbd(hdev);
break;
}
@@ -1140,6 +1180,12 @@ static const struct hid_device_id lenovo_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_IBM, USB_DEVICE_ID_IBM_SCROLLPOINT_800DPI_OPTICAL_PRO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_SCROLLPOINT_OPTICAL) },
{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TP10UBKBD) },
+ /*
+ * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard
+ * part, while letting hid-multitouch.c handle the touchpad and trackpoint.
+ */
+ { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
+ USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_X1_TAB) },
{ }
};
--
2.30.1
