Date:   Mon, 20 Dec 2021 14:25:41 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     lianzhi chang <changlianzhi@uniontech.com>
Cc:     linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, andriy.shevchenko@linux.intel.com,
        282827961@qq.com
Subject: Re: [PATCH v22] tty: Fix the keyboard led light display problem
Message-ID: <YcEC5XOdRwWHz1di@google.com>
References: <20211215125125.10554-1-changlianzhi@uniontech.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211215125125.10554-1-changlianzhi@uniontech.com>
Precedence: bulk

On Wed, Dec 15, 2021 at 08:51:25PM +0800, lianzhi chang wrote:
> Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> desktop or switch the system from desktop to tty. After the switch is
> completed, it is found that the state of the keyboard lock is
> inconsistent with the state of the keyboard Led light.The reasons are
> as follows:
> 
> * The desktop environment (Xorg and other services) is bound to a tty
>   (assuming it is tty1), and the kb->kbdmode attribute value of tty1
>   will be set to VC_OFF. According to the current code logic, in the
>   desktop environment, the values of ledstate and kb->ledflagstate
>   of tty1 will not be modified anymore, so they are always 0.
> 
> * When switching between each tty, the final value of ledstate set by
>   the previous tty is compared with the kb->ledflagstate value of the
>   current tty to determine whether to set the state of the keyboard
>   light. The process of switching between desktop and tty is also the
>   process of switching between tty1 and other ttys. There are two
>   situations:
> 
>   - (1) In the desktop environment, tty1 will not set the ledstate,
>   which will cause when switching from the desktop to other ttys,
>   if the desktop lights up the keyboard's led, after the switch is
>   completed, the keyboard's led light will always be on;
> 
>   - (2) When switching from another tty to the desktop, this
>   mechanism will trigger tty1 to set the led state. If other tty
>   lights up the led of the keyboard before switching to the desktop,
>   the led will be forcibly turned off. This situation should
>   be avoided.
> 
> * The current patch is to solve these problems: When VT is switched,
>   the keyboard led needs to be set once.Ensure that after the
>   switch is completed, the state of the keyboard LED is consistent
>   with the state of the keyboard lock.
> 
> Signed-off-by: lianzhi chang <changlianzhi@uniontech.com>
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  v22:
>  Return to the v19 plan, remove the judgment condition in
>  the vt_set_leds_compute_shiftstate() function.

Thank you for making the changes. The patch looks OK to me now.

>  
>  drivers/tty/vt/keyboard.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..00283ba9a9e4 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
> @@ -414,6 +415,12 @@ void vt_set_leds_compute_shiftstate(void)
>  {
>  	unsigned long flags;
>  
> +	/*
> +	 * When VT is switched, the keyboard led needs to be set once.
> +	 * Ensure that after the switch is completed, the state of the
> +	 * keyboard LED is consistent with the state of the keyboard lock.
> +	 */
> +	vt_switch = true;
>  	set_leds();
>  
>  	spin_lock_irqsave(&kbd_event_lock, flags);
> @@ -1255,6 +1262,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
> 
> 
> 

-- 
Dmitry