2021-11-08 10:54:05

by lianzhi chang

[permalink] [raw]
Subject: [PATCH v14] tty: Fix the keyboard led light display problem

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.
In order to ensure that the keyboard LED lights are displayed
normally during the VT switching process, when the VT is
switched, the current VT LED configuration is forced to be issued.

Signed-off-by: lianzhi chang <[email protected]>
Suggested-by: dmitry.torokhov <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
---
v13:
The kbd_bh function no longer handles the "kb->kbdmode == VC_OFF"
scene, but puts this process in vt_set_leds_compute_shiftstate
together. Because the current circumvention is that other ttys
switch to the Xorg-bound tty scene, so this Better.
v14:
Sorry, I forgot to verify the format, it is good now.

drivers/tty/vt/keyboard.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c7fbbcdcc346..91e1c5d92029 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -153,6 +153,7 @@ static int shift_state = 0;

static unsigned int ledstate = -1U; /* undefined */
static unsigned char ledioctl;
+static bool vt_switch;

/*
* Notifier list for console keyboard events
@@ -412,9 +413,20 @@ static void do_compute_shiftstate(void)
/* We still have to export this method to vt.c */
void vt_set_leds_compute_shiftstate(void)
{
+ struct kbd_struct *kb;
unsigned long flags;

- set_leds();
+ /* Xorg will bind a tty, the kb->kbdmode of this tty will be set to
+ * VC_OFF, and this tty will no longer set the keyboard light. If
+ * there is no such restriction, when switching from other tty to
+ * Xorg-bound tty, the tty will set the keyboard light, which is
+ * unreasonable
+ */
+ kb = kbd_table + fg_console;
+ if (kb->kbdmode != VC_OFF) {
+ vt_switch = true;
+ set_leds();
+ }

spin_lock_irqsave(&kbd_event_lock, flags);
do_compute_shiftstate();
@@ -1255,6 +1267,11 @@ static void kbd_bh(struct tasklet_struct *unused)
leds |= (unsigned int)kbd->lockstate << 8;
spin_unlock_irqrestore(&led_lock, flags);

+ if (vt_switch) {
+ ledstate = ~leds;
+ vt_switch = false;
+ }
+
if (leds != ledstate) {
kbd_propagate_led_state(ledstate, leds);
ledstate = leds;
--
2.20.1




2021-11-08 12:47:19

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v14] tty: Fix the keyboard led light display problem

On Mon, Nov 08, 2021 at 01:51:39PM +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
> 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.
> In order to ensure that the keyboard LED lights are displayed
> normally during the VT switching process, when the VT is
> switched, the current VT LED configuration is forced to be issued.

...

> Suggested-by: dmitry.torokhov <[email protected]>

Dmitry Torokhov

> Suggested-by: Andy Shevchenko <[email protected]>

As per v13 and previous ones, I haven't suggested anything in the
_current_ implementation.

--
With Best Regards,
Andy Shevchenko


2021-11-10 06:45:00

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH v14] tty: Fix the keyboard led light display problem

Hi lianzhi,

On Mon, Nov 08, 2021 at 01:51:39PM +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
> VC_OFF, which causes the ledstate and kb->ledflagstate

We know that Xorg sets kbdmode mode to VC_OFF, but it does not mean that
you can say for sure that it is Xorg instance that controls a VT simply
by observing kb->kbdmode. There may be something else entirely. That is
why you want drivers/tty/vt/keyboard.c to reset LEDs and leave it to
whoever is controlling VT to set them to something else if it is
desired.

> 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.
> In order to ensure that the keyboard LED lights are displayed
> normally during the VT switching process, when the VT is
> switched, the current VT LED configuration is forced to be issued.
>
> Signed-off-by: lianzhi chang <[email protected]>
> Suggested-by: dmitry.torokhov <[email protected]>
> Suggested-by: Andy Shevchenko <[email protected]>
> ---
> v13:
> The kbd_bh function no longer handles the "kb->kbdmode == VC_OFF"
> scene, but puts this process in vt_set_leds_compute_shiftstate
> together. Because the current circumvention is that other ttys
> switch to the Xorg-bound tty scene, so this Better.
> v14:
> Sorry, I forgot to verify the format, it is good now.
>
> drivers/tty/vt/keyboard.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..91e1c5d92029 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -153,6 +153,7 @@ static int shift_state = 0;
>
> static unsigned int ledstate = -1U; /* undefined */
> static unsigned char ledioctl;
> +static bool vt_switch;
>
> /*
> * Notifier list for console keyboard events
> @@ -412,9 +413,20 @@ static void do_compute_shiftstate(void)
> /* We still have to export this method to vt.c */
> void vt_set_leds_compute_shiftstate(void)
> {
> + struct kbd_struct *kb;
> unsigned long flags;
>
> - set_leds();
> + /* Xorg will bind a tty, the kb->kbdmode of this tty will be set to
> + * VC_OFF, and this tty will no longer set the keyboard light. If
> + * there is no such restriction, when switching from other tty to
> + * Xorg-bound tty, the tty will set the keyboard light, which is
> + * unreasonable
> + */
> + kb = kbd_table + fg_console;
> + if (kb->kbdmode != VC_OFF) {
> + vt_switch = true;
> + set_leds();
> + }
>
> spin_lock_irqsave(&kbd_event_lock, flags);
> do_compute_shiftstate();
> @@ -1255,6 +1267,11 @@ static void kbd_bh(struct tasklet_struct *unused)
> leds |= (unsigned int)kbd->lockstate << 8;
> spin_unlock_irqrestore(&led_lock, flags);
>
> + if (vt_switch) {
> + ledstate = ~leds;
> + vt_switch = false;
> + }
> +
> if (leds != ledstate) {
> kbd_propagate_led_state(ledstate, leds);
> ledstate = leds;
> --
> 2.20.1
>
>
>

Thanks.

--
Dmitry

2021-11-10 07:28:07

by lianzhi chang

[permalink] [raw]
Subject: Re: [PATCH v14] tty: Fix the keyboard led light display problem

> On Mon, Nov 08, 2021 at 01:51:39PM +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
> > VC_OFF, which causes the ledstate and kb->ledflagstate

> We know that Xorg sets kbdmode mode to VC_OFF, but it does not mean that
> you can say for sure that it is Xorg instance that controls a VT simply
> by observing kb->kbdmode. There may be something else entirely. That is
> why you want drivers/tty/vt/keyboard.c to reset LEDs and leave it to
> whoever is controlling VT to set them to something else if it is
> desired.

Does this mean let me change the description information? Or is the
judgment of VC_OFF in patch incorrect?
The setup method of Xorg mentioned here is just to describe a process
in which I found the problem;
My understanding is that when the mode of kbdmode is set to VC_OFF,
VT shouldn't interfere with the state of the keyboard light, right? This is
how functions such as kbd_keycode() are implemented.
When VT is switched, if the VT mode is VC_OFF, there is also no need
to set the state of the keyboard light. I think this is reasonable.

> > 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.
> > In order to ensure that the keyboard LED lights are displayed
> > normally during the VT switching process, when the VT is
> > switched, the current VT LED configuration is forced to be issued.
> >
> > Signed-off-by: lianzhi chang <[email protected]>
> > Suggested-by: dmitry.torokhov <[email protected]>
> > Suggested-by: Andy Shevchenko <[email protected]>
> > ---
> > v13:
> > The kbd_bh function no longer handles the "kb->kbdmode == VC_OFF"
> > scene, but puts this process in vt_set_leds_compute_shiftstate
> > together. Because the current circumvention is that other ttys
> > switch to the Xorg-bound tty scene, so this Better.
> > v14:
> > Sorry, I forgot to verify the format, it is good now.
> >
> > drivers/tty/vt/keyboard.c | 19 ++++++++++++++++++-
> > 1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> > index c7fbbcdcc346..91e1c5d92029 100644
> > --- a/drivers/tty/vt/keyboard.c
> > +++ b/drivers/tty/vt/keyboard.c
> > @@ -153,6 +153,7 @@ static int shift_state = 0;
> >
> > static unsigned int ledstate = -1U; /* undefined */
> > static unsigned char ledioctl;
> > +static bool vt_switch;
> >
> > /*
> > * Notifier list for console keyboard events
> > @@ -412,9 +413,20 @@ static void do_compute_shiftstate(void)
> > /* We still have to export this method to vt.c */
> > void vt_set_leds_compute_shiftstate(void)
> > {
> > + struct kbd_struct *kb;
> > unsigned long flags;
> >
> > - set_leds();
> > + /* Xorg will bind a tty, the kb->kbdmode of this tty will be set to
> > + * VC_OFF, and this tty will no longer set the keyboard light. If
> > + * there is no such restriction, when switching from other tty to
> > + * Xorg-bound tty, the tty will set the keyboard light, which is
> > + * unreasonable
> > + */
> > + kb = kbd_table + fg_console;
> > + if (kb->kbdmode != VC_OFF) {
> > + vt_switch = true;
> > + set_leds();
> > + }
> >
> > spin_lock_irqsave(&kbd_event_lock, flags);
> > do_compute_shiftstate();
> > @@ -1255,6 +1267,11 @@ static void kbd_bh(struct tasklet_struct *unused)
> > leds |= (unsigned int)kbd->lockstate << 8;
> > spin_unlock_irqrestore(&led_lock, flags);
> >
> > + if (vt_switch) {
> > + ledstate = ~leds;
> > + vt_switch = false;
> > + }
> > +
> > if (leds != ledstate) {
> > kbd_propagate_led_state(ledstate, leds);
> > ledstate = leds;
> > --
> > 2.20.1

Thanks.
--
lianzhi chang

2021-11-25 02:35:41

by lianzhi chang

[permalink] [raw]
Subject: Re: [PATCH v14] tty: Fix the keyboard led light display problem

> > On Mon, Nov 08, 2021 at 01:51:39PM +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
> > > VC_OFF, which causes the ledstate and kb->ledflagstate

> > We know that Xorg sets kbdmode mode to VC_OFF, but it does not mean that
> > you can say for sure that it is Xorg instance that controls a VT simply
> > by observing kb->kbdmode. There may be something else entirely. That is
> > why you want drivers/tty/vt/keyboard.c to reset LEDs and leave it to
> > whoever is controlling VT to set them to something else if it is
> > desired.

> Does this mean let me change the description information? Or is the
> judgment of VC_OFF in patch incorrect?
> The setup method of Xorg mentioned here is just to describe a process
> in which I found the problem;
> My understanding is that when the mode of kbdmode is set to VC_OFF,
> VT shouldn't interfere with the state of the keyboard light, right? This is
> how functions such as kbd_keycode() are implemented.
> When VT is switched, if the VT mode is VC_OFF, there is also no need
> to set the state of the keyboard light. I think this is reasonable.

> > > > 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.
> > > > In order to ensure that the keyboard LED lights are displayed
> > > > normally during the VT switching process, when the VT is
> > > > switched, the current VT LED configuration is forced to be issued.
> > >
> > > Signed-off-by: lianzhi chang <[email protected]>
> > > Suggested-by: dmitry.torokhov <[email protected]>
> > > Suggested-by: Andy Shevchenko <[email protected]>
> > > ---
> > > v13:
> > > The kbd_bh function no longer handles the "kb->kbdmode == VC_OFF"
> > > scene, but puts this process in vt_set_leds_compute_shiftstate
> > > together. Because the current circumvention is that other ttys
> > > switch to the Xorg-bound tty scene, so this Better.
> > > v14:
> > > Sorry, I forgot to verify the format, it is good now.
> > >
> > > drivers/tty/vt/keyboard.c | 19 ++++++++++++++++++-
> > > 1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> > > index c7fbbcdcc346..91e1c5d92029 100644
> > > --- a/drivers/tty/vt/keyboard.c
> > > +++ b/drivers/tty/vt/keyboard.c
> > > > @@ -153,6 +153,7 @@ static int shift_state = 0;
> > >
> > > static unsigned int ledstate = -1U; /* undefined */
> > > static unsigned char ledioctl;
> > > +static bool vt_switch;
> > >
> > > /*
> > > * Notifier list for console keyboard events
> > > @@ -412,9 +413,20 @@ static void do_compute_shiftstate(void)
> > > /* We still have to export this method to vt.c */
> > > void vt_set_leds_compute_shiftstate(void)
> > > {
> > > + struct kbd_struct *kb;
> > > unsigned long flags;
> > >
> > > - set_leds();
> > > + /* Xorg will bind a tty, the kb->kbdmode of this tty will be set to
> > > + * VC_OFF, and this tty will no longer set the keyboard light. If
> > > + * there is no such restriction, when switching from other tty to
> > > + * Xorg-bound tty, the tty will set the keyboard light, which is
> > > + * unreasonable
> > > + */
> > > + kb = kbd_table + fg_console;
> > > + if (kb->kbdmode != VC_OFF) {
> > > + vt_switch = true;
> > > + set_leds();
> > > + }
> > >
> > > spin_lock_irqsave(&kbd_event_lock, flags);
> > > do_compute_shiftstate();
> > > @@ -1255,6 +1267,11 @@ static void kbd_bh(struct tasklet_struct *unused)
> > > leds |= (unsigned int)kbd->lockstate << 8;
> > > spin_unlock_irqrestore(&led_lock, flags);
> > >
> > > + if (vt_switch) {
> > > + ledstate = ~leds;
> > > + vt_switch = false;
> > > + }
> > > +
> > > if (leds != ledstate) {
> > > kbd_propagate_led_state(ledstate, leds);
> > > ledstate = leds;
> > > --

Hi friends, how is this patch progressing now, do I need to modify it further?

Thanks.
--
lianzhi chang

2021-11-25 11:45:22

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v14] tty: Fix the keyboard led light display problem

On Thu, Nov 25, 2021 at 10:33:23AM +0800, lianzhi chang wrote:
> > > On Mon, Nov 08, 2021 at 01:51:39PM +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
> > > > VC_OFF, which causes the ledstate and kb->ledflagstate
>
> > > We know that Xorg sets kbdmode mode to VC_OFF, but it does not mean that
> > > you can say for sure that it is Xorg instance that controls a VT simply
> > > by observing kb->kbdmode. There may be something else entirely. That is
> > > why you want drivers/tty/vt/keyboard.c to reset LEDs and leave it to
> > > whoever is controlling VT to set them to something else if it is
> > > desired.
>
> > Does this mean let me change the description information? Or is the
> > judgment of VC_OFF in patch incorrect?
> > The setup method of Xorg mentioned here is just to describe a process
> > in which I found the problem;
> > My understanding is that when the mode of kbdmode is set to VC_OFF,
> > VT shouldn't interfere with the state of the keyboard light, right? This is
> > how functions such as kbd_keycode() are implemented.
> > When VT is switched, if the VT mode is VC_OFF, there is also no need
> > to set the state of the keyboard light. I think this is reasonable.
>
> > > > > 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.
> > > > > In order to ensure that the keyboard LED lights are displayed
> > > > > normally during the VT switching process, when the VT is
> > > > > switched, the current VT LED configuration is forced to be issued.

> Hi friends, how is this patch progressing now, do I need to modify it further?

At least I have told you already twice that I'm not the guy who suggested this
solution. Others seem gave you other comments.

--
With Best Regards,
Andy Shevchenko