lkml.org 
[lkml]   [2021]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2021-10-26 16:54    [W:0.669 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site