Date:   Mon, 8 Nov 2021 09:33:56 +0800
From:   "lianzhi chang" <changlianzhi@...ontech.com>
To:     "dmitry.torokhov" <dmitry.torokhov@...il.com>
Cc:     "linux-kernel" <linux-kernel@...r.kernel.org>,
        "Greg KH" <gregkh@...uxfoundation.org>,
        "jirislaby" <jirislaby@...nel.org>,
        "Andy Shevchenko" 
        <andriy.shevchenko@...ux.intel.com>,
        "282827961" <282827961@...com>
Subject: Re: [PATCH v12] tty: Fix the keyboard led light display problem

Hi ,Dmitry
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@...ontech.com>
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@...ux.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).
Assuming that Xorg is bound to tty1, the kb->kbdmode of tty1 is equal to VC_OFF, and tty1 
will not set the keyboard light. If there is no restriction here, when we switch from other tty to
tty1, the value of vt_switch will always be true, which is not good.

> >  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?
In the kbd_keycode function, there is already this restriction:

At this time, the state of the keyboard light cannot be modified through the NumLock and
other keys of the keyboard. Earlier emails also said that it is for this reason that ledstate and
kb->ledflagstate under tty1 (assuming Xorg is bound to tty1) will not be modified.
However, the limitation here does not affect the scene of switching from other tty (tty2～6) 
to tty1. As a result, when switching from another tty to tty1, the keyboard light state may be
set to the value of kbd->ledflagstate of tty1 (leds!=ledstate). The setting here will interfere 
with the setting of system software such as Xorg, which is unreasonable.
Therefore, the restriction added by patch here should be reasonable.

> > +
> >  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).
Okay, I will revise this.

Thanks.
--
lianzhi chang

Powered by blists - more mailing lists