linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] TTY: fix Caps Lock LED
@ 2017-01-27 17:13 Benjamin Tissoires
  2017-01-27 17:13 ` [PATCH 1/4] tty/vt/keyboard: use defined macros for masks Benjamin Tissoires
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2017-01-27 17:13 UTC (permalink / raw)
  To: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, Samuel Thibault
  Cc: linux-input, linux-kernel

Hi,

Well, it's quite an old issue, but it looks like no one cared much before :)

So by default, on Fedora and RHEL at least*, the Caps Lock LED is broken while
in a VT. I tracked down the issue to be a change in ckbcomp introduced because
the kernel just can't properly handle all keymaps. However, if the keymap now
works thanks to the work around in place, the LED just doesn't.

This series aims at trying to have a consistent LEDs status while in VT.
It detects the ckbcomp workaround (which seems mainline now), and syncs
both caps lock with left control lock when it has to. This way, we shouldn't
break existing user-space if the distribution changes the trigger to
kbd-controllllock instead of kbd-capslock.

Cheers,
Benjamin

* ubuntu also seems affected:
  https://bugs.launchpad.net/ubuntu/+source/console-setup/+bug/425704


Benjamin Tissoires (4):
  tty/vt/keyboard: use defined macros for masks
  tty/vt/keyboard: Fix Caps Lock LED on major distributions
  tty/vt/keyboard: reset the LEDs state at each console change
  Input: leds - force the LED status after .probe()

 drivers/input/input-leds.c | 33 ++++++++++++++++++++++++++++
 drivers/tty/vt/keyboard.c  | 55 +++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 83 insertions(+), 5 deletions(-)

-- 
2.9.3

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/4] tty/vt/keyboard: use defined macros for masks
  2017-01-27 17:13 [PATCH 0/4] TTY: fix Caps Lock LED Benjamin Tissoires
@ 2017-01-27 17:13 ` Benjamin Tissoires
  2017-01-27 17:18   ` Samuel Thibault
  2017-01-27 17:13 ` [PATCH 2/4] tty/vt/keyboard: Fix Caps Lock LED on major distributions Benjamin Tissoires
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2017-01-27 17:13 UTC (permalink / raw)
  To: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, Samuel Thibault
  Cc: linux-input, linux-kernel

Better using the defined macros to express the bit masks.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/tty/vt/keyboard.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 3dd6a49..4a3907e 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1044,9 +1044,9 @@ static int kbd_update_leds_helper(struct input_handle *handle, void *data)
 	unsigned int leds = *(unsigned int *)data;
 
 	if (test_bit(EV_LED, handle->dev->evbit)) {
-		input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & 0x01));
-		input_inject_event(handle, EV_LED, LED_NUML,    !!(leds & 0x02));
-		input_inject_event(handle, EV_LED, LED_CAPSL,   !!(leds & 0x04));
+		input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & BIT(VC_SCROLLOCK)));
+		input_inject_event(handle, EV_LED, LED_NUML,    !!(leds & BIT(VC_NUMLOCK)));
+		input_inject_event(handle, EV_LED, LED_CAPSL,   !!(leds & BIT(VC_CAPSLOCK)));
 		input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
 	}
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/4] tty/vt/keyboard: Fix Caps Lock LED on major distributions
  2017-01-27 17:13 [PATCH 0/4] TTY: fix Caps Lock LED Benjamin Tissoires
  2017-01-27 17:13 ` [PATCH 1/4] tty/vt/keyboard: use defined macros for masks Benjamin Tissoires
@ 2017-01-27 17:13 ` Benjamin Tissoires
  2017-01-27 17:25   ` Samuel Thibault
  2017-01-27 17:13 ` [PATCH 3/4] tty/vt/keyboard: reset the LEDs state at each console change Benjamin Tissoires
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2017-01-27 17:13 UTC (permalink / raw)
  To: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, Samuel Thibault
  Cc: linux-input, linux-kernel

The way the kernel handles caps lock on TTYs is not compatible with
all keymaps[1].
The solution that was found was to replace the native Caps_Lock by an
other modifier - ControlL_Lock - from user space. ckbcomp generates
a keymap which can emulate the Caps Lock behavior without relying on
the kernel to translate keys into upper case.

On Fedora and RHEL, the generic keymaps are generated by ckbcomp, and
when systemd-localed loads the locale, it calls "loadkeys -u us-intl",
which uses these generated keymaps.

The problem with these custom keymaps is that the Caps Lock LED is now
not updated properly. In the INPUT_LEDS version, the trigger that can
now handle the Caps Lock LED is "kbd-ctrlllock", but the default is
"kbd-capslock".
We can detect such custom keymaps when they are set by the console ioctl,
and in that case, we can sync both triggers "kbd-capslock" and
"kbd-ctrlllock" to restore a proper LED behavior.

Both version of LEDs (with or without INPUT_LEDS) are fixed.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=7746#c24

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/tty/vt/keyboard.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 4a3907e..ca1d614 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -65,6 +65,8 @@ static inline int kbd_defleds(void)
 
 #define KBD_DEFLOCK 0
 
+#define KBD_LOCKSTATE_OFFSET 8
+
 /*
  * Handler Tables.
  */
@@ -132,6 +134,8 @@ static int shift_state = 0;
 
 static unsigned int ledstate = -1U;			/* undefined */
 static unsigned char ledioctl;
+static bool caps_as_controlllock;
+static bool task_caps_as_controlllock;
 
 /*
  * Notifier list for console keyboard events
@@ -979,7 +983,7 @@ static void kbd_led_trigger_activate(struct led_classdev *cdev)
 	}
 
 #define KBD_LOCKSTATE_TRIGGER(_led_bit, _name)		\
-	KBD_LED_TRIGGER((_led_bit) + 8, _name)
+	KBD_LED_TRIGGER((_led_bit) + KBD_LOCKSTATE_OFFSET, _name)
 
 static struct kbd_led_trigger kbd_led_triggers[] = {
 	KBD_LED_TRIGGER(VC_SCROLLOCK, "kbd-scrolllock"),
@@ -1004,6 +1008,18 @@ static void kbd_propagate_led_state(unsigned int old_state,
 	unsigned int changed = old_state ^ new_state;
 	int i;
 
+	/*
+	 * special case where user space uses its own caps lock implementation
+	 * through ControlL_Lock.
+	 */
+	if (task_caps_as_controlllock) {
+		trigger = &kbd_led_triggers[VC_CAPSLOCK];
+		trigger->mask |= BIT(VC_CTRLLLOCK) << KBD_LOCKSTATE_OFFSET;
+	} else {
+		trigger = &kbd_led_triggers[VC_CAPSLOCK];
+		trigger->mask &= ~(BIT(VC_CTRLLLOCK) << KBD_LOCKSTATE_OFFSET);
+	}
+
 	for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); i++) {
 		trigger = &kbd_led_triggers[i];
 
@@ -1042,11 +1058,19 @@ static void kbd_init_leds(void)
 static int kbd_update_leds_helper(struct input_handle *handle, void *data)
 {
 	unsigned int leds = *(unsigned int *)data;
+	unsigned int capsl_mask = BIT(VC_CAPSLOCK);
+
+	/*
+	 * special case where user space uses its own caps lock implementation
+	 * through ControlL_Lock.
+	 */
+	if (task_caps_as_controlllock)
+		capsl_mask |= BIT(VC_CTRLLLOCK) << KBD_LOCKSTATE_OFFSET;
 
 	if (test_bit(EV_LED, handle->dev->evbit)) {
 		input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & BIT(VC_SCROLLOCK)));
 		input_inject_event(handle, EV_LED, LED_NUML,    !!(leds & BIT(VC_NUMLOCK)));
-		input_inject_event(handle, EV_LED, LED_CAPSL,   !!(leds & BIT(VC_CAPSLOCK)));
+		input_inject_event(handle, EV_LED, LED_CAPSL,   !!(leds & capsl_mask));
 		input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
 	}
 
@@ -1188,7 +1212,8 @@ static void kbd_bh(unsigned long dummy)
 
 	spin_lock_irqsave(&led_lock, flags);
 	leds = getleds();
-	leds |= (unsigned int)kbd->lockstate << 8;
+	leds |= (unsigned int)kbd->lockstate << KBD_LOCKSTATE_OFFSET;
+	task_caps_as_controlllock = caps_as_controlllock;
 	spin_unlock_irqrestore(&led_lock, flags);
 
 	if (leds != ledstate) {
@@ -1939,6 +1964,18 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
 			spin_unlock_irqrestore(&kbd_event_lock, flags);
 			return -EPERM;
 		}
+
+		/*
+		 * See https://bugzilla.kernel.org/show_bug.cgi?id=7746#c24
+		 *
+		 * The kernel can't properly deal with all keymaps/capslock
+		 * interactions, so userspace (ckbcomp) may use ControlL_Lock
+		 * as a replacement for Caps_Lock.
+		 * It works but breaks the LED, so mark it there that we need
+		 * to forward proper Caps Lock LED on K_CTRLLLOCK.
+		 */
+		if (i == KEY_CAPSLOCK)
+			caps_as_controlllock = (v == K_CTRLLLOCK);
 		key_map[i] = U(v);
 		if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
 			do_compute_shiftstate();
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/4] tty/vt/keyboard: reset the LEDs state at each console change
  2017-01-27 17:13 [PATCH 0/4] TTY: fix Caps Lock LED Benjamin Tissoires
  2017-01-27 17:13 ` [PATCH 1/4] tty/vt/keyboard: use defined macros for masks Benjamin Tissoires
  2017-01-27 17:13 ` [PATCH 2/4] tty/vt/keyboard: Fix Caps Lock LED on major distributions Benjamin Tissoires
@ 2017-01-27 17:13 ` Benjamin Tissoires
  2017-01-27 17:31   ` Samuel Thibault
  2017-01-27 17:13 ` [PATCH 4/4] Input: leds - force the LED status after .probe() Benjamin Tissoires
  2017-01-27 17:23 ` [PATCH 0/4] TTY: fix Caps Lock LED Samuel Thibault
  4 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2017-01-27 17:13 UTC (permalink / raw)
  To: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, Samuel Thibault
  Cc: linux-input, linux-kernel

When switching between consoles, the LEDs state is correctly
assigned, as long as the kernel manages the console.
When switching back from Gnome, the VT is not aware of the
current state of the LEDs. So if Gnome changes them, the
kernel still bellieves they are off, and it won't turn them
off when switching to a new TTY.

To keep the LEDs status in sync with the actual modifiers,
simply force a reset of the LEDs when we detect a vt change.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/tty/vt/keyboard.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index ca1d614..410b21e 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -137,6 +137,8 @@ static unsigned char ledioctl;
 static bool caps_as_controlllock;
 static bool task_caps_as_controlllock;
 
+static int saved_cur_kbd_console = -1;
+
 /*
  * Notifier list for console keyboard events
  */
@@ -1482,6 +1484,12 @@ static void kbd_event(struct input_handle *handle, unsigned int event_type,
 	/* We are called with interrupts disabled, just take the lock */
 	spin_lock(&kbd_event_lock);
 
+	/* reset the led state on console switch */
+	if (saved_cur_kbd_console != fg_console) {
+		ledstate = -1U;
+		saved_cur_kbd_console = fg_console;
+	}
+
 	if (event_type == EV_MSC && event_code == MSC_RAW && HW_RAW(handle->dev))
 		kbd_rawcode(value);
 	if (event_type == EV_KEY)
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/4] Input: leds - force the LED status after .probe()
  2017-01-27 17:13 [PATCH 0/4] TTY: fix Caps Lock LED Benjamin Tissoires
                   ` (2 preceding siblings ...)
  2017-01-27 17:13 ` [PATCH 3/4] tty/vt/keyboard: reset the LEDs state at each console change Benjamin Tissoires
@ 2017-01-27 17:13 ` Benjamin Tissoires
  2017-01-27 17:34   ` Samuel Thibault
  2017-01-27 17:23 ` [PATCH 0/4] TTY: fix Caps Lock LED Samuel Thibault
  4 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2017-01-27 17:13 UTC (permalink / raw)
  To: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, Samuel Thibault
  Cc: linux-input, linux-kernel

Once the host has set the initial setting of the LEDs after a new
USB keyboard gets connected, they appear to have a SET_IDLE command
sent which turns of the LEDs.
This means that the initial step of setting the LEDs on a keyboard is
just reset once .probe() finishes.
To be sure we set the LEDs, start a delayed task to try to resend
them after 1 sec.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/input-leds.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
index 766bf26..83d930f 100644
--- a/drivers/input/input-leds.c
+++ b/drivers/input/input-leds.c
@@ -46,6 +46,7 @@ struct input_led {
 
 struct input_leds {
 	struct input_handle handle;
+	struct delayed_work init_work;
 	unsigned int num_leds;
 	struct input_led leds[];
 };
@@ -83,6 +84,33 @@ static int input_leds_get_count(struct input_dev *dev)
 	return count;
 }
 
+#ifdef CONFIG_LEDS_TRIGGERS
+static void leds_init_work(struct work_struct *work)
+{
+	struct input_leds *leds = container_of(work, struct input_leds,
+					       init_work.work);
+	struct input_led *led;
+	unsigned int led_code;
+	int led_no = 0;
+
+	for_each_set_bit(led_code, leds->handle.dev->ledbit, LED_CNT) {
+		led = &leds->leds[led_no];
+
+		down_read(&led->cdev.trigger_lock);
+		if (led->cdev.trigger && led->cdev.trigger->activate) {
+			led_set_brightness(&led->cdev, LED_OFF);
+			led->cdev.trigger->activate(&led->cdev);
+		}
+		up_read(&led->cdev.trigger_lock);
+		led_no++;
+	}
+}
+#else  /* !CONFIG_LEDS_TRIGGERS */
+static void leds_init_work(struct work_struct *work)
+{
+}
+#endif
+
 static int input_leds_connect(struct input_handler *handler,
 			      struct input_dev *dev,
 			      const struct input_device_id *id)
@@ -108,6 +136,7 @@ static int input_leds_connect(struct input_handler *handler,
 	leds->handle.handler = handler;
 	leds->handle.name = "leds";
 	leds->handle.private = leds;
+	INIT_DELAYED_WORK(&leds->init_work, leds_init_work);
 
 	error = input_register_handle(&leds->handle);
 	if (error)
@@ -151,6 +180,8 @@ static int input_leds_connect(struct input_handler *handler,
 		led_no++;
 	}
 
+	schedule_delayed_work(&leds->init_work, msecs_to_jiffies(1000));
+
 	return 0;
 
 err_unregister_leds:
@@ -176,6 +207,8 @@ static void input_leds_disconnect(struct input_handle *handle)
 	struct input_leds *leds = handle->private;
 	int i;
 
+	cancel_delayed_work_sync(&leds->init_work);
+
 	for (i = 0; i < leds->num_leds; i++) {
 		struct input_led *led = &leds->leds[i];
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] tty/vt/keyboard: use defined macros for masks
  2017-01-27 17:13 ` [PATCH 1/4] tty/vt/keyboard: use defined macros for masks Benjamin Tissoires
@ 2017-01-27 17:18   ` Samuel Thibault
  0 siblings, 0 replies; 15+ messages in thread
From: Samuel Thibault @ 2017-01-27 17:18 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, linux-input,
	linux-kernel

Benjamin Tissoires, on Fri 27 Jan 2017 18:13:15 +0100, wrote:
> Better using the defined macros to express the bit masks.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  drivers/tty/vt/keyboard.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 3dd6a49..4a3907e 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -1044,9 +1044,9 @@ static int kbd_update_leds_helper(struct input_handle *handle, void *data)
>  	unsigned int leds = *(unsigned int *)data;
>  
>  	if (test_bit(EV_LED, handle->dev->evbit)) {
> -		input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & 0x01));
> -		input_inject_event(handle, EV_LED, LED_NUML,    !!(leds & 0x02));
> -		input_inject_event(handle, EV_LED, LED_CAPSL,   !!(leds & 0x04));
> +		input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & BIT(VC_SCROLLOCK)));
> +		input_inject_event(handle, EV_LED, LED_NUML,    !!(leds & BIT(VC_NUMLOCK)));
> +		input_inject_event(handle, EV_LED, LED_CAPSL,   !!(leds & BIT(VC_CAPSLOCK)));
>  		input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
>  	}
>  
> -- 
> 2.9.3
> 

-- 
Samuel
<y> update-menus: relocation error: update-menus: symbol _ZNSt9basic_iosIcSt11char_traitsIcEE4initEPSt15basic_streambufIcS1_E, version GLIBCPP_3.2 not defined in file libstdc++.so.5 with link time reference
<y> quoi que ça peut bien vouloir dire ?
<D> N a eu la meme merde
<y> c ça que ça veut dire ? wow, c'est bien crypté :)
 -+- #ens-mim s'entraide -+-

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] TTY: fix Caps Lock LED
  2017-01-27 17:13 [PATCH 0/4] TTY: fix Caps Lock LED Benjamin Tissoires
                   ` (3 preceding siblings ...)
  2017-01-27 17:13 ` [PATCH 4/4] Input: leds - force the LED status after .probe() Benjamin Tissoires
@ 2017-01-27 17:23 ` Samuel Thibault
  2017-01-27 18:34   ` Benjamin Tissoires
  4 siblings, 1 reply; 15+ messages in thread
From: Samuel Thibault @ 2017-01-27 17:23 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, linux-input,
	linux-kernel

Hello,

Benjamin Tissoires, on Fri 27 Jan 2017 18:13:14 +0100, wrote:
> Well, it's quite an old issue, but it looks like no one cared much before :)

I did, actually.

> So by default, on Fedora and RHEL at least*, the Caps Lock LED is broken while
> in a VT.

Yes, and in Debian too, see
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=514464
That was the trigger for my kbd LED work.

> I tracked down the issue to be a change in ckbcomp introduced because
> the kernel just can't properly handle all keymaps. However, if the keymap now
> works thanks to the work around in place, the LED just doesn't.

Yes, and ckbcomp now just has to properly set the LED trigger for
capslock. Something like:

echo kbd-ctrlllock > /sys/class/leds/input0::capslock/trigger

see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=514464#114

> This series aims at trying to have a consistent LEDs status while in VT.
> It detects the ckbcomp workaround (which seems mainline now),

Urgl.

> and syncs both caps lock with left control lock when it has to.

Urgl.

> This way, we shouldn't
> break existing user-space if the distribution changes the trigger to
> kbd-controllllock instead of kbd-capslock.

Urgl.

It's ckbcomp's fault for using another trigger. It's up to ckbcomp to
make sure that keyboard use the right trigger for the capslock led.  The
kernel shouldn't try to circumvent that.

Samuel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] tty/vt/keyboard: Fix Caps Lock LED on major distributions
  2017-01-27 17:13 ` [PATCH 2/4] tty/vt/keyboard: Fix Caps Lock LED on major distributions Benjamin Tissoires
@ 2017-01-27 17:25   ` Samuel Thibault
  2017-01-27 17:29     ` Dmitry Torokhov
  0 siblings, 1 reply; 15+ messages in thread
From: Samuel Thibault @ 2017-01-27 17:25 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, linux-input,
	linux-kernel

Benjamin Tissoires, on Fri 27 Jan 2017 18:13:16 +0100, wrote:
> +static bool caps_as_controlllock;

Really, we don't want these.  Userspace could invent using another
modifier, so it's really not the way forward.  Please have a look at
what I suggested.

Samuel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] tty/vt/keyboard: Fix Caps Lock LED on major distributions
  2017-01-27 17:25   ` Samuel Thibault
@ 2017-01-27 17:29     ` Dmitry Torokhov
  0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Torokhov @ 2017-01-27 17:29 UTC (permalink / raw)
  To: Samuel Thibault, Benjamin Tissoires
  Cc: Greg Kroah-Hartman, Jiri Slaby, linux-input, linux-kernel

On January 27, 2017 9:25:14 AM PST, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
>Benjamin Tissoires, on Fri 27 Jan 2017 18:13:16 +0100, wrote:
>> +static bool caps_as_controlllock;
>
>Really, we don't want these.  Userspace could invent using another
>modifier, so it's really not the way forward.  Please have a look at
>what I suggested.

Completely agree.


Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] tty/vt/keyboard: reset the LEDs state at each console change
  2017-01-27 17:13 ` [PATCH 3/4] tty/vt/keyboard: reset the LEDs state at each console change Benjamin Tissoires
@ 2017-01-27 17:31   ` Samuel Thibault
  2017-01-27 18:18     ` Benjamin Tissoires
  0 siblings, 1 reply; 15+ messages in thread
From: Samuel Thibault @ 2017-01-27 17:31 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, linux-input,
	linux-kernel

Hello,

Benjamin Tissoires, on Fri 27 Jan 2017 18:13:17 +0100, wrote:
> When switching back from Gnome, the VT is not aware of the
> current state of the LEDs. So if Gnome changes them, the
> kernel still bellieves they are off, and it won't turn them
> off when switching to a new TTY.

I'm not getting that issue, please detail which graphical stack you are
using (Xorg/Wayland? input-libinput?)

Samuel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] Input: leds - force the LED status after .probe()
  2017-01-27 17:13 ` [PATCH 4/4] Input: leds - force the LED status after .probe() Benjamin Tissoires
@ 2017-01-27 17:34   ` Samuel Thibault
  2017-01-27 18:19     ` Benjamin Tissoires
  0 siblings, 1 reply; 15+ messages in thread
From: Samuel Thibault @ 2017-01-27 17:34 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, linux-input,
	linux-kernel

Benjamin Tissoires, on Fri 27 Jan 2017 18:13:18 +0100, wrote:
> Once the host has set the initial setting of the LEDs after a new
> USB keyboard gets connected, they appear to have a SET_IDLE command
> sent which turns of the LEDs.
> This means that the initial step of setting the LEDs on a keyboard is
> just reset once .probe() finishes.
> To be sure we set the LEDs, start a delayed task to try to resend
> them after 1 sec.

If there is such issue, I'd say fix the issue (what is sending
SET_IDLE?) instead of working around it.

Samuel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] tty/vt/keyboard: reset the LEDs state at each console change
  2017-01-27 17:31   ` Samuel Thibault
@ 2017-01-27 18:18     ` Benjamin Tissoires
  0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2017-01-27 18:18 UTC (permalink / raw)
  To: Samuel Thibault, Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby,
	linux-input, linux-kernel

On Jan 27 2017 or thereabouts, Samuel Thibault wrote:
> Hello,
> 
> Benjamin Tissoires, on Fri 27 Jan 2017 18:13:17 +0100, wrote:
> > When switching back from Gnome, the VT is not aware of the
> > current state of the LEDs. So if Gnome changes them, the
> > kernel still bellieves they are off, and it won't turn them
> > off when switching to a new TTY.
> 
> I'm not getting that issue, please detail which graphical stack you are
> using (Xorg/Wayland? input-libinput?)

Well, libinput won't have anything to do here, and the issue applies on
Gnome and gdm with both Xorg and Wayland. Given that you don't have the
issue, I had a feeling that it might be logind interfering (given that
it revokes the input file descriptor). So I tested a startx from root,
and indeed the issue is not present in that case.

I'll check with the systemd guys if this is something that comes from
them and if this can be fixable.

Cheers,
Benjamin

> 
> Samuel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] Input: leds - force the LED status after .probe()
  2017-01-27 17:34   ` Samuel Thibault
@ 2017-01-27 18:19     ` Benjamin Tissoires
  0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2017-01-27 18:19 UTC (permalink / raw)
  To: Samuel Thibault, Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby,
	linux-input, linux-kernel

On Jan 27 2017 or thereabouts, Samuel Thibault wrote:
> Benjamin Tissoires, on Fri 27 Jan 2017 18:13:18 +0100, wrote:
> > Once the host has set the initial setting of the LEDs after a new
> > USB keyboard gets connected, they appear to have a SET_IDLE command
> > sent which turns of the LEDs.
> > This means that the initial step of setting the LEDs on a keyboard is
> > just reset once .probe() finishes.
> > To be sure we set the LEDs, start a delayed task to try to resend
> > them after 1 sec.
> 
> If there is such issue, I'd say fix the issue (what is sending
> SET_IDLE?) instead of working around it.
> 

That is the question. I am not even sure who sends the SET_IDLE, and if
this is something that can be controlled without disturbing too many
devices. I'll dig further then.

Cheers,
Benjamin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] TTY: fix Caps Lock LED
  2017-01-27 17:23 ` [PATCH 0/4] TTY: fix Caps Lock LED Samuel Thibault
@ 2017-01-27 18:34   ` Benjamin Tissoires
  2017-01-27 18:47     ` Samuel Thibault
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2017-01-27 18:34 UTC (permalink / raw)
  To: Samuel Thibault, Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby,
	linux-input, linux-kernel

On Jan 27 2017 or thereabouts, Samuel Thibault wrote:
> Hello,
> 
> Benjamin Tissoires, on Fri 27 Jan 2017 18:13:14 +0100, wrote:
> > Well, it's quite an old issue, but it looks like no one cared much before :)
> 
> I did, actually.

Yes, sorry, I was more putting it in the way "no one in the Fedora/RHEL
world ever noticed the caps lock led was dead". I knew you made progress
in that direction, but we probably didn't port all the fixes if there was.

> 
> > So by default, on Fedora and RHEL at least*, the Caps Lock LED is broken while
> > in a VT.
> 
> Yes, and in Debian too, see
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=514464
> That was the trigger for my kbd LED work.

Do you have a full working solution? Instead of me trying to reinvent
the wheel?

> 
> > I tracked down the issue to be a change in ckbcomp introduced because
> > the kernel just can't properly handle all keymaps. However, if the keymap now
> > works thanks to the work around in place, the LED just doesn't.
> 
> Yes, and ckbcomp now just has to properly set the LED trigger for
> capslock. Something like:
> 
> echo kbd-ctrlllock > /sys/class/leds/input0::capslock/trigger

Ack, but there are 3 (solvable) issues:
- in Fedora/RHEL, ckbcomp is used statically when creating the kbd
  package, the keymaps are generated and then forgot. So ckbcomp is not
  the component to fix for us
- you need to trigger this for each keyboard that appears on the bus.
  This can be achieved by a udev rule, but...
- ... if you blindly set a udev rule to change the trigger, you just
  break every users who manually call loadkeys with a non-patched caps
  lock (when forcing a legacy keymap).

So indeed, this can be fixed by an elaborate udev rule (dumping the
current keymap, checking whether ctrlllock is used in place of capslock,
and set the correct trigger), but I just felt that exposing the trigger
as ctrlllock for the caps lock LED for a plain install was not OK from
the user point of view. (OK, users don't care about triggers as long as
they work).

> 
> see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=514464#114
> 
> > This series aims at trying to have a consistent LEDs status while in VT.
> > It detects the ckbcomp workaround (which seems mainline now),
> 
> Urgl.
> 
> > and syncs both caps lock with left control lock when it has to.
> 
> Urgl.
> 
> > This way, we shouldn't
> > break existing user-space if the distribution changes the trigger to
> > kbd-controllllock instead of kbd-capslock.
> 
> Urgl.
> 
> It's ckbcomp's fault for using another trigger. It's up to ckbcomp to
> make sure that keyboard use the right trigger for the capslock led.  The
> kernel shouldn't try to circumvent that.

We agree that it's ckbcomp's fault :)

Cheers,
Benjamin

> 
> Samuel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] TTY: fix Caps Lock LED
  2017-01-27 18:34   ` Benjamin Tissoires
@ 2017-01-27 18:47     ` Samuel Thibault
  0 siblings, 0 replies; 15+ messages in thread
From: Samuel Thibault @ 2017-01-27 18:47 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Dmitry Torokhov, Greg Kroah-Hartman, Jiri Slaby, linux-input,
	linux-kernel

Benjamin Tissoires, on Fri 27 Jan 2017 19:34:36 +0100, wrote:
> On Jan 27 2017 or thereabouts, Samuel Thibault wrote:
> > > So by default, on Fedora and RHEL at least*, the Caps Lock LED is broken while
> > > in a VT.
> > 
> > Yes, and in Debian too, see
> > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=514464
> > That was the trigger for my kbd LED work.
> 
> Do you have a full working solution? Instead of me trying to reinvent
> the wheel?

The one described in comment

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=514464#114

Anton hasn't implemented it in console-setup yet, but I don't think his
code would be reusable as such by ckbcomp anyway.

> > > I tracked down the issue to be a change in ckbcomp introduced because
> > > the kernel just can't properly handle all keymaps. However, if the keymap now
> > > works thanks to the work around in place, the LED just doesn't.
> > 
> > Yes, and ckbcomp now just has to properly set the LED trigger for
> > capslock. Something like:
> > 
> > echo kbd-ctrlllock > /sys/class/leds/input0::capslock/trigger
> 
> Ack, but there are 3 (solvable) issues:
> - in Fedora/RHEL, ckbcomp is used statically when creating the kbd
>   package, the keymaps are generated and then forgot. So ckbcomp is not
>   the component to fix for us

But it can emit a file which says which modifier is used for capslock.

> - you need to trigger this for each keyboard that appears on the bus.
>   This can be achieved by a udev rule, but...
> - ... if you blindly set a udev rule to change the trigger, you just
>   break every users who manually call loadkeys with a non-patched caps
>   lock (when forcing a legacy keymap).

The file I mentioned above could be actually inlined in what loadkeys
eats.

Samuel

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-01-27 19:06 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-27 17:13 [PATCH 0/4] TTY: fix Caps Lock LED Benjamin Tissoires
2017-01-27 17:13 ` [PATCH 1/4] tty/vt/keyboard: use defined macros for masks Benjamin Tissoires
2017-01-27 17:18   ` Samuel Thibault
2017-01-27 17:13 ` [PATCH 2/4] tty/vt/keyboard: Fix Caps Lock LED on major distributions Benjamin Tissoires
2017-01-27 17:25   ` Samuel Thibault
2017-01-27 17:29     ` Dmitry Torokhov
2017-01-27 17:13 ` [PATCH 3/4] tty/vt/keyboard: reset the LEDs state at each console change Benjamin Tissoires
2017-01-27 17:31   ` Samuel Thibault
2017-01-27 18:18     ` Benjamin Tissoires
2017-01-27 17:13 ` [PATCH 4/4] Input: leds - force the LED status after .probe() Benjamin Tissoires
2017-01-27 17:34   ` Samuel Thibault
2017-01-27 18:19     ` Benjamin Tissoires
2017-01-27 17:23 ` [PATCH 0/4] TTY: fix Caps Lock LED Samuel Thibault
2017-01-27 18:34   ` Benjamin Tissoires
2017-01-27 18:47     ` Samuel Thibault

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).