| Message ID | 20240126160724.13278-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Input: atkbd - Fix Dell XPS 13 line suspend/resume regression | expand |
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.
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
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.
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(-)