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

[Hans de Goede <hdegoede@redhat.com>]

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
>>
>