Re: [PATCH v7] tty: Fix the keyboard led light display problem

[Greg KH <gregkh@linuxfoundation.org>]

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