Message-ID: <tencent_0215A22726EA8F7807FF43A9@qq.com>
Date:   Tue, 2 Nov 2021 11:16:56 +0800
From:   "常廉志" <changlianzhi@...ontech.com>
To:     "dmitry.torokhov" <dmitry.torokhov@...il.com>,
        "Greg KH" <gregkh@...uxfoundation.org>
Cc:     "linux-kernel" <linux-kernel@...r.kernel.org>,
        "jirislaby" <jirislaby@...nel.org>,
        "Andy Shevchenko" 
        <andriy.shevchenko@...ux.intel.com>,
        "282827961" <282827961@...com>
Subject: Re: [PATCH v9] tty: Fix the keyboard led light display problem

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 <changlianzhi@...ontech.com>
> > > > ---
> > > >  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

Powered by blists - more mailing lists