[PATCH] Input: atkbd - fix LED state at suspend/resume

[PATCH] Input: atkbd - fix LED state at suspend/resume

[Qiang Ma]

After we turn on the keyboard CAPSL LED and let the system suspend,
the keyboard LED is not off, and the kernel log is as follows:

[  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
[  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
[  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
[  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)

The log shows that after the command 0xed is sent, the data
sent is 0x04 instead of 0x00.

Solution:
Add a bitmap variable ledon in the atkbd structure, and then set ledon
according to code-value in the event, in the atkbd_set_leds function,
first look at the value of lenon, if it is 0, there is no need to
look at the value of dev->led, otherwise, Need to look at dev->led
to determine the keyboard LED on/off.

Signed-off-by: Qiang Ma <maqianga@uniontech.com>
---
 drivers/input/keyboard/atkbd.c | 35 +++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 7f67f9f2946b..27d8f375929e 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -237,6 +237,8 @@ struct atkbd {
 	struct mutex mutex;
 
 	struct vivaldi_data vdata;
+
+	unsigned long ledon[BITS_TO_LONGS(LED_CNT)];
 };
 
 /*
@@ -604,24 +606,34 @@ static int atkbd_set_repeat_rate(struct atkbd *atkbd)
 	return ps2_command(&atkbd->ps2dev, &param, ATKBD_CMD_SETREP);
 }
 
+#define ATKBD_DO_LED_TOGGLE(dev, atkbd, type, v, bits, on)		\
+({									\
+	unsigned char __tmp = 0;					\
+	if (test_bit(LED_##type, atkbd->on))				\
+		__tmp = test_bit(LED_##type, dev->bits) ? v : 0;	\
+	else								\
+		__tmp = 0;						\
+	__tmp;								\
+})
+
 static int atkbd_set_leds(struct atkbd *atkbd)
 {
 	struct input_dev *dev = atkbd->dev;
-	unsigned char param[2];
+	unsigned char param[2] = {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);
+	param[0] = ATKBD_DO_LED_TOGGLE(dev, atkbd, SCROLLL, 1, led, ledon)
+		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, NUML,    2, led, ledon)
+		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, CAPSL,   4, led, ledon);
 	if (ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_SETLEDS))
 		return -1;
 
 	if (atkbd->extra) {
 		param[0] = 0;
-		param[1] = (test_bit(LED_COMPOSE, dev->led) ? 0x01 : 0)
-			 | (test_bit(LED_SLEEP,   dev->led) ? 0x02 : 0)
-			 | (test_bit(LED_SUSPEND, dev->led) ? 0x04 : 0)
-			 | (test_bit(LED_MISC,    dev->led) ? 0x10 : 0)
-			 | (test_bit(LED_MUTE,    dev->led) ? 0x20 : 0);
+		param[1] = ATKBD_DO_LED_TOGGLE(dev, atkbd, COMPOSE, 0x01, led, ledon)
+			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, SLEEP,   0x02, led, ledon)
+			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, SUSPEND, 0x04, led, ledon)
+			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, MISC,    0x10, led, ledon)
+			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, MUTE,    0x20, led, ledon);
 		if (ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_EX_SETLEDS))
 			return -1;
 	}
@@ -695,6 +707,11 @@ static int atkbd_event(struct input_dev *dev,
 	switch (type) {
 
 	case EV_LED:
+		if (value)
+			__set_bit(code, atkbd->ledon);
+		else
+			__clear_bit(code, atkbd->ledon);
+
 		atkbd_schedule_event_work(atkbd, ATKBD_LED_EVENT_BIT);
 		return 0;
 
-- 
2.20.1

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Qiang Ma]

On Fri, 26 Jul 2024 18:27:30 +0800
Qiang Ma <maqianga@uniontech.com> wrote:

Sorry, add the correct email address: linux-input@vger.kernel.org

> After we turn on the keyboard CAPSL LED and let the system suspend,
> the keyboard LED is not off, and the kernel log is as follows:
> 
> [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> 
> The log shows that after the command 0xed is sent, the data
> sent is 0x04 instead of 0x00.
> 
> Solution:
> Add a bitmap variable ledon in the atkbd structure, and then set ledon
> according to code-value in the event, in the atkbd_set_leds function,
> first look at the value of lenon, if it is 0, there is no need to
> look at the value of dev->led, otherwise, Need to look at dev->led
> to determine the keyboard LED on/off.
> 
> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> ---
>  drivers/input/keyboard/atkbd.c | 35
> +++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+),
> 9 deletions(-)
> 
> diff --git a/drivers/input/keyboard/atkbd.c
> b/drivers/input/keyboard/atkbd.c index 7f67f9f2946b..27d8f375929e
> 100644 --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -237,6 +237,8 @@ struct atkbd {
>  	struct mutex mutex;
>  
>  	struct vivaldi_data vdata;
> +
> +	unsigned long ledon[BITS_TO_LONGS(LED_CNT)];
>  };
>  
>  /*
> @@ -604,24 +606,34 @@ static int atkbd_set_repeat_rate(struct atkbd
> *atkbd) return ps2_command(&atkbd->ps2dev, &param, ATKBD_CMD_SETREP);
>  }
>  
> +#define ATKBD_DO_LED_TOGGLE(dev, atkbd, type, v, bits,
> on)		\
> +({
> \
> +	unsigned char __tmp =
> 0;					\
> +	if (test_bit(LED_##type,
> atkbd->on))				\
> +		__tmp = test_bit(LED_##type, dev->bits) ? v :
> 0;	\
> +
> else								\
> +		__tmp =
> 0;						\
> +
> __tmp;
> \ +}) +
>  static int atkbd_set_leds(struct atkbd *atkbd)
>  {
>  	struct input_dev *dev = atkbd->dev;
> -	unsigned char param[2];
> +	unsigned char param[2] = {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);
> +	param[0] = ATKBD_DO_LED_TOGGLE(dev, atkbd, SCROLLL, 1, led,
> ledon)
> +		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, NUML,    2, led,
> ledon)
> +		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, CAPSL,   4, led,
> ledon); if (ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_SETLEDS))
>  		return -1;
>  
>  	if (atkbd->extra) {
>  		param[0] = 0;
> -		param[1] = (test_bit(LED_COMPOSE, dev->led) ? 0x01 :
> 0)
> -			 | (test_bit(LED_SLEEP,   dev->led) ? 0x02 :
> 0)
> -			 | (test_bit(LED_SUSPEND, dev->led) ? 0x04 :
> 0)
> -			 | (test_bit(LED_MISC,    dev->led) ? 0x10 :
> 0)
> -			 | (test_bit(LED_MUTE,    dev->led) ? 0x20 :
> 0);
> +		param[1] = ATKBD_DO_LED_TOGGLE(dev, atkbd, COMPOSE,
> 0x01, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, SLEEP,
> 0x02, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, SUSPEND,
> 0x04, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, MISC,
> 0x10, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, MUTE,
> 0x20, led, ledon); if (ps2_command(&atkbd->ps2dev, param,
> ATKBD_CMD_EX_SETLEDS)) return -1;
>  	}
> @@ -695,6 +707,11 @@ static int atkbd_event(struct input_dev *dev,
>  	switch (type) {
>  
>  	case EV_LED:
> +		if (value)
> +			__set_bit(code, atkbd->ledon);
> +		else
> +			__clear_bit(code, atkbd->ledon);
> +
>  		atkbd_schedule_event_work(atkbd,
> ATKBD_LED_EVENT_BIT); return 0;
>  

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Qiang Ma]

On Fri, 26 Jul 2024 18:27:30 +0800
Qiang Ma <maqianga@uniontech.com> wrote:

Sorry, add the correct email address: linux-input@vger.kernel.org

> After we turn on the keyboard CAPSL LED and let the system suspend,
> the keyboard LED is not off, and the kernel log is as follows:
> 
> [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> 
> The log shows that after the command 0xed is sent, the data
> sent is 0x04 instead of 0x00.
> 
> Solution:
> Add a bitmap variable ledon in the atkbd structure, and then set ledon
> according to code-value in the event, in the atkbd_set_leds function,
> first look at the value of lenon, if it is 0, there is no need to
> look at the value of dev->led, otherwise, Need to look at dev->led
> to determine the keyboard LED on/off.
> 
> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> ---
>  drivers/input/keyboard/atkbd.c | 35
> +++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+),
> 9 deletions(-)
> 
> diff --git a/drivers/input/keyboard/atkbd.c
> b/drivers/input/keyboard/atkbd.c index 7f67f9f2946b..27d8f375929e
> 100644 --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -237,6 +237,8 @@ struct atkbd {
>  	struct mutex mutex;
>  
>  	struct vivaldi_data vdata;
> +
> +	unsigned long ledon[BITS_TO_LONGS(LED_CNT)];
>  };
>  
>  /*
> @@ -604,24 +606,34 @@ static int atkbd_set_repeat_rate(struct atkbd
> *atkbd) return ps2_command(&atkbd->ps2dev, &param, ATKBD_CMD_SETREP);
>  }
>  
> +#define ATKBD_DO_LED_TOGGLE(dev, atkbd, type, v, bits,
> on)		\
> +({
> \
> +	unsigned char __tmp =
> 0;					\
> +	if (test_bit(LED_##type,
> atkbd->on))				\
> +		__tmp = test_bit(LED_##type, dev->bits) ? v :
> 0;	\
> +
> else								\
> +		__tmp =
> 0;						\
> +
> __tmp;
> \ +}) +
>  static int atkbd_set_leds(struct atkbd *atkbd)
>  {
>  	struct input_dev *dev = atkbd->dev;
> -	unsigned char param[2];
> +	unsigned char param[2] = {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);
> +	param[0] = ATKBD_DO_LED_TOGGLE(dev, atkbd, SCROLLL, 1, led,
> ledon)
> +		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, NUML,    2, led,
> ledon)
> +		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, CAPSL,   4, led,
> ledon); if (ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_SETLEDS))
>  		return -1;
>  
>  	if (atkbd->extra) {
>  		param[0] = 0;
> -		param[1] = (test_bit(LED_COMPOSE, dev->led) ? 0x01 :
> 0)
> -			 | (test_bit(LED_SLEEP,   dev->led) ? 0x02 :
> 0)
> -			 | (test_bit(LED_SUSPEND, dev->led) ? 0x04 :
> 0)
> -			 | (test_bit(LED_MISC,    dev->led) ? 0x10 :
> 0)
> -			 | (test_bit(LED_MUTE,    dev->led) ? 0x20 :
> 0);
> +		param[1] = ATKBD_DO_LED_TOGGLE(dev, atkbd, COMPOSE,
> 0x01, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, SLEEP,
> 0x02, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, SUSPEND,
> 0x04, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, MISC,
> 0x10, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, MUTE,
> 0x20, led, ledon); if (ps2_command(&atkbd->ps2dev, param,
> ATKBD_CMD_EX_SETLEDS)) return -1;
>  	}
> @@ -695,6 +707,11 @@ static int atkbd_event(struct input_dev *dev,
>  	switch (type) {
>  
>  	case EV_LED:
> +		if (value)
> +			__set_bit(code, atkbd->ledon);
> +		else
> +			__clear_bit(code, atkbd->ledon);
> +
>  		atkbd_schedule_event_work(atkbd,
> ATKBD_LED_EVENT_BIT); return 0;
>  

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Christophe JAILLET]

Le 26/07/2024 à 12:27, Qiang Ma a écrit :
> After we turn on the keyboard CAPSL LED and let the system suspend,
> the keyboard LED is not off, and the kernel log is as follows:
> 
> [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> 
> The log shows that after the command 0xed is sent, the data
> sent is 0x04 instead of 0x00.
> 
> Solution:
> Add a bitmap variable ledon in the atkbd structure, and then set ledon
> according to code-value in the event, in the atkbd_set_leds function,

Hi,

s/atkbd_set_leds function/atkbd_set_leds()/

> first look at the value of lenon, if it is 0, there is no need to

s/lenon/ledon/

> look at the value of dev->led, otherwise, Need to look at dev->led

s/Need/need/

> to determine the keyboard LED on/off.
> 
> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> ---
>   drivers/input/keyboard/atkbd.c | 35 +++++++++++++++++++++++++---------
>   1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 7f67f9f2946b..27d8f375929e 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -237,6 +237,8 @@ struct atkbd {
>   	struct mutex mutex;
>   
>   	struct vivaldi_data vdata;
> +
> +	unsigned long ledon[BITS_TO_LONGS(LED_CNT)];

DECLARE_BITMAP()?

>   };
>   
>   /*
> @@ -604,24 +606,34 @@ static int atkbd_set_repeat_rate(struct atkbd *atkbd)
>   	return ps2_command(&atkbd->ps2dev, &param, ATKBD_CMD_SETREP);
>   }
>   
> +#define ATKBD_DO_LED_TOGGLE(dev, atkbd, type, v, bits, on)		\
> +({									\
> +	unsigned char __tmp = 0;					\
> +	if (test_bit(LED_##type, atkbd->on))				\
> +		__tmp = test_bit(LED_##type, dev->bits) ? v : 0;	\
> +	else								\
> +		__tmp = 0;						\

Is it needed? __tmp is already initialized as 0.

> +	__tmp;								\
> +})
> +
>   static int atkbd_set_leds(struct atkbd *atkbd)
>   {
>   	struct input_dev *dev = atkbd->dev;
> -	unsigned char param[2];
> +	unsigned char param[2] = {0};

This extra initialization does not seem necessary. IIUC, the length to 
sent is already encoded in ATKBD_CMD_SETLEDS and ATKBD_CMD_EX_SETLEDS.

>   
> -	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);
> +	param[0] = ATKBD_DO_LED_TOGGLE(dev, atkbd, SCROLLL, 1, led, ledon)
> +		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, NUML,    2, led, ledon)
> +		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, CAPSL,   4, led, ledon);
>   	if (ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_SETLEDS))
>   		return -1;
>   
>   	if (atkbd->extra) {
>   		param[0] = 0;
> -		param[1] = (test_bit(LED_COMPOSE, dev->led) ? 0x01 : 0)
> -			 | (test_bit(LED_SLEEP,   dev->led) ? 0x02 : 0)
> -			 | (test_bit(LED_SUSPEND, dev->led) ? 0x04 : 0)
> -			 | (test_bit(LED_MISC,    dev->led) ? 0x10 : 0)
> -			 | (test_bit(LED_MUTE,    dev->led) ? 0x20 : 0);
> +		param[1] = ATKBD_DO_LED_TOGGLE(dev, atkbd, COMPOSE, 0x01, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, SLEEP,   0x02, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, SUSPEND, 0x04, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, MISC,    0x10, led, ledon)
> +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd, MUTE,    0x20, led, ledon);
>   		if (ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_EX_SETLEDS))
>   			return -1;
>   	}
> @@ -695,6 +707,11 @@ static int atkbd_event(struct input_dev *dev,
>   	switch (type) {
>   
>   	case EV_LED:
> +		if (value)
> +			__set_bit(code, atkbd->ledon);
> +		else
> +			__clear_bit(code, atkbd->ledon);
> +
>   		atkbd_schedule_event_work(atkbd, ATKBD_LED_EVENT_BIT);
>   		return 0;
>   

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Dmitry Torokhov]

Hi Qiang,

On Fri, Jul 26, 2024 at 06:27:30PM +0800, Qiang Ma wrote:
> After we turn on the keyboard CAPSL LED and let the system suspend,
> the keyboard LED is not off, and the kernel log is as follows:
> 
> [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> 
> The log shows that after the command 0xed is sent, the data
> sent is 0x04 instead of 0x00.
> 
> Solution:
> Add a bitmap variable ledon in the atkbd structure, and then set ledon
> according to code-value in the event, in the atkbd_set_leds function,
> first look at the value of lenon, if it is 0, there is no need to
> look at the value of dev->led, otherwise, Need to look at dev->led
> to determine the keyboard LED on/off.

I am not sure why duplicating input_dev->led which is supposed to record
which LEDs are currently active on an input device would solve the
issue. Could you please explain?

The input core is supposed to turn off all LEDs on suspend. This happens
in input_dev_toggle() which is called from input_dev_suspend(). It
iterates over all LEDs on a device and turns off active ones one by
one.

I think what happens here is we are running afoul of the throttling done
in atkbd (see atkbd_schedule_event_work), and it does not actually turn
off all LEDs in time. But on the other hand atkbd_cleanup() (which is
called to suspend the keyboard) calls

	ps2_command(&atkbd->ps2dev, NULL, ATKBD_CMD_RESET_DEF);

which should turn off everything anyways.

I think we need better understand what is going on here. Maybe post the
entire communication between the kernel and i8042?

Thanks.

-- 
Dmitry

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Dmitry Torokhov]

On Fri, Jul 26, 2024 at 02:57:03PM -0700, Dmitry Torokhov wrote:
> Hi Qiang,
> 
> On Fri, Jul 26, 2024 at 06:27:30PM +0800, Qiang Ma wrote:
> > After we turn on the keyboard CAPSL LED and let the system suspend,
> > the keyboard LED is not off, and the kernel log is as follows:
> > 
> > [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> > [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> > [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > 
> > The log shows that after the command 0xed is sent, the data
> > sent is 0x04 instead of 0x00.
> > 
> > Solution:
> > Add a bitmap variable ledon in the atkbd structure, and then set ledon
> > according to code-value in the event, in the atkbd_set_leds function,
> > first look at the value of lenon, if it is 0, there is no need to
> > look at the value of dev->led, otherwise, Need to look at dev->led
> > to determine the keyboard LED on/off.
> 
> I am not sure why duplicating input_dev->led which is supposed to record
> which LEDs are currently active on an input device would solve the
> issue. Could you please explain?

Ah, OK, I see it now. We do not actually toggle input_dev->led when
suspending, so atkbd uses wrong data to determine the LED state.

> 
> The input core is supposed to turn off all LEDs on suspend. This happens
> in input_dev_toggle() which is called from input_dev_suspend(). It
> iterates over all LEDs on a device and turns off active ones one by
> one.
> 
> I think what happens here is we are running afoul of the throttling done
> in atkbd (see atkbd_schedule_event_work), and it does not actually turn
> off all LEDs in time. But on the other hand atkbd_cleanup() (which is
> called to suspend the keyboard) calls
> 
> 	ps2_command(&atkbd->ps2dev, NULL, ATKBD_CMD_RESET_DEF);
> 
> which should turn off everything anyways.

But still, why ATKBD_CMD_RESET_DEF does not shut off the LEDs for you?

Thanks.

-- 
Dmitry

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Qiang Ma]

On Fri, 26 Jul 2024 14:57:03 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

Hi Dmitry,

> Hi Qiang,
> 
> On Fri, Jul 26, 2024 at 06:27:30PM +0800, Qiang Ma wrote:
> > After we turn on the keyboard CAPSL LED and let the system suspend,
> > the keyboard LED is not off, and the kernel log is as follows:
> > 
> > [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> > [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> > [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > 
> > The log shows that after the command 0xed is sent, the data
> > sent is 0x04 instead of 0x00.
> > 
> > Solution:
> > Add a bitmap variable ledon in the atkbd structure, and then set
> > ledon according to code-value in the event, in the atkbd_set_leds
> > function, first look at the value of lenon, if it is 0, there is no
> > need to look at the value of dev->led, otherwise, Need to look at
> > dev->led to determine the keyboard LED on/off.  
> 
> I am not sure why duplicating input_dev->led which is supposed to
> record which LEDs are currently active on an input device would solve
> the issue. Could you please explain?

At this point, the suspend purpose is to turn off the led(calling
input_dev_toggle(input_dev, false) in function input_dev_suspend()),
but the dev->led gets the status is on, so it will not turn off the led.

> 
> The input core is supposed to turn off all LEDs on suspend. This
> happens in input_dev_toggle() which is called from
> input_dev_suspend(). It iterates over all LEDs on a device and turns
> off active ones one by one.
> 
> I think what happens here is we are running afoul of the throttling
> done in atkbd (see atkbd_schedule_event_work), and it does not
> actually turn off all LEDs in time. But on the other hand
> atkbd_cleanup() (which is called to suspend the keyboard) calls
> 
> 	ps2_command(&atkbd->ps2dev, NULL, ATKBD_CMD_RESET_DEF);
> 
> which should turn off everything anyways.

Send reset command under the actual test did not turn off the led, get
it just reset the i8042 register? Did not change the led corresponding
gpio status?

> 
> I think we need better understand what is going on here. Maybe post
> the entire communication between the kernel and i8042?
> 
> Thanks.
> 

---
Qiang

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Qiang Ma]

On Fri, 26 Jul 2024 16:43:54 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> Le 26/07/2024 à 12:27, Qiang Ma a écrit :
> > After we turn on the keyboard CAPSL LED and let the system suspend,
> > the keyboard LED is not off, and the kernel log is as follows:
> > 
> > [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> > [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> > [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > 
> > The log shows that after the command 0xed is sent, the data
> > sent is 0x04 instead of 0x00.
> > 
> > Solution:
> > Add a bitmap variable ledon in the atkbd structure, and then set
> > ledon according to code-value in the event, in the atkbd_set_leds
> > function,  
> 
> Hi,
> 
> s/atkbd_set_leds function/atkbd_set_leds()/
> 
> > first look at the value of lenon, if it is 0, there is no need to  
> 
> s/lenon/ledon/
> 
> > look at the value of dev->led, otherwise, Need to look at dev->led  
> 
> s/Need/need/
> 

Thank you very much for pointing it out, I will fix it in the V2 patch
version.

> > to determine the keyboard LED on/off.
> > 
> > Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> > ---
> >   drivers/input/keyboard/atkbd.c | 35
> > +++++++++++++++++++++++++--------- 1 file changed, 26
> > insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/input/keyboard/atkbd.c
> > b/drivers/input/keyboard/atkbd.c index 7f67f9f2946b..27d8f375929e
> > 100644 --- a/drivers/input/keyboard/atkbd.c
> > +++ b/drivers/input/keyboard/atkbd.c
> > @@ -237,6 +237,8 @@ struct atkbd {
> >   	struct mutex mutex;
> >   
> >   	struct vivaldi_data vdata;
> > +
> > +	unsigned long ledon[BITS_TO_LONGS(LED_CNT)];  
> 
> DECLARE_BITMAP()?
> 
Refer to dev->led, instead of calling DECLARE_BITMAP(), consider using
it.
> >   };
> >   
> >   /*
> > @@ -604,24 +606,34 @@ static int atkbd_set_repeat_rate(struct atkbd
> > *atkbd) return ps2_command(&atkbd->ps2dev, &param,
> > ATKBD_CMD_SETREP); }
> >   
> > +#define ATKBD_DO_LED_TOGGLE(dev, atkbd, type, v, bits,
> > on)		\
> > +({
> > \
> > +	unsigned char __tmp =
> > 0;					\
> > +	if (test_bit(LED_##type,
> > atkbd->on))				\
> > +		__tmp = test_bit(LED_##type, dev->bits) ? v :
> > 0;	\
> > +
> > else
> > \
> > +		__tmp =
> > 0;						\  
> 
> Is it needed? __tmp is already initialized as 0.

It's really not necessary.

> 
> > +
> > __tmp;
> > \ +}) +
> >   static int atkbd_set_leds(struct atkbd *atkbd)
> >   {
> >   	struct input_dev *dev = atkbd->dev;
> > -	unsigned char param[2];
> > +	unsigned char param[2] = {0};  
> 
> This extra initialization does not seem necessary. IIUC, the length
> to sent is already encoded in ATKBD_CMD_SETLEDS and
> ATKBD_CMD_EX_SETLEDS.
> 

Looking again at the ps2_command function, yes, the length to sent is
already encoded in ATKBD_CMD_SETLEDS and ATKBD_CMD_EX_SETLEDS.

> >   
> > -	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);
> > +	param[0] = ATKBD_DO_LED_TOGGLE(dev, atkbd, SCROLLL, 1,
> > led, ledon)
> > +		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, NUML,    2,
> > led, ledon)
> > +		 | ATKBD_DO_LED_TOGGLE(dev, atkbd, CAPSL,   4,
> > led, ledon); if (ps2_command(&atkbd->ps2dev, param,
> > ATKBD_CMD_SETLEDS)) return -1;
> >   
> >   	if (atkbd->extra) {
> >   		param[0] = 0;
> > -		param[1] = (test_bit(LED_COMPOSE, dev->led) ?
> > 0x01 : 0)
> > -			 | (test_bit(LED_SLEEP,   dev->led) ?
> > 0x02 : 0)
> > -			 | (test_bit(LED_SUSPEND, dev->led) ?
> > 0x04 : 0)
> > -			 | (test_bit(LED_MISC,    dev->led) ?
> > 0x10 : 0)
> > -			 | (test_bit(LED_MUTE,    dev->led) ?
> > 0x20 : 0);
> > +		param[1] = ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > COMPOSE, 0x01, led, ledon)
> > +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > SLEEP,   0x02, led, ledon)
> > +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > SUSPEND, 0x04, led, ledon)
> > +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > MISC,    0x10, led, ledon)
> > +			 | ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > MUTE,    0x20, led, ledon); if (ps2_command(&atkbd->ps2dev, param,
> > ATKBD_CMD_EX_SETLEDS)) return -1;
> >   	}
> > @@ -695,6 +707,11 @@ static int atkbd_event(struct input_dev *dev,
> >   	switch (type) {
> >   
> >   	case EV_LED:
> > +		if (value)
> > +			__set_bit(code, atkbd->ledon);
> > +		else
> > +			__clear_bit(code, atkbd->ledon);
> > +
> >   		atkbd_schedule_event_work(atkbd,
> > ATKBD_LED_EVENT_BIT); return 0;
> >     
> 
> 

Thank review, I will fix them in the v2 patch version.

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Qiang Ma]

On Tue, 30 Jul 2024 10:17:08 +0800
Qiang Ma <maqianga@uniontech.com> wrote:

> On Fri, 26 Jul 2024 14:57:03 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> Hi Dmitry,
> 
> > Hi Qiang,
> > 
> > On Fri, Jul 26, 2024 at 06:27:30PM +0800, Qiang Ma wrote:  
> > > After we turn on the keyboard CAPSL LED and let the system
> > > suspend, the keyboard LED is not off, and the kernel log is as
> > > follows:
> > > 
> > > [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> > > [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > > [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> > > [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > > 
> > > The log shows that after the command 0xed is sent, the data
> > > sent is 0x04 instead of 0x00.
> > > 
> > > Solution:
> > > Add a bitmap variable ledon in the atkbd structure, and then set
> > > ledon according to code-value in the event, in the atkbd_set_leds
> > > function, first look at the value of lenon, if it is 0, there is
> > > no need to look at the value of dev->led, otherwise, Need to look
> > > at dev->led to determine the keyboard LED on/off.    
> > 
> > I am not sure why duplicating input_dev->led which is supposed to
> > record which LEDs are currently active on an input device would
> > solve the issue. Could you please explain?  
> 
> At this point, the suspend purpose is to turn off the led(calling
> input_dev_toggle(input_dev, false) in function input_dev_suspend()),
> but the dev->led gets the status is on, so it will not turn off the
> led.
> 
> > 
> > The input core is supposed to turn off all LEDs on suspend. This
> > happens in input_dev_toggle() which is called from
> > input_dev_suspend(). It iterates over all LEDs on a device and turns
> > off active ones one by one.
> > 
> > I think what happens here is we are running afoul of the throttling
> > done in atkbd (see atkbd_schedule_event_work), and it does not
> > actually turn off all LEDs in time. But on the other hand
> > atkbd_cleanup() (which is called to suspend the keyboard) calls
> > 
> > 	ps2_command(&atkbd->ps2dev, NULL, ATKBD_CMD_RESET_DEF);
> > 
> > which should turn off everything anyways.  
> 
> Send reset command under the actual test did not turn off the led, get
> it just reset the i8042 register? Did not change the led corresponding
> gpio status?
> 
> > 
> > I think we need better understand what is going on here. Maybe post
> > the entire communication between the kernel and i8042?
> > 

Description of ps/2 keyboard interface on this
website(http://www-ug.eecg.toronto.edu/msl/nios_devices/datasheets/PS2%20Keyboard%20Protocol.htm):

0xF6 (Set Default) - Load default typematic rate/delay (10.9cps /
500ms), key types (all keys typematic/make/break), and scan code set
(2).

It seems that sending this F6 reset command does not mention what
effect it has on the LED.

> > Thanks.
> >   
> 
> ---
> Qiang

Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Qiang Ma]

On Fri, 26 Jul 2024 14:57:03 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Fri, Jul 26, 2024 at 02:57:03PM -0700, Dmitry Torokhov wrote:
> > Hi Qiang,
> > 
> > On Fri, Jul 26, 2024 at 06:27:30PM +0800, Qiang Ma wrote:
> > > After we turn on the keyboard CAPSL LED and let the system
> > > suspend, the keyboard LED is not off, and the kernel log is as
> > > follows:
> > > 
> > > [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> > > [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > > [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> > > [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > > 
> > > The log shows that after the command 0xed is sent, the data
> > > sent is 0x04 instead of 0x00.
> > > 
> > > Solution:
> > > Add a bitmap variable ledon in the atkbd structure, and then set
> > > ledon according to code-value in the event, in the atkbd_set_leds
> > > function, first look at the value of lenon, if it is 0, there is
> > > no need to look at the value of dev->led, otherwise, Need to look
> > > at dev->led to determine the keyboard LED on/off.
> > 
> > I am not sure why duplicating input_dev->led which is supposed to
> > record which LEDs are currently active on an input device would
> > solve the issue. Could you please explain?
> 
> Ah, OK, I see it now. We do not actually toggle input_dev->led when
> suspending, so atkbd uses wrong data to determine the LED state.
> 

Yes, that's true.

> > 
> > The input core is supposed to turn off all LEDs on suspend. This
> > happens in input_dev_toggle() which is called from
> > input_dev_suspend(). It iterates over all LEDs on a device and
> > turns off active ones one by one.
> > 
> > I think what happens here is we are running afoul of the throttling
> > done in atkbd (see atkbd_schedule_event_work), and it does not
> > actually turn off all LEDs in time. But on the other hand
> > atkbd_cleanup() (which is called to suspend the keyboard) calls
> > 
> >     ps2_command(&atkbd->ps2dev, NULL, ATKBD_CMD_RESET_DEF);
> > 
> > which should turn off everything anyways.
> 
> But still, why ATKBD_CMD_RESET_DEF does not shut off the LEDs for you?
> 
Looking at a ps/2 keyboard
document(http://www-ug.eecg.toronto.edu/msl/nios_devices/datasheets/PS2%20Keyboard%20Protocol.htm),
the F6 command does not seem to affect the state of the LED.

> Thanks.
> 
> -- 
> Dmitry

--
Qiang