Use the "ctrl+alt+Fn" key combination to switch the system from tty to
desktop or switch the system from desktop to tty. After the switch is
completed, it is found that the state of the keyboard lock is
inconsistent with the state of the keyboard Led light.The reasons are
as follows:
* The desktop environment (Xorg and other services) is bound to a tty
(assuming it is tty1), and the kb->kbdmode attribute value of tty1
will be set to VC_OFF. According to the current code logic, in the
desktop environment, the values of ledstate and kb->ledflagstate
of tty1 will not be modified anymore, so they are always 0.
* When switching between each tty, the final value of ledstate set by
the previous tty is compared with the kb->ledflagstate value of the
current tty to determine whether to set the state of the keyboard
light. The process of switching between desktop and tty is also the
process of switching between tty1 and other ttys. There are two
situations:
- (1) In the desktop environment, tty1 will not set the ledstate,
which will cause when switching from the desktop to other ttys,
if the desktop lights up the keyboard's led, after the switch is
completed, the keyboard's led light will always be on;
- (2) When switching from another tty to the desktop, this
mechanism will trigger tty1 to set the led state. If other tty
lights up the led of the keyboard before switching to the desktop,
the led will be forcibly turned off. This situation should
be avoided.
* Current patch explanation:When VT is switched,the keyboard LED
status will be set to the current tty saved value;the value of
kb->kbdledctl can be used to confirm whether the current VT
can change the status of the keyboard led light;kb->kbdledctl
is a new addition,you can use ioctl get or set.
Signed-off-by: lianzhi chang <[email protected]>
Suggested-by: Dmitry Torokhov <[email protected]>
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
---
v20:
New solution: kbd_struct adds a new "kbdledctl" attribute,
which can be obtained or changed through ioctl;
"kbdledctl" is used to determine whether the current VT
can set the keyboard led light;
drivers/tty/vt/keyboard.c | 58 +++++++++++++++++++++++++++++++++++++++
drivers/tty/vt/vt_ioctl.c | 12 ++++++++
include/linux/kbd_kern.h | 4 +++
include/linux/vt_kern.h | 2 ++
include/uapi/linux/kd.h | 7 ++++-
5 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c7fbbcdcc346..248c8d790d91 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
@@ -414,6 +415,12 @@ void vt_set_leds_compute_shiftstate(void)
{
unsigned long flags;
+ /*
+ * When switching VT,according to the value of vt_switch,
+ * judge whether it is necessary to force the keyboard light
+ * state to be issued.
+ */
+ vt_switch = true;
set_leds();
spin_lock_irqsave(&kbd_event_lock, flags);
@@ -1247,14 +1254,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->kbdledctl == VC_LEDCTL_OFF)
+ return;
+
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;
@@ -1857,6 +1874,35 @@ int vt_do_kdskbmode(unsigned int console, unsigned int arg)
return ret;
}
+/**
+ * vt_do_kdskbledctl
+ * @console: the console to use
+ * @arg: the requested mode
+ *
+ * Whether to allow the current vt to change the
+ * keyboard light
+ */
+int vt_do_kdskbledctl(unsigned int console, unsigned int arg)
+{
+ struct kbd_struct *kb = &kbd_table[console];
+ int ret = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&kbd_event_lock, flags);
+ switch (arg) {
+ case K_LEDCTL_ON:
+ kb->kbdledctl = VC_LEDCTL_ON;
+ break;
+ case K_LEDCTL_OFF:
+ kb->kbdledctl = VC_LEDCTL_OFF;
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ spin_unlock_irqrestore(&kbd_event_lock, flags);
+ return ret;
+}
+
/**
* vt_do_kdskbmeta - set keyboard meta state
* @console: the console to use
@@ -2157,6 +2203,18 @@ int vt_do_kdgkbmode(unsigned int console)
}
}
+int vt_do_kdgkbledctl(unsigned int console)
+{
+ struct kbd_struct *kb = &kbd_table[console];
+ /* This is a spot read so needs no locking */
+ switch (kb->kbdledctl) {
+ case VC_LEDCTL_ON:
+ return K_LEDCTL_ON;
+ case VC_LEDCTL_OFF:
+ return K_LEDCTL_OFF;
+ }
+}
+
/**
* vt_do_kdgkbmeta - report meta status
* @console: console to report
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 3639bb6dc372..c78bd452af55 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -405,6 +405,18 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
case KDGKBMODE:
return put_user(vt_do_kdgkbmode(console), (int __user *)arg);
+ case KDSKBLEDCTL:
+ if (!perm)
+ return -EPERM;
+ ret = vt_do_kdskbledctl(console, arg);
+ if (ret)
+ return ret;
+ tty_ldisc_flush(tty);
+ break;
+
+ case KDGKBLEDCTL:
+ return put_user(vt_do_kdgkbledctl(console), (int __user *)arg);
+
/* this could be folded into KDSKBMODE, but for compatibility
reasons it is not so easy to fold KDGKBMETA into KDGKBMODE */
case KDSKBMETA:
diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
index c40811d79769..769b946c2cf9 100644
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -32,6 +32,10 @@ struct kbd_struct {
#define VC_CTRLRLOCK KG_CTRLR /* ctrlr lock mode */
unsigned char slockstate; /* for `sticky' Shift, Ctrl, etc. */
+ unsigned char kbdledctl:1； /*Whether to allow to control the led of the keyboard */
+#define VC_LEDCTL_ON 0 /* VT can set the keyboard light */
+#define VC_LEDCTL_OFF 1 /* Prohibit VT to set the keyboard light */
+
unsigned char ledmode:1;
#define LED_SHOW_FLAGS 0 /* traditional state */
#define LED_SHOW_IOCTL 1 /* only change leds upon ioctl */
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index b5ab452fca5b..67bdf3eddb5a 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -149,6 +149,7 @@ void hide_boot_cursor(bool hide);
/* keyboard provided interfaces */
int vt_do_diacrit(unsigned int cmd, void __user *up, int eperm);
int vt_do_kdskbmode(unsigned int console, unsigned int arg);
+int vt_do_kdskbledctl(unsigned int console, unsigned int arg);
int vt_do_kdskbmeta(unsigned int console, unsigned int arg);
int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc,
int perm);
@@ -157,6 +158,7 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm);
int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm);
int vt_do_kdgkbmode(unsigned int console);
+int vt_do_kdgkbledctl(unsigned int console);
int vt_do_kdgkbmeta(unsigned int console);
void vt_reset_unicode(unsigned int console);
int vt_get_shift_state(void);
diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
index ee929ece4112..138d636adf3e 100644
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -86,6 +86,11 @@ struct unimapinit {
#define KDGKBMODE 0x4B44 /* gets current keyboard mode */
#define KDSKBMODE 0x4B45 /* sets current keyboard mode */
+#define K_LEDCTL_ON 0x00
+#define K_LEDCTL_OFF 0x01
+#define KDGKBLEDCTL 0x4B73 /* set whether to allow control of keyboard lights */
+#define KDSKBLEDCTL 0x4B74 /* get whether the keyboard light is currently allowed to be set */
+
#define K_METABIT 0x03
#define K_ESCPREFIX 0x04
#define KDGKBMETA 0x4B62 /* gets meta key handling mode */
@@ -179,6 +184,6 @@ struct console_font {
/* note: 0x4B00-0x4B4E all have had a value at some time;
don't reuse for the time being */
-/* note: 0x4B60-0x4B6D, 0x4B70-0x4B72 used above */
+/* note: 0x4B60-0x4B6D, 0x4B70-0x4B74 used above */
#endif /* _UAPI_LINUX_KD_H */
--
2.20.1
On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> desktop or switch the system from desktop to tty. After the switch is
> completed, it is found that the state of the keyboard lock is
> inconsistent with the state of the keyboard Led light.The reasons are
> as follows:
>
> * The desktop environment (Xorg and other services) is bound to a tty
> (assuming it is tty1), and the kb->kbdmode attribute value of tty1
> will be set to VC_OFF. According to the current code logic, in the
> desktop environment, the values of ledstate and kb->ledflagstate
> of tty1 will not be modified anymore, so they are always 0.
>
> * When switching between each tty, the final value of ledstate set by
> the previous tty is compared with the kb->ledflagstate value of the
> current tty to determine whether to set the state of the keyboard
> light. The process of switching between desktop and tty is also the
> process of switching between tty1 and other ttys. There are two
> situations:
>
> - (1) In the desktop environment, tty1 will not set the ledstate,
> which will cause when switching from the desktop to other ttys,
> if the desktop lights up the keyboard's led, after the switch is
> completed, the keyboard's led light will always be on;
>
> - (2) When switching from another tty to the desktop, this
> mechanism will trigger tty1 to set the led state. If other tty
> lights up the led of the keyboard before switching to the desktop,
> the led will be forcibly turned off. This situation should
> be avoided.
>
> * Current patch explanation:When VT is switched,the keyboard LED
> status will be set to the current tty saved value;the value of
> kb->kbdledctl can be used to confirm whether the current VT
> can change the status of the keyboard led light;kb->kbdledctl
> is a new addition,you can use ioctl get or set.
>
> Signed-off-by: lianzhi chang <[email protected]>
> Suggested-by: Dmitry Torokhov <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
I believe the original issue hasn't been reported by above mentioned
CIs/people. If you want to give a credit, use changelog for that.
...
> +/**
> + * vt_do_kdskbledctl
Description?
> + * @console: the console to use
> + * @arg: the requested mode
> + *
> + * Whether to allow the current vt to change the
> + * keyboard light
> + */
...
> + struct kbd_struct *kb = &kbd_table[console];
> + int ret = 0;
> + unsigned long flags;
Slightly better to read:
struct kbd_struct *kb = &kbd_table[console];
unsigned long flags;
int ret = 0;
...
> +int vt_do_kdgkbledctl(unsigned int console)
> +{
> + struct kbd_struct *kb = &kbd_table[console];
+ Blank line here. Checkpatch should complain.
> + /* This is a spot read so needs no locking */
> + switch (kb->kbdledctl) {
> + case VC_LEDCTL_ON:
> + return K_LEDCTL_ON;
> + case VC_LEDCTL_OFF:
> + return K_LEDCTL_OFF;
> + }
Indentation issue.
Consider adding default case.
> +}
...
> + unsigned char kbdledctl:1； /*Whether to allow to control the led of the keyboard */
Missed space in the comment.
--
With Best Regards,
Andy Shevchenko
On Mon, 2021-12-13 at 14:15 +0200, Andy Shevchenko wrote:
> On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> > + struct kbd_struct *kb = &kbd_table[console];
> > + int ret = 0;
> > + unsigned long flags;
>
> Slightly better to read:
>
> struct kbd_struct *kb = &kbd_table[console];
> unsigned long flags;
> int ret = 0;
I don't think so. Why do you?
"the keyboard led light display problem" is pretty vague. Perhaps say
"Fix incorrect "Num Lock" LED indicator"
On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> desktop or switch the system from desktop to tty. After the switch is
> completed, it is found that the state of the keyboard lock is
Is this one of those fancy gaming keyboards with LEDs under the keys or
are we talking about Num Lock?
> +int vt_do_kdgkbledctl(unsigned int console)
> +{
> + struct kbd_struct *kb = &kbd_table[console];
> + /* This is a spot read so needs no locking */
> + switch (kb->kbdledctl) {
> + case VC_LEDCTL_ON:
> + return K_LEDCTL_ON;
> + case VC_LEDCTL_OFF:
> + return K_LEDCTL_OFF;
> + }
Extra tab.
> +}
> +
> +#define VC_LEDCTL_ON 0 /* VT can set the keyboard light */
> +#define VC_LEDCTL_OFF 1 /* Prohibit VT to set the keyboard light */
> +#define K_LEDCTL_ON 0x00
> +#define K_LEDCTL_OFF 0x01
It's weird that ON is zero and OFF one. Really, it's unfortunate that
we need a new ioctl to fix this bug...
regards,
dan carpenter
On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> desktop or switch the system from desktop to tty. After the switch is
> completed, it is found that the state of the keyboard lock is
> inconsistent with the state of the keyboard Led light.The reasons are
> as follows:
>
> * The desktop environment (Xorg and other services) is bound to a tty
> (assuming it is tty1), and the kb->kbdmode attribute value of tty1
> will be set to VC_OFF. According to the current code logic, in the
> desktop environment, the values of ledstate and kb->ledflagstate
> of tty1 will not be modified anymore, so they are always 0.
>
> * When switching between each tty, the final value of ledstate set by
> the previous tty is compared with the kb->ledflagstate value of the
> current tty to determine whether to set the state of the keyboard
> light. The process of switching between desktop and tty is also the
> process of switching between tty1 and other ttys. There are two
> situations:
>
> - (1) In the desktop environment, tty1 will not set the ledstate,
> which will cause when switching from the desktop to other ttys,
> if the desktop lights up the keyboard's led, after the switch is
> completed, the keyboard's led light will always be on;
>
> - (2) When switching from another tty to the desktop, this
> mechanism will trigger tty1 to set the led state. If other tty
> lights up the led of the keyboard before switching to the desktop,
> the led will be forcibly turned off. This situation should
> be avoided.
>
> * Current patch explanation:When VT is switched,the keyboard LED
> status will be set to the current tty saved value;the value of
> kb->kbdledctl can be used to confirm whether the current VT
> can change the status of the keyboard led light;kb->kbdledctl
> is a new addition,you can use ioctl get or set.
>
> Signed-off-by: lianzhi chang <[email protected]>
> Suggested-by: Dmitry Torokhov <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> ---
>
> v20:
> New solution: kbd_struct adds a new "kbdledctl" attribute,
> which can be obtained or changed through ioctl;
> "kbdledctl" is used to determine whether the current VT
> can set the keyboard led light;
>
> drivers/tty/vt/keyboard.c | 58 +++++++++++++++++++++++++++++++++++++++
> drivers/tty/vt/vt_ioctl.c | 12 ++++++++
> include/linux/kbd_kern.h | 4 +++
> include/linux/vt_kern.h | 2 ++
> include/uapi/linux/kd.h | 7 ++++-
> 5 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..248c8d790d91 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;
This is an odd name for a variable that is there for led issues, right?
> /*
> * Notifier list for console keyboard events
> @@ -414,6 +415,12 @@ void vt_set_leds_compute_shiftstate(void)
> {
> unsigned long flags;
>
> + /*
> + * When switching VT,according to the value of vt_switch,
Extra space after ',' please.
> + * judge whether it is necessary to force the keyboard light
> + * state to be issued.
> + */
> + vt_switch = true;
> set_leds();
How does this new variable have anything to do with the set_leds() call?
>
> spin_lock_irqsave(&kbd_event_lock, flags);
> @@ -1247,14 +1254,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;
So an pointer to an array? Raw pointer math is odd.
And no bounds checking?
> + if (kb->kbdledctl == VC_LEDCTL_OFF)
> + return;
Why exit now?
> +
> 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;
> + }
Please explain what you are doing here with vt_switch.
> +
> if (leds != ledstate) {
> kbd_propagate_led_state(ledstate, leds);
> ledstate = leds;
> @@ -1857,6 +1874,35 @@ int vt_do_kdskbmode(unsigned int console, unsigned int arg)
> return ret;
> }
>
> +/**
> + * vt_do_kdskbledctl
> + * @console: the console to use
> + * @arg: the requested mode
> + *
> + * Whether to allow the current vt to change the
> + * keyboard light
> + */
> +int vt_do_kdskbledctl(unsigned int console, unsigned int arg)
> +{
> + struct kbd_struct *kb = &kbd_table[console];
> + int ret = 0;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&kbd_event_lock, flags);
> + switch (arg) {
> + case K_LEDCTL_ON:
> + kb->kbdledctl = VC_LEDCTL_ON;
> + break;
> + case K_LEDCTL_OFF:
> + kb->kbdledctl = VC_LEDCTL_OFF;
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + spin_unlock_irqrestore(&kbd_event_lock, flags);
> + return ret;
> +}
> +
> /**
> * vt_do_kdskbmeta - set keyboard meta state
> * @console: the console to use
> @@ -2157,6 +2203,18 @@ int vt_do_kdgkbmode(unsigned int console)
> }
> }
>
> +int vt_do_kdgkbledctl(unsigned int console)
> +{
> + struct kbd_struct *kb = &kbd_table[console];
> + /* This is a spot read so needs no locking */
Blank line before comment?
And what do you mean by "spot read"?
> + switch (kb->kbdledctl) {
> + case VC_LEDCTL_ON:
> + return K_LEDCTL_ON;
> + case VC_LEDCTL_OFF:
> + return K_LEDCTL_OFF;
> + }
Why does usespace need to know this value? What will it do with it?
> +}
> +
> /**
> * vt_do_kdgkbmeta - report meta status
> * @console: console to report
> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index 3639bb6dc372..c78bd452af55 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -405,6 +405,18 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> case KDGKBMODE:
> return put_user(vt_do_kdgkbmode(console), (int __user *)arg);
>
> + case KDSKBLEDCTL:
> + if (!perm)
> + return -EPERM;
> + ret = vt_do_kdskbledctl(console, arg);
> + if (ret)
> + return ret;
> + tty_ldisc_flush(tty);
> + break;
> +
> + case KDGKBLEDCTL:
> + return put_user(vt_do_kdgkbledctl(console), (int __user *)arg);
> +
> /* this could be folded into KDSKBMODE, but for compatibility
> reasons it is not so easy to fold KDGKBMETA into KDGKBMODE */
> case KDSKBMETA:
> diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
> index c40811d79769..769b946c2cf9 100644
> --- a/include/linux/kbd_kern.h
> +++ b/include/linux/kbd_kern.h
> @@ -32,6 +32,10 @@ struct kbd_struct {
> #define VC_CTRLRLOCK KG_CTRLR /* ctrlr lock mode */
> unsigned char slockstate; /* for `sticky' Shift, Ctrl, etc. */
>
> + unsigned char kbdledctl:1； /*Whether to allow to control the led of the keyboard */
> +#define VC_LEDCTL_ON 0 /* VT can set the keyboard light */
> +#define VC_LEDCTL_OFF 1 /* Prohibit VT to set the keyboard light */
> +
> unsigned char ledmode:1;
> #define LED_SHOW_FLAGS 0 /* traditional state */
> #define LED_SHOW_IOCTL 1 /* only change leds upon ioctl */
> diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> index b5ab452fca5b..67bdf3eddb5a 100644
> --- a/include/linux/vt_kern.h
> +++ b/include/linux/vt_kern.h
> @@ -149,6 +149,7 @@ void hide_boot_cursor(bool hide);
> /* keyboard provided interfaces */
> int vt_do_diacrit(unsigned int cmd, void __user *up, int eperm);
> int vt_do_kdskbmode(unsigned int console, unsigned int arg);
> +int vt_do_kdskbledctl(unsigned int console, unsigned int arg);
> int vt_do_kdskbmeta(unsigned int console, unsigned int arg);
> int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc,
> int perm);
> @@ -157,6 +158,7 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
> int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm);
> int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm);
> int vt_do_kdgkbmode(unsigned int console);
> +int vt_do_kdgkbledctl(unsigned int console);
> int vt_do_kdgkbmeta(unsigned int console);
> void vt_reset_unicode(unsigned int console);
> int vt_get_shift_state(void);
> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> index ee929ece4112..138d636adf3e 100644
> --- a/include/uapi/linux/kd.h
> +++ b/include/uapi/linux/kd.h
> @@ -86,6 +86,11 @@ struct unimapinit {
> #define KDGKBMODE 0x4B44 /* gets current keyboard mode */
> #define KDSKBMODE 0x4B45 /* sets current keyboard mode */
>
> +#define K_LEDCTL_ON 0x00
> +#define K_LEDCTL_OFF 0x01
As Dan points out, these should be the other way around.
> +#define KDGKBLEDCTL 0x4B73 /* set whether to allow control of keyboard lights */
> +#define KDSKBLEDCTL 0x4B74 /* get whether the keyboard light is currently allowed to be set */
What userspace code is going to use these new ioctls?
I still don't understand the problem that this is supposed to be
solving. How can we have never had a problem until now with regards to
LED settings on keyboards? What commit caused this to change? Has it
always been broken for 30 years and no one noticed?
thanks,
greg k-h
On Mon, Dec 13, 2021 at 02:36:12PM +0100, Greg KH wrote:
> On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> > Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> > desktop or switch the system from desktop to tty. After the switch is
> > completed, it is found that the state of the keyboard lock is
> > inconsistent with the state of the keyboard Led light.The reasons are
> > as follows:
> >
> > * The desktop environment (Xorg and other services) is bound to a tty
> > (assuming it is tty1), and the kb->kbdmode attribute value of tty1
> > will be set to VC_OFF. According to the current code logic, in the
> > desktop environment, the values of ledstate and kb->ledflagstate
> > of tty1 will not be modified anymore, so they are always 0.
> >
> > * When switching between each tty, the final value of ledstate set by
> > the previous tty is compared with the kb->ledflagstate value of the
> > current tty to determine whether to set the state of the keyboard
> > light. The process of switching between desktop and tty is also the
> > process of switching between tty1 and other ttys. There are two
> > situations:
> >
> > - (1) In the desktop environment, tty1 will not set the ledstate,
> > which will cause when switching from the desktop to other ttys,
> > if the desktop lights up the keyboard's led, after the switch is
> > completed, the keyboard's led light will always be on;
> >
> > - (2) When switching from another tty to the desktop, this
> > mechanism will trigger tty1 to set the led state. If other tty
> > lights up the led of the keyboard before switching to the desktop,
> > the led will be forcibly turned off. This situation should
> > be avoided.
> >
> > * Current patch explanation:When VT is switched,the keyboard LED
> > status will be set to the current tty saved value;the value of
> > kb->kbdledctl can be used to confirm whether the current VT
> > can change the status of the keyboard led light;kb->kbdledctl
> > is a new addition,you can use ioctl get or set.
> >
> > Signed-off-by: lianzhi chang <[email protected]>
> > Suggested-by: Dmitry Torokhov <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Reported-by: Dan Carpenter <[email protected]>
> > ---
> >
> > v20:
> > New solution: kbd_struct adds a new "kbdledctl" attribute,
> > which can be obtained or changed through ioctl;
> > "kbdledctl" is used to determine whether the current VT
> > can set the keyboard led light;
> >
> > drivers/tty/vt/keyboard.c | 58 +++++++++++++++++++++++++++++++++++++++
> > drivers/tty/vt/vt_ioctl.c | 12 ++++++++
> > include/linux/kbd_kern.h | 4 +++
> > include/linux/vt_kern.h | 2 ++
> > include/uapi/linux/kd.h | 7 ++++-
> > 5 files changed, 82 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> > index c7fbbcdcc346..248c8d790d91 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;
>
> This is an odd name for a variable that is there for led issues, right?
No this describes exactly what this variable represents.
>
> > /*
> > * Notifier list for console keyboard events
> > @@ -414,6 +415,12 @@ void vt_set_leds_compute_shiftstate(void)
> > {
> > unsigned long flags;
> >
> > + /*
> > + * When switching VT,according to the value of vt_switch,
>
> Extra space after ',' please.
>
> > + * judge whether it is necessary to force the keyboard light
> > + * state to be issued.
> > + */
> > + vt_switch = true;
> > set_leds();
>
> How does this new variable have anything to do with the set_leds() call?
>
> >
> > spin_lock_irqsave(&kbd_event_lock, flags);
> > @@ -1247,14 +1254,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;
>
> So an pointer to an array? Raw pointer math is odd.
>
> And no bounds checking?
>
> > + if (kb->kbdledctl == VC_LEDCTL_OFF)
> > + return;
>
> Why exit now?
>
> > +
> > 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;
> > + }
>
> Please explain what you are doing here with vt_switch.
>
>
> > +
> > if (leds != ledstate) {
> > kbd_propagate_led_state(ledstate, leds);
> > ledstate = leds;
> > @@ -1857,6 +1874,35 @@ int vt_do_kdskbmode(unsigned int console, unsigned int arg)
> > return ret;
> > }
> >
> > +/**
> > + * vt_do_kdskbledctl
> > + * @console: the console to use
> > + * @arg: the requested mode
> > + *
> > + * Whether to allow the current vt to change the
> > + * keyboard light
> > + */
> > +int vt_do_kdskbledctl(unsigned int console, unsigned int arg)
> > +{
> > + struct kbd_struct *kb = &kbd_table[console];
> > + int ret = 0;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&kbd_event_lock, flags);
> > + switch (arg) {
> > + case K_LEDCTL_ON:
> > + kb->kbdledctl = VC_LEDCTL_ON;
> > + break;
> > + case K_LEDCTL_OFF:
> > + kb->kbdledctl = VC_LEDCTL_OFF;
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > + spin_unlock_irqrestore(&kbd_event_lock, flags);
> > + return ret;
> > +}
> > +
> > /**
> > * vt_do_kdskbmeta - set keyboard meta state
> > * @console: the console to use
> > @@ -2157,6 +2203,18 @@ int vt_do_kdgkbmode(unsigned int console)
> > }
> > }
> >
> > +int vt_do_kdgkbledctl(unsigned int console)
> > +{
> > + struct kbd_struct *kb = &kbd_table[console];
> > + /* This is a spot read so needs no locking */
>
> Blank line before comment?
>
> And what do you mean by "spot read"?
>
> > + switch (kb->kbdledctl) {
> > + case VC_LEDCTL_ON:
> > + return K_LEDCTL_ON;
> > + case VC_LEDCTL_OFF:
> > + return K_LEDCTL_OFF;
> > + }
>
> Why does usespace need to know this value? What will it do with it?
>
> > +}
> > +
> > /**
> > * vt_do_kdgkbmeta - report meta status
> > * @console: console to report
> > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > index 3639bb6dc372..c78bd452af55 100644
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -405,6 +405,18 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> > case KDGKBMODE:
> > return put_user(vt_do_kdgkbmode(console), (int __user *)arg);
> >
> > + case KDSKBLEDCTL:
> > + if (!perm)
> > + return -EPERM;
> > + ret = vt_do_kdskbledctl(console, arg);
> > + if (ret)
> > + return ret;
> > + tty_ldisc_flush(tty);
> > + break;
> > +
> > + case KDGKBLEDCTL:
> > + return put_user(vt_do_kdgkbledctl(console), (int __user *)arg);
> > +
> > /* this could be folded into KDSKBMODE, but for compatibility
> > reasons it is not so easy to fold KDGKBMETA into KDGKBMODE */
> > case KDSKBMETA:
> > diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
> > index c40811d79769..769b946c2cf9 100644
> > --- a/include/linux/kbd_kern.h
> > +++ b/include/linux/kbd_kern.h
> > @@ -32,6 +32,10 @@ struct kbd_struct {
> > #define VC_CTRLRLOCK KG_CTRLR /* ctrlr lock mode */
> > unsigned char slockstate; /* for `sticky' Shift, Ctrl, etc. */
> >
> > + unsigned char kbdledctl:1； /*Whether to allow to control the led of the keyboard */
> > +#define VC_LEDCTL_ON 0 /* VT can set the keyboard light */
> > +#define VC_LEDCTL_OFF 1 /* Prohibit VT to set the keyboard light */
> > +
> > unsigned char ledmode:1;
> > #define LED_SHOW_FLAGS 0 /* traditional state */
> > #define LED_SHOW_IOCTL 1 /* only change leds upon ioctl */
> > diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> > index b5ab452fca5b..67bdf3eddb5a 100644
> > --- a/include/linux/vt_kern.h
> > +++ b/include/linux/vt_kern.h
> > @@ -149,6 +149,7 @@ void hide_boot_cursor(bool hide);
> > /* keyboard provided interfaces */
> > int vt_do_diacrit(unsigned int cmd, void __user *up, int eperm);
> > int vt_do_kdskbmode(unsigned int console, unsigned int arg);
> > +int vt_do_kdskbledctl(unsigned int console, unsigned int arg);
> > int vt_do_kdskbmeta(unsigned int console, unsigned int arg);
> > int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc,
> > int perm);
> > @@ -157,6 +158,7 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
> > int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm);
> > int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm);
> > int vt_do_kdgkbmode(unsigned int console);
> > +int vt_do_kdgkbledctl(unsigned int console);
> > int vt_do_kdgkbmeta(unsigned int console);
> > void vt_reset_unicode(unsigned int console);
> > int vt_get_shift_state(void);
> > diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> > index ee929ece4112..138d636adf3e 100644
> > --- a/include/uapi/linux/kd.h
> > +++ b/include/uapi/linux/kd.h
> > @@ -86,6 +86,11 @@ struct unimapinit {
> > #define KDGKBMODE 0x4B44 /* gets current keyboard mode */
> > #define KDSKBMODE 0x4B45 /* sets current keyboard mode */
> >
> > +#define K_LEDCTL_ON 0x00
> > +#define K_LEDCTL_OFF 0x01
>
> As Dan points out, these should be the other way around.
>
> > +#define KDGKBLEDCTL 0x4B73 /* set whether to allow control of keyboard lights */
> > +#define KDSKBLEDCTL 0x4B74 /* get whether the keyboard light is currently allowed to be set */
>
> What userspace code is going to use these new ioctls?
>
> I still don't understand the problem that this is supposed to be
> solving. How can we have never had a problem until now with regards to
> LED settings on keyboards? What commit caused this to change? Has it
> always been broken for 30 years and no one noticed?
Yes, it's been going on since forever (I guess at least 2.6 where input
core was introduced) and nobody really cared as very few people bounce
between graphical environment and VTs _and_ use CapsLock or NumLock
_and_ have keyboards with these LEDs).
Now, there is a couple-line solution that was in v19, lianzhi chang just
needed to drop condition that was being introduced in
vt_set_leds_compute_shiftstate(). That would ensure that proper state of
LEDs is restored on every VT switch. It also might have resulted in
LEDs switching momentarily off before turning on according to the
graphical desktop preferences, but I do not thing this is a big issue.
Now we are building this new infra with new ioctls, etc, for miniscule
gain of avoiding this blink. I do not believe this is worth it.
Thanks.
--
Dmitry
On Mon, Dec 13, 2021 at 04:41:24AM -0800, Joe Perches wrote:
> On Mon, 2021-12-13 at 14:15 +0200, Andy Shevchenko wrote:
> > On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
>
> > > + struct kbd_struct *kb = &kbd_table[console];
> > > + int ret = 0;
> > > + unsigned long flags;
> >
> > Slightly better to read:
> >
> > struct kbd_struct *kb = &kbd_table[console];
> > unsigned long flags;
> > int ret = 0;
>
> I don't think so. Why do you?
I wonder why we comment on cosmetics of a patch (and have the submitter
rush to fix that) without considering if the proposed solution makes
sense in the first place?
Thanks.
--
Dmitry
On Mon, Dec 13, 2021 at 11:41:09AM -0800, Dmitry Torokhov wrote:
> On Mon, Dec 13, 2021 at 04:41:24AM -0800, Joe Perches wrote:
> > On Mon, 2021-12-13 at 14:15 +0200, Andy Shevchenko wrote:
> > > On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> >
> > > > + struct kbd_struct *kb = &kbd_table[console];
> > > > + int ret = 0;
> > > > + unsigned long flags;
> > >
> > > Slightly better to read:
> > >
> > > struct kbd_struct *kb = &kbd_table[console];
> > > unsigned long flags;
> > > int ret = 0;
> >
> > I don't think so. Why do you?
>
> I wonder why we comment on cosmetics of a patch
> (and have the submitter rush to fix that)
Not sure where you got this from...
> without considering if the proposed solution makes
> sense in the first place?
...but answering your question it's quite natural in open source mailing list
that people give a comment regarding their area of expertise. One does for
style another for the whole solution. I don't think anything is wrong with this
approach, do you?
--
With Best Regards,
Andy Shevchenko
On Mon, Dec 13, 2021 at 10:06:12PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 13, 2021 at 11:41:09AM -0800, Dmitry Torokhov wrote:
> > On Mon, Dec 13, 2021 at 04:41:24AM -0800, Joe Perches wrote:
> > > On Mon, 2021-12-13 at 14:15 +0200, Andy Shevchenko wrote:
> > > > On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> > >
> > > > > + struct kbd_struct *kb = &kbd_table[console];
> > > > > + int ret = 0;
> > > > > + unsigned long flags;
> > > >
> > > > Slightly better to read:
> > > >
> > > > struct kbd_struct *kb = &kbd_table[console];
> > > > unsigned long flags;
> > > > int ret = 0;
> > >
> > > I don't think so. Why do you?
> >
> > I wonder why we comment on cosmetics of a patch
>
> > (and have the submitter rush to fix that)
>
> Not sure where you got this from...
That is people's natural reaction.
>
> > without considering if the proposed solution makes
> > sense in the first place?
>
> ...but answering your question it's quite natural in open source mailing list
> that people give a comment regarding their area of expertise. One does for
> style another for the whole solution. I don't think anything is wrong with this
> approach, do you?
I think it depends. It is definitely fine when we are putting finishing
touches on a patch, or when there is little indication that the patch is
uncontroversial. In this case we are on iteration #20, with several
initial approaches to solving the problem have been rejected and the
latest one out of blue introduced a whole lot of new
functionality/public facing ABI, so I think waiting a bit to see if this
brand new approach is something that is viable would be prudent.
Thanks.
--
Dmitry
> On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> > > Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> > > desktop or switch the system from desktop to tty. After the switch is
> > > completed, it is found that the state of the keyboard lock is
> > > inconsistent with the state of the keyboard Led light.The reasons are
> > > as follows:
> > >
> > > * The desktop environment (Xorg and other services) is bound to a tty
> > > (assuming it is tty1), and the kb->kbdmode attribute value of tty1
> > > will be set to VC_OFF. According to the current code logic, in the
> > > desktop environment, the values of ledstate and kb->ledflagstate
> > > of tty1 will not be modified anymore, so they are always 0.
> > >
> > > * When switching between each tty, the final value of ledstate set by
> > > the previous tty is compared with the kb->ledflagstate value of the
> > > current tty to determine whether to set the state of the keyboard
> > > light. The process of switching between desktop and tty is also the
> > > process of switching between tty1 and other ttys. There are two
> > > situations:
> > >
> > > - (1) In the desktop environment, tty1 will not set the ledstate,
> > > which will cause when switching from the desktop to other ttys,
> > > if the desktop lights up the keyboard's led, after the switch is
> > > completed, the keyboard's led light will always be on;
> > >
> > > - (2) When switching from another tty to the desktop, this
> > > mechanism will trigger tty1 to set the led state. If other tty
> > > lights up the led of the keyboard before switching to the desktop,
> > > the led will be forcibly turned off. This situation should
> > > be avoided.
> > >
> > > * Current patch explanation:When VT is switched,the keyboard LED
> > > status will be set to the current tty saved value;the value of
> > > kb->kbdledctl can be used to confirm whether the current VT
> > > can change the status of the keyboard led light;kb->kbdledctl
> > > is a new addition,you can use ioctl get or set.
> > >
> > > Signed-off-by: lianzhi chang <[email protected]>
> > > Suggested-by: Dmitry Torokhov <[email protected]>
> > > Reported-by: kernel test robot <[email protected]>
> > > Reported-by: Dan Carpenter <[email protected]>
> > > ---
> > >
> > > v20:
> > > New solution: kbd_struct adds a new "kbdledctl" attribute,
> > > which can be obtained or changed through ioctl;
> > > "kbdledctl" is used to determine whether the current VT
> > > can set the keyboard led light;
> > >
> > > drivers/tty/vt/keyboard.c | 58 +++++++++++++++++++++++++++++++++++++++
> > > drivers/tty/vt/vt_ioctl.c | 12 ++++++++
> > > include/linux/kbd_kern.h | 4 +++
> > > include/linux/vt_kern.h | 2 ++
> > > include/uapi/linux/kd.h | 7 ++++-
> > > 5 files changed, 82 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> > > index c7fbbcdcc346..248c8d790d91 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;
> >
> > This is an odd name for a variable that is there for led issues, right?
>
> No this describes exactly what this variable represents.
>
> >
> > > /*
> > > * Notifier list for console keyboard events
> > > @@ -414,6 +415,12 @@ void vt_set_leds_compute_shiftstate(void)
> > > {
> > > unsigned long flags;
> > >
> > > + /*
> > > + * When switching VT,according to the value of vt_switch,
> >
> > Extra space after ',' please.
> >
> > > + * judge whether it is necessary to force the keyboard light
> > > + * state to be issued.
> > > + */
> > > + vt_switch = true;
> > > set_leds();
> >
> > How does this new variable have anything to do with the set_leds() call?
> >
> > >
> > > spin_lock_irqsave(&kbd_event_lock, flags);
> > > @@ -1247,14 +1254,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;
> >
> > So an pointer to an array? Raw pointer math is odd.
> >
> > And no bounds checking?
> >
> > > + if (kb->kbdledctl == VC_LEDCTL_OFF)
> > > + return;
> >
> > Why exit now?
The meaning of adding a "kbdledctl" here is to control the VT to set the keyboard led;
when its value is "VC_LEDCTL_OFF", the current VT is not allowed to change the
keyboard led state. In order not to destroy the previous code logic, "return" is here.
> >
> > > +
> > > 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;
> > > + }
> >
> > Please explain what you are doing here with vt_switch.
"Vt_switch" is only used to indicate whether the tty is switching. At this time, the
current tty needs to reset the led state of the keyboard. Otherwise, after the tty
switch is completed, the state of the keyboard led and the state of the keyboard
lock may be inconsistent.
> >
> >
> > > +
> > > if (leds != ledstate) {
> > > kbd_propagate_led_state(ledstate, leds);
> > > ledstate = leds;
> > > @@ -1857,6 +1874,35 @@ int vt_do_kdskbmode(unsigned int console, unsigned int arg)
> > > return ret;
> > > }
> > >
> > > +/**
> > > + * vt_do_kdskbledctl
> > > + * @console: the console to use
> > > + * @arg: the requested mode
> > > + *
> > > + * Whether to allow the current vt to change the
> > > + * keyboard light
> > > + */
> > > +int vt_do_kdskbledctl(unsigned int console, unsigned int arg)
> > > +{
> > > + struct kbd_struct *kb = &kbd_table[console];
> > > + int ret = 0;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&kbd_event_lock, flags);
> > > + switch (arg) {
> > > + case K_LEDCTL_ON:
> > > + kb->kbdledctl = VC_LEDCTL_ON;
> > > + break;
> > > + case K_LEDCTL_OFF:
> > > + kb->kbdledctl = VC_LEDCTL_OFF;
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + }
> > > + spin_unlock_irqrestore(&kbd_event_lock, flags);
> > > + return ret;
> > > +}
> > > +
> > > /**
> > > * vt_do_kdskbmeta - set keyboard meta state
> > > * @console: the console to use
> > > @@ -2157,6 +2203,18 @@ int vt_do_kdgkbmode(unsigned int console)
> > > }
> > > }
> > >
> > > +int vt_do_kdgkbledctl(unsigned int console)
> > > +{
> > > + struct kbd_struct *kb = &kbd_table[console];
> > > + /* This is a spot read so needs no locking */
> >
> > Blank line before comment?
> >
> > And what do you mean by "spot read"?
Well, the code here is imitated "int vt_do_kdgkbmode(unsigned int console)",
maybe it can be removed.
> >
> > > + switch (kb->kbdledctl) {
> > > + case VC_LEDCTL_ON:
> > > + return K_LEDCTL_ON;
> > > + case VC_LEDCTL_OFF:
> > > + return K_LEDCTL_OFF;
> > > + }
> >
> > Why does usespace need to know this value? What will it do with it?
> >
> > > +}
> > > +
> > > /**
> > > * vt_do_kdgkbmeta - report meta status
> > > * @console: console to report
> > > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > > index 3639bb6dc372..c78bd452af55 100644
> > > --- a/drivers/tty/vt/vt_ioctl.c
> > > +++ b/drivers/tty/vt/vt_ioctl.c
> > > @@ -405,6 +405,18 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> > > case KDGKBMODE:
> > > return put_user(vt_do_kdgkbmode(console), (int __user *)arg);
> > >
> > > + case KDSKBLEDCTL:
> > > + if (!perm)
> > > + return -EPERM;
> > > + ret = vt_do_kdskbledctl(console, arg);
> > > + if (ret)
> > > + return ret;
> > > + tty_ldisc_flush(tty);
> > > + break;
> > > +
> > > + case KDGKBLEDCTL:
> > > + return put_user(vt_do_kdgkbledctl(console), (int __user *)arg);
> > > +
> > > /* this could be folded into KDSKBMODE, but for compatibility
> > > reasons it is not so easy to fold KDGKBMETA into KDGKBMODE */
> > > case KDSKBMETA:
> > > diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
> > > index c40811d79769..769b946c2cf9 100644
> > > --- a/include/linux/kbd_kern.h
> > > +++ b/include/linux/kbd_kern.h
> > > @@ -32,6 +32,10 @@ struct kbd_struct {
> > > #define VC_CTRLRLOCK KG_CTRLR /* ctrlr lock mode */
> > > unsigned char slockstate; /* for `sticky' Shift, Ctrl, etc. */
> > >
> > > + unsigned char kbdledctl:1； /*Whether to allow to control the led of the keyboard */
> > > +#define VC_LEDCTL_ON 0 /* VT can set the keyboard light */
> > > +#define VC_LEDCTL_OFF 1 /* Prohibit VT to set the keyboard light */
> > > +
> > > unsigned char ledmode:1;
> > > #define LED_SHOW_FLAGS 0 /* traditional state */
> > > #define LED_SHOW_IOCTL 1 /* only change leds upon ioctl */
> > > diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> > > index b5ab452fca5b..67bdf3eddb5a 100644
> > > --- a/include/linux/vt_kern.h
> > > +++ b/include/linux/vt_kern.h
> > > @@ -149,6 +149,7 @@ void hide_boot_cursor(bool hide);
> > > /* keyboard provided interfaces */
> > > int vt_do_diacrit(unsigned int cmd, void __user *up, int eperm);
> > > int vt_do_kdskbmode(unsigned int console, unsigned int arg);
> > > +int vt_do_kdskbledctl(unsigned int console, unsigned int arg);
> > > int vt_do_kdskbmeta(unsigned int console, unsigned int arg);
> > > int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc,
> > > int perm);
> > > @@ -157,6 +158,7 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
> > > int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm);
> > > int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm);
> > > int vt_do_kdgkbmode(unsigned int console);
> > > +int vt_do_kdgkbledctl(unsigned int console);
> > > int vt_do_kdgkbmeta(unsigned int console);
> > > void vt_reset_unicode(unsigned int console);
> > > int vt_get_shift_state(void);
> > > diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> > > index ee929ece4112..138d636adf3e 100644
> > > --- a/include/uapi/linux/kd.h
> > > +++ b/include/uapi/linux/kd.h
> > > @@ -86,6 +86,11 @@ struct unimapinit {
> > > #define KDGKBMODE 0x4B44 /* gets current keyboard mode */
> > > #define KDSKBMODE 0x4B45 /* sets current keyboard mode */
> > >
> > > +#define K_LEDCTL_ON 0x00
> > > +#define K_LEDCTL_OFF 0x01
> >
> > As Dan points out, these should be the other way around.
Okay, if the current plan is allowed, I will fix it in the next version.
> >
> > > +#define KDGKBLEDCTL 0x4B73 /* set whether to allow control of keyboard lights */
> > > +#define KDSKBLEDCTL 0x4B74 /* get whether the keyboard light is currently allowed to be set */
> >
> > What userspace code is going to use these new ioctls?
> >
> > I still don't understand the problem that this is supposed to be
> > solving. How can we have never had a problem until now with regards to
> > LED settings on keyboards? What commit caused this to change? Has it
> > always been broken for 30 years and no one noticed?
>
> Yes, it's been going on since forever (I guess at least 2.6 where input
> core was introduced) and nobody really cared as very few people bounce
> between graphical environment and VTs _and_ use CapsLock or NumLock
> _and_ have keyboards with these LEDs).
>
> Now, there is a couple-line solution that was in v19, lianzhi chang just
> needed to drop condition that was being introduced in
> vt_set_leds_compute_shiftstate(). That would ensure that proper state of
> LEDs is restored on every VT switch. It also might have resulted in
> LEDs switching momentarily off before turning on according to the
> graphical desktop preferences, but I do not thing this is a big issue.
As you said, in the V19 version, there is a judgment condition in
vt_set_leds_compute_shiftstate(). If you delete it, there will be
some minor flaws. I always want to solve it perfectly. This may
be my selfish intention, but my ability is lacking, which has led
to iterating 21 versions.
>
> Now we are building this new infra with new ioctls, etc, for miniscule
> gain of avoiding this blink. I do not believe this is worth it.
If the current solution is not worth it, or the solution optimized for v19
is easier to accept, I will go back to the v19 code to submit.
Thanks.
--
lianzhi chang
On Tue, Dec 14, 2021 at 10:06:38AM +0800, lianzhi chang wrote:
> > On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> > > > Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> > > > +#define KDGKBLEDCTL 0x4B73 /* set whether to allow control of keyboard lights */
> > > > +#define KDSKBLEDCTL 0x4B74 /* get whether the keyboard light is currently allowed to be set */
> > >
> > > What userspace code is going to use these new ioctls?
> > >
> > > I still don't understand the problem that this is supposed to be
> > > solving. How can we have never had a problem until now with regards to
> > > LED settings on keyboards? What commit caused this to change? Has it
> > > always been broken for 30 years and no one noticed?
> >
> > Yes, it's been going on since forever (I guess at least 2.6 where input
> > core was introduced) and nobody really cared as very few people bounce
> > between graphical environment and VTs _and_ use CapsLock or NumLock
> > _and_ have keyboards with these LEDs).
> >
> > Now, there is a couple-line solution that was in v19, lianzhi chang just
> > needed to drop condition that was being introduced in
> > vt_set_leds_compute_shiftstate(). That would ensure that proper state of
> > LEDs is restored on every VT switch. It also might have resulted in
> > LEDs switching momentarily off before turning on according to the
> > graphical desktop preferences, but I do not thing this is a big issue.
>
> As you said, in the V19 version, there is a judgment condition in
> vt_set_leds_compute_shiftstate(). If you delete it, there will be
> some minor flaws. I always want to solve it perfectly. This may
> be my selfish intention, but my ability is lacking, which has led
> to iterating 21 versions.
>
> >
> > Now we are building this new infra with new ioctls, etc, for miniscule
> > gain of avoiding this blink. I do not believe this is worth it.
> If the current solution is not worth it, or the solution optimized for v19
> is easier to accept, I will go back to the v19 code to submit.
My recommendation would be to land the adjusted v19 (the one without the
conditional) that fixes original issue of LED state not being restored
on VT switch, and then as a followup, and if you so inclined, offer the
patch that introduces special ioctls to notify keyboard driver that
it should avoid touching LED state completely for a given VT, so that we
can discuss it separately and decide if such functionality is
needed/wanted.
Thanks.
--
Dmitry