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 <[email protected]>
---
v7-->v8:
Optimize the implementation of kbd_update_ledstate function
Why not adopt the opinions of Greg KH and Andy Shevchenko:
(1) In the structure struct input_dev, the definition of led is
like this: unsigned long led[BITS_TO_LONGS(LED_CNT)]; If you
define it like this: unsigned long newstate = *dev->led; I
always feel that there is still big end and Little endian problem.
(2) The test_bit function is used to avoid the problem of large
and small ends, and the current algorithm (v8) also exists
elsewhere in the kernel: the atkbd_set_leds function (drivers/
input/keyboard/atkbd.c).
(3) In the current keyboard.c code, the code is already very good,
and it is already relatively independent. If you modify the type
of ledstate to u64 or modify the macro definitions such as
VC_NUMLOCK, it feels that it is not very meaningful, and this It
will also cause other related modifications. Of course, this is
only my current opinion. If everyone still feels that it is
necessary to modify, I will do it this way. Of course, this
process may be a bit longer, and I think it is necessary to
conduct more tests.
v9: Change description information: xorg-->Xorg
drivers/tty/vt/keyboard.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c7fbbcdcc346..f564fcf61304 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1130,6 +1130,13 @@ static void kbd_init_leds(void)
#endif
+static void kbd_update_ledstate(struct input_dev *dev)
+{
+ ledstate = (test_bit(LED_SCROLLL, dev->led) ? BIT(VC_SCROLLOCK) : 0)
+ | (test_bit(LED_NUML, dev->led) ? BIT(VC_NUMLOCK) : 0)
+ | (test_bit(LED_CAPSL, dev->led) ? BIT(VC_CAPSLOCK) : 0);
+}
+
/*
* 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 +1254,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 +1536,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
> 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 <[email protected]>
> ---
> v7-->v8:
> Optimize the implementation of kbd_update_ledstate function
>
> Why not adopt the opinions of Greg KH and Andy Shevchenko:
> (1) In the structure struct input_dev, the definition of led is
> like this: unsigned long led[BITS_TO_LONGS(LED_CNT)]; If you
> define it like this: unsigned long newstate = *dev->led; I
> always feel that there is still big end and Little endian problem.
> (2) The test_bit function is used to avoid the problem of large
> and small ends, and the current algorithm (v8) also exists
> elsewhere in the kernel: the atkbd_set_leds function (drivers/
> input/keyboard/atkbd.c).
> (3) In the current keyboard.c code, the code is already very good,
> and it is already relatively independent. If you modify the type
> of ledstate to u64 or modify the macro definitions such as
> VC_NUMLOCK, it feels that it is not very meaningful, and this It
> will also cause other related modifications. Of course, this is
> only my current opinion. If everyone still feels that it is
> necessary to modify, I will do it this way. Of course, this
> process may be a bit longer, and I think it is necessary to
> conduct more tests.
>
> v9: Change description information: xorg-->Xorg
> ……
Hi, friends, I would like to ask whether this version of patch is possible, if not,
I will try my best to find a way to complete the next version!
Thanks.
--
lianzhi chang
On Mon, Nov 01, 2021 at 08:35:47PM +0800, 常廉志 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.
> >
> > Signed-off-by: lianzhi chang <[email protected]>
> > ---
> > v7-->v8:
> > Optimize the implementation of kbd_update_ledstate function
> >
> > Why not adopt the opinions of Greg KH and Andy Shevchenko:
> > (1) In the structure struct input_dev, the definition of led is
> > like this: unsigned long led[BITS_TO_LONGS(LED_CNT)]; If you
> > define it like this: unsigned long newstate = *dev->led; I
> > always feel that there is still big end and Little endian problem.
> > (2) The test_bit function is used to avoid the problem of large
> > and small ends, and the current algorithm (v8) also exists
> > elsewhere in the kernel: the atkbd_set_leds function (drivers/
> > input/keyboard/atkbd.c).
> > (3) In the current keyboard.c code, the code is already very good,
> > and it is already relatively independent. If you modify the type
> > of ledstate to u64 or modify the macro definitions such as
> > VC_NUMLOCK, it feels that it is not very meaningful, and this It
> > will also cause other related modifications. Of course, this is
> > only my current opinion. If everyone still feels that it is
> > necessary to modify, I will do it this way. Of course, this
> > process may be a bit longer, and I think it is necessary to
> > conduct more tests.
> >
> > v9: Change description information: xorg-->Xorg
> > ……
>
> Hi, friends, I would like to ask whether this version of patch is possible, if not,
> I will try my best to find a way to complete the next version!
It's the merge window right now, we can't do anything with this until
after 5.16-rc1 is out. So give us until then at the least please, then
I will review it again.
thanks,
greg k-h
On Mon, Nov 01, 2021 at 01:48:25PM +0100, Greg KH wrote:
> On Mon, Nov 01, 2021 at 08:35:47PM +0800, 常廉志 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.
> > >
> > > Signed-off-by: lianzhi chang <[email protected]>
> > > ---
> > > v7-->v8:
> > > Optimize the implementation of kbd_update_ledstate function
> > >
> > > Why not adopt the opinions of Greg KH and Andy Shevchenko:
> > > (1) In the structure struct input_dev, the definition of led is
> > > like this: unsigned long led[BITS_TO_LONGS(LED_CNT)]; If you
> > > define it like this: unsigned long newstate = *dev->led; I
> > > always feel that there is still big end and Little endian problem.
> > > (2) The test_bit function is used to avoid the problem of large
> > > and small ends, and the current algorithm (v8) also exists
> > > elsewhere in the kernel: the atkbd_set_leds function (drivers/
> > > input/keyboard/atkbd.c).
> > > (3) In the current keyboard.c code, the code is already very good,
> > > and it is already relatively independent. If you modify the type
> > > of ledstate to u64 or modify the macro definitions such as
> > > VC_NUMLOCK, it feels that it is not very meaningful, and this It
> > > will also cause other related modifications. Of course, this is
> > > only my current opinion. If everyone still feels that it is
> > > necessary to modify, I will do it this way. Of course, this
> > > process may be a bit longer, and I think it is necessary to
> > > conduct more tests.
> > >
> > > v9: Change description information: xorg-->Xorg
> > > ……
> >
> > Hi, friends, I would like to ask whether this version of patch is possible, if not,
> > I will try my best to find a way to complete the next version!
>
> It's the merge window right now, we can't do anything with this until
> after 5.16-rc1 is out. So give us until then at the least please, then
> I will review it again.
As I mentioned, the state of physical LED on a keyboard does not
necessarily reflect state of logical LED state in tty/vt. One can assign
LED on their keyboard to be an indicator of being connected to mains by
assigning "AC-trigger" to it. So the way this patch tries to fix the
issue (by reading internal state of an individual input device) is
wrong.
We keep separate led and lock states for each VT instance, and they
should be applied when switching to/from a VT. Are you saying that in
certain scenarios this switch is not happening? Can see if that can be
fixed?
Thanks.
--
Dmitry
On Mon, Nov 01, 2021 at 01:48:25PM +0100, Greg KH wrote:
> > On Mon, Nov 01, 2021 at 08:35:47PM +0800, 常廉志 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.
> > > >
> > > > Signed-off-by: lianzhi chang <[email protected]>
> > > > ---
> > > > v7-->v8:
> > > > Optimize the implementation of kbd_update_ledstate function
> > > >
> > > > Why not adopt the opinions of Greg KH and Andy Shevchenko:
> > > > (1) In the structure struct input_dev, the definition of led is
> > > > like this: unsigned long led[BITS_TO_LONGS(LED_CNT)]; If you
> > > > define it like this: unsigned long newstate = *dev->led; I
> > > > always feel that there is still big end and Little endian problem.
> > > > (2) The test_bit function is used to avoid the problem of large
> > > > and small ends, and the current algorithm (v8) also exists
> > > > elsewhere in the kernel: the atkbd_set_leds function (drivers/
> > > > input/keyboard/atkbd.c).
> > > > (3) In the current keyboard.c code, the code is already very good,
> > > > and it is already relatively independent. If you modify the type
> > > > of ledstate to u64 or modify the macro definitions such as
> > > > VC_NUMLOCK, it feels that it is not very meaningful, and this It
> > > > will also cause other related modifications. Of course, this is
> > > > only my current opinion. If everyone still feels that it is
> > > > necessary to modify, I will do it this way. Of course, this
> > > > process may be a bit longer, and I think it is necessary to
> > > > conduct more tests.
> > > >
> > > > v9: Change description information: xorg-->Xorg
> > > > ……
> > >
> > > Hi, friends, I would like to ask whether this version of patch is possible, if not,
> > > I will try my best to find a way to complete the next version!
> >
> > It's the merge window right now, we can't do anything with this until
> > after 5.16-rc1 is out. So give us until then at the least please, then
> > I will review it again.
>
> As I mentioned, the state of physical LED on a keyboard does not
> necessarily reflect state of logical LED state in tty/vt. One can assign
> LED on their keyboard to be an indicator of being connected to mains by
> assigning "AC-trigger" to it. So the way this patch tries to fix the
> issue (by reading internal state of an individual input device) is
> wrong.
>
> We keep separate led and lock states for each VT instance, and they
> should be applied when switching to/from a VT. Are you saying that in
> certain scenarios this switch is not happening? Can see if that can be
> fixed?
>
Hi Dmitry, I don’t know if I fully understand what you mean, but I will
try to fully explain the intent of the current patch.
(1) What is the current bug phenomenon? I will describe with the Num
Lock indicator as the object here.
Phenomenon 1: Suppose that Xorg is bound to tty1 in the desktop environment.
At this time, the Num light of the keyboard is on, and the keypad can input numbers
normally; assume that the state of the keyboard light saved by tty2 itself is the
opposite (the Num light is off, The keypad cannot enter numbers); at this time,
if we use the key combination "ctrl+alt+F2" to switch the system to tty2, we will find
that the Num light is still on, but the keypad cannot enter numbers.
Phenomenon 2: Assuming that you are currently in the tty2 environment, the Num
light of the keyboard is on, and the keypad can input numbers normally; assume that
the Num state saved by Xorg is the same as that of tty2 (the Num light is on, and the
keypad can input numbers normally); At this point, if we use the key combination
"ctrl+alt+F1" to switch the system to tty1 (that is, to switch to the desktop environment)
, we will find that the Num light will not light up, but the small keyboard can input numbers .
(2) Why do these two phenomena occur?
The variable static unsigned int ledstate is defined in keyboard.c. ledstate should be used to
tell VT the current state of the keyboard light, because after each VT sets the state of the
keyboard light, it will synchronize the latest keyboard light state to ledstate( (Implemented
in the kbd_bh() function).
Then the problem comes. The scope of ledstate is only in VT, and it cannot include all the
scenes where the keyboard light is set. And, in the desktop environment, "kb->kbdmode ==
VC_OFF" of tty1, at this time, through the NumLock button, only Xorg's own state can be
changed, and the led state stored by tty1 cannot be changed (implemented in the kbd_keycode()
function), This results in that the kb->ledflagstate stored by tty1 itself and the ledstate in the tty
environment are always 0 at this time.
When we switch tty, the VT code compares the current tty's kb->ledflagstate and ledstate values.
If they are inconsistent, change the state of the keyboard light (implemented in the kbd_bh() function).
In phenomenon 1, in the desktop environment, although the actual state saved by xorg is 1, the state
of ledstate of tty is always 0. In the environment of tty2, the state of kb->ledflagstate of tty2 is also 0.
At this point, in the kbd_bh() function, comparing these two values is equal, there is no need to set the
led light state to the keyboard. So after switching to tty2, the Num light is still on, but the small
keyboard cannot input numbers.
In phenomenon 2, in the tty2 environment, the state of ledstate is set to 1, but the kb->ledflagstate of
tty1 is 0. At this time, the two values are not equal in the kbd_bh() function, so set the led The light
status to the keyboard. Xorg did not redistribute the configuration during this process is also one of
the reasons. And even if Xorg re-issues the configuration at this time, it will cause confusion and only
one can be set.
(3) How to solve it?
To solve the problem of phenomenon 1, we must first enable ledstate to correctly reflect the current
state of the keyboard light. Therefore, the solution to all versions of patch is to synchronize the
latest led state of the input device to ledstate.
At the same time, to solve the problem of phenomenon 2, when switching to tty1, kbd_bh() also
determines the current tty's "kb->kbdmode == VC_OFF". If it is true, don't set the keyboard light
state. At the same time, Xorg should also re-issue the status of the keyboard light to ensure that it
is correct (I also submitted a patch for this, refer to
https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/764).
(4) At this time, each VT instance still maintains a separate led and lock state, which are applied
when switching to a VT. This is unaffected.
Thanks.
--
lianzhi chang
Hi lianzhi,
On Tue, Nov 02, 2021 at 11:16:56AM +0800, 常廉志 wrote:
>
> Hi Dmitry, I don’t know if I fully understand what you mean, but I will
> try to fully explain the intent of the current patch.
> (1) What is the current bug phenomenon? I will describe with the Num
> Lock indicator as the object here.
>
> Phenomenon 1: Suppose that Xorg is bound to tty1 in the desktop environment.
> At this time, the Num light of the keyboard is on, and the keypad can input numbers
> normally; assume that the state of the keyboard light saved by tty2 itself is the
> opposite (the Num light is off, The keypad cannot enter numbers); at this time,
> if we use the key combination "ctrl+alt+F2" to switch the system to tty2, we will find
> that the Num light is still on, but the keypad cannot enter numbers.
>
> Phenomenon 2: Assuming that you are currently in the tty2 environment, the Num
> light of the keyboard is on, and the keypad can input numbers normally; assume that
> the Num state saved by Xorg is the same as that of tty2 (the Num light is on, and the
> keypad can input numbers normally); At this point, if we use the key combination
> "ctrl+alt+F1" to switch the system to tty1 (that is, to switch to the desktop environment)
> , we will find that the Num light will not light up, but the small keyboard can input numbers .
>
> (2) Why do these two phenomena occur?
> The variable static unsigned int ledstate is defined in keyboard.c. ledstate should be used to
> tell VT the current state of the keyboard light, because after each VT sets the state of the
> keyboard light, it will synchronize the latest keyboard light state to ledstate( (Implemented
> in the kbd_bh() function).
>
> Then the problem comes. The scope of ledstate is only in VT, and it cannot include all the
> scenes where the keyboard light is set. And, in the desktop environment, "kb->kbdmode ==
> VC_OFF" of tty1, at this time, through the NumLock button, only Xorg's own state can be
> changed, and the led state stored by tty1 cannot be changed (implemented in the kbd_keycode()
> function), This results in that the kb->ledflagstate stored by tty1 itself and the ledstate in the tty
> environment are always 0 at this time.
>
> When we switch tty, the VT code compares the current tty's kb->ledflagstate and ledstate values.
> If they are inconsistent, change the state of the keyboard light (implemented in the kbd_bh() function).
>
> In phenomenon 1, in the desktop environment, although the actual state saved by xorg is 1, the state
> of ledstate of tty is always 0. In the environment of tty2, the state of kb->ledflagstate of tty2 is also 0.
> At this point, in the kbd_bh() function, comparing these two values is equal, there is no need to set the
> led light state to the keyboard. So after switching to tty2, the Num light is still on, but the small
> keyboard cannot input numbers.
>
> In phenomenon 2, in the tty2 environment, the state of ledstate is set to 1, but the kb->ledflagstate of
> tty1 is 0. At this time, the two values are not equal in the kbd_bh() function, so set the led The light
> status to the keyboard. Xorg did not redistribute the configuration during this process is also one of
> the reasons. And even if Xorg re-issues the configuration at this time, it will cause confusion and only
> one can be set.
>
> (3) How to solve it?
> To solve the problem of phenomenon 1, we must first enable ledstate to correctly reflect the current
> state of the keyboard light. Therefore, the solution to all versions of patch is to synchronize the
> latest led state of the input device to ledstate.
You assume that input's device NumLock LED reflects the state of
terminal. That does not have to be the case.
Now how to solve this... On VT switch redraw_screen() calls
vt_set_leds_compute_shiftstate(). Can we do something like:
/*
* On VT switch pretend our led state is opposite of target
* state to ensure we refresh all leds.
*/
spin_lock_irqsave(&led_lock, flags);
leds = getleds();
leds |= (unsigned int)kbd->lockstate << 8;
ledstate = ~leds;
spin_unlock_irqrestore(&led_lock, flags);
set_leds();
?
> At the same time, to solve the problem of phenomenon 2, when switching to tty1, kbd_bh() also
> determines the current tty's "kb->kbdmode == VC_OFF". If it is true, don't set the keyboard light
> state. At the same time, Xorg should also re-issue the status of the keyboard light to ensure that it
> is correct (I also submitted a patch for this, refer to
> https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/764).
That makes sense.
Thanks.
--
Dmitry
> >
> > Hi Dmitry, I don’t know if I fully understand what you mean, but I will
> > try to fully explain the intent of the current patch.
> > (1) What is the current bug phenomenon? I will describe with the Num
> > Lock indicator as the object here.
> >
> > Phenomenon 1: Suppose that Xorg is bound to tty1 in the desktop environment.
> > At this time, the Num light of the keyboard is on, and the keypad can input numbers
> > normally; assume that the state of the keyboard light saved by tty2 itself is the
> > opposite (the Num light is off, The keypad cannot enter numbers); at this time,
> > if we use the key combination "ctrl+alt+F2" to switch the system to tty2, we will find
> > that the Num light is still on, but the keypad cannot enter numbers.
> >
> > Phenomenon 2: Assuming that you are currently in the tty2 environment, the Num
> > light of the keyboard is on, and the keypad can input numbers normally; assume that
> > the Num state saved by Xorg is the same as that of tty2 (the Num light is on, and the
> > keypad can input numbers normally); At this point, if we use the key combination
> > "ctrl+alt+F1" to switch the system to tty1 (that is, to switch to the desktop environment)
> > , we will find that the Num light will not light up, but the small keyboard can input numbers .
> >
> > (2) Why do these two phenomena occur?
> > The variable static unsigned int ledstate is defined in keyboard.c. ledstate should be used to
> > tell VT the current state of the keyboard light, because after each VT sets the state of the
> > keyboard light, it will synchronize the latest keyboard light state to ledstate( (Implemented
> > in the kbd_bh() function).
> >
> > Then the problem comes. The scope of ledstate is only in VT, and it cannot include all the
> > scenes where the keyboard light is set. And, in the desktop environment, "kb->kbdmode ==
> > VC_OFF" of tty1, at this time, through the NumLock button, only Xorg's own state can be
> > changed, and the led state stored by tty1 cannot be changed (implemented in the kbd_keycode()
> > function), This results in that the kb->ledflagstate stored by tty1 itself and the ledstate in the tty
> > environment are always 0 at this time.
> >
> > When we switch tty, the VT code compares the current tty's kb->ledflagstate and ledstate values.
> > If they are inconsistent, change the state of the keyboard light (implemented in the kbd_bh() function).
> >
> > In phenomenon 1, in the desktop environment, although the actual state saved by xorg is 1, the state
> > of ledstate of tty is always 0. In the environment of tty2, the state of kb->ledflagstate of tty2 is also 0.
> > At this point, in the kbd_bh() function, comparing these two values is equal, there is no need to set the
> > led light state to the keyboard. So after switching to tty2, the Num light is still on, but the small
> > keyboard cannot input numbers.
> >
> > In phenomenon 2, in the tty2 environment, the state of ledstate is set to 1, but the kb->ledflagstate of
> > tty1 is 0. At this time, the two values are not equal in the kbd_bh() function, so set the led The light
> > status to the keyboard. Xorg did not redistribute the configuration during this process is also one of
> > the reasons. And even if Xorg re-issues the configuration at this time, it will cause confusion and only
> > one can be set.
> >
> > (3) How to solve it?
> > To solve the problem of phenomenon 1, we must first enable ledstate to correctly reflect the current
> > state of the keyboard light. Therefore, the solution to all versions of patch is to synchronize the
> > latest led state of the input device to ledstate.
> You assume that input's device NumLock LED reflects the state of
> terminal. That does not have to be the case.
> Now how to solve this... On VT switch redraw_screen() calls
> vt_set_leds_compute_shiftstate(). Can we do something like:
> /*
> * On VT switch pretend our led state is opposite of target
> * state to ensure we refresh all leds.
> */
> spin_lock_irqsave(&led_lock, flags);
> leds = getleds();
> leds |= (unsigned int)kbd->lockstate << 8;
> ledstate = ~leds;
> spin_unlock_irqrestore(&led_lock, flags);
>
> set_leds();
>
> ?
Hi Dmitry:
/*
* The following piece of code exists in the kbd_bh() function
*/
spin_lock_irqsave(&led_lock, flags);
leds = getleds();
leds |= (unsigned int)kbd->lockstate << 8;
ledstate = ~leds;
spin_unlock_irqrestore(&led_lock, flags);
Moreover, the process of calling the set_leds() function is
the process of calling the kbd_bh function:
static void set_leds(void)
{
tasklet_schedule(&keyboard_tasklet);
}
static DECLARE_TASKLET_DISABLED(keyboard_tasklet, kbd_bh);
I don't really understand what you mean here, but one thing
can be confirmed, my patch just synchronizes the current input
device's led state to ledstate. Moreover, after VT's
kb->ledflagstate is set to the input device, it will also
be synchronized to ledstate (the original logic of the kbd_bh()
function), which does not destroy the original internal logic of
VT. In addition, I have tested it, whether it is switching
between the desktop environment (tty1) and tty2~6, or switching
between tty2~6, the indicator status of the keyboard light is
correct, and it is normal in the multi-keyboard state. .
Of course, I need to add the Xorg repair patch I mentioned
earlier.
Thanks.
--
lianzhi chang
On Tue, Nov 02, 2021 at 03:09:20PM +0800, 常廉志 wrote:
> > >
> > > Hi Dmitry, I don’t know if I fully understand what you mean, but I will
> > > try to fully explain the intent of the current patch.
> > > (1) What is the current bug phenomenon? I will describe with the Num
> > > Lock indicator as the object here.
> > >
> > > Phenomenon 1: Suppose that Xorg is bound to tty1 in the desktop environment.
> > > At this time, the Num light of the keyboard is on, and the keypad can input numbers
> > > normally; assume that the state of the keyboard light saved by tty2 itself is the
> > > opposite (the Num light is off, The keypad cannot enter numbers); at this time,
> > > if we use the key combination "ctrl+alt+F2" to switch the system to tty2, we will find
> > > that the Num light is still on, but the keypad cannot enter numbers.
> > >
> > > Phenomenon 2: Assuming that you are currently in the tty2 environment, the Num
> > > light of the keyboard is on, and the keypad can input numbers normally; assume that
> > > the Num state saved by Xorg is the same as that of tty2 (the Num light is on, and the
> > > keypad can input numbers normally); At this point, if we use the key combination
> > > "ctrl+alt+F1" to switch the system to tty1 (that is, to switch to the desktop environment)
> > > , we will find that the Num light will not light up, but the small keyboard can input numbers .
> > >
> > > (2) Why do these two phenomena occur?
> > > The variable static unsigned int ledstate is defined in keyboard.c. ledstate should be used to
> > > tell VT the current state of the keyboard light, because after each VT sets the state of the
> > > keyboard light, it will synchronize the latest keyboard light state to ledstate( (Implemented
> > > in the kbd_bh() function).
> > >
> > > Then the problem comes. The scope of ledstate is only in VT, and it cannot include all the
> > > scenes where the keyboard light is set. And, in the desktop environment, "kb->kbdmode ==
> > > VC_OFF" of tty1, at this time, through the NumLock button, only Xorg's own state can be
> > > changed, and the led state stored by tty1 cannot be changed (implemented in the kbd_keycode()
> > > function), This results in that the kb->ledflagstate stored by tty1 itself and the ledstate in the tty
> > > environment are always 0 at this time.
> > >
> > > When we switch tty, the VT code compares the current tty's kb->ledflagstate and ledstate values.
> > > If they are inconsistent, change the state of the keyboard light (implemented in the kbd_bh() function).
> > >
> > > In phenomenon 1, in the desktop environment, although the actual state saved by xorg is 1, the state
> > > of ledstate of tty is always 0. In the environment of tty2, the state of kb->ledflagstate of tty2 is also 0.
> > > At this point, in the kbd_bh() function, comparing these two values is equal, there is no need to set the
> > > led light state to the keyboard. So after switching to tty2, the Num light is still on, but the small
> > > keyboard cannot input numbers.
> > >
> > > In phenomenon 2, in the tty2 environment, the state of ledstate is set to 1, but the kb->ledflagstate of
> > > tty1 is 0. At this time, the two values are not equal in the kbd_bh() function, so set the led The light
> > > status to the keyboard. Xorg did not redistribute the configuration during this process is also one of
> > > the reasons. And even if Xorg re-issues the configuration at this time, it will cause confusion and only
> > > one can be set.
> > >
> > > (3) How to solve it?
> > > To solve the problem of phenomenon 1, we must first enable ledstate to correctly reflect the current
> > > state of the keyboard light. Therefore, the solution to all versions of patch is to synchronize the
> > > latest led state of the input device to ledstate.
>
> > You assume that input's device NumLock LED reflects the state of
> > terminal. That does not have to be the case.
>
> > Now how to solve this... On VT switch redraw_screen() calls
> > vt_set_leds_compute_shiftstate(). Can we do something like:
>
> > /*
> > * On VT switch pretend our led state is opposite of target
> > * state to ensure we refresh all leds.
> > */
> > spin_lock_irqsave(&led_lock, flags);
> > leds = getleds();
> > leds |= (unsigned int)kbd->lockstate << 8;
> > ledstate = ~leds;
> > spin_unlock_irqrestore(&led_lock, flags);
> >
> > set_leds();
> >
> > ?
> Hi Dmitry:
> /*
> * The following piece of code exists in the kbd_bh() function
> */
> spin_lock_irqsave(&led_lock, flags);
> leds = getleds();
> leds |= (unsigned int)kbd->lockstate << 8;
> ledstate = ~leds;
^^^^^^^^^^^^^^^^^
This is not the exact code that exists in kbd_bh(). It lacks the line
above which should cause LEDs be synchronized once set_leds()/kbd_bh()
runs.
> spin_unlock_irqrestore(&led_lock, flags);
>
> Moreover, the process of calling the set_leds() function is
> the process of calling the kbd_bh function:
> static void set_leds(void)
> {
> tasklet_schedule(&keyboard_tasklet);
> }
> static DECLARE_TASKLET_DISABLED(keyboard_tasklet, kbd_bh);
>
> I don't really understand what you mean here, but one thing
> can be confirmed, my patch just synchronizes the current input
> device's led state to ledstate. Moreover, after VT's
> kb->ledflagstate is set to the input device, it will also
> be synchronized to ledstate (the original logic of the kbd_bh()
> function), which does not destroy the original internal logic of
> VT. In addition, I have tested it, whether it is switching
> between the desktop environment (tty1) and tty2~6, or switching
> between tty2~6, the indicator status of the keyboard light is
> correct, and it is normal in the multi-keyboard state. .
And I keep telling you that your approach to solving the problem is not
correct because state of a random input device is not necessarily
connected to the state of a VT.
Thanks.
--
Dmitry