[PATCH regression fix 0/2] Input: atkbd - Fix Dell XPS 13 line suspend/resume regression

[PATCH regression fix 0/2] Input: atkbd - Fix Dell XPS 13 line suspend/resume regression

[Hans de Goede]

Hi Dmitry,

There have been multiple reports that the keyboard on
Dell XPS 13 9350 / 9360 / 9370 models has stopped working after
a suspend/resume after the merging of commit 936e4d49ecbc ("Input:
atkbd - skip ATKBD_CMD_GETID in translated mode").

See the 4 closes tags in the first patch for 4 reports of this.

I have been working with the first reporter on resolving this
and testing on his Dell XPS 13 9360 confirms that these patches
fix things.

Unfortunately the commit causing the issue has also been picked
up by multiple stable kernel series now. Can you please send
these fixes to Linus ASAP, so that they can also be backported
to the stable series ASAP ?

Alternatively we could revert the commit causing this, but that
commit is know to fix issues on a whole bunch of other laptops
so I would rather not revert it.

Regards,

Hans


Hans de Goede (2):
  Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
  Input: atkbd - Do not skip atkbd_deactivate() when skipping
    ATKBD_CMD_GETID

 drivers/input/keyboard/atkbd.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH regression fix 1/2] Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID

[Hans de Goede]

After commit 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in
translated mode") the keyboard on Dell XPS 13 9350 / 9360 / 9370 models
has stopped working after a suspend/resume.

The problem appears to be that atkbd_probe() fails when called
from atkbd_reconnect() on resume, which on systems where
ATKBD_CMD_GETID is skipped can only happen by ATKBD_CMD_SETLEDS
failing. ATKBD_CMD_SETLEDS failing because ATKBD_CMD_GETID was
skipped is weird, but apparently that is what is happening.

Fix this by also skipping ATKBD_CMD_SETLEDS when skipping
ATKBD_CMD_GETID.

Fixes: 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/linux-input/0aa4a61f-c939-46fe-a572-08022e8931c7@molgen.mpg.de/
Closes: https://bbs.archlinux.org/viewtopic.php?pid=2146300
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218424
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2260517
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/keyboard/atkbd.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 13ef6284223d..c229bd6b3f7f 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -811,7 +811,6 @@ static int atkbd_probe(struct atkbd *atkbd)
 {
 	struct ps2dev *ps2dev = &atkbd->ps2dev;
 	unsigned char param[2];
-	bool skip_getid;
 
 /*
  * Some systems, where the bit-twiddling when testing the io-lines of the
@@ -825,6 +824,11 @@ static int atkbd_probe(struct atkbd *atkbd)
 				 "keyboard reset failed on %s\n",
 				 ps2dev->serio->phys);
 
+	if (atkbd_skip_getid(atkbd)) {
+		atkbd->id = 0xab83;
+		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
@@ -833,18 +837,17 @@ static int atkbd_probe(struct atkbd *atkbd)
  */
 
 	param[0] = param[1] = 0xa5;	/* initialize with invalid values */
-	skip_getid = atkbd_skip_getid(atkbd);
-	if (skip_getid || ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
+	if (ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
 
 /*
- * If the get ID command was skipped or failed, we check if we can at least set
+ * If the get ID command failed, we check if we can at least set
  * the LEDs on the keyboard. This should work on every keyboard out there.
  * It also turns the LEDs off, which we want anyway.
  */
 		param[0] = 0;
 		if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
 			return -1;
-		atkbd->id = skip_getid ? 0xab83 : 0xabba;
+		atkbd->id = 0xabba;
 		return 0;
 	}
 
-- 
2.43.0

Re: [PATCH regression fix 1/2] Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID

[Hans de Goede]

Hi,

On 1/26/24 17:07, Hans de Goede wrote:
> After commit 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in
> translated mode") the keyboard on Dell XPS 13 9350 / 9360 / 9370 models
> has stopped working after a suspend/resume.
> 
> The problem appears to be that atkbd_probe() fails when called
> from atkbd_reconnect() on resume, which on systems where
> ATKBD_CMD_GETID is skipped can only happen by ATKBD_CMD_SETLEDS
> failing. ATKBD_CMD_SETLEDS failing because ATKBD_CMD_GETID was
> skipped is weird, but apparently that is what is happening.

Thinking more about it, what is likely happening here is that
ATKBD_CMD_SETLEDS is being send from atkbd_probe() where as
before atkbd_probe() would call ATKBD_CMD_GETID() and if that
succeeded (which it likely did) atkbd_probe() would continue with
calling atkbd_deactivate() and then exit, never calling
ATKBD_CMD_SETLEDS (at least not from atkbd_probe()).

So the problem seems to be that the embedded controller
does not like receiving ending ATKBD_CMD_SETLEDS as
the first command after resume and that being the first
command after resume is new behavior introduced by
936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")

After applying both patches from this set (which is what
Paul tested), the ATKBD_CMD_GETID will still be skipped
but instead of replacing it with a ATKBD_CMD_SETLEDS
atkbd_probe() now continues with calling atkbd_deactivate()
and then exits as before the recent changes.

So after applying both patches here the behavior change
compared to before 936e4d49ecbc is limited to just
skipping ATKBD_CMD_GETID rather then effectively
replacing it with ATKBD_CMD_SETLEDS.

Regards,

Hans






> 
> Fix this by also skipping ATKBD_CMD_SETLEDS when skipping
> ATKBD_CMD_GETID.
> 
> Fixes: 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Closes: https://lore.kernel.org/linux-input/0aa4a61f-c939-46fe-a572-08022e8931c7@molgen.mpg.de/
> Closes: https://bbs.archlinux.org/viewtopic.php?pid=2146300
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218424
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2260517
> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/keyboard/atkbd.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 13ef6284223d..c229bd6b3f7f 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -811,7 +811,6 @@ static int atkbd_probe(struct atkbd *atkbd)
>  {
>  	struct ps2dev *ps2dev = &atkbd->ps2dev;
>  	unsigned char param[2];
> -	bool skip_getid;
>  
>  /*
>   * Some systems, where the bit-twiddling when testing the io-lines of the
> @@ -825,6 +824,11 @@ static int atkbd_probe(struct atkbd *atkbd)
>  				 "keyboard reset failed on %s\n",
>  				 ps2dev->serio->phys);
>  
> +	if (atkbd_skip_getid(atkbd)) {
> +		atkbd->id = 0xab83;
> +		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
> @@ -833,18 +837,17 @@ static int atkbd_probe(struct atkbd *atkbd)
>   */
>  
>  	param[0] = param[1] = 0xa5;	/* initialize with invalid values */
> -	skip_getid = atkbd_skip_getid(atkbd);
> -	if (skip_getid || ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
> +	if (ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
>  
>  /*
> - * If the get ID command was skipped or failed, we check if we can at least set
> + * If the get ID command failed, we check if we can at least set
>   * the LEDs on the keyboard. This should work on every keyboard out there.
>   * It also turns the LEDs off, which we want anyway.
>   */
>  		param[0] = 0;
>  		if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
>  			return -1;
> -		atkbd->id = skip_getid ? 0xab83 : 0xabba;
> +		atkbd->id = 0xabba;
>  		return 0;
>  	}
>  

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

[Hans de Goede]

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

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

[Dmitry Torokhov]

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.

Thanks.

-- 
Dmitry

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

[Hans de Goede]

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

Re: [PATCH regression fix 0/2] Input: atkbd - Fix Dell XPS 13 line suspend/resume regression

[Linux regression tracking (Thorsten Leemhuis)]

On 26.01.24 17:07, Hans de Goede wrote:
> Hi Dmitry,
> 
> There have been multiple reports that the keyboard on
> Dell XPS 13 9350 / 9360 / 9370 models has stopped working after
> a suspend/resume after the merging of commit 936e4d49ecbc ("Input:
> atkbd - skip ATKBD_CMD_GETID in translated mode").
> 
> See the 4 closes tags in the first patch for 4 reports of this.
> 
> I have been working with the first reporter on resolving this
> and testing on his Dell XPS 13 9360 confirms that these patches
> fix things.
> 
> Unfortunately the commit causing the issue has also been picked
> up by multiple stable kernel series now. Can you please send
> these fixes to Linus ASAP, so that they can also be backported
> to the stable series ASAP ?

Dmitry,  Hans, what's the status here? I wonder if there is still a
chance to get this into -rc3 so that Greg can fix the affected stable
trees as well next week or so.

Ciao, Thorsten

> Hans de Goede (2):
>   Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
>   Input: atkbd - Do not skip atkbd_deactivate() when skipping
>     ATKBD_CMD_GETID
> 
>  drivers/input/keyboard/atkbd.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 

Re: [PATCH regression fix 0/2] Input: atkbd - Fix Dell XPS 13 line suspend/resume regression

[Hans de Goede]

Hi,

On 2/1/24 12:12, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 26.01.24 17:07, Hans de Goede wrote:
>> Hi Dmitry,
>>
>> There have been multiple reports that the keyboard on
>> Dell XPS 13 9350 / 9360 / 9370 models has stopped working after
>> a suspend/resume after the merging of commit 936e4d49ecbc ("Input:
>> atkbd - skip ATKBD_CMD_GETID in translated mode").
>>
>> See the 4 closes tags in the first patch for 4 reports of this.
>>
>> I have been working with the first reporter on resolving this
>> and testing on his Dell XPS 13 9360 confirms that these patches
>> fix things.
>>
>> Unfortunately the commit causing the issue has also been picked
>> up by multiple stable kernel series now. Can you please send
>> these fixes to Linus ASAP, so that they can also be backported
>> to the stable series ASAP ?
> 
> Dmitry,  Hans, what's the status here? I wonder if there is still a
> chance to get this into -rc3 so that Greg can fix the affected stable
> trees as well next week or so.

From my pov these are ready to go. I'm waiting for feedback
from Dmitry on these.

Regards,

Hans

Re: [PATCH regression fix 0/2] Input: atkbd - Fix Dell XPS 13 line suspend/resume regression

[Dmitry Torokhov]

On Fri, Jan 26, 2024 at 05:07:22PM +0100, Hans de Goede wrote:
> Hi Dmitry,
> 
> There have been multiple reports that the keyboard on
> Dell XPS 13 9350 / 9360 / 9370 models has stopped working after
> a suspend/resume after the merging of commit 936e4d49ecbc ("Input:
> atkbd - skip ATKBD_CMD_GETID in translated mode").
> 
> See the 4 closes tags in the first patch for 4 reports of this.
> 
> I have been working with the first reporter on resolving this
> and testing on his Dell XPS 13 9360 confirms that these patches
> fix things.
> 
> Unfortunately the commit causing the issue has also been picked
> up by multiple stable kernel series now. Can you please send
> these fixes to Linus ASAP, so that they can also be backported
> to the stable series ASAP ?
> 
> Alternatively we could revert the commit causing this, but that
> commit is know to fix issues on a whole bunch of other laptops
> so I would rather not revert it.
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (2):
>   Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
>   Input: atkbd - Do not skip atkbd_deactivate() when skipping
>     ATKBD_CMD_GETID

Applied the lot to 'for-linus' branch, I will get it into the next push.

Thanks.

-- 
Dmitry