Messages in this thread | | | Date | Tue, 26 Oct 2021 16:53:32 +0200 | From | Greg KH <> | Subject | Re: [PATCH v7] tty: Fix the keyboard led light display problem |
| |
On Tue, Oct 26, 2021 at 05:23:35PM +0300, Andy Shevchenko wrote: > On Tue, Oct 26, 2021 at 10:40:32AM +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 > > Xorg > > I think I already pointed that out. > > > 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. > > ... > > > +static void kbd_update_ledstate(struct input_dev *dev) > > +{ > > + if (!!test_bit(LED_NUML, dev->led) != > > + !!(ledstate & BIT(VC_NUMLOCK))) > > + ledstate ^= BIT(VC_NUMLOCK); > > + if (!!test_bit(LED_CAPSL, dev->led) != > > + !!(ledstate & BIT(VC_CAPSLOCK))) > > + ledstate ^= BIT(VC_CAPSLOCK); > > + if (!!test_bit(LED_SCROLLL, dev->led) != > > + !!(ledstate & BIT(VC_SCROLLOCK))) > > + ledstate ^= BIT(VC_SCROLLOCK); > > This looks ugly.
Agreed, we also have the vc_kbd_led() and friend functions that seem to be ignored here :(
> > Since LED_* are part of uAPI and VC_* are not, perhaps > makes sense to modify kbd_kern.h and optimize above > (disclaimer: compile tested only, no locking or other > synchronization have been checked / reviewed / etc): > > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c > index c7fbbcdcc346..2b04eaa5c6fd 100644 > --- a/drivers/tty/vt/keyboard.c > +++ b/drivers/tty/vt/keyboard.c > @@ -49,6 +49,11 @@ > > #include <asm/irq_regs.h> > > +/* We rely on these in kbd_update_ledstate() */ > +static_assert(VC_NUMLOCK == LED_NUML); > +static_assert(VC_CAPSLOCK == LED_CAPSL); > +static_assert(VC_SCROLLOCK == LED_SCROLLL);
This makes more sense, these should be in sync.
> + > /* > * Exported functions/variables > */ > @@ -1130,6 +1135,15 @@ static void kbd_init_leds(void) > > #endif > > +static void kbd_update_ledstate(struct input_dev *dev) > +{ > + unsigned long leds = BIT(LED_NUML) | BIT(LED_CAPSL) | BIT(LED_SCROLLL); > + unsigned long newstate = *dev->led; > + > + newstate ^= ledstate; > + ledstate ^= newstate & leds;
You should document the bit twiddling hack you are doing here just to make it easier to understand :)
Also, ledstate is an unsigned int, not unsigned long, perhaps make them all u32 for now to make it obvious they are the same? Or u64?
thanks,
greg k-h
| |