[PATCH v7] tty: Fix the keyboard led light display problem

[PATCH v7] tty: Fix the keyboard led light display problem

[lianzhi chang]

Switching from the desktop environment to the tty environment,
the state of the keyboard led lights and the state of the keyboard
lock are inconsistent. This is because the attribute kb->kbdmode
of the tty bound in the desktop environment (xorg) is set to
VC_OFF, which causes the ledstate and kb->ledflagstate
values of the bound tty to always be 0, which causes the switch
from the desktop When to the tty environment, the LED light
status is inconsistent with the keyboard lock status.

Signed-off-by: lianzhi chang <changlianzhi@uniontech.com>
---
 v6-->v7:
 Delete the casting in the kbd_update_ledstate function, and the
 new code no longer needs casting.
 
 drivers/tty/vt/keyboard.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c7fbbcdcc346..e2cf40d06483 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1130,6 +1130,19 @@ static void kbd_init_leds(void)
 
 #endif
 
+static void kbd_update_ledstate(struct input_dev *dev)
+{
+	if (!!test_bit(LED_NUML, dev->led) !=
+	    !!(ledstate & BIT(VC_NUMLOCK)))
+		ledstate ^= BIT(VC_NUMLOCK);
+	if (!!test_bit(LED_CAPSL, dev->led) !=
+	    !!(ledstate & BIT(VC_CAPSLOCK)))
+		ledstate ^= BIT(VC_CAPSLOCK);
+	if (!!test_bit(LED_SCROLLL, dev->led) !=
+	    !!(ledstate & BIT(VC_SCROLLOCK)))
+		ledstate ^= BIT(VC_SCROLLOCK);
+}
+
 /*
  * The leds display either (i) the status of NumLock, CapsLock, ScrollLock,
  * or (ii) whatever pattern of lights people want to show using KDSETLED,
@@ -1247,9 +1260,14 @@ void vt_kbd_con_stop(unsigned int console)
  */
 static void kbd_bh(struct tasklet_struct *unused)
 {
+	struct kbd_struct *kb;
 	unsigned int leds;
 	unsigned long flags;
 
+	kb = kbd_table + fg_console;
+	if (kb->kbdmode == VC_OFF)
+		return;
+
 	spin_lock_irqsave(&led_lock, flags);
 	leds = getleds();
 	leds |= (unsigned int)kbd->lockstate << 8;
@@ -1524,6 +1542,9 @@ static void kbd_event(struct input_handle *handle, unsigned int event_type,
 	/* We are called with interrupts disabled, just take the lock */
 	spin_lock(&kbd_event_lock);
 
+	if (test_bit(EV_LED, handle->dev->evbit))
+		kbd_update_ledstate(handle->dev);
+
 	if (event_type == EV_MSC && event_code == MSC_RAW &&
 			kbd_is_hw_raw(handle->dev))
 		kbd_rawcode(value);
-- 
2.20.1

Re: [PATCH v7] tty: Fix the keyboard led light display problem

[Andy Shevchenko]

On Tue, Oct 26, 2021 at 10:40:32AM +0800, lianzhi chang wrote:
> Switching from the desktop environment to the tty environment,
> the state of the keyboard led lights and the state of the keyboard
> lock are inconsistent. This is because the attribute kb->kbdmode
> of the tty bound in the desktop environment (xorg) is set to

Xorg

I think I already pointed that out.

> VC_OFF, which causes the ledstate and kb->ledflagstate
> values of the bound tty to always be 0, which causes the switch
> from the desktop When to the tty environment, the LED light
> status is inconsistent with the keyboard lock status.

...

> +static void kbd_update_ledstate(struct input_dev *dev)
> +{
> +	if (!!test_bit(LED_NUML, dev->led) !=
> +	    !!(ledstate & BIT(VC_NUMLOCK)))
> +		ledstate ^= BIT(VC_NUMLOCK);
> +	if (!!test_bit(LED_CAPSL, dev->led) !=
> +	    !!(ledstate & BIT(VC_CAPSLOCK)))
> +		ledstate ^= BIT(VC_CAPSLOCK);
> +	if (!!test_bit(LED_SCROLLL, dev->led) !=
> +	    !!(ledstate & BIT(VC_SCROLLOCK)))
> +		ledstate ^= BIT(VC_SCROLLOCK);

This looks ugly.

Since LED_* are part of uAPI and VC_* are not, perhaps
makes sense to modify kbd_kern.h and optimize above
(disclaimer: compile tested only, no locking or other
 synchronization have been checked / reviewed / etc):

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c7fbbcdcc346..2b04eaa5c6fd 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -49,6 +49,11 @@
 
 #include <asm/irq_regs.h>
 
+/* We rely on these in kbd_update_ledstate() */
+static_assert(VC_NUMLOCK == LED_NUML);
+static_assert(VC_CAPSLOCK == LED_CAPSL);
+static_assert(VC_SCROLLOCK == LED_SCROLLL);
+
 /*
  * Exported functions/variables
  */
@@ -1130,6 +1135,15 @@ static void kbd_init_leds(void)
 
 #endif
 
+static void kbd_update_ledstate(struct input_dev *dev)
+{
+	unsigned long leds = BIT(LED_NUML) | BIT(LED_CAPSL) | BIT(LED_SCROLLL);
+	unsigned long newstate = *dev->led;
+
+	newstate ^= ledstate;
+	ledstate ^= newstate & leds;
+}
+
 /*
  * The leds display either (i) the status of NumLock, CapsLock, ScrollLock,
  * or (ii) whatever pattern of lights people want to show using KDSETLED,
@@ -1247,9 +1261,14 @@ void vt_kbd_con_stop(unsigned int console)
  */
 static void kbd_bh(struct tasklet_struct *unused)
 {
+	struct kbd_struct *kb;
 	unsigned int leds;
 	unsigned long flags;
 
+	kb = kbd_table + fg_console;
+	if (kb->kbdmode == VC_OFF)
+		return;
+
 	spin_lock_irqsave(&led_lock, flags);
 	leds = getleds();
 	leds |= (unsigned int)kbd->lockstate << 8;
@@ -1524,6 +1543,9 @@ static void kbd_event(struct input_handle *handle, unsigned int event_type,
 	/* We are called with interrupts disabled, just take the lock */
 	spin_lock(&kbd_event_lock);
 
+	if (test_bit(EV_LED, handle->dev->evbit))
+		kbd_update_ledstate(handle->dev);
+
 	if (event_type == EV_MSC && event_code == MSC_RAW &&
 			kbd_is_hw_raw(handle->dev))
 		kbd_rawcode(value);
diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
index c40811d79769..4c131dd21fc9 100644
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -38,9 +38,9 @@ struct kbd_struct {
 
 	unsigned char ledflagstate:4;	/* flags, not lights */
 	unsigned char default_ledflagstate:4;
-#define VC_SCROLLOCK	0	/* scroll-lock mode */
-#define VC_NUMLOCK	1	/* numeric lock mode */
-#define VC_CAPSLOCK	2	/* capslock mode */
+#define VC_NUMLOCK	0	/* numeric lock mode */
+#define VC_CAPSLOCK	1	/* capslock mode */
+#define VC_SCROLLOCK	2	/* scroll-lock mode */
 #define VC_KANALOCK	3	/* kanalock mode */
 
 	unsigned char kbdmode:3;	/* one 3-bit value */

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7] tty: Fix the keyboard led light display problem

[Greg KH]

On Tue, Oct 26, 2021 at 05:23:35PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 26, 2021 at 10:40:32AM +0800, lianzhi chang wrote:
> > Switching from the desktop environment to the tty environment,
> > the state of the keyboard led lights and the state of the keyboard
> > lock are inconsistent. This is because the attribute kb->kbdmode
> > of the tty bound in the desktop environment (xorg) is set to
> 
> Xorg
> 
> I think I already pointed that out.
> 
> > VC_OFF, which causes the ledstate and kb->ledflagstate
> > values of the bound tty to always be 0, which causes the switch
> > from the desktop When to the tty environment, the LED light
> > status is inconsistent with the keyboard lock status.
> 
> ...
> 
> > +static void kbd_update_ledstate(struct input_dev *dev)
> > +{
> > +	if (!!test_bit(LED_NUML, dev->led) !=
> > +	    !!(ledstate & BIT(VC_NUMLOCK)))
> > +		ledstate ^= BIT(VC_NUMLOCK);
> > +	if (!!test_bit(LED_CAPSL, dev->led) !=
> > +	    !!(ledstate & BIT(VC_CAPSLOCK)))
> > +		ledstate ^= BIT(VC_CAPSLOCK);
> > +	if (!!test_bit(LED_SCROLLL, dev->led) !=
> > +	    !!(ledstate & BIT(VC_SCROLLOCK)))
> > +		ledstate ^= BIT(VC_SCROLLOCK);
> 
> This looks ugly.

Agreed, we also have the vc_kbd_led() and friend functions that seem to
be ignored here :(

> 
> Since LED_* are part of uAPI and VC_* are not, perhaps
> makes sense to modify kbd_kern.h and optimize above
> (disclaimer: compile tested only, no locking or other
>  synchronization have been checked / reviewed / etc):
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..2b04eaa5c6fd 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -49,6 +49,11 @@
>  
>  #include <asm/irq_regs.h>
>  
> +/* We rely on these in kbd_update_ledstate() */
> +static_assert(VC_NUMLOCK == LED_NUML);
> +static_assert(VC_CAPSLOCK == LED_CAPSL);
> +static_assert(VC_SCROLLOCK == LED_SCROLLL);

This makes more sense, these should be in sync.

> +
>  /*
>   * Exported functions/variables
>   */
> @@ -1130,6 +1135,15 @@ static void kbd_init_leds(void)
>  
>  #endif
>  
> +static void kbd_update_ledstate(struct input_dev *dev)
> +{
> +	unsigned long leds = BIT(LED_NUML) | BIT(LED_CAPSL) | BIT(LED_SCROLLL);
> +	unsigned long newstate = *dev->led;
> +
> +	newstate ^= ledstate;
> +	ledstate ^= newstate & leds;

You should document the bit twiddling hack you are doing here just to
make it easier to understand :)

Also, ledstate is an unsigned int, not unsigned long, perhaps make them
all u32 for now to make it obvious they are the same?  Or u64?

thanks,

greg k-h

Re: [PATCH v7] tty: Fix the keyboard led light display problem

[常廉志]

On Tue, Oct 26, 2021 at 05:23:35PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 26, 2021 at 10:40:32AM +0800, lianzhi chang wrote:
> > Switching from the desktop environment to the tty environment,
> > the state of the keyboard led lights and the state of the keyboard
> > lock are inconsistent. This is because the attribute kb->kbdmode
> > of the tty bound in the desktop environment (xorg) is set to
>
> Xorg
>
> I think I already pointed that out.
>
> > VC_OFF, which causes the ledstate and kb->ledflagstate
> > values of the bound tty to always be 0, which causes the switch
> > from the desktop When to the tty environment, the LED light
> > status is inconsistent with the keyboard lock status.
>
> ...
>
> > +static void kbd_update_ledstate(struct input_dev *dev)
> > +{
> > + if (!!test_bit(LED_NUML, dev->led) !=
> > +     !!(ledstate & BIT(VC_NUMLOCK)))
> > + ledstate ^= BIT(VC_NUMLOCK);
> > + if (!!test_bit(LED_CAPSL, dev->led) !=
> > +     !!(ledstate & BIT(VC_CAPSLOCK)))
> > + ledstate ^= BIT(VC_CAPSLOCK);
> > + if (!!test_bit(LED_SCROLLL, dev->led) !=
> > +     !!(ledstate & BIT(VC_SCROLLOCK)))
> > + ledstate ^= BIT(VC_SCROLLOCK);
>
> This looks ugly.

I think it can be done like this:
 static void kbd_update_ledstate(struct input_dev *dev)
 {
    ledstate = (test_bit(LED_SCROLL,dev->led) ？1 ：0）
         | (test_bit(LED_NUML,dev->led) ？2 ：0）
         | (test_bit(LED_CAPSL,dev->led) ？3 ：0）
 }

If this change does not work, please reply to me. I have not 
understood the other content. It is too late today, I will continue tomorrow!

Thanks.
--
lianzhi chang

Re: [PATCH v7] tty: Fix the keyboard led light display problem

[Greg KH]

On Tue, Oct 26, 2021 at 11:41:17PM +0800, 常廉志 wrote:
> On Tue, Oct 26, 2021 at 05:23:35PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 26, 2021 at 10:40:32AM +0800, lianzhi chang wrote:
> > > Switching from the desktop environment to the tty environment,
> > > the state of the keyboard led lights and the state of the keyboard
> > > lock are inconsistent. This is because the attribute kb->kbdmode
> > > of the tty bound in the desktop environment (xorg) is set to
> >
> > Xorg
> >
> > I think I already pointed that out.
> >
> > > VC_OFF, which causes the ledstate and kb->ledflagstate
> > > values of the bound tty to always be 0, which causes the switch
> > > from the desktop When to the tty environment, the LED light
> > > status is inconsistent with the keyboard lock status.
> >
> > ...
> >
> > > +static void kbd_update_ledstate(struct input_dev *dev)
> > > +{
> > > + if (!!test_bit(LED_NUML, dev->led) !=
> > > +     !!(ledstate & BIT(VC_NUMLOCK)))
> > > + ledstate ^= BIT(VC_NUMLOCK);
> > > + if (!!test_bit(LED_CAPSL, dev->led) !=
> > > +     !!(ledstate & BIT(VC_CAPSLOCK)))
> > > + ledstate ^= BIT(VC_CAPSLOCK);
> > > + if (!!test_bit(LED_SCROLLL, dev->led) !=
> > > +     !!(ledstate & BIT(VC_SCROLLOCK)))
> > > + ledstate ^= BIT(VC_SCROLLOCK);
> >
> > This looks ugly.
> 
> I think it can be done like this:
>  static void kbd_update_ledstate(struct input_dev *dev)
>  {
>     ledstate = (test_bit(LED_SCROLL,dev->led) ？1 ：0）
>          | (test_bit(LED_NUML,dev->led) ？2 ：0）
>          | (test_bit(LED_CAPSL,dev->led) ？3 ：0）
>  }

Please write code for developers to read first, and compilers second.
That ? : stuff is a mess, do not do that.

thanks,

greg k-h