[PATCH 0/3] Input: atkbd - add skip_commands module parameter

[PATCH 0/3] Input: atkbd - add skip_commands module parameter

[Hans de Goede]

Hi all,

While debugging a keyboard issue on some HP laptops adding i8042.dumbkbd
helped to avoid the issue. So one of the commands send by atkbd.c seemed
to be the culprit.

This series a skip_commands option to help debug cases like this by adding
a bit-field which allows disabling a subset of the ps2_command()
calls the atkbd driver makes.

It also replaces the existing atkbd_skip_deactivate flag
with the new parameter and adds a DMI quirk for the HP laptops
to avoid the keyboard issue there.

Regards,

Hans


Hans de Goede (3):
  Input: atkbd - add skip_commands module parameter
  Input: atkbd - drop atkbd_skip_deactivate flag
  Input: atkbd - set skip_commands = ATKBD_SKIP_GETID for HP laptop
    15s-fq* laptops

 drivers/input/keyboard/atkbd.c | 88 ++++++++++++++++++++++++++--------
 1 file changed, 69 insertions(+), 19 deletions(-)

-- 
2.41.0

[PATCH 1/3] Input: atkbd - add skip_commands module parameter

[Hans de Goede]

While debugging a keyboard issue on some HP laptops (see link)
adding i8042.dumbkbd helped to avoid the issue. So one of the commands
send by atkbd.c seemed to be the culprit.

Add a skip_commands option to help debug cases like this by adding
a bit-field which allows disabling a subset of the ps2_command()
calls the atkbd driver makes.

This also avoids the need to add special flags for each command
which atkbd needs to skip on certain models, like e.g. the existing
atkbd_skip_deactivate flag.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2086156
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
The next patch in this series replaces the atkbd_skip_deactivate flag
with using atbkd_skip_commands to avoid having 2 deactivate skip checks.
---
 drivers/input/keyboard/atkbd.c | 52 ++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index c92e544c792d..7c16f9cc1e40 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -65,6 +65,10 @@ static bool atkbd_terminal;
 module_param_named(terminal, atkbd_terminal, bool, 0);
 MODULE_PARM_DESC(terminal, "Enable break codes on an IBM Terminal keyboard connected via AT/PS2");
 
+static int atkbd_skip_commands;
+module_param_named(skip_commands, atkbd_skip_commands, int, 0444);
+MODULE_PARM_DESC(skip_commands, "Bitfield where each bits skips a specific keyboard cmd (0 - 0x3f)");
+
 #define SCANCODE(keymap)	((keymap >> 16) & 0xFFFF)
 #define KEYCODE(keymap)		(keymap & 0xFFFF)
 
@@ -182,6 +186,13 @@ static const unsigned short atkbd_unxlate_table[128] = {
 #define ATKBD_XL_HANGEUL	0x10
 #define ATKBD_XL_HANJA		0x20
 
+#define ATKBD_SKIP_GETID	0x01l
+#define ATKBD_SKIP_ACTIVATE	0x02l
+#define ATKBD_SKIP_DEACTIVATE	0x04l
+#define ATKBD_SKIP_SETREP	0x08l
+#define ATKBD_SKIP_SETLEDS	0x10l
+#define ATKBD_SKIP_RESET_DEF	0x20l
+
 static const struct {
 	unsigned short keycode;
 	unsigned char set2;
@@ -592,6 +603,9 @@ static int atkbd_set_repeat_rate(struct atkbd *atkbd)
 	unsigned char param;
 	int i = 0, j = 0;
 
+	if (atkbd_skip_commands & ATKBD_SKIP_SETREP)
+		return 0;
+
 	while (i < ARRAY_SIZE(period) - 1 && period[i] < dev->rep[REP_PERIOD])
 		i++;
 	dev->rep[REP_PERIOD] = period[i];
@@ -609,6 +623,9 @@ static int atkbd_set_leds(struct atkbd *atkbd)
 	struct input_dev *dev = atkbd->dev;
 	unsigned char param[2];
 
+	if (atkbd_skip_commands & ATKBD_SKIP_SETLEDS)
+		return 0;
+
 	param[0] = (test_bit(LED_SCROLLL, dev->led) ? 1 : 0)
 		 | (test_bit(LED_NUML,    dev->led) ? 2 : 0)
 		 | (test_bit(LED_CAPSL,   dev->led) ? 4 : 0);
@@ -736,6 +753,9 @@ static int atkbd_activate(struct atkbd *atkbd)
 {
 	struct ps2dev *ps2dev = &atkbd->ps2dev;
 
+	if (atkbd_skip_commands & ATKBD_SKIP_ACTIVATE)
+		return 0;
+
 /*
  * Enable the keyboard to receive keystrokes.
  */
@@ -759,6 +779,9 @@ static void atkbd_deactivate(struct atkbd *atkbd)
 {
 	struct ps2dev *ps2dev = &atkbd->ps2dev;
 
+	if (atkbd_skip_commands & ATKBD_SKIP_DEACTIVATE)
+		return;
+
 	if (ps2_command(ps2dev, NULL, ATKBD_CMD_RESET_DIS))
 		dev_err(&ps2dev->serio->dev,
 			"Failed to deactivate keyboard on %s\n",
@@ -786,6 +809,13 @@ static int atkbd_probe(struct atkbd *atkbd)
 				 "keyboard reset failed on %s\n",
 				 ps2dev->serio->phys);
 
+/* Only skip probe for translated keyboards to avoid mis-identifying mice */
+	if (atkbd->translated && (atkbd_skip_commands & ATKBD_SKIP_GETID)) {
+		atkbd->id = 0xab00;
+		atkbd_deactivate(atkbd);
+		return 0;
+	}
+
 /*
  * Then we check the keyboard ID. We should get 0xab83 under normal conditions.
  * Some keyboards report different values, but the first byte is always 0xab or
@@ -802,7 +832,8 @@ static int atkbd_probe(struct atkbd *atkbd)
  * the LEDs off, which we want anyway.
  */
 		param[0] = 0;
-		if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
+		if (!(atkbd_skip_commands & ATKBD_SKIP_SETLEDS) &&
+		    ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
 			return -1;
 		atkbd->id = 0xabba;
 		return 0;
@@ -906,17 +937,21 @@ static int atkbd_reset_state(struct atkbd *atkbd)
  * Set the LEDs to a predefined state (all off).
  */
 
-	param[0] = 0;
-	if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
-		return -1;
+	if (!(atkbd_skip_commands & ATKBD_SKIP_SETLEDS)) {
+		param[0] = 0;
+		if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
+			return -1;
+	}
 
 /*
  * Set autorepeat to fastest possible.
  */
 
-	param[0] = 0;
-	if (ps2_command(ps2dev, param, ATKBD_CMD_SETREP))
-		return -1;
+	if (!(atkbd_skip_commands & ATKBD_SKIP_SETREP)) {
+		param[0] = 0;
+		if (ps2_command(ps2dev, param, ATKBD_CMD_SETREP))
+			return -1;
+	}
 
 	return 0;
 }
@@ -931,7 +966,8 @@ static void atkbd_cleanup(struct serio *serio)
 	struct atkbd *atkbd = atkbd_from_serio(serio);
 
 	atkbd_disable(atkbd);
-	ps2_command(&atkbd->ps2dev, NULL, ATKBD_CMD_RESET_DEF);
+	if (!(atkbd_skip_commands & ATKBD_SKIP_RESET_DEF))
+		ps2_command(&atkbd->ps2dev, NULL, ATKBD_CMD_RESET_DEF);
 }
 
 
-- 
2.41.0

[PATCH 2/3] Input: atkbd - drop atkbd_skip_deactivate flag

[Hans de Goede]

Drop the atkbd_skip_deactivate flag and make the DMI quirk for LG laptops
set atkbd_skip_commands = ATKBD_SKIP_DEACTIVATE instead.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/keyboard/atkbd.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 7c16f9cc1e40..19a01e763871 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -257,12 +257,6 @@ static void (*atkbd_platform_fixup)(struct atkbd *, const void *data);
 static void *atkbd_platform_fixup_data;
 static unsigned int (*atkbd_platform_scancode_fixup)(struct atkbd *, unsigned int);
 
-/*
- * Certain keyboards to not like ATKBD_CMD_RESET_DIS and stop responding
- * to many commands until full reset (ATKBD_CMD_RESET_BAT) is performed.
- */
-static bool atkbd_skip_deactivate;
-
 static ssize_t atkbd_attr_show_helper(struct device *dev, char *buf,
 				ssize_t (*handler)(struct atkbd *, char *));
 static ssize_t atkbd_attr_set_helper(struct device *dev, const char *buf, size_t count,
@@ -855,8 +849,7 @@ static int atkbd_probe(struct atkbd *atkbd)
  * Make sure nothing is coming from the keyboard and disturbs our
  * internal state.
  */
-	if (!atkbd_skip_deactivate)
-		atkbd_deactivate(atkbd);
+	atkbd_deactivate(atkbd);
 
 	return 0;
 }
@@ -1781,9 +1774,12 @@ static int __init atkbd_setup_scancode_fixup(const struct dmi_system_id *id)
 	return 1;
 }
 
-static int __init atkbd_deactivate_fixup(const struct dmi_system_id *id)
+static int __init atkbd_skip_commands_fixup(const struct dmi_system_id *id)
 {
-	atkbd_skip_deactivate = true;
+	/* Module parameter overrules quirks */
+	if (!atkbd_skip_commands)
+		atkbd_skip_commands = (long)id->driver_data;
+
 	return 1;
 }
 
@@ -1931,10 +1927,15 @@ static const struct dmi_system_id atkbd_dmi_quirk_table[] __initconst = {
 		.driver_data = atkbd_oqo_01plus_scancode_fixup,
 	},
 	{
+		/*
+		 * LG keyboards seem to not like ATKBD_CMD_RESET_DIS and stop responding
+		 * to many commands until full reset (ATKBD_CMD_RESET_BAT) is performed.
+		 */
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LG Electronics"),
 		},
-		.callback = atkbd_deactivate_fixup,
+		.callback = atkbd_skip_commands_fixup,
+		.driver_data = (void *)ATKBD_SKIP_DEACTIVATE,
 	},
 	{ }
 };
-- 
2.41.0

[PATCH 3/3] Input: atkbd - set skip_commands = ATKBD_SKIP_GETID for HP laptop 15s-fq* laptops

[Hans de Goede]

On "HP Laptop 15s-fq2xxx" and "HP laptop 15s-fq4xxx" laptops
the keyboard gets confused by Linux probing it and won't work
for the first 2 - 5 minutes or so (waiting for EC watchdog reset?).

Testing has shown that Linux sending ATKBD_CMD_GETID causes
the keyboard to not work for the first 2 - 5 minutes.

Add a quirk setting skip_commands = ATKBD_SKIP_GETID to fix this.

Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2086156
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/keyboard/atkbd.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 19a01e763871..7ebd686b61b3 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -1937,6 +1937,19 @@ static const struct dmi_system_id atkbd_dmi_quirk_table[] __initconst = {
 		.callback = atkbd_skip_commands_fixup,
 		.driver_data = (void *)ATKBD_SKIP_DEACTIVATE,
 	},
+	{
+		/*
+		 * "HP Laptop 15s-fq2xxx" and "HP laptop 15s-fq4xxx" both get
+		 * confused by ATKBD_CMD_GETID.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			/* Partial match to match both systems */
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP Laptop 15s-fq"),
+		},
+		.callback = atkbd_skip_commands_fixup,
+		.driver_data = (void *)ATKBD_SKIP_GETID,
+	},
 	{ }
 };
 
-- 
2.41.0

Re: [PATCH 0/3] Input: atkbd - add skip_commands module parameter

[Shang Ye]

Hi Hans,

I very much support the inclusion of this patch, because there has been
a similar keyboard issue on at least 3 (presumably 9) types of Lenovo
laptops, which may also be avoided by simply skipping the GETID command.
My patch and a list of the affected laptop types may be found at:
https://github.com/yescallop/atkbd-nogetid

In my last patch submission, I have included the issue details:
https://lore.kernel.org/linux-input/20230530131340.39961-1-yesh25@mail2.sysu.edu.cn/

There were also two other patch submissions aimed at enabling
`i8042.dumbkbd` on some HP laptops in order to avoid sending the GETID
command, which isn't very desirable because it breaks the Caps Lock LED:
https://lore.kernel.org/linux-input/2iAJTwqZV6lQs26cTb38RNYqxvsink6SRmrZ5h0cBUSuf9NT0tZTsf9fEAbbto2maavHJEOP8GA1evlKa6xjKOsaskDhtJWxjcnrgPigzVo=@gurevit.ch/
https://lore.kernel.org/linux-input/20210609073333.8425-1-egori@altlinux.org/

And another patch submisson aimed at fixing the issue generically,
which, sadly, did not work on my laptop because the GETID command would
trigger more errornous behaviours on it:
https://lore.kernel.org/linux-input/20210201160336.16008-1-anton@cpp.in/

I hope that these materials will help people better understand the
nature of the issue and the urgency to address it.

Below are some comments on the patch:

> +MODULE_PARM_DESC(skip_commands, "Bitfield where each bits skips a specific keyboard cmd (0 - 0x3f)");

"bits" -> "bit"?

I think we may also need to document the new module parameter at
Documentation/admin-guide/kernel-parameters.txt and clarify which bit
skips which keyboard command.

Lastly, would you think it is appropriate to include in this patch
series the quirks for Lenovo laptops on which my patch was tested to
work? If so, the quirk table entries would be:

System vendor: "LENOVO"
Product names: "82G2", "82NC", "82TK"
Driver data  : ATKBD_SKIP_GETID

Above all, thank you for working out this nice patch.

Regards,

Shang

On 2023/10/06 04:15, Hans de Goede wrote:
> Hi all,
> 
> While debugging a keyboard issue on some HP laptops adding i8042.dumbkbd
> helped to avoid the issue. So one of the commands send by atkbd.c seemed
> to be the culprit.
> 
> This series a skip_commands option to help debug cases like this by adding
> a bit-field which allows disabling a subset of the ps2_command()
> calls the atkbd driver makes.
> 
> It also replaces the existing atkbd_skip_deactivate flag
> with the new parameter and adds a DMI quirk for the HP laptops
> to avoid the keyboard issue there.
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (3):
>   Input: atkbd - add skip_commands module parameter
>   Input: atkbd - drop atkbd_skip_deactivate flag
>   Input: atkbd - set skip_commands = ATKBD_SKIP_GETID for HP laptop
>     15s-fq* laptops
> 
>  drivers/input/keyboard/atkbd.c | 88 ++++++++++++++++++++++++++--------
>  1 file changed, 69 insertions(+), 19 deletions(-)
> 
> -- 
> 2.41.0
>

Re: [PATCH 0/3] Input: atkbd - add skip_commands module parameter

[Hans de Goede]

Hi Shang,

On 10/17/23 15:53, Shang Ye wrote:
> Hi Hans,
> 
> I very much support the inclusion of this patch, because there has been
> a similar keyboard issue on at least 3 (presumably 9) types of Lenovo
> laptops, which may also be avoided by simply skipping the GETID command.
> My patch and a list of the affected laptop types may be found at:
> https://github.com/yescallop/atkbd-nogetid
> 
> In my last patch submission, I have included the issue details:
> https://lore.kernel.org/linux-input/20230530131340.39961-1-yesh25@mail2.sysu.edu.cn/
> 
> There were also two other patch submissions aimed at enabling
> `i8042.dumbkbd` on some HP laptops in order to avoid sending the GETID
> command, which isn't very desirable because it breaks the Caps Lock LED:
> https://lore.kernel.org/linux-input/2iAJTwqZV6lQs26cTb38RNYqxvsink6SRmrZ5h0cBUSuf9NT0tZTsf9fEAbbto2maavHJEOP8GA1evlKa6xjKOsaskDhtJWxjcnrgPigzVo=@gurevit.ch/
> https://lore.kernel.org/linux-input/20210609073333.8425-1-egori@altlinux.org/
> 
> And another patch submisson aimed at fixing the issue generically,
> which, sadly, did not work on my laptop because the GETID command would
> trigger more errornous behaviours on it:
> https://lore.kernel.org/linux-input/20210201160336.16008-1-anton@cpp.in/

Interesting, that might be the issue which is hitting the HP models
which I wrote this series for too.

> I hope that these materials will help people better understand the
> nature of the issue and the urgency to address it.
> 
> Below are some comments on the patch:
> 
>> +MODULE_PARM_DESC(skip_commands, "Bitfield where each bits skips a specific keyboard cmd (0 - 0x3f)");
> 
> "bits" -> "bit"?

Indeed, if we go with this patch-series this should be fixed.

> I think we may also need to document the new module parameter at
> Documentation/admin-guide/kernel-parameters.txt and clarify which bit
> skips which keyboard command.
> 
> Lastly, would you think it is appropriate to include in this patch
> series the quirks for Lenovo laptops on which my patch was tested to
> work? If so, the quirk table entries would be:
> 
> System vendor: "LENOVO"
> Product names: "82G2", "82NC", "82TK"
> Driver data  : ATKBD_SKIP_GETID

Looking at your github and seeing how many models are affected,
I'm thinking that we should maybe just skip the entire keyboard
atkbd_probe() when atkbd->translated is set.

The probe is really only necessary in the untranslated case
to check if there is a mouse there or if there is one of the
(quite old) special ps/2 keyboards there which have some
special handling (search for "id == 0x" to find the special
cases) these special cases are all only hit/valid when
(atkbd->translated == 0) is true, so when atkbd->translated
is true we can just skip the probe and use an assumed id of 0xab00
(already used when i8042.dumbkbd is set) and then immediately bail
from atkbd_probe(). I think this generic solution is a better
approach then any of the previous approaches since it is nice
and KISS and does not rely on any DMI quirks.

Regards,

Hans





> On 2023/10/06 04:15, Hans de Goede wrote:
>> Hi all,
>>
>> While debugging a keyboard issue on some HP laptops adding i8042.dumbkbd
>> helped to avoid the issue. So one of the commands send by atkbd.c seemed
>> to be the culprit.
>>
>> This series a skip_commands option to help debug cases like this by adding
>> a bit-field which allows disabling a subset of the ps2_command()
>> calls the atkbd driver makes.
>>
>> It also replaces the existing atkbd_skip_deactivate flag
>> with the new parameter and adds a DMI quirk for the HP laptops
>> to avoid the keyboard issue there.
>>
>> Regards,
>>
>> Hans
>>
>>
>> Hans de Goede (3):
>>   Input: atkbd - add skip_commands module parameter
>>   Input: atkbd - drop atkbd_skip_deactivate flag
>>   Input: atkbd - set skip_commands = ATKBD_SKIP_GETID for HP laptop
>>     15s-fq* laptops
>>
>>  drivers/input/keyboard/atkbd.c | 88 ++++++++++++++++++++++++++--------
>>  1 file changed, 69 insertions(+), 19 deletions(-)
>>
>> -- 
>> 2.41.0
>>
>