Re: [PATCH regression fix 2/2] Input: atkbd - Do not skip atkbd_deactivate() when skipping ATKBD_CMD_GETID

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

Hi Dmitry,

Thank you for picking up these fixes and
sorry about the breakage.

On 2/2/24 05:56, Dmitry Torokhov wrote:
> On Fri, Jan 26, 2024 at 05:07:24PM +0100, Hans de Goede wrote:
>> After commit 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in
>> translated mode") not only the getid command is skipped, but also
>> the de-activating of the keyboard at the end of atkbd_probe(), potentially
>> re-introducing the problem fixed by commit be2d7e4233a4 ("Input: atkbd -
>> fix multi-byte scancode handling on reconnect").
>>
>> Make sure multi-byte scancode handling on reconnect is still handled
>> correctly by not skipping the atkbd_deactivate() call.
>>
>> Fixes: 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
>> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/input/keyboard/atkbd.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
>> index c229bd6b3f7f..7f67f9f2946b 100644
>> --- a/drivers/input/keyboard/atkbd.c
>> +++ b/drivers/input/keyboard/atkbd.c
>> @@ -826,7 +826,7 @@ static int atkbd_probe(struct atkbd *atkbd)
>>  
>>  	if (atkbd_skip_getid(atkbd)) {
>>  		atkbd->id = 0xab83;
>> -		return 0;
>> +		goto deactivate_kbd;
>>  	}
>>  
>>  /*
>> @@ -863,6 +863,7 @@ static int atkbd_probe(struct atkbd *atkbd)
>>  		return -1;
>>  	}
>>  
>> +deactivate_kbd:
>>  /*
>>   * Make sure nothing is coming from the keyboard and disturbs our
>>   * internal state.
> 
> I wonder if we need to do the same for the case when we go into SET LEDS
> branch... This can be done in a separate patch though.

Right my goal with this series was to make the behavior change from
936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
as small as possible (just skip ATKBD_CMD_GETID and no other behavior
change like calling SET_LEDS).

I'm not sure about doing the same as this patch for the SET_LEDS
path. We only hit that path if GETID fails which means we are
already dealing with quirky hardware and we already have quirks
to skip the deactivate command for some keyboards which don't
like it...

Let me know if you still want to give making the SET_LEDS
path consistent with the others a go and have it call deactive
too. IMHO that would only be something for -next though,
so that it gets the maximum amount of testing time.

Regards,

Hans