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

[dmitry.torokhov]

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