Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 5 Nov 2021 15:36:55 -0700
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 v12] tty: Fix the keyboard led light display problem
Message-ID: <YYWyB1UmQxo0a2WU@google.com>
References: <20211105134816.13982-1-changlianzhi@uniontech.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211105134816.13982-1-changlianzhi@uniontech.com>
Precedence: bulk

Hi lianzhi
On Fri, Nov 05, 2021 at 09:48:16PM +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.
> 
> Signed-off-by: lianzhi chang <changlianzhi@uniontech.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  v10:
>  The led state of the input device is no longer synchronized to
>  ledstate, and the related code is deleted. The current plan is
>  changed to: when the VT is switched, the LED state saved by the
>  current VT is forced to be issued.
>  v11:
>  Supplement the signature of the collaborator.
>  
>  drivers/tty/vt/keyboard.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..20013c45b979 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,8 +413,12 @@ 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;
>  
> +	kb = kbd_table + fg_console;
> +	if (kb->kbdmode != VC_OFF)
> +		vt_switch = true;

Could you please add explanation here why it is not racy (I do not think
it is as I believe currently executed tasklets can be scheduled).

>  	set_leds();
>  
>  	spin_lock_irqsave(&kbd_event_lock, flags);
> @@ -1247,14 +1252,24 @@ 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;

Why do we need to do this? Won't this stop setting arbitrary LEDs via
console ioctl?

> +
>  	spin_lock_irqsave(&led_lock, flags);
>  	leds = getleds();
>  	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;
> @@ -1643,6 +1658,8 @@ int __init kbd_init(void)
>  	int i;
>  	int error;
>  
> +	vt_switch = false;

No need to explicitly initialize it here, as a static variable it will
be automatically initialized to 0 (false).

> +
>  	for (i = 0; i < MAX_NR_CONSOLES; i++) {
>  		kbd_table[i].ledflagstate = kbd_defleds();
>  		kbd_table[i].default_ledflagstate = kbd_defleds();

Thanks.

-- 
Dmitry