[PATCH 0/3] Switch input leds over to standard LED class devices

[PATCH 0/3] Switch input leds over to standard LED class devices

[Dmitry Torokhov]

Hi,

I finally was able to spend some time looking over Samuel's patch set
switching input LEDs from custom implementation over to standard LED class
devices and I think this is the shape I am reasonably happy with. The
changes:

1. Instead of making LED class devices part of the input device they are
implemented as an input handler (and thus are completely separate from
input core). The old way of controlling the leds (via writing
EV_LED/LED_XXX events into an event device) is still there and may override
LED state set up via a trigger or through sysfs attribute. Also when input
device is "grabbed" requests coming from LED subsystem are ignored until
the device is released.

2. There are no per-input device triggers. Input devices only carry LEDs
and those LEDs use one of the system-wide triggers. Which ones is to user
to decide. The default triggers are the one defines by keyboard handler for
it's standard LED states.

3. There are no VT "LEDs" combining state of multiple keyboards/input
devices anymore. Having such virtual multiplexing object just adds
complexity and is hard to untange (see /dev/input/mice and all the issues
we had with synaptics driver trying to exclude it's data stream from it).
If user wants all keyboards to light up CapsLock LED when VT state locks
CtrlL modifier they need to write a udev rule or similar to set up
"kbd-ctrlllock" trigger for all appearing "input%::capslock" LED class
devices.

Please take a look and see if you see any holes.

Thanks.

-- 
Dmitry

[PATCH 1/3] Input: export LEDs as class devices in sysfs

[Dmitry Torokhov]

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

This change creates a new input handler called "leds" that exports LEDs on input
devices as standard LED class devices in sysfs and allows controlling their
ptate via sysfs or via any of the standard LED triggers. This allows to
re-purpose and reassign LDEs on the keyboards to represent states other
than the standard keyboard states (CapsLock, NumLock, etc).

The old API of controlling input LEDs by writing into /dev/input/eventX
devices is still present and will take precedence over acessing via LEDs
subsystem (i.e. it may override state set by a trigger). If input device is
"grabbed" then requests coming through LED subsystem will be ignored.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 Documentation/leds/leds-class.txt |   3 -
 drivers/input/Kconfig             |  13 +++
 drivers/input/Makefile            |   1 +
 drivers/input/input-leds.c        | 210 ++++++++++++++++++++++++++++++++++++++
 drivers/leds/Kconfig              |   3 -
 5 files changed, 224 insertions(+), 6 deletions(-)
 create mode 100644 drivers/input/input-leds.c

diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
index 79699c2..62261c0 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -2,9 +2,6 @@
 LED handling under Linux
 ========================
 
-If you're reading this and thinking about keyboard leds, these are
-handled by the input subsystem and the led class is *not* needed.
-
 In its simplest form, the LED class just allows control of LEDs from
 userspace. LEDs appear in /sys/class/leds/. The maximum brightness of the
 LED is defined in max_brightness file. The brightness file will set the brightness
diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index a11ff74..a35532e 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -25,6 +25,19 @@ config INPUT
 
 if INPUT
 
+config INPUT_LEDS
+	tristate "Export input device LEDs in sysfs"
+	depends on LEDS_CLASS
+	default INPUT
+	help
+	  Say Y here if you would like to export LEDs on input devices
+	  as standard LED class devices in sysfs.
+
+	  If unsure, say Y.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called input-leds.
+
 config INPUT_FF_MEMLESS
 	tristate "Support for memoryless force-feedback devices"
 	help
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index 5ca3f63..0c9302c 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_INPUT_POLLDEV)	+= input-polldev.o
 obj-$(CONFIG_INPUT_SPARSEKMAP)	+= sparse-keymap.o
 obj-$(CONFIG_INPUT_MATRIXKMAP)	+= matrix-keymap.o
 
+obj-$(CONFIG_INPUT_LEDS)	+= input-leds.o
 obj-$(CONFIG_INPUT_MOUSEDEV)	+= mousedev.o
 obj-$(CONFIG_INPUT_JOYDEV)	+= joydev.o
 obj-$(CONFIG_INPUT_EVDEV)	+= evdev.o
diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
new file mode 100644
index 0000000..029a004
--- /dev/null
+++ b/drivers/input/input-leds.c
@@ -0,0 +1,210 @@
+/*
+ * LED support for the input layer
+ *
+ * Copyright 2010-2015 Samuel Thibault <samuel.thibault@ens-lyon.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/input.h>
+
+#if IS_ENABLED(CONFIG_VT)
+#define VT_TRIGGER(_name)	.trigger = _name
+#else
+#define VT_TRIGGER(_name)	.trigger = NULL
+#endif
+
+static const struct {
+	const char *name;
+	const char *trigger;
+} input_led_info[LED_CNT] = {
+	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
+	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
+	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },
+	[LED_COMPOSE]	= { "compose" },
+	[LED_KANA]	= { "kana", VT_TRIGGER("kbd-kanalock") },
+	[LED_SLEEP]	= { "sleep" } ,
+	[LED_SUSPEND]	= { "suspend" },
+	[LED_MUTE]	= { "mute" },
+	[LED_MISC]	= { "misc" },
+	[LED_MAIL]	= { "mail" },
+	[LED_CHARGING]	= { "charging" },
+};
+
+struct input_led {
+	struct led_classdev cdev;
+	struct input_handle *handle;
+	unsigned int code; /* One of LED_* constants */
+};
+
+struct input_leds {
+	struct input_handle handle;
+	unsigned int num_leds;
+	struct input_led leds[];
+};
+
+static enum led_brightness input_leds_brightness_get(struct led_classdev *cdev)
+{
+	struct input_led *led = container_of(cdev, struct input_led, cdev);
+	struct input_dev *input = led->handle->dev;
+
+	return test_bit(led->code, input->ledbit) ? LED_FULL : LED_OFF;
+}
+
+static void input_leds_brightness_set(struct led_classdev *cdev,
+				      enum led_brightness brightness)
+{
+	struct input_led *led = container_of(cdev, struct input_led, cdev);
+
+	input_inject_event(led->handle, EV_LED, led->code, !!brightness);
+}
+
+static void input_leds_event(struct input_handle *handle, unsigned int type,
+			     unsigned int code, int value)
+{
+}
+
+static int input_leds_connect(struct input_handler *handler,
+			      struct input_dev *dev,
+			      const struct input_device_id *id)
+{
+	struct input_leds *leds;
+	unsigned int num_leds;
+	int led_no = 0;
+	int led_code;
+	int error;
+
+	num_leds = bitmap_weight(dev->ledbit, LED_CNT);
+	if (!num_leds)
+		return -ENXIO;
+
+	leds = kzalloc(sizeof(*leds) + num_leds * sizeof(*leds->leds),
+		       GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	leds->num_leds = num_leds;
+
+	leds->handle.dev = dev;
+	leds->handle.handler = handler;
+	leds->handle.name = "leds";
+	leds->handle.private = leds;
+
+	error = input_register_handle(&leds->handle);
+	if (error)
+		goto err_free_mem;
+
+	error = input_open_device(&leds->handle);
+	if (error)
+		goto err_unregister_handle;
+
+	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
+		struct input_led *led = &leds->leds[led_no];
+
+		led->handle = &leds->handle;
+		led->code = led_code;
+
+		if (WARN_ON(!input_led_info[led_code].name))
+			continue;
+
+		led->cdev.name = kasprintf(GFP_KERNEL, "%s::%s",
+					   dev_name(&dev->dev),
+					   input_led_info[led_code].name);
+		if (!led->cdev.name) {
+			error = -ENOMEM;
+			goto err_unregister_leds;
+		}
+
+		led->cdev.brightness_get = input_leds_brightness_get;
+		led->cdev.brightness_set = input_leds_brightness_set;
+		led->cdev.default_trigger = input_led_info[led_code].trigger;
+
+		error = led_classdev_register(&dev->dev, &led->cdev);
+		if (error) {
+			dev_err(&dev->dev, "failed to register LED %s: %d\n",
+				led->cdev.name, error);
+			kfree(led->cdev.name);
+			goto err_unregister_leds;
+		}
+
+		led_no++;
+	}
+
+	return 0;
+
+err_unregister_leds:
+	while (--led_no >= 0) {
+		struct input_led *led = &leds->leds[led_no];
+
+		led_classdev_unregister(&led->cdev);
+		kfree(led->cdev.name);
+	}
+
+	input_close_device(&leds->handle);
+
+err_unregister_handle:
+	input_unregister_handle(&leds->handle);
+
+err_free_mem:
+	kfree(leds);
+	return error;
+}
+
+static void input_leds_disconnect(struct input_handle *handle)
+{
+	struct input_leds *leds = handle->private;
+	int i;
+
+	for (i = 0; i < leds->num_leds; i++) {
+		struct input_led *led = &leds->leds[i];
+
+		led_classdev_unregister(&led->cdev);
+		kfree(led->cdev.name);
+	}
+
+	input_close_device(handle);
+	input_unregister_handle(handle);
+
+	kfree(leds);
+}
+
+static const struct input_device_id input_leds_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT,
+		.evbit = { BIT_MASK(EV_LED) },
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(input, input_leds_ids);
+
+static struct input_handler input_leds_handler = {
+	.event =	input_leds_event,
+	.connect =	input_leds_connect,
+	.disconnect =	input_leds_disconnect,
+	.name =		"leds",
+	.id_table =	input_leds_ids,
+};
+
+static int __init input_leds_init(void)
+{
+	return input_register_handler(&input_leds_handler);
+}
+module_init(input_leds_init);
+
+static void __exit input_leds_exit(void)
+{
+	input_unregister_handler(&input_leds_handler);
+}
+module_exit(input_leds_exit);
+
+MODULE_AUTHOR("Samuel Thibault <samuel.thibault@ens-lyon.org>");
+MODULE_AUTHOR("Dmitry Torokhov <dmitry.torokhov@gmail.com>");
+MODULE_DESCRIPTION("Input -> LEDs Bridge");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 966b960..4191614 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -11,9 +11,6 @@ menuconfig NEW_LEDS
 	  Say Y to enable Linux LED support.  This allows control of supported
 	  LEDs from both userspace and optionally, by kernel events (triggers).
 
-	  This is not related to standard keyboard LEDs which are controlled
-	  via the input system.
-
 if NEW_LEDS
 
 config LEDS_CLASS
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 2/3] tty/vt/keyboard: define LED triggers for VT LED states

[Dmitry Torokhov]

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

Now that input core allows controlling keyboards LEDs via standard LED
subsystem triggers let's switch VT keyboard code to make use of this
feature. We will define the following standard triggers: "kbd-scrollock",
"kbd-numlock", "kbd-capslock", and "kbd-kanalock" which are default
triggers for respective LEDs on keyboards.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/tty/vt/keyboard.c | 141 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 117 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 8a89f6e..cff193e 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -33,6 +33,7 @@
 #include <linux/string.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/leds.h>
 
 #include <linux/kbd_kern.h>
 #include <linux/kbd_diacr.h>
@@ -961,6 +962,110 @@ static void k_brl(struct vc_data *vc, unsigned char value, char up_flag)
 	}
 }
 
+#if IS_ENABLED(CONFIG_INPUT_LEDS)
+
+struct kbd_led_trigger {
+	struct led_trigger trigger;
+	unsigned int mask;
+};
+
+static void kbd_led_trigger_activate(struct led_classdev *cdev)
+{
+	struct kbd_led_trigger *trigger =
+		container_of(cdev->trigger, struct kbd_led_trigger, trigger);
+
+	tasklet_disable(&keyboard_tasklet);
+	if (ledstate != 0xff)
+		led_trigger_event(&trigger->trigger,
+				  ledstate & trigger->mask ?
+					LED_FULL : LED_OFF);
+	tasklet_enable(&keyboard_tasklet);
+}
+
+#define KBD_LED_TRIGGER(_led_bit, _name) {			\
+		.trigger = {					\
+			.name = _name,				\
+			.activate = kbd_led_trigger_activate,	\
+		},						\
+		.mask	= BIT(_led_bit),			\
+	}
+
+static struct kbd_led_trigger kbd_led_triggers[] = {
+	KBD_LED_TRIGGER(VC_SCROLLOCK, "kbd-scrollock"),
+	KBD_LED_TRIGGER(VC_NUMLOCK,   "kbd-numlock"),
+	KBD_LED_TRIGGER(VC_CAPSLOCK,  "kbd-capslock"),
+	KBD_LED_TRIGGER(VC_KANALOCK,  "kbd-kanalock"),
+};
+
+static void kbd_propagate_led_state(unsigned int old_state,
+				    unsigned int new_state)
+{
+	struct kbd_led_trigger *trigger;
+	unsigned int changed = old_state ^ new_state;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); i++) {
+		trigger = &kbd_led_triggers[i];
+
+		if (changed & trigger->mask)
+			led_trigger_event(&trigger->trigger,
+					  new_state & trigger->mask ?
+						LED_FULL : LED_OFF);
+	}
+}
+
+static int kbd_update_leds_helper(struct input_handle *handle, void *data)
+{
+	unsigned int led_state = *(unsigned int *)data;
+
+	if (test_bit(EV_LED, handle->dev->evbit))
+		kbd_propagate_led_state(~led_state, led_state);
+
+	return 0;
+}
+
+static void kbd_init_leds(void)
+{
+	int error;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); i++) {
+		error = led_trigger_register(&kbd_led_triggers[i].trigger);
+		if (error)
+			pr_err("error %d while registering trigger %s\n",
+			       error, kbd_led_triggers[i].trigger.name);
+	}
+}
+
+#else
+
+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_SYN, SYN_REPORT, 0);
+	}
+
+	return 0;
+}
+
+static void kbd_propagate_led_state(unsigned int old_state,
+				    unsigned int new_state)
+{
+	input_handler_for_each_handle(&kbd_handler, &new_state,
+				      kbd_update_leds_helper);
+}
+
+static void kbd_init_leds(void)
+{
+}
+
+#endif
+
 /*
  * The leds display either (i) the status of NumLock, CapsLock, ScrollLock,
  * or (ii) whatever pattern of lights people want to show using KDSETLED,
@@ -995,20 +1100,6 @@ static inline unsigned char getleds(void)
 	return kb->ledflagstate;
 }
 
-static int kbd_update_leds_helper(struct input_handle *handle, void *data)
-{
-	unsigned char leds = *(unsigned char *)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_SYN, SYN_REPORT, 0);
-	}
-
-	return 0;
-}
-
 /**
  *	vt_get_leds	-	helper for braille console
  *	@console: console to read
@@ -1085,24 +1176,22 @@ void vt_kbd_con_stop(int console)
 }
 
 /*
- * This is the tasklet that updates LED state on all keyboards
- * attached to the box. The reason we use tasklet is that we
- * need to handle the scenario when keyboard handler is not
- * registered yet but we already getting updates from the VT to
- * update led state.
+ * This is the tasklet that updates LED state of LEDs using standard
+ * keyboard triggers. The reason we use tasklet is that we need to
+ * handle the scenario when keyboard handler is not registered yet
+ * but we already getting updates from the VT to update led state.
  */
 static void kbd_bh(unsigned long dummy)
 {
 	unsigned char leds;
 	unsigned long flags;
-	
+
 	spin_lock_irqsave(&led_lock, flags);
 	leds = getleds();
 	spin_unlock_irqrestore(&led_lock, flags);
 
 	if (leds != ledstate) {
-		input_handler_for_each_handle(&kbd_handler, &leds,
-					      kbd_update_leds_helper);
+		kbd_propagate_led_state(ledstate, leds);
 		ledstate = leds;
 	}
 }
@@ -1450,8 +1539,10 @@ static void kbd_start(struct input_handle *handle)
 {
 	tasklet_disable(&keyboard_tasklet);
 
-	if (ledstate != 0xff)
-		kbd_update_leds_helper(handle, &ledstate);
+	if (ledstate != 0xff) {
+		unsigned int state = ledstate;
+		kbd_update_leds_helper(handle, &state);
+	}
 
 	tasklet_enable(&keyboard_tasklet);
 }
@@ -1497,6 +1588,8 @@ int __init kbd_init(void)
 		kbd_table[i].kbdmode = default_utf8 ? VC_UNICODE : VC_XLATE;
 	}
 
+	kbd_init_leds();
+
 	error = input_register_handler(&kbd_handler);
 	if (error)
 		return error;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 3/3] tty/vt/keyboard: define LED triggers for VT keyboard lock states

[Dmitry Torokhov]

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

In addition to defining triggers for VT LED states, let's define triggers
for VT keyboard lock states, such as "kbd-shiftlock", "kbd-altgrlock", etc.

This permits to fix #7063 from userland by using a modifier to implement
proper CapsLock behavior and have the keyboard caps lock led show that
modifier state.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/tty/vt/keyboard.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index cff193e..1c420d9 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -130,7 +130,7 @@ static char rep;					/* flag telling character repeat */
 
 static int shift_state = 0;
 
-static unsigned char ledstate = 0xff;			/* undefined */
+static unsigned int ledstate = -1U;			/* undefined */
 static unsigned char ledioctl;
 
 /*
@@ -975,7 +975,7 @@ static void kbd_led_trigger_activate(struct led_classdev *cdev)
 		container_of(cdev->trigger, struct kbd_led_trigger, trigger);
 
 	tasklet_disable(&keyboard_tasklet);
-	if (ledstate != 0xff)
+	if (ledstate != -1U)
 		led_trigger_event(&trigger->trigger,
 				  ledstate & trigger->mask ?
 					LED_FULL : LED_OFF);
@@ -990,11 +990,23 @@ static void kbd_led_trigger_activate(struct led_classdev *cdev)
 		.mask	= BIT(_led_bit),			\
 	}
 
+#define KBD_LOCKSTATE_TRIGGER(_led_bit, _name)		\
+	KBD_LED_TRIGGER((_led_bit) + 8, _name)
+
 static struct kbd_led_trigger kbd_led_triggers[] = {
 	KBD_LED_TRIGGER(VC_SCROLLOCK, "kbd-scrollock"),
 	KBD_LED_TRIGGER(VC_NUMLOCK,   "kbd-numlock"),
 	KBD_LED_TRIGGER(VC_CAPSLOCK,  "kbd-capslock"),
 	KBD_LED_TRIGGER(VC_KANALOCK,  "kbd-kanalock"),
+
+	KBD_LOCKSTATE_TRIGGER(VC_SHIFTLOCK,  "kbd-shiftlock"),
+	KBD_LOCKSTATE_TRIGGER(VC_ALTGRLOCK,  "kbd-altgrlock"),
+	KBD_LOCKSTATE_TRIGGER(VC_CTRLLOCK,   "kbd-ctrllock"),
+	KBD_LOCKSTATE_TRIGGER(VC_ALTLOCK,    "kbd-altlock"),
+	KBD_LOCKSTATE_TRIGGER(VC_SHIFTLLOCK, "kbd-shiftllock"),
+	KBD_LOCKSTATE_TRIGGER(VC_SHIFTRLOCK, "kbd-shiftrlock"),
+	KBD_LOCKSTATE_TRIGGER(VC_CTRLLLOCK,  "kbd-ctrlllock"),
+	KBD_LOCKSTATE_TRIGGER(VC_CTRLRLOCK,  "kbd-ctrlrlock"),
 };
 
 static void kbd_propagate_led_state(unsigned int old_state,
@@ -1073,7 +1085,7 @@ static void kbd_init_leds(void)
  */
 static unsigned char getledstate(void)
 {
-	return ledstate;
+	return ledstate & 0xff;
 }
 
 void setledstate(struct kbd_struct *kb, unsigned int led)
@@ -1183,11 +1195,12 @@ void vt_kbd_con_stop(int console)
  */
 static void kbd_bh(unsigned long dummy)
 {
-	unsigned char leds;
+	unsigned int leds;
 	unsigned long flags;
 
 	spin_lock_irqsave(&led_lock, flags);
 	leds = getleds();
+	leds |= (unsigned int)kbd->lockstate << 8;
 	spin_unlock_irqrestore(&led_lock, flags);
 
 	if (leds != ledstate) {
@@ -1539,10 +1552,8 @@ static void kbd_start(struct input_handle *handle)
 {
 	tasklet_disable(&keyboard_tasklet);
 
-	if (ledstate != 0xff) {
-		unsigned int state = ledstate;
-		kbd_update_leds_helper(handle, &state);
-	}
+	if (ledstate != -1U)
+		kbd_update_leds_helper(handle, &ledstate);
 
 	tasklet_enable(&keyboard_tasklet);
 }
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Bastien Nocera]

On Mon, 2015-06-08 at 14:43 -0700, Dmitry Torokhov wrote:
> Hi,
> 
> I finally was able to spend some time looking over Samuel's patch set
> switching input LEDs from custom implementation over to standard LED 
> class
> devices and I think this is the shape I am reasonably happy with. The
> changes:
> 
<snip>
> 2. There are no per-input device triggers. Input devices only carry 
> LEDs
> and those LEDs use one of the system-wide triggers. Which ones is to 
> user
> to decide. The default triggers are the one defines by keyboard 
> handler for
> it's standard LED states.
> 
<snip>
> Please take a look and see if you see any holes.

What would the quirk for a keyboard not having LEDs look like?

In GNOME, we want to pop up a on-screen display when pressing, say, the
Caps Lock key on a keyboard that doesn't have any such keys (such as my
Lenovo X1 Carbon and the Logitech K750 that's plugged into it in my
case).

Cheers

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Dmitry Torokhov]

On Tue, Jun 09, 2015 at 12:58:11AM +0200, Bastien Nocera wrote:
> On Mon, 2015-06-08 at 14:43 -0700, Dmitry Torokhov wrote:
> > Hi,
> > 
> > I finally was able to spend some time looking over Samuel's patch set
> > switching input LEDs from custom implementation over to standard LED 
> > class
> > devices and I think this is the shape I am reasonably happy with. The
> > changes:
> > 
> <snip>
> > 2. There are no per-input device triggers. Input devices only carry 
> > LEDs
> > and those LEDs use one of the system-wide triggers. Which ones is to 
> > user
> > to decide. The default triggers are the one defines by keyboard 
> > handler for
> > it's standard LED states.
> > 
> <snip>
> > Please take a look and see if you see any holes.
> 
> What would the quirk for a keyboard not having LEDs look like?
> 
> In GNOME, we want to pop up a on-screen display when pressing, say, the
> Caps Lock key on a keyboard that doesn't have any such keys (such as my
> Lenovo X1 Carbon and the Logitech K750 that's plugged into it in my
> case).

This is not really related to this patch set, but LED bitmap for input
devices is sent in their uevents so you can check it in udev. This will
assumes that input driver knows whether LED is present on device or not
(and for PS/2 keyboards it does not and assumes they are there so your
X1's keyboard tell you that it has 3 standard LEDs).

Thanks.

-- 
Dmitry

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Pavel Machek]

Hi!

> I finally was able to spend some time looking over Samuel's patch set
> switching input LEDs from custom implementation over to standard LED class
> devices and I think this is the shape I am reasonably happy with. The
> changes:

Thanks!

> Please take a look and see if you see any holes.

Tested on thinkpad t40p. Started X, but then I went to text console
for testing.

1) input4::caps-lock -- default trigger was not set, so caps lock did
not control the LED. That's bug, right?

I did echo kbd-capslock > trigger, but still no luck. (That might be
bug in Debian, IIRC?)

echo heartbeat > trigger produces expected results.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Pavel Machek]

On Tue 2015-06-09 12:54:21, Pavel Machek wrote:
> Hi!
> 
> > I finally was able to spend some time looking over Samuel's patch set
> > switching input LEDs from custom implementation over to standard LED class
> > devices and I think this is the shape I am reasonably happy with. The
> > changes:
> 
> Thanks!
> 
> > Please take a look and see if you see any holes.
> 
> Tested on thinkpad t40p. Started X, but then I went to text console
> for testing.
> 
> 1) input4::caps-lock -- default trigger was not set, so caps lock did
> not control the LED. That's bug, right?

Strange, on next reboot default triggers were set.

> I did echo kbd-capslock > trigger, but still no luck. (That might be
> bug in Debian, IIRC?)

I tried rebooting with stock Debian kernel, and the bug is there, too.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Pali Rohár]

On Tuesday 09 June 2015 13:12:42 Pavel Machek wrote:
> On Tue 2015-06-09 12:54:21, Pavel Machek wrote:
> > I did echo kbd-capslock > trigger, but still no luck. (That might be
> > bug in Debian, IIRC?)
> 
> I tried rebooting with stock Debian kernel, and the bug is there, too.
> 
> Best regards,
> 									Pavel

Is not trigger only kernel part? If yes, then replacing kernel should
not cause any Debian related bug, right?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Pavel Machek]

On Tue 2015-06-09 13:12:42, Pavel Machek wrote:
> On Tue 2015-06-09 12:54:21, Pavel Machek wrote:
> > Hi!
> > 
> > > I finally was able to spend some time looking over Samuel's patch set
> > > switching input LEDs from custom implementation over to standard LED class
> > > devices and I think this is the shape I am reasonably happy with. The
> > > changes:
> > 
> > Thanks!
> > 
> > > Please take a look and see if you see any holes.
> > 
> > Tested on thinkpad t40p. Started X, but then I went to text console
> > for testing.
> > 
> > 1) input4::caps-lock -- default trigger was not set, so caps lock did
> > not control the LED. That's bug, right?
> 
> Strange, on next reboot default triggers were set.

Even more strange... setting brightness to zero resets the trigger,
setting it to one does not. Oh well, that is problem with other LEDs,
too.

max_brightness should be 1, AFAICT, but that's also mistake other LEDs
make.

So

Tested-by: Pavel Machek <pavel@ucw.cz>

but you don't get acked-by, yet :-).

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Pavel Machek]

On Tue 2015-06-09 13:22:21, Pali Rohár wrote:
> On Tuesday 09 June 2015 13:12:42 Pavel Machek wrote:
> > On Tue 2015-06-09 12:54:21, Pavel Machek wrote:
> > > I did echo kbd-capslock > trigger, but still no luck. (That might be
> > > bug in Debian, IIRC?)
> > 
> > I tried rebooting with stock Debian kernel, and the bug is there, too.
> > 
> > Best regards,
> > 									Pavel
> 
> Is not trigger only kernel part? If yes, then replacing kernel should
> not cause any Debian related bug, right?

I'm not input expert, but when the LED light does not work with kernel
debian ships, I don't expect it to work with patched 4.1.

(And someone mentioned Debian being broken in that regard.)
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Samuel Thibault]

Pavel Machek, le Tue 09 Jun 2015 12:54:21 +0200, a écrit :
> 1) input4::caps-lock -- default trigger was not set, so caps lock did
> not control the LED. That's bug, right?
> 
> I did echo kbd-capslock > trigger, but still no luck. (That might be
> bug in Debian, IIRC?)

In console-setup, yers.

Samuel

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Samuel Thibault]

Pali Rohár, le Tue 09 Jun 2015 13:22:21 +0200, a écrit :
> On Tuesday 09 June 2015 13:12:42 Pavel Machek wrote:
> > On Tue 2015-06-09 12:54:21, Pavel Machek wrote:
> > > I did echo kbd-capslock > trigger, but still no luck. (That might be
> > > bug in Debian, IIRC?)
> > 
> > I tried rebooting with stock Debian kernel, and the bug is there, too.
> > 
> > Best regards,
> 
> Is not trigger only kernel part?

Yes, but depending on the keyboard layout configuration, the capslock
key will toggle a different modifier, and only the capslock modifier
modifies the capslock LED by default.

Whether it's a Debian kernel or a pristine kernel does not matter,
though, as you said.

Samuel

Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Samuel Thibault]

Hello,

Dmitry Torokhov, le Mon 08 Jun 2015 14:43:08 -0700, a écrit :
> 1. Instead of making LED class devices part of the input device they are
> implemented as an input handler (and thus are completely separate from
> input core).

That's nicer indeed.  Not defining triggers per LED however does not
permit to e.g. switch two leds of a keyboard independently of what
produces input events.  I'm personally fine with it, I just wanted to
mention it since this example of usage was cited at some point in the
thread.

> +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },

I'd tend to think we'd want to harmonize the user-visible LED names and
kbd trigger names, i.e. use "numlock", "capslock" and "scrollock" in
both case (or something else, but the same for LED and trigger). In my
patch I simply used the corresponding LED or kbd macro names, but there
is probably no strong reason to this, while there is probably a good
reason to choose coherent and nice user-visible names.

> +static enum led_brightness input_leds_brightness_get(struct led_classdev *cdev)
> +{
> +	struct input_led *led = container_of(cdev, struct input_led, cdev);
> +	struct input_dev *input = led->handle->dev;
> +
> +	return test_bit(led->code, input->ledbit) ? LED_FULL : LED_OFF;

This always returns LED_FULL, whatever the current state of the LED, is
that really what we want?  Userspace will be surprised to read 255 from
sysfs whatever it writes to it (with actual proper effect on the LED).
Simply not defining input_leds_brightness_get and letting the LED core
manage the value does get a proper "brightness" sysfs file behavior, is
there a reason not to do that?

> +	int led_no = 0;

...

> +	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
> +		struct input_led *led = &leds->leds[led_no];

When reading this I wondered what value led_no was, perhaps the
initialization to 0 should be moved to right before the for_each_set_bit
loop, to make the code clearer.

Samuel

Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Samuel Thibault]

Samuel Thibault, le Tue 09 Jun 2015 15:19:55 +0200, a écrit :
> > +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> > +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> > +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },
> 
> I'd tend to think we'd want to harmonize the user-visible LED names and
> kbd trigger names,

And perhaps fix that odd-looking "scrollock"?

Samuel

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Samuel Thibault]

Dmitry Torokhov, le Mon 08 Jun 2015 14:43:07 -0700, a écrit :
> If user wants all keyboards to light up CapsLock LED when VT state locks
> CtrlL modifier they need to write a udev rule or similar to set up
> "kbd-ctrlllock" trigger for all appearing "input%::capslock" LED class
> devices.

I guess console-setup's maintainer will not be happy about it.  I'd
say it'd be good to at least give examples how to do it in the
documentation.

That being said, patches 2 & 3 looked fine to me.

I have only my internal laptop keyboard right now so I haven't
completely tested this yet.

Thanks for your time on this,
Samuel

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Pali Rohár]

On Tuesday 09 June 2015 15:42:41 Samuel Thibault wrote:
> Dmitry Torokhov, le Mon 08 Jun 2015 14:43:07 -0700, a écrit :
> > If user wants all keyboards to light up CapsLock LED when VT state locks
> > CtrlL modifier they need to write a udev rule or similar to set up
> > "kbd-ctrlllock" trigger for all appearing "input%::capslock" LED class
> > devices.
> 
> I guess console-setup's maintainer will not be happy about it.  I'd
> say it'd be good to at least give examples how to do it in the
> documentation.
> 

What about CC relevant people who maintains those userspace packages?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Samuel Thibault]

Pali Rohár, le Tue 09 Jun 2015 15:50:18 +0200, a écrit :
> On Tuesday 09 June 2015 15:42:41 Samuel Thibault wrote:
> > Dmitry Torokhov, le Mon 08 Jun 2015 14:43:07 -0700, a écrit :
> > > If user wants all keyboards to light up CapsLock LED when VT state locks
> > > CtrlL modifier they need to write a udev rule or similar to set up
> > > "kbd-ctrlllock" trigger for all appearing "input%::capslock" LED class
> > > devices.
> > 
> > I guess console-setup's maintainer will not be happy about it.  I'd
> > say it'd be good to at least give examples how to do it in the
> > documentation.
> 
> What about CC relevant people who maintains those userspace packages?

I'll write him, yes.  Just give me time to do it :)

Samuel

Re: caps lock led does not show up

[Samuel Thibault]

Hello,

Samuel Thibault, le Thu 05 Feb 2009 12:39:08 +0100, a écrit :
> And about the led issue, we need to ask the kernel for an interface to
> be able to configure which lock should drive which led.

Dmitry Torokhov, le Mon 08 Jun 2015 14:43:07 -0700, a écrit :
> If user wants all keyboards to light up CapsLock LED when VT state locks
> CtrlL modifier they need to write a udev rule or similar to set up
> "kbd-ctrlllock" trigger for all appearing "input%::capslock" LED class
> devices.

Anton, this is the interface proposed by the input maintainer, Dmitry,
to change which modifiers gets to light the keyboard LEDs (the exact
names may change, but the principle should be firm).  I know this is
inconvenient for console-setup for handling hotplugged keyboards, but
Dmitry prefers to avoid introducing a virtual multiplexer as explained
below:

> Having such virtual multiplexing object just adds complexity and is
> hard to untange (see /dev/input/mice and all the issues we had with
> synaptics driver trying to exclude it's data stream from it).

Samuel

Re: Bug#514464: caps lock led does not show up

[Anton Zinoviev]

On Tue, Jun 09, 2015 at 04:17:23PM +0200, Samuel Thibault wrote:
> 
> Dmitry Torokhov, le Mon 08 Jun 2015 14:43:07 -0700, a écrit :
> > If user wants all keyboards to light up CapsLock LED when VT state locks
> > CtrlL modifier they need to write a udev rule or similar to set up
> > "kbd-ctrlllock" trigger for all appearing "input%::capslock" LED class
> > devices.
> 
> Anton, this is the interface proposed by the input maintainer, Dmitry,
> to change which modifiers gets to light the keyboard LEDs (the exact
> names may change, but the principle should be firm).  I know this is
> inconvenient for console-setup for handling hotplugged keyboards,

Ok, the inconvenience is not a problem.  The problem is I don't 
understant the meaning of this. :)

Is there some documentation or a sample code I can read?

Anton Zinoviev

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Pavel Machek]

Hi!

> > I finally was able to spend some time looking over Samuel's patch set
> > switching input LEDs from custom implementation over to standard LED class
> > devices and I think this is the shape I am reasonably happy with. The
> > changes:
> 
> Thanks!
> 
> > Please take a look and see if you see any holes.
> 
> Tested on thinkpad t40p. Started X, but then I went to text console
> for testing.
> 
> 1) input4::caps-lock -- default trigger was not set, so caps lock did
> not control the LED. That's bug, right?

max_brightness for caps-lock led is 255. I believe that should be 1,
as keyboard LEDs can not do PWM.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Dmitry Torokhov]

Hi Pavel,

On Tue, Jun 09, 2015 at 06:18:18PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > I finally was able to spend some time looking over Samuel's patch set
> > > switching input LEDs from custom implementation over to standard LED class
> > > devices and I think this is the shape I am reasonably happy with. The
> > > changes:
> > 
> > Thanks!
> > 
> > > Please take a look and see if you see any holes.
> > 
> > Tested on thinkpad t40p. Started X, but then I went to text console
> > for testing.
> > 
> > 1) input4::caps-lock -- default trigger was not set, so caps lock did
> > not control the LED. That's bug, right?
> 
> max_brightness for caps-lock led is 255. I believe that should be 1,
> as keyboard LEDs can not do PWM.

Yes, you are right, I'll change max_brightness to 1 in the next
revision.

Thanks.

-- 
Dmitry

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Dmitry Torokhov]

On Tue, Jun 09, 2015 at 12:54:21PM +0200, Pavel Machek wrote:
> Hi!
> 
> > I finally was able to spend some time looking over Samuel's patch set
> > switching input LEDs from custom implementation over to standard LED class
> > devices and I think this is the shape I am reasonably happy with. The
> > changes:
> 
> Thanks!
> 
> > Please take a look and see if you see any holes.
> 
> Tested on thinkpad t40p. Started X, but then I went to text console
> for testing.
> 
> 1) input4::caps-lock -- default trigger was not set, so caps lock did
> not control the LED. That's bug, right?

Since you mention that after rebooting it assigned the right trigger
that is expected - the triigers are coming from keyboard.c which is
built-in, not a module.

> 
> I did echo kbd-capslock > trigger, but still no luck. (That might be
> bug in Debian, IIRC?)

On Debian and Ubunty the keycode emitted by CapsLock ke=y is CtrlL_Lock
for whatever reason, so CapsLock LED does not light up by default
(neither with these patcher nor without them). If you change setkeycodes
to assign CapsLock to keycode 58 it should work. Or set trigger for the
led to "kbd-ctrlllock".

Thanks.

-- 
Dmitry

Re: [PATCH 0/3] Switch input leds over to standard LED class devices

[Dmitry Torokhov]

On Tue, Jun 09, 2015 at 01:26:34PM +0200, Pavel Machek wrote:
> On Tue 2015-06-09 13:12:42, Pavel Machek wrote:
> > On Tue 2015-06-09 12:54:21, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > I finally was able to spend some time looking over Samuel's patch set
> > > > switching input LEDs from custom implementation over to standard LED class
> > > > devices and I think this is the shape I am reasonably happy with. The
> > > > changes:
> > > 
> > > Thanks!
> > > 
> > > > Please take a look and see if you see any holes.
> > > 
> > > Tested on thinkpad t40p. Started X, but then I went to text console
> > > for testing.
> > > 
> > > 1) input4::caps-lock -- default trigger was not set, so caps lock did
> > > not control the LED. That's bug, right?
> > 
> > Strange, on next reboot default triggers were set.
> 
> Even more strange... setting brightness to zero resets the trigger,
> setting it to one does not. Oh well, that is problem with other LEDs,
> too.

That is current led core behavior. See
drivers/led/led-class.c::brightness_store():

	if (state == LED_OFF)
		led_trigger_remove(led_cdev);
	led_set_brightness(led_cdev, state);

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Dmitry Torokhov]

On Tue, Jun 09, 2015 at 03:19:55PM +0200, Samuel Thibault wrote:
> Hello,
> 
> Dmitry Torokhov, le Mon 08 Jun 2015 14:43:08 -0700, a écrit :
> > 1. Instead of making LED class devices part of the input device they are
> > implemented as an input handler (and thus are completely separate from
> > input core).
> 
> That's nicer indeed.  Not defining triggers per LED however does not
> permit to e.g. switch two leds of a keyboard independently of what
> produces input events.  I'm personally fine with it, I just wanted to
> mention it since this example of usage was cited at some point in the
> thread.

I might have over-though the issue a bit in the past ;) But I think I am
happy with the current behavior, it separates input events and leds and
just says that you can select what tgrigges led state change. And you
shoudl still be able to do:

echo "kbd-ctrlllock" > /sys/..../input22::caps-lock/trigger
echo "heartbit" > /sys/..../input22::num-lock/trigger
echo "kbd-numlock" > /sys/..../input22::scroll-lock/trigger

But you can't say that pressing CapsLock on keyboard1 should light up
ScrollLock led on keyboard2, nor do we want it I think. If such control
is truly needed userspace can take over and managed leds as it sees fit,
like X does.

> 
> > +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> > +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> > +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },
> 
> I'd tend to think we'd want to harmonize the user-visible LED names and
> kbd trigger names, i.e. use "numlock", "capslock" and "scrollock" in
> both case (or something else, but the same for LED and trigger). In my
> patch I simply used the corresponding LED or kbd macro names, but there
> is probably no strong reason to this, while there is probably a good
> reason to choose coherent and nice user-visible names.

I can do either "num_lock - kbd-num-lock" or "numlock - kbd-numlock"
with slight preference to the 1st. What is your preference?

> 
> > +static enum led_brightness input_leds_brightness_get(struct led_classdev *cdev)
> > +{
> > +	struct input_led *led = container_of(cdev, struct input_led, cdev);
> > +	struct input_dev *input = led->handle->dev;
> > +
> > +	return test_bit(led->code, input->ledbit) ? LED_FULL : LED_OFF;
> 
> This always returns LED_FULL, whatever the current state of the LED, is
> that really what we want?  Userspace will be surprised to read 255 from
> sysfs whatever it writes to it (with actual proper effect on the LED).

Right, I will change it to 0 and led->max_brightness (which I will set
to 1).

> Simply not defining input_leds_brightness_get and letting the LED core
> manage the value does get a proper "brightness" sysfs file behavior, is
> there a reason not to do that?

Yes, we want LED sysfs show correct result if state is altered via
EV_LED/LED_* event. Basically the led bit state is the source of truth
here.

> 
> > +	int led_no = 0;
> 
> ...
> 
> > +	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
> > +		struct input_led *led = &leds->leds[led_no];
> 
> When reading this I wondered what value led_no was, perhaps the
> initialization to 0 should be moved to right before the for_each_set_bit
> loop, to make the code clearer.

Fair enough, will change.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Dmitry Torokhov]

On Tue, Jun 09, 2015 at 03:27:34PM +0200, Samuel Thibault wrote:
> Samuel Thibault, le Tue 09 Jun 2015 15:19:55 +0200, a écrit :
> > > +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> > > +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> > > +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },
> > 
> > I'd tend to think we'd want to harmonize the user-visible LED names and
> > kbd trigger names,
> 
> And perhaps fix that odd-looking "scrollock"?

If you are talking about LED_SCROLLL then iut is a part of published
userspace API and we can only add an alias. If we want to do that I'd do
it in a separate patch.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Samuel Thibault]

Dmitry Torokhov, le Tue 09 Jun 2015 09:50:52 -0700, a écrit :
> On Tue, Jun 09, 2015 at 03:27:34PM +0200, Samuel Thibault wrote:
> > Samuel Thibault, le Tue 09 Jun 2015 15:19:55 +0200, a écrit :
> > > > +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> > > > +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> > > > +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },
> > > 
> > > I'd tend to think we'd want to harmonize the user-visible LED names and
> > > kbd trigger names,
> > 
> > And perhaps fix that odd-looking "scrollock"?
> 
> If you are talking about LED_SCROLLL

I mean kbd-scrollock in the above code which looks like a typo to me.

Samuel

Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Samuel Thibault]

Dmitry Torokhov, le Tue 09 Jun 2015 09:49:35 -0700, a écrit :
> > > +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> > > +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> > > +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },
> > 
> > I'd tend to think we'd want to harmonize the user-visible LED names and
> > kbd trigger names, i.e. use "numlock", "capslock" and "scrollock" in
> > both case (or something else, but the same for LED and trigger). In my
> > patch I simply used the corresponding LED or kbd macro names, but there
> > is probably no strong reason to this, while there is probably a good
> > reason to choose coherent and nice user-visible names.
> 
> I can do either "num_lock - kbd-num-lock" or "numlock - kbd-numlock"
> with slight preference to the 1st. What is your preference?

I'd prefer numlock - kbd-numlock.

> > > +static enum led_brightness input_leds_brightness_get(struct led_classdev *cdev)
> > > +{
> > > +	struct input_led *led = container_of(cdev, struct input_led, cdev);
> > > +	struct input_dev *input = led->handle->dev;
> > > +
> > > +	return test_bit(led->code, input->ledbit) ? LED_FULL : LED_OFF;
> > 
> > This always returns LED_FULL, whatever the current state of the LED, is
> > that really what we want?  Userspace will be surprised to read 255 from
> > sysfs whatever it writes to it (with actual proper effect on the LED).
> 
> Right, I will change it to 0 and led->max_brightness (which I will set
> to 1).

Well, that won't fix the issue I'm having, see below.

> > Simply not defining input_leds_brightness_get and letting the LED core
> > manage the value does get a proper "brightness" sysfs file behavior, is
> > there a reason not to do that?
> 
> Yes, we want LED sysfs show correct result if state is altered via
> EV_LED/LED_* event. Basically the led bit state is the source of truth
> here.

Ok, I understand that. But it happens that your code does not work! It
is always 255 (or will be 1 with the modifications you mention above).
input->ledbit is whether the LED exists or not, not its state, right?
Did your perhaps mean input->led in input_leds_brightness_get instead of
input->ledbit?

Samuel

Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Dmitry Torokhov]

On Tue, Jun 09, 2015 at 07:22:31PM +0200, Samuel Thibault wrote:
> Dmitry Torokhov, le Tue 09 Jun 2015 09:49:35 -0700, a écrit :
> > > > +	[LED_NUML]	= { "num-lock", VT_TRIGGER("kbd-numlock") },
> > > > +	[LED_CAPSL]	= { "caps-lock", VT_TRIGGER("kbd-capslock") },
> > > > +	[LED_SCROLLL]	= { "scroll-lock", VT_TRIGGER("kbd-scrollock") },
> > > 
> > > I'd tend to think we'd want to harmonize the user-visible LED names and
> > > kbd trigger names, i.e. use "numlock", "capslock" and "scrollock" in
> > > both case (or something else, but the same for LED and trigger). In my
> > > patch I simply used the corresponding LED or kbd macro names, but there
> > > is probably no strong reason to this, while there is probably a good
> > > reason to choose coherent and nice user-visible names.
> > 
> > I can do either "num_lock - kbd-num-lock" or "numlock - kbd-numlock"
> > with slight preference to the 1st. What is your preference?
> 
> I'd prefer numlock - kbd-numlock.

OK.

> 
> > > > +static enum led_brightness input_leds_brightness_get(struct led_classdev *cdev)
> > > > +{
> > > > +	struct input_led *led = container_of(cdev, struct input_led, cdev);
> > > > +	struct input_dev *input = led->handle->dev;
> > > > +
> > > > +	return test_bit(led->code, input->ledbit) ? LED_FULL : LED_OFF;
> > > 
> > > This always returns LED_FULL, whatever the current state of the LED, is
> > > that really what we want?  Userspace will be surprised to read 255 from
> > > sysfs whatever it writes to it (with actual proper effect on the LED).
> > 
> > Right, I will change it to 0 and led->max_brightness (which I will set
> > to 1).
> 
> Well, that won't fix the issue I'm having, see below.
> 
> > > Simply not defining input_leds_brightness_get and letting the LED core
> > > manage the value does get a proper "brightness" sysfs file behavior, is
> > > there a reason not to do that?
> > 
> > Yes, we want LED sysfs show correct result if state is altered via
> > EV_LED/LED_* event. Basically the led bit state is the source of truth
> > here.
> 
> Ok, I understand that. But it happens that your code does not work! It
> is always 255 (or will be 1 with the modifications you mention above).
> input->ledbit is whether the LED exists or not, not its state, right?
> Did your perhaps mean input->led in input_leds_brightness_get instead of
> input->ledbit?

Yep. I'll change it.

-- 
Dmitry

[PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Dmitry Torokhov]

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

This change creates a new input handler called "leds" that exports LEDs on input
devices as standard LED class devices in sysfs and allows controlling their
ptate via sysfs or via any of the standard LED triggers. This allows to
re-purpose and reassign LDEs on the keyboards to represent states other
than the standard keyboard states (CapsLock, NumLock, etc).

The old API of controlling input LEDs by writing into /dev/input/eventX
devices is still present and will take precedence over acessing via LEDs
subsystem (i.e. it may override state set by a trigger). If input device is
"grabbed" then requests coming through LED subsystem will be ignored.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

Changes since V1:

- Adjusted names of LEDs to better match triggers;
- Changed input_leds_brightness_get() to properly test led state
  (dev->led, not dev->ledbit);
- Changed input_leds_brightness_get() to return 0 or max_brightness, not
  LED_FULL;
- Initialize max_brightness to 1;
- Moved initialization of led-no.

 Documentation/leds/leds-class.txt |    3 -
 drivers/input/Kconfig             |   13 ++
 drivers/input/Makefile            |    1 
 drivers/input/input-leds.c        |  212 +++++++++++++++++++++++++++++++++++++
 drivers/leds/Kconfig              |    3 -
 5 files changed, 226 insertions(+), 6 deletions(-)
 create mode 100644 drivers/input/input-leds.c

diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
index 79699c2..62261c0 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -2,9 +2,6 @@
 LED handling under Linux
 ========================
 
-If you're reading this and thinking about keyboard leds, these are
-handled by the input subsystem and the led class is *not* needed.
-
 In its simplest form, the LED class just allows control of LEDs from
 userspace. LEDs appear in /sys/class/leds/. The maximum brightness of the
 LED is defined in max_brightness file. The brightness file will set the brightness
diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index a11ff74..a35532e 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -25,6 +25,19 @@ config INPUT
 
 if INPUT
 
+config INPUT_LEDS
+	tristate "Export input device LEDs in sysfs"
+	depends on LEDS_CLASS
+	default INPUT
+	help
+	  Say Y here if you would like to export LEDs on input devices
+	  as standard LED class devices in sysfs.
+
+	  If unsure, say Y.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called input-leds.
+
 config INPUT_FF_MEMLESS
 	tristate "Support for memoryless force-feedback devices"
 	help
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index 5ca3f63..0c9302c 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_INPUT_POLLDEV)	+= input-polldev.o
 obj-$(CONFIG_INPUT_SPARSEKMAP)	+= sparse-keymap.o
 obj-$(CONFIG_INPUT_MATRIXKMAP)	+= matrix-keymap.o
 
+obj-$(CONFIG_INPUT_LEDS)	+= input-leds.o
 obj-$(CONFIG_INPUT_MOUSEDEV)	+= mousedev.o
 obj-$(CONFIG_INPUT_JOYDEV)	+= joydev.o
 obj-$(CONFIG_INPUT_EVDEV)	+= evdev.o
diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
new file mode 100644
index 0000000..074a65e
--- /dev/null
+++ b/drivers/input/input-leds.c
@@ -0,0 +1,212 @@
+/*
+ * LED support for the input layer
+ *
+ * Copyright 2010-2015 Samuel Thibault <samuel.thibault@ens-lyon.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/input.h>
+
+#if IS_ENABLED(CONFIG_VT)
+#define VT_TRIGGER(_name)	.trigger = _name
+#else
+#define VT_TRIGGER(_name)	.trigger = NULL
+#endif
+
+static const struct {
+	const char *name;
+	const char *trigger;
+} input_led_info[LED_CNT] = {
+	[LED_NUML]	= { "numlock", VT_TRIGGER("kbd-numlock") },
+	[LED_CAPSL]	= { "capslock", VT_TRIGGER("kbd-capslock") },
+	[LED_SCROLLL]	= { "scrolllock", VT_TRIGGER("kbd-scrolllock") },
+	[LED_COMPOSE]	= { "compose" },
+	[LED_KANA]	= { "kana", VT_TRIGGER("kbd-kanalock") },
+	[LED_SLEEP]	= { "sleep" } ,
+	[LED_SUSPEND]	= { "suspend" },
+	[LED_MUTE]	= { "mute" },
+	[LED_MISC]	= { "misc" },
+	[LED_MAIL]	= { "mail" },
+	[LED_CHARGING]	= { "charging" },
+};
+
+struct input_led {
+	struct led_classdev cdev;
+	struct input_handle *handle;
+	unsigned int code; /* One of LED_* constants */
+};
+
+struct input_leds {
+	struct input_handle handle;
+	unsigned int num_leds;
+	struct input_led leds[];
+};
+
+static enum led_brightness input_leds_brightness_get(struct led_classdev *cdev)
+{
+	struct input_led *led = container_of(cdev, struct input_led, cdev);
+	struct input_dev *input = led->handle->dev;
+
+	return test_bit(led->code, input->led) ? cdev->max_brightness : 0;
+}
+
+static void input_leds_brightness_set(struct led_classdev *cdev,
+				      enum led_brightness brightness)
+{
+	struct input_led *led = container_of(cdev, struct input_led, cdev);
+
+	input_inject_event(led->handle, EV_LED, led->code, !!brightness);
+}
+
+static void input_leds_event(struct input_handle *handle, unsigned int type,
+			     unsigned int code, int value)
+{
+}
+
+static int input_leds_connect(struct input_handler *handler,
+			      struct input_dev *dev,
+			      const struct input_device_id *id)
+{
+	struct input_leds *leds;
+	unsigned int num_leds;
+	unsigned int led_code;
+	int led_no;
+	int error;
+
+	num_leds = bitmap_weight(dev->ledbit, LED_CNT);
+	if (!num_leds)
+		return -ENXIO;
+
+	leds = kzalloc(sizeof(*leds) + num_leds * sizeof(*leds->leds),
+		       GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	leds->num_leds = num_leds;
+
+	leds->handle.dev = dev;
+	leds->handle.handler = handler;
+	leds->handle.name = "leds";
+	leds->handle.private = leds;
+
+	error = input_register_handle(&leds->handle);
+	if (error)
+		goto err_free_mem;
+
+	error = input_open_device(&leds->handle);
+	if (error)
+		goto err_unregister_handle;
+
+	led_no = 0;
+	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
+		struct input_led *led = &leds->leds[led_no];
+
+		led->handle = &leds->handle;
+		led->code = led_code;
+
+		if (WARN_ON(!input_led_info[led_code].name))
+			continue;
+
+		led->cdev.name = kasprintf(GFP_KERNEL, "%s::%s",
+					   dev_name(&dev->dev),
+					   input_led_info[led_code].name);
+		if (!led->cdev.name) {
+			error = -ENOMEM;
+			goto err_unregister_leds;
+		}
+
+		led->cdev.max_brightness = 1;
+		led->cdev.brightness_get = input_leds_brightness_get;
+		led->cdev.brightness_set = input_leds_brightness_set;
+		led->cdev.default_trigger = input_led_info[led_code].trigger;
+
+		error = led_classdev_register(&dev->dev, &led->cdev);
+		if (error) {
+			dev_err(&dev->dev, "failed to register LED %s: %d\n",
+				led->cdev.name, error);
+			kfree(led->cdev.name);
+			goto err_unregister_leds;
+		}
+
+		led_no++;
+	}
+
+	return 0;
+
+err_unregister_leds:
+	while (--led_no >= 0) {
+		struct input_led *led = &leds->leds[led_no];
+
+		led_classdev_unregister(&led->cdev);
+		kfree(led->cdev.name);
+	}
+
+	input_close_device(&leds->handle);
+
+err_unregister_handle:
+	input_unregister_handle(&leds->handle);
+
+err_free_mem:
+	kfree(leds);
+	return error;
+}
+
+static void input_leds_disconnect(struct input_handle *handle)
+{
+	struct input_leds *leds = handle->private;
+	int i;
+
+	for (i = 0; i < leds->num_leds; i++) {
+		struct input_led *led = &leds->leds[i];
+
+		led_classdev_unregister(&led->cdev);
+		kfree(led->cdev.name);
+	}
+
+	input_close_device(handle);
+	input_unregister_handle(handle);
+
+	kfree(leds);
+}
+
+static const struct input_device_id input_leds_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT,
+		.evbit = { BIT_MASK(EV_LED) },
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(input, input_leds_ids);
+
+static struct input_handler input_leds_handler = {
+	.event =	input_leds_event,
+	.connect =	input_leds_connect,
+	.disconnect =	input_leds_disconnect,
+	.name =		"leds",
+	.id_table =	input_leds_ids,
+};
+
+static int __init input_leds_init(void)
+{
+	return input_register_handler(&input_leds_handler);
+}
+module_init(input_leds_init);
+
+static void __exit input_leds_exit(void)
+{
+	input_unregister_handler(&input_leds_handler);
+}
+module_exit(input_leds_exit);
+
+MODULE_AUTHOR("Samuel Thibault <samuel.thibault@ens-lyon.org>");
+MODULE_AUTHOR("Dmitry Torokhov <dmitry.torokhov@gmail.com>");
+MODULE_DESCRIPTION("Input -> LEDs Bridge");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 966b960..4191614 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -11,9 +11,6 @@ menuconfig NEW_LEDS
 	  Say Y to enable Linux LED support.  This allows control of supported
 	  LEDs from both userspace and optionally, by kernel events (triggers).
 
-	  This is not related to standard keyboard LEDs which are controlled
-	  via the input system.
-
 if NEW_LEDS
 
 config LEDS_CLASS

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Samuel Thibault]

Hello,

Thanks for the modified version.  This all seems to be working as
expected with multiple keyboards.

Samuel

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Dmitry Torokhov]

On Wed, Jun 10, 2015 at 02:32:32AM +0200, Samuel Thibault wrote:
> Hello,
> 
> Thanks for the modified version.  This all seems to be working as
> expected with multiple keyboards.

Excellent. Let's give Pavel and Pali chance to test it out and let's get
it in 4.2.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Input: export LEDs as class devices in sysfs

[Pavel Machek]

On Tue 2015-06-09 09:49:35, Dmitry Torokhov wrote:
> On Tue, Jun 09, 2015 at 03:19:55PM +0200, Samuel Thibault wrote:
> > Hello,
> > 
> > Dmitry Torokhov, le Mon 08 Jun 2015 14:43:08 -0700, a écrit :
> > > 1. Instead of making LED class devices part of the input device they are
> > > implemented as an input handler (and thus are completely separate from
> > > input core).
> > 
> > That's nicer indeed.  Not defining triggers per LED however does not
> > permit to e.g. switch two leds of a keyboard independently of what
> > produces input events.  I'm personally fine with it, I just wanted to
> > mention it since this example of usage was cited at some point in the
> > thread.
> 
> I might have over-though the issue a bit in the past ;) But I think I am
> happy with the current behavior, it separates input events and leds and
> just says that you can select what tgrigges led state change. And you
> shoudl still be able to do:
> 
> echo "kbd-ctrlllock" > /sys/..../input22::caps-lock/trigger
> echo "heartbit" > /sys/..../input22::num-lock/trigger
> echo "kbd-numlock" > /sys/..../input22::scroll-lock/trigger
> 
> But you can't say that pressing CapsLock on keyboard1 should light up
> ScrollLock led on keyboard2, nor do we want it I think. If such control
> is truly needed userspace can take over and managed leds as it sees fit,
> like X does.

Ack. Please keep it simple.
									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Bug#514464: caps lock led does not show up

[Samuel Thibault]

Hello,

Anton Zinoviev, le Tue 09 Jun 2015 19:03:39 +0300, a écrit :
> On Tue, Jun 09, 2015 at 04:17:23PM +0200, Samuel Thibault wrote:
> > 
> > Dmitry Torokhov, le Mon 08 Jun 2015 14:43:07 -0700, a écrit :
> > > If user wants all keyboards to light up CapsLock LED when VT state locks
> > > CtrlL modifier they need to write a udev rule or similar to set up
> > > "kbd-ctrlllock" trigger for all appearing "input%::capslock" LED class
> > > devices.
> > 
> > Anton, this is the interface proposed by the input maintainer, Dmitry,
> > to change which modifiers gets to light the keyboard LEDs (the exact
> > names may change, but the principle should be firm).  I know this is
> > inconvenient for console-setup for handling hotplugged keyboards,
> 
> Ok, the inconvenience is not a problem.  The problem is I don't 
> understant the meaning of this. :)
> 
> Is there some documentation or a sample code I can read?

Putting

SUBSYSTEM=="leds", ENV{DEVPATH}=="*/input*::capslock", ATTR{trigger}="kbd-ctrlllock"

in /etc/udev/rules.d/50-leds.rules seems to be doing the work. It'd be
good to include this in the documentation along the patch.

Samuel

Re: Bug#514464: caps lock led does not show up

[Samuel Thibault]

Samuel Thibault, le Thu 11 Jun 2015 10:08:09 +0200, a écrit :
> Putting
> 
> SUBSYSTEM=="leds", ENV{DEVPATH}=="*/input*::capslock", ATTR{trigger}="kbd-ctrlllock"
> 
> in /etc/udev/rules.d/50-leds.rules seems to be doing the work.

Except that it makes my system unbootable. For whatever reason, systemd
indefinitely waits for the network to become available...

Samuel

Re: Bug#514464: caps lock led does not show up

[Samuel Thibault]

Samuel Thibault, le Thu 11 Jun 2015 16:28:03 +0200, a écrit :
> Samuel Thibault, le Thu 11 Jun 2015 10:08:09 +0200, a écrit :
> > Putting
> > 
> > SUBSYSTEM=="leds", ENV{DEVPATH}=="*/input*::capslock", ATTR{trigger}="kbd-ctrlllock"
> > 
> > in /etc/udev/rules.d/50-leds.rules seems to be doing the work.
> 
> Except that it makes my system unbootable. For whatever reason, systemd
> indefinitely waits for the network to become available...

ACTION=="add", SUBSYSTEM=="leds", ENV{DEVPATH}=="*/input*::capslock", ATTR{trigger}="kbd-ctrlllock"

works, however :)

Samuel

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Pavel Machek]

On Tue 2015-06-09 18:24:54, Dmitry Torokhov wrote:
> On Wed, Jun 10, 2015 at 02:32:32AM +0200, Samuel Thibault wrote:
> > Hello,
> > 
> > Thanks for the modified version.  This all seems to be working as
> > expected with multiple keyboards.
> 
> Excellent. Let's give Pavel and Pali chance to test it out and let's get
> it in 4.2.

Tested-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Pavel Machek <pavel@ucw.cz>
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Pavel Machek]

On Tue 2015-06-09 18:24:54, Dmitry Torokhov wrote:
> On Wed, Jun 10, 2015 at 02:32:32AM +0200, Samuel Thibault wrote:
> > Hello,
> > 
> > Thanks for the modified version.  This all seems to be working as
> > expected with multiple keyboards.
> 
> Excellent. Let's give Pavel and Pali chance to test it out and let's get
> it in 4.2.

I guess Pali is busy... but please don't let that stop 4.2 merge :-).

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Pali Rohár]

On Monday 15 June 2015 12:03:08 Pavel Machek wrote:
> On Tue 2015-06-09 18:24:54, Dmitry Torokhov wrote:
> > On Wed, Jun 10, 2015 at 02:32:32AM +0200, Samuel Thibault wrote:
> > > Hello,
> > > 
> > > Thanks for the modified version.  This all seems to be working as
> > > expected with multiple keyboards.
> > 
> > Excellent. Let's give Pavel and Pali chance to test it out and let's get
> > it in 4.2.
> 
> I guess Pali is busy... but please don't let that stop 4.2 merge :-).
> 
> Thanks,
> 									Pavel

Yes... Do you need to do tests also by me? Pavel and Samuel already
tested it, so in this case I would trust them and if there will be some
problems I will report them later... I also want to see this option
finally in 4.2 kernel.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Vlastimil Babka]

On 06/09/2015 07:42 PM, Dmitry Torokhov wrote:
> From: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> This change creates a new input handler called "leds" that exports LEDs on input
> devices as standard LED class devices in sysfs and allows controlling their
> ptate via sysfs or via any of the standard LED triggers. This allows to
> re-purpose and reassign LDEs on the keyboards to represent states other
> than the standard keyboard states (CapsLock, NumLock, etc).
> 
> The old API of controlling input LEDs by writing into /dev/input/eventX
> devices is still present and will take precedence over acessing via LEDs
> subsystem (i.e. it may override state set by a trigger). If input device is
> "grabbed" then requests coming through LED subsystem will be ignored.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

> +	led_no = 0;
> +	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
> +		struct input_led *led = &leds->leds[led_no];
> +
> +		led->handle = &leds->handle;
> +		led->code = led_code;
> +
> +		if (WARN_ON(!input_led_info[led_code].name))
> +			continue;
> +

Hi,
I get several warnings on booting 4.2-rc2 here. Should I be worried?

[    2.782432] ------------[ cut here ]------------
[    2.782440] WARNING: CPU: 2 PID: 356 at ../drivers/input/input-leds.c:115 input_leds_connect+0x22b/0x260()
[    2.782441] Modules linked in: btrfs xor raid6_pq crc32c_intel radeon i2c_algo_bit sr_mod cdrom drm_kms_helper ttm e1000e drm xhci_pci ptp pps_core xhci_hcd sg
[    2.782453] CPU: 2 PID: 356 Comm: kworker/2:2 Not tainted 4.2.0-rc2-1.g288d56b-desktop #1
[    2.782454] Hardware name: Dell Inc. OptiPlex 9010/0M9KCM, BIOS A13 03/27/2013
[    2.782458] Workqueue: usb_hub_wq hub_event
[    2.782459]  ffffffff81a917b7 ffff880213ecf298 ffffffff8169f19d 0000000000000007
[    2.782462]  0000000000000000 ffff880213ecf2d8 ffffffff810674f6 ffff880213ecf2f8
[    2.782463]  ffff8802132fb000 0000000000000003 000000000000000b ffff8800369ff000
[    2.782465] Call Trace:
[    2.782470]  [<ffffffff8169f19d>] dump_stack+0x4c/0x6e
[    2.782474]  [<ffffffff810674f6>] warn_slowpath_common+0x86/0xc0
[    2.782476]  [<ffffffff810675ea>] warn_slowpath_null+0x1a/0x20
[    2.782478]  [<ffffffff8152ccdb>] input_leds_connect+0x22b/0x260
[    2.782480]  [<ffffffff815281b2>] input_attach_handler+0x1a2/0x1f0
[    2.782483]  [<ffffffff81528640>] input_register_device+0x440/0x4f0
[    2.782486]  [<ffffffff8156e494>] hidinput_connect+0x334/0x5d0
[    2.782488]  [<ffffffff815683d4>] hid_connect+0x324/0x400
[    2.782490]  [<ffffffff81578b7a>] ? usbhid_start+0x54a/0x780
[    2.782492]  [<ffffffff81569858>] hid_device_probe+0x118/0x140
[    2.782495]  [<ffffffff81482134>] driver_probe_device+0x1f4/0x460
[    2.782497]  [<ffffffff814824b9>] __device_attach_driver+0x79/0xa0
[    2.782499]  [<ffffffff81482440>] ? __driver_attach+0xa0/0xa0
[    2.782501]  [<ffffffff8147ff6b>] bus_for_each_drv+0x5b/0x90
[    2.782502]  [<ffffffff81481e58>] __device_attach+0xa8/0x120
[    2.782504]  [<ffffffff81482523>] device_initial_probe+0x13/0x20
[    2.782506]  [<ffffffff8148116a>] bus_probe_device+0x9a/0xb0
[    2.782508]  [<ffffffff8147edee>] device_add+0x3fe/0x670
[    2.782511]  [<ffffffff812bf9d3>] ? debugfs_create_file+0xd3/0x110
[    2.782513]  [<ffffffff8156956c>] hid_add_device+0xcc/0x2a0
[    2.782515]  [<ffffffff81578404>] usbhid_probe+0x2e4/0x410
[    2.782518]  [<ffffffff814fe0d2>] usb_probe_interface+0x1b2/0x2d0
[    2.782519]  [<ffffffff81482134>] driver_probe_device+0x1f4/0x460
[    2.782521]  [<ffffffff814824b9>] __device_attach_driver+0x79/0xa0
[    2.782523]  [<ffffffff81482440>] ? __driver_attach+0xa0/0xa0
[    2.782525]  [<ffffffff8147ff6b>] bus_for_each_drv+0x5b/0x90
[    2.782526]  [<ffffffff81481e58>] __device_attach+0xa8/0x120
[    2.782528]  [<ffffffff81482523>] device_initial_probe+0x13/0x20
[    2.782530]  [<ffffffff8148116a>] bus_probe_device+0x9a/0xb0
[    2.782531]  [<ffffffff8147edee>] device_add+0x3fe/0x670
[    2.782533]  [<ffffffff814fbfb1>] usb_set_configuration+0x501/0x8e0
[    2.782535]  [<ffffffff81253efe>] ? kernfs_add_one+0xee/0x140
[    2.782537]  [<ffffffff8150656e>] generic_probe+0x2e/0x80
[    2.782539]  [<ffffffff814fdee2>] usb_probe_device+0x32/0x70
[    2.782541]  [<ffffffff81482134>] driver_probe_device+0x1f4/0x460
[    2.782542]  [<ffffffff814824b9>] __device_attach_driver+0x79/0xa0
[    2.782544]  [<ffffffff81482440>] ? __driver_attach+0xa0/0xa0
[    2.782546]  [<ffffffff8147ff6b>] bus_for_each_drv+0x5b/0x90
[    2.782547]  [<ffffffff81481e58>] __device_attach+0xa8/0x120
[    2.782549]  [<ffffffff81482523>] device_initial_probe+0x13/0x20
[    2.782551]  [<ffffffff8148116a>] bus_probe_device+0x9a/0xb0
[    2.782552]  [<ffffffff8147edee>] device_add+0x3fe/0x670
[    2.782554]  [<ffffffff814f15c6>] ? usb_new_device+0x216/0x5d0
[    2.782556]  [<ffffffff814f164e>] usb_new_device+0x29e/0x5d0
[    2.782559]  [<ffffffff8148cff1>] ? pm_runtime_set_autosuspend_delay+0x51/0x60
[    2.782561]  [<ffffffff81506612>] ? __usb_detect_quirks+0x52/0x110
[    2.782563]  [<ffffffff814f2c3f>] hub_port_connect+0x31f/0x9c0
[    2.782565]  [<ffffffff814f39b1>] hub_event+0x6d1/0xb10
[    2.782568]  [<ffffffff8107f039>] process_one_work+0x159/0x470
[    2.782570]  [<ffffffff8107f398>] worker_thread+0x48/0x4a0
[    2.782572]  [<ffffffff8107f350>] ? process_one_work+0x470/0x470
[    2.782574]  [<ffffffff8107f350>] ? process_one_work+0x470/0x470
[    2.782576]  [<ffffffff81085089>] kthread+0xc9/0xe0
[    2.782579]  [<ffffffff81084fc0>] ? kthread_worker_fn+0x170/0x170
[    2.782581]  [<ffffffff816a55df>] ret_from_fork+0x3f/0x70
[    2.782583]  [<ffffffff81084fc0>] ? kthread_worker_fn+0x170/0x170
[    2.782585] ---[ end trace ce1cb4ca0f2cbd0b ]---
[    2.782586] ------------[ cut here ]------------

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Dmitry Torokhov]

On Tue, Jul 21, 2015 at 01:14:39PM +0200, Vlastimil Babka wrote:
> On 06/09/2015 07:42 PM, Dmitry Torokhov wrote:
> > From: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > 
> > This change creates a new input handler called "leds" that exports LEDs on input
> > devices as standard LED class devices in sysfs and allows controlling their
> > ptate via sysfs or via any of the standard LED triggers. This allows to
> > re-purpose and reassign LDEs on the keyboards to represent states other
> > than the standard keyboard states (CapsLock, NumLock, etc).
> > 
> > The old API of controlling input LEDs by writing into /dev/input/eventX
> > devices is still present and will take precedence over acessing via LEDs
> > subsystem (i.e. it may override state set by a trigger). If input device is
> > "grabbed" then requests coming through LED subsystem will be ignored.
> > 
> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> 
> > +	led_no = 0;
> > +	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
> > +		struct input_led *led = &leds->leds[led_no];
> > +
> > +		led->handle = &leds->handle;
> > +		led->code = led_code;
> > +
> > +		if (WARN_ON(!input_led_info[led_code].name))
> > +			continue;
> > +
> 
> Hi,
> I get several warnings on booting 4.2-rc2 here. Should I be worried?
> 
> [    2.782432] ------------[ cut here ]------------
> [    2.782440] WARNING: CPU: 2 PID: 356 at ../drivers/input/input-leds.c:115 input_leds_connect+0x22b/0x260()
> [    2.782441] Modules linked in: btrfs xor raid6_pq crc32c_intel radeon i2c_algo_bit sr_mod cdrom drm_kms_helper ttm e1000e drm xhci_pci ptp pps_core xhci_hcd sg
> [    2.782453] CPU: 2 PID: 356 Comm: kworker/2:2 Not tainted 4.2.0-rc2-1.g288d56b-desktop #1
> [    2.782454] Hardware name: Dell Inc. OptiPlex 9010/0M9KCM, BIOS A13 03/27/2013
> [    2.782458] Workqueue: usb_hub_wq hub_event
> [    2.782459]  ffffffff81a917b7 ffff880213ecf298 ffffffff8169f19d 0000000000000007
> [    2.782462]  0000000000000000 ffff880213ecf2d8 ffffffff810674f6 ffff880213ecf2f8
> [    2.782463]  ffff8802132fb000 0000000000000003 000000000000000b ffff8800369ff000
> [    2.782465] Call Trace:
> [    2.782470]  [<ffffffff8169f19d>] dump_stack+0x4c/0x6e
> [    2.782474]  [<ffffffff810674f6>] warn_slowpath_common+0x86/0xc0
> [    2.782476]  [<ffffffff810675ea>] warn_slowpath_null+0x1a/0x20
> [    2.782478]  [<ffffffff8152ccdb>] input_leds_connect+0x22b/0x260
> [    2.782480]  [<ffffffff815281b2>] input_attach_handler+0x1a2/0x1f0
> [    2.782483]  [<ffffffff81528640>] input_register_device+0x440/0x4f0
> [    2.782486]  [<ffffffff8156e494>] hidinput_connect+0x334/0x5d0
> [    2.782488]  [<ffffffff815683d4>] hid_connect+0x324/0x400

No, this is benign. I guess your keyboard has more LEDs than input
system has defined or several usages refer to the same LED.

Can you try debug patch below and post the messages?

-- 
Dmitry

---
 drivers/hid/hid-input.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 14aebe4..8690a84 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -657,6 +657,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 
 		default: goto ignore;
 		}
+		hid_info(device, "Mapped LED usage %02x as LED %d\n", usage->hid & 0xffff, usage->code);
 		break;
 
 	case HID_UP_DIGITIZER:
@@ -971,6 +972,8 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		if (field->report_size == 1) {
 			if (field->report->type == HID_OUTPUT_REPORT) {
 				map_led(LED_MISC);
+				hid_info(device, "Mapped output report usage %08x as MISC LED %d\n",
+					 usage->hid, usage->code);
 				break;
 			}
 			map_key(BTN_MISC);

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Pavel Machek]

On Tue 2015-07-21 10:01:33, Dmitry Torokhov wrote:
> On Tue, Jul 21, 2015 at 01:14:39PM +0200, Vlastimil Babka wrote:
> > On 06/09/2015 07:42 PM, Dmitry Torokhov wrote:
> > > From: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > > 
> > > This change creates a new input handler called "leds" that exports LEDs on input
> > > devices as standard LED class devices in sysfs and allows controlling their
> > > ptate via sysfs or via any of the standard LED triggers. This allows to
> > > re-purpose and reassign LDEs on the keyboards to represent states other
> > > than the standard keyboard states (CapsLock, NumLock, etc).
> > > 
> > > The old API of controlling input LEDs by writing into /dev/input/eventX
> > > devices is still present and will take precedence over acessing via LEDs
> > > subsystem (i.e. it may override state set by a trigger). If input device is
> > > "grabbed" then requests coming through LED subsystem will be ignored.
> > > 
> > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > 
> > > +	led_no = 0;
> > > +	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
> > > +		struct input_led *led = &leds->leds[led_no];
> > > +
> > > +		led->handle = &leds->handle;
> > > +		led->code = led_code;
> > > +
> > > +		if (WARN_ON(!input_led_info[led_code].name))
> > > +			continue;
> > > +
> > 
> > Hi,
> > I get several warnings on booting 4.2-rc2 here. Should I be worried?
> > 
> > [    2.782432] ------------[ cut here ]------------
> > [    2.782440] WARNING: CPU: 2 PID: 356 at ../drivers/input/input-leds.c:115 input_leds_connect+0x22b/0x260()
> > [    2.782441] Modules linked in: btrfs xor raid6_pq crc32c_intel radeon i2c_algo_bit sr_mod cdrom drm_kms_helper ttm e1000e drm xhci_pci ptp pps_core xhci_hcd sg
> > [    2.782453] CPU: 2 PID: 356 Comm: kworker/2:2 Not tainted 4.2.0-rc2-1.g288d56b-desktop #1
> > [    2.782454] Hardware name: Dell Inc. OptiPlex 9010/0M9KCM, BIOS A13 03/27/2013
> > [    2.782458] Workqueue: usb_hub_wq hub_event
> > [    2.782459]  ffffffff81a917b7 ffff880213ecf298 ffffffff8169f19d 0000000000000007
> > [    2.782462]  0000000000000000 ffff880213ecf2d8 ffffffff810674f6 ffff880213ecf2f8
> > [    2.782463]  ffff8802132fb000 0000000000000003 000000000000000b ffff8800369ff000
> > [    2.782465] Call Trace:
> > [    2.782470]  [<ffffffff8169f19d>] dump_stack+0x4c/0x6e
> > [    2.782474]  [<ffffffff810674f6>] warn_slowpath_common+0x86/0xc0
> > [    2.782476]  [<ffffffff810675ea>] warn_slowpath_null+0x1a/0x20
> > [    2.782478]  [<ffffffff8152ccdb>] input_leds_connect+0x22b/0x260
> > [    2.782480]  [<ffffffff815281b2>] input_attach_handler+0x1a2/0x1f0
> > [    2.782483]  [<ffffffff81528640>] input_register_device+0x440/0x4f0
> > [    2.782486]  [<ffffffff8156e494>] hidinput_connect+0x334/0x5d0
> > [    2.782488]  [<ffffffff815683d4>] hid_connect+0x324/0x400
> 
> No, this is benign. I guess your keyboard has more LEDs than input
> system has defined or several usages refer to the same LED.

Can we get rid of WARN_ON, then? It is nasty to flood logs, and people
are likely to flood our inboxes, soon. printk(KERN_ERR) should be
adequate...

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Vlastimil Babka]

On 07/21/2015 11:08 PM, Pavel Machek wrote:
> On Tue 2015-07-21 10:01:33, Dmitry Torokhov wrote:
>> On Tue, Jul 21, 2015 at 01:14:39PM +0200, Vlastimil Babka wrote:
>>> On 06/09/2015 07:42 PM, Dmitry Torokhov wrote:
>>>> From: Samuel Thibault <samuel.thibault@ens-lyon.org>
>>>>
>>>> This change creates a new input handler called "leds" that exports LEDs on input
>>>> devices as standard LED class devices in sysfs and allows controlling their
>>>> ptate via sysfs or via any of the standard LED triggers. This allows to
>>>> re-purpose and reassign LDEs on the keyboards to represent states other
>>>> than the standard keyboard states (CapsLock, NumLock, etc).
>>>>
>>>> The old API of controlling input LEDs by writing into /dev/input/eventX
>>>> devices is still present and will take precedence over acessing via LEDs
>>>> subsystem (i.e. it may override state set by a trigger). If input device is
>>>> "grabbed" then requests coming through LED subsystem will be ignored.
>>>>
>>>> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>> ---
>>>
>>>> +	led_no = 0;
>>>> +	for_each_set_bit(led_code, dev->ledbit, LED_CNT) {
>>>> +		struct input_led *led = &leds->leds[led_no];
>>>> +
>>>> +		led->handle = &leds->handle;
>>>> +		led->code = led_code;
>>>> +
>>>> +		if (WARN_ON(!input_led_info[led_code].name))
>>>> +			continue;
>>>> +
>>>
>>> Hi,
>>> I get several warnings on booting 4.2-rc2 here. Should I be worried?
>>>
>>> [    2.782432] ------------[ cut here ]------------
>>> [    2.782440] WARNING: CPU: 2 PID: 356 at ../drivers/input/input-leds.c:115 input_leds_connect+0x22b/0x260()
>>> [    2.782441] Modules linked in: btrfs xor raid6_pq crc32c_intel radeon i2c_algo_bit sr_mod cdrom drm_kms_helper ttm e1000e drm xhci_pci ptp pps_core xhci_hcd sg
>>> [    2.782453] CPU: 2 PID: 356 Comm: kworker/2:2 Not tainted 4.2.0-rc2-1.g288d56b-desktop #1
>>> [    2.782454] Hardware name: Dell Inc. OptiPlex 9010/0M9KCM, BIOS A13 03/27/2013
>>> [    2.782458] Workqueue: usb_hub_wq hub_event
>>> [    2.782459]  ffffffff81a917b7 ffff880213ecf298 ffffffff8169f19d 0000000000000007
>>> [    2.782462]  0000000000000000 ffff880213ecf2d8 ffffffff810674f6 ffff880213ecf2f8
>>> [    2.782463]  ffff8802132fb000 0000000000000003 000000000000000b ffff8800369ff000
>>> [    2.782465] Call Trace:
>>> [    2.782470]  [<ffffffff8169f19d>] dump_stack+0x4c/0x6e
>>> [    2.782474]  [<ffffffff810674f6>] warn_slowpath_common+0x86/0xc0
>>> [    2.782476]  [<ffffffff810675ea>] warn_slowpath_null+0x1a/0x20
>>> [    2.782478]  [<ffffffff8152ccdb>] input_leds_connect+0x22b/0x260
>>> [    2.782480]  [<ffffffff815281b2>] input_attach_handler+0x1a2/0x1f0
>>> [    2.782483]  [<ffffffff81528640>] input_register_device+0x440/0x4f0
>>> [    2.782486]  [<ffffffff8156e494>] hidinput_connect+0x334/0x5d0
>>> [    2.782488]  [<ffffffff815683d4>] hid_connect+0x324/0x400
>>
>> No, this is benign. I guess your keyboard has more LEDs than input
>> system has defined or several usages refer to the same LED.

It's a mouse actually:

[   69.413682] usb 3-4: new low-speed USB device number 3 using xhci_hcd
[   69.587651] usb 3-4: New USB device found, idVendor=046d, idProduct=c50e
[   69.587656] usb 3-4: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   69.587659] usb 3-4: Product: USB Receiver
[   69.587661] usb 3-4: Manufacturer: Logitech
[   69.587865] usb 3-4: ep 0x81 - rounding interval to 64 microframes, ep desc says 80 microframes
[   69.596494] input: Logitech USB Receiver as /devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4:1.0/0003:046D:C50E.0003/input/input14

followed by 5 warnings as the one I posted (all look the same at first
glance) and then:

[   69.648581] hid-generic 0003:046D:C50E.0003: input,hidraw2: USB HID v1.11 Mouse [Logitech USB Receiver] on usb-0000:00:14.0-4/input0

The mouse has 3 green leds and one red to indicate battery status, but I think
they operate autonomously.

> Can we get rid of WARN_ON, then? It is nasty to flood logs, and people
> are likely to flood our inboxes, soon. printk(KERN_ERR) should be
> adequate...

I agree warning like this is inappropriate. But unfortunately things get worse
when I try to unplug the receiver...

Jul 22 14:46:11 gusiac kernel: BUG: unable to handle kernel NULL pointer dereference at           (null)
Jul 22 14:46:11 gusiac kernel: IP: [<ffffffff8147d807>] device_del+0x17/0x260
Jul 22 14:46:11 gusiac kernel: PGD 0 
Jul 22 14:46:12 gusiac kernel: Oops: 0000 [#1] PREEMPT SMP 
Jul 22 14:46:12 gusiac kernel: Modules linked in: bnep bluetooth rfkill fuse nfsv3 nfs_acl rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace sunrpc fscache iscsi_ibft iscsi_boo
Jul 22 14:46:12 gusiac kernel:  drm xhci_hcd sg
Jul 22 14:46:12 gusiac kernel: CPU: 6 PID: 194 Comm: kworker/6:1 Tainted: G        W       4.2.0-rc2-2.g7010139-desktop #1
Jul 22 14:46:12 gusiac kernel: Hardware name: Dell Inc. OptiPlex 9010/0M9KCM, BIOS A13 03/27/2013
Jul 22 14:46:12 gusiac kernel: Workqueue: usb_hub_wq hub_event
Jul 22 14:46:12 gusiac kernel: task: ffff880213d28100 ti: ffff880213c8c000 task.ti: ffff880213c8c000
Jul 22 14:46:12 gusiac kernel: RIP: 0010:[<ffffffff8147d807>]  [<ffffffff8147d807>] device_del+0x17/0x260
Jul 22 14:46:12 gusiac kernel: RSP: 0018:ffff880213c8f828  EFLAGS: 00010286
Jul 22 14:46:12 gusiac kernel: RAX: 00000000ffffffea RBX: 0000000000000000 RCX: 0000000000000000
Jul 22 14:46:12 gusiac kernel: RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000000
Jul 22 14:46:12 gusiac kernel: RBP: ffff880213c8f868 R08: 0000000000000000 R09: ffffffff8135b2b0
Jul 22 14:46:12 gusiac kernel: R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
Jul 22 14:46:12 gusiac kernel: R13: ffff880213622000 R14: ffff880213622000 R15: ffff880213d618d0
Jul 22 14:46:12 gusiac kernel: FS:  0000000000000000(0000) GS:ffff88021dd80000(0000) knlGS:0000000000000000
Jul 22 14:46:12 gusiac kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Jul 22 14:46:12 gusiac kernel: CR2: 0000000000000000 CR3: 0000000001c0c000 CR4: 00000000001407e0
Jul 22 14:46:12 gusiac kernel: Stack:
Jul 22 14:46:12 gusiac kernel:  ffff88003697dc10 0000000000000292 ffff880213622540 ffff8802136225c8
Jul 22 14:46:12 gusiac kernel:  0000000000000000 ffff8802136225c8 ffff880213622000 ffff880213622000
Jul 22 14:46:12 gusiac kernel:  ffff880213c8f888 ffffffff8147da72 ffff880213c8f888 ffff8802136224d0
Jul 22 14:46:12 gusiac kernel: Call Trace:
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8147da72>] device_unregister+0x22/0x70
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81560c61>] led_classdev_unregister+0x61/0xb0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8152ca1a>] input_leds_disconnect+0x3a/0x70
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81529e2c>] __input_unregister_device+0xac/0x170
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81529faf>] input_unregister_device+0x4f/0x80
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8156b358>] hidinput_disconnect+0x98/0xd0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81568526>] hid_disconnect+0x76/0x80
Jul 22 14:46:12 gusiac kernel:  [<ffffffff815685ea>] hid_device_remove+0xba/0xd0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481b11>] __device_release_driver+0xa1/0x150
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481be3>] device_release_driver+0x23/0x30
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481285>] bus_remove_device+0x105/0x180
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8147d929>] device_del+0x139/0x260
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81568687>] hid_destroy_device+0x27/0x60
Jul 22 14:46:12 gusiac kernel:  [<ffffffff815766cd>] usbhid_disconnect+0x4d/0x80
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814fd8c3>] usb_unbind_interface+0x83/0x270
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8148c57b>] ? rpm_idle+0x5b/0x260
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481b11>] __device_release_driver+0xa1/0x150
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481be3>] device_release_driver+0x23/0x30
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481285>] bus_remove_device+0x105/0x180
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8147d929>] device_del+0x139/0x260
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814fb169>] usb_disable_device+0x89/0x270
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814f0d02>] usb_disconnect+0x92/0x2b0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814f2993>] hub_port_connect+0x73/0x9c0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814f39b1>] hub_event+0x6d1/0xb10
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8109dd5b>] ? dequeue_task_fair+0x36b/0x700
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8109d121>] ? put_prev_entity+0x31/0x420
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107f039>] process_one_work+0x159/0x470
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107f398>] worker_thread+0x48/0x4a0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107f350>] ? process_one_work+0x470/0x470
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107f350>] ? process_one_work+0x470/0x470
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81085089>] kthread+0xc9/0xe0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81084fc0>] ? kthread_worker_fn+0x170/0x170
Jul 22 14:46:12 gusiac kernel:  [<ffffffff816a55df>] ret_from_fork+0x3f/0x70
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81084fc0>] ? kthread_worker_fn+0x170/0x170
Jul 22 14:46:12 gusiac kernel: Code: 48 89 d7 48 89 e5 e8 89 ff ff ff 5d c3 0f 1f 80 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 41 56 41 55 41 54 53 49 89 fc 48 83 ec 20 <4c> 8b 2f 65 48 8b 04 
Jul 22 14:46:12 gusiac kernel: RIP  [<ffffffff8147d807>] device_del+0x17/0x260
Jul 22 14:46:12 gusiac kernel:  RSP <ffff880213c8f828>
Jul 22 14:46:12 gusiac kernel: CR2: 0000000000000000
Jul 22 14:46:12 gusiac kernel: ---[ end trace 0f333fa3e11a3225 ]---
Jul 22 14:46:12 gusiac kernel: BUG: unable to handle kernel paging request at ffffffffffffffd8
Jul 22 14:46:12 gusiac kernel: IP: [<ffffffff810854f0>] kthread_data+0x10/0x20
Jul 22 14:46:12 gusiac kernel: PGD 1c0d067 PUD 1c0f067 PMD 0 
Jul 22 14:46:12 gusiac kernel: Oops: 0000 [#2] PREEMPT SMP 
Jul 22 14:46:12 gusiac kernel: Modules linked in: bnep bluetooth rfkill fuse nfsv3 nfs_acl rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace sunrpc fscache iscsi_ibft iscsi_boo
Jul 22 14:46:12 gusiac kernel:  drm xhci_hcd sg
Jul 22 14:46:12 gusiac kernel: CPU: 6 PID: 194 Comm: kworker/6:1 Tainted: G      D W       4.2.0-rc2-2.g7010139-desktop #1
Jul 22 14:46:12 gusiac kernel: Hardware name: Dell Inc. OptiPlex 9010/0M9KCM, BIOS A13 03/27/2013
Jul 22 14:46:12 gusiac kernel: task: ffff880213d28100 ti: ffff880213c8c000 task.ti: ffff880213c8c000
Jul 22 14:46:12 gusiac kernel: RIP: 0010:[<ffffffff810854f0>]  [<ffffffff810854f0>] kthread_data+0x10/0x20
Jul 22 14:46:12 gusiac kernel: RSP: 0018:ffff880213c8f4c8  EFLAGS: 00010096
Jul 22 14:46:12 gusiac kernel: RAX: 0000000000000000 RBX: 0000000000000006 RCX: 0000000000000005
Jul 22 14:46:12 gusiac kernel: RDX: 0000000000000005 RSI: 0000000000000006 RDI: ffff880213d28100
Jul 22 14:46:12 gusiac kernel: RBP: ffff880213c8f4c8 R08: 00000000ffffffff R09: 0000000000000000
Jul 22 14:46:12 gusiac kernel: R10: 0000000073f4002f R11: 000000000000002f R12: 0000000000015600
Jul 22 14:46:12 gusiac kernel: R13: ffff88021dd95600 R14: ffff880213d28100 R15: 0000000000000006
Jul 22 14:46:12 gusiac kernel: FS:  0000000000000000(0000) GS:ffff88021dd80000(0000) knlGS:0000000000000000
Jul 22 14:46:12 gusiac kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Jul 22 14:46:12 gusiac kernel: CR2: 0000000000000028 CR3: 0000000001c0c000 CR4: 00000000001407e0
Jul 22 14:46:12 gusiac kernel: Stack:
Jul 22 14:46:12 gusiac kernel:  ffff880213c8f4e8 ffffffff8107fd55 ffff880213c8f4e8 ffff88021dd95600
Jul 22 14:46:12 gusiac kernel:  ffff880213c8f538 ffffffff816a0bc7 ffff880200000000 ffff880213d28100
Jul 22 14:46:12 gusiac kernel:  ffff880036d77cc0 ffff880213c90000 ffff880213d28d70 ffff880213c8f0c0
Jul 22 14:46:12 gusiac kernel: Call Trace:
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107fd55>] wq_worker_sleeping+0x15/0xa0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff816a0bc7>] __schedule+0x667/0x970
Jul 22 14:46:12 gusiac kernel:  [<ffffffff816a0f0e>] schedule+0x3e/0x90
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8106a096>] do_exit+0x806/0xb00
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8100753e>] oops_end+0x9e/0xd0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff810532eb>] no_context+0x10b/0x360
Jul 22 14:46:12 gusiac kernel:  [<ffffffff810535c0>] __bad_area_nosemaphore+0x80/0x1f0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81053743>] bad_area_nosemaphore+0x13/0x20
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81053a29>] __do_page_fault+0xb9/0x410
Jul 22 14:46:12 gusiac kernel:  [<ffffffff815d8186>] ? netlink_broadcast_filtered+0x136/0x3b0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81053da2>] do_page_fault+0x22/0x30
Jul 22 14:46:12 gusiac kernel:  [<ffffffff816a6f88>] page_fault+0x28/0x30
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8135b2b0>] ? cleanup_uevent_env+0x10/0x10
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8147d807>] ? device_del+0x17/0x260
Jul 22 14:46:12 gusiac kernel:  [<ffffffff810d0f3f>] ? try_to_del_timer_sync+0x4f/0x70
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8147da72>] device_unregister+0x22/0x70
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81560c61>] led_classdev_unregister+0x61/0xb0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8152ca1a>] input_leds_disconnect+0x3a/0x70
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81529e2c>] __input_unregister_device+0xac/0x170
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81529faf>] input_unregister_device+0x4f/0x80
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8156b358>] hidinput_disconnect+0x98/0xd0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81568526>] hid_disconnect+0x76/0x80
Jul 22 14:46:12 gusiac kernel:  [<ffffffff815685ea>] hid_device_remove+0xba/0xd0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481b11>] __device_release_driver+0xa1/0x150
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481be3>] device_release_driver+0x23/0x30
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481285>] bus_remove_device+0x105/0x180
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8147d929>] device_del+0x139/0x260
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81568687>] hid_destroy_device+0x27/0x60
Jul 22 14:46:12 gusiac kernel:  [<ffffffff815766cd>] usbhid_disconnect+0x4d/0x80
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814fd8c3>] usb_unbind_interface+0x83/0x270
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8148c57b>] ? rpm_idle+0x5b/0x260
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481b11>] __device_release_driver+0xa1/0x150
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481be3>] device_release_driver+0x23/0x30
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81481285>] bus_remove_device+0x105/0x180
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8147d929>] device_del+0x139/0x260
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814fb169>] usb_disable_device+0x89/0x270
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814f0d02>] usb_disconnect+0x92/0x2b0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814f2993>] hub_port_connect+0x73/0x9c0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff814f39b1>] hub_event+0x6d1/0xb10
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8109dd5b>] ? dequeue_task_fair+0x36b/0x700
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8109d121>] ? put_prev_entity+0x31/0x420
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107f039>] process_one_work+0x159/0x470
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107f398>] worker_thread+0x48/0x4a0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107f350>] ? process_one_work+0x470/0x470
Jul 22 14:46:12 gusiac kernel:  [<ffffffff8107f350>] ? process_one_work+0x470/0x470
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81085089>] kthread+0xc9/0xe0
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81084fc0>] ? kthread_worker_fn+0x170/0x170
Jul 22 14:46:12 gusiac kernel:  [<ffffffff816a55df>] ret_from_fork+0x3f/0x70
Jul 22 14:46:12 gusiac kernel:  [<ffffffff81084fc0>] ? kthread_worker_fn+0x170/0x170
Jul 22 14:46:12 gusiac kernel: Code: 00 48 89 e5 5d 48 8b 40 c8 48 c1 e8 02 83 e0 01 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 8b 87 60 05 00 00 55 48 89 e5 <48> 8b 40 d8 5d c3 66 
Jul 22 14:46:12 gusiac kernel: RIP  [<ffffffff810854f0>] kthread_data+0x10/0x20
Jul 22 14:46:12 gusiac kernel:  RSP <ffff880213c8f4c8>
Jul 22 14:46:12 gusiac kernel: CR2: ffffffffffffffd8
Jul 22 14:46:12 gusiac kernel: ---[ end trace 0f333fa3e11a3226 ]---
Jul 22 14:46:12 gusiac kernel: Fixing recursive fault but reboot is needed!

I think some more followed including rcu stall etc, but were not
logged permanently. I suspect the warnings and the bugs are
related :)

I'll now try collect the debug prints you suggested.


> Thanks,
> 									Pavel
> 

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Vlastimil Babka]

On 07/21/2015 07:01 PM, Dmitry Torokhov wrote:
> No, this is benign. I guess your keyboard has more LEDs than input
> system has defined or several usages refer to the same LED.
> 
> Can you try debug patch below and post the messages?
> 

[  101.805120] usb 3-4: new low-speed USB device number 3 using xhci_hcd
[  101.979584] usb 3-4: New USB device found, idVendor=046d, idProduct=c50e
[  101.979589] usb 3-4: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[  101.979592] usb 3-4: Product: USB Receiver
[  101.979594] usb 3-4: Manufacturer: Logitech
[  101.979805] usb 3-4: ep 0x81 - rounding interval to 64 microframes, ep desc says 80 microframes
[  101.989010] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
[  101.989014] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
[  101.989017] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
[  101.989019] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
[  101.989021] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
[  101.989023] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
[  101.989025] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
[  101.989027] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
[  101.989091] input: Logitech USB Receiver as /devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4:1.0/0003:046D:C50E.0003/input/input14
[  102.039320] ------------[ cut here ]------------
[  102.039329] WARNING: CPU: 6 PID: 168 at ../drivers/input/input-leds.c:115 input_leds_connect+0x22b/0x260()

(5 WARNINGs as before)

[  102.040729] hid-generic 0003:046D:C50E.0003: input,hidraw2: USB HID v1.11 Mouse [Logitech USB Receiver] on usb-0000:00:14.0-4/input0

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Jiri Kosina]

On Wed, 22 Jul 2015, Vlastimil Babka wrote:

[ ... snip ... ]
> The mouse has 3 green leds and one red to indicate battery status, but I think
> they operate autonomously.

It's possible that the mouse is presenting them in the report descriptor 
though (and maybe it's even possible to control them from the host).

Could you please provide contents of

	/sys/kernel/debug/hid/<device>/rdesc

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Jiri Kosina]

On Wed, 22 Jul 2015, Vlastimil Babka wrote:

> [  101.805120] usb 3-4: new low-speed USB device number 3 using xhci_hcd
> [  101.979584] usb 3-4: New USB device found, idVendor=046d, idProduct=c50e
> [  101.979589] usb 3-4: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [  101.979592] usb 3-4: Product: USB Receiver
> [  101.979594] usb 3-4: Manufacturer: Logitech
> [  101.979805] usb 3-4: ep 0x81 - rounding interval to 64 microframes, ep desc says 80 microframes
> [  101.989010] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> [  101.989014] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> [  101.989017] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> [  101.989019] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> [  101.989021] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> [  101.989023] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> [  101.989025] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> [  101.989027] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> [  101.989091] input: Logitech USB Receiver as /devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4:1.0/0003:046D:C50E.0003/input/input14
> [  102.039320] ------------[ cut here ]------------
> [  102.039329] WARNING: CPU: 6 PID: 168 at ../drivers/input/input-leds.c:115 input_leds_connect+0x22b/0x260()
> 
> (5 WARNINGs as before)
> 
> [  102.040729] hid-generic 0003:046D:C50E.0003: input,hidraw2: USB HID v1.11 Mouse [Logitech USB Receiver] on usb-0000:00:14.0-4/input0

Alright, I think it's pretty obvious what's happening. Vlastimil, am I 
right that the patch below fixes the issue?

I am however not sure whether input_leds_connect() is not too unkind to 
unnamed LEDs and shouldn't be more tolerant to those in the long term.





From: Jiri Kosina <jkosina@suse.com>
Subject: [PATCH] Input: leds: don't attempt to deregister unnamed LEDs

input_leds_connect() is skipping registration of LEDs for which
there is no symbolic name in input_led_info[].

The way how usages are mapped in hidinput_configure_usage() directly
implies that this will trigger also when there are duplicate mappings
of LEDs (as only the first one will be mapped accordingly to the descriptor,
and the remaining ones will be mapped to first free bits in the ledbit
bitfield).

We are however not skipping such mappings when deregistering, which
causes memory corruption.

Reported-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
---
 drivers/input/input-leds.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
index 074a65e..3be81ac 100644
--- a/drivers/input/input-leds.c
+++ b/drivers/input/input-leds.c
@@ -145,6 +145,10 @@ err_unregister_leds:
 	while (--led_no >= 0) {
 		struct input_led *led = &leds->leds[led_no];
 
+		/* Unnamed LEDs are not registered with led class */
+		if (!input_led_info[led->code].name)
+			continue;
+
 		led_classdev_unregister(&led->cdev);
 		kfree(led->cdev.name);
 	}

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Pavel Machek]

On Wed 2015-07-22 21:49:06, Jiri Kosina wrote:
> On Wed, 22 Jul 2015, Vlastimil Babka wrote:
> 
> > [  101.805120] usb 3-4: new low-speed USB device number 3 using xhci_hcd
> > [  101.979584] usb 3-4: New USB device found, idVendor=046d, idProduct=c50e
> > [  101.979589] usb 3-4: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> > [  101.979592] usb 3-4: Product: USB Receiver
> > [  101.979594] usb 3-4: Manufacturer: Logitech
> > [  101.979805] usb 3-4: ep 0x81 - rounding interval to 64 microframes, ep desc says 80 microframes
> > [  101.989010] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989014] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989017] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989019] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989021] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989023] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989025] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989027] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989091] input: Logitech USB Receiver as /devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4:1.0/0003:046D:C50E.0003/input/input14
> > [  102.039320] ------------[ cut here ]------------
> > [  102.039329] WARNING: CPU: 6 PID: 168 at ../drivers/input/input-leds.c:115 input_leds_connect+0x22b/0x260()
> > 
> > (5 WARNINGs as before)
> > 
> > [  102.040729] hid-generic 0003:046D:C50E.0003: input,hidraw2: USB HID v1.11 Mouse [Logitech USB Receiver] on usb-0000:00:14.0-4/input0
> 
> Alright, I think it's pretty obvious what's happening. Vlastimil, am I 
> right that the patch below fixes the issue?
> 
> I am however not sure whether input_leds_connect() is not too unkind to 
> unnamed LEDs and shouldn't be more tolerant to those in the long term.


Maybe.

> From: Jiri Kosina <jkosina@suse.com>
> Subject: [PATCH] Input: leds: don't attempt to deregister unnamed LEDs
> 
> input_leds_connect() is skipping registration of LEDs for which
> there is no symbolic name in input_led_info[].

Yes, and rather than testing for "no name" special case at two places,
what about simply giving up when we see such error?

S-o-b: me.

diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
index 074a65e..5f300e6 100644
--- a/drivers/input/input-leds.c
+++ b/drivers/input/input-leds.c
@@ -112,7 +112,11 @@ static int input_leds_connect(struct input_handler *handler,
 		led->handle = &leds->handle;
 		led->code = led_code;
 
-		if (WARN_ON(!input_led_info[led_code].name))
+		if (!input_led_info[led_code].name) {
+			printk(KERN_ERR, "LED with no name\n");
+			error = -EINVAL;
+			goto err_unregister_leds;
+		}
 			continue;
 
 		led->cdev.name = kasprintf(GFP_KERNEL, "%s::%s",


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Dmitry Torokhov]

On Wed, Jul 22, 2015 at 09:49:06PM +0200, Jiri Kosina wrote:
> On Wed, 22 Jul 2015, Vlastimil Babka wrote:
> 
> > [  101.805120] usb 3-4: new low-speed USB device number 3 using xhci_hcd
> > [  101.979584] usb 3-4: New USB device found, idVendor=046d, idProduct=c50e
> > [  101.979589] usb 3-4: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> > [  101.979592] usb 3-4: Product: USB Receiver
> > [  101.979594] usb 3-4: Manufacturer: Logitech
> > [  101.979805] usb 3-4: ep 0x81 - rounding interval to 64 microframes, ep desc says 80 microframes
> > [  101.989010] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989014] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989017] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989019] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989021] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989023] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989025] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989027] hid-generic 0003:046D:C50E.0003: Mapped LED usage 4b as LED 8
> > [  101.989091] input: Logitech USB Receiver as /devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4:1.0/0003:046D:C50E.0003/input/input14
> > [  102.039320] ------------[ cut here ]------------
> > [  102.039329] WARNING: CPU: 6 PID: 168 at ../drivers/input/input-leds.c:115 input_leds_connect+0x22b/0x260()
> > 
> > (5 WARNINGs as before)
> > 
> > [  102.040729] hid-generic 0003:046D:C50E.0003: input,hidraw2: USB HID v1.11 Mouse [Logitech USB Receiver] on usb-0000:00:14.0-4/input0
> 
> Alright, I think it's pretty obvious what's happening. Vlastimil, am I 
> right that the patch below fixes the issue?
> 
> I am however not sure whether input_leds_connect() is not too unkind to 
> unnamed LEDs and shouldn't be more tolerant to those in the long term.

Yes, I'll change how we count the leds to skip unnamed, then the patch
below won't be needed.

BTW, maybe we should be using hid_map_usage_clear() in map_led() as it
does not make much sense to convert CAPS LOCK into SCROLL LOCK in case
keyboard happens to have 2 usages for caps...

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Jiri Kosina]

On Wed, 22 Jul 2015, Pavel Machek wrote:

> > I am however not sure whether input_leds_connect() is not too unkind to 
> > unnamed LEDs and shouldn't be more tolerant to those in the long term.
> 
> Maybe.
> 
> > From: Jiri Kosina <jkosina@suse.com>
> > Subject: [PATCH] Input: leds: don't attempt to deregister unnamed LEDs
> > 
> > input_leds_connect() is skipping registration of LEDs for which
> > there is no symbolic name in input_led_info[].
> 
> Yes, and rather than testing for "no name" special case at two places,
> what about simply giving up when we see such error?

Well, that's quite the oposite of the "be more tolerant" aproach proposed 
above :)

I don't see a reason why the sole fact that the device has at least one 
LED that kernel can't handle properly should mean that neither of the 
other LEDs will work.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Jiri Kosina]

On Wed, 22 Jul 2015, Dmitry Torokhov wrote:

> Yes, I'll change how we count the leds to skip unnamed, then the patch
> below won't be needed.

Agreed.

> BTW, maybe we should be using hid_map_usage_clear() in map_led() as it 
> does not make much sense to convert CAPS LOCK into SCROLL LOCK in case 
> keyboard happens to have 2 usages for caps...

Indeed, makes sense generally for all LEDs I think.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Vlastimil Babka]

On 07/22/2015 08:55 PM, Jiri Kosina wrote:
> On Wed, 22 Jul 2015, Vlastimil Babka wrote:
> 
> [ ... snip ... ]
>> The mouse has 3 green leds and one red to indicate battery status, but I think
>> they operate autonomously.
> 
> It's possible that the mouse is presenting them in the report descriptor 
> though (and maybe it's even possible to control them from the host).
> 
> Could you please provide contents of
> 
> 	/sys/kernel/debug/hid/<device>/rdesc

For the record, below. I wonder why there's two more "LED.?" lines (7) than the
warnings I get (5)?

gusiac:~ # cat /sys/kernel/debug/hid/0003\:046D\:C50E.0003/rdesc
05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 08 15 00 25 01 95 08 75 01 81 02 05
01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 05 0c 0a 38 02 95 01 81 06
09 3c 15 00 25 01 75 01 95 01 b1 22 95 07 b1 01 05 08 09 4b 15 00 25 01 95 08 75
01 81 02 05 09 19 09 29 10 81 02 c0

  INPUT[INPUT]
    Field(0)
      Physical(GenericDesktop.Pointer)
      Application(GenericDesktop.Mouse)
      Usage(8)
        Button.0001
        Button.0002
        Button.0003
        Button.0004
        Button.0005
        Button.0006
        Button.0007
        Button.0008
      Logical Minimum(0)
      Logical Maximum(1)
      Report Size(1)
      Report Count(8)
      Report Offset(0)
      Flags( Variable Absolute )
    Field(1)
      Physical(GenericDesktop.Pointer)
      Application(GenericDesktop.Mouse)
      Usage(3)
        GenericDesktop.X
        GenericDesktop.Y
        GenericDesktop.Wheel
      Logical Minimum(-127)
      Logical Maximum(127)
      Report Size(8)
      Report Count(3)
      Report Offset(8)
      Flags( Variable Relative )
    Field(2)
      Application(GenericDesktop.Mouse)
      Usage(1)
        Consumer.HorizontalWheel
      Logical Minimum(-127)
      Logical Maximum(127)
      Report Size(8)
      Report Count(1)
      Report Offset(32)
      Flags( Variable Relative )
    Field(3)
      Application(GenericDesktop.Mouse)
      Usage(8)
        LED.GenericIndicator
        LED.GenericIndicator
        LED.GenericIndicator
        LED.GenericIndicator
        LED.GenericIndicator
        LED.GenericIndicator
        LED.GenericIndicator
        LED.GenericIndicator
      Logical Minimum(0)
      Logical Maximum(1)
      Report Size(1)
      Report Count(8)
      Report Offset(40)
      Flags( Variable Absolute )
    Field(4)
      Application(GenericDesktop.Mouse)
      Usage(8)
        Button.0009
        Button.000a
        Button.000b
        Button.000c
        Button.000d
        Button.000e
        Button.000f
        Button.0010
      Logical Minimum(0)
      Logical Maximum(1)
      Report Size(1)
      Report Count(8)
      Report Offset(48)
      Flags( Variable Absolute )
  FEATURE[FEATURE]
    Field(0)
      Application(GenericDesktop.Mouse)
      Usage(1)
        Consumer.003c
      Logical Minimum(0)
      Logical Maximum(1)
      Report Size(1)
      Report Count(1)
      Report Offset(0)
      Flags( Variable Absolute NoPreferredState )

Button.0001 ---> Key.LeftBtn
Button.0002 ---> Key.RightBtn
Button.0003 ---> Key.MiddleBtn
Button.0004 ---> Key.SideBtn
Button.0005 ---> Key.ExtraBtn
Button.0006 ---> Key.ForwardBtn
Button.0007 ---> Key.BackBtn
Button.0008 ---> Key.TaskBtn
GenericDesktop.X ---> Relative.X
GenericDesktop.Y ---> Relative.Y
GenericDesktop.Wheel ---> Relative.Wheel
Consumer.HorizontalWheel ---> Relative.HWheel
LED.GenericIndicator ---> LED.Misc
LED.GenericIndicator ---> LED.?
LED.GenericIndicator ---> LED.?
LED.GenericIndicator ---> LED.?
LED.GenericIndicator ---> LED.?
LED.GenericIndicator ---> LED.?
LED.GenericIndicator ---> LED.?
LED.GenericIndicator ---> LED.?
Button.0009 ---> Key.?
Button.000a ---> Key.?
Button.000b ---> Key.?
Button.000c ---> Key.?
Button.000d ---> Key.?
Button.000e ---> Key.?
Button.000f ---> Key.?
Button.0010 ---> Key.?

Re: [PATCH v2 1/3] Input: export LEDs as class devices in sysfs

[Jiri Kosina]

On Thu, 23 Jul 2015, Vlastimil Babka wrote:

> On 07/22/2015 08:55 PM, Jiri Kosina wrote:
> > On Wed, 22 Jul 2015, Vlastimil Babka wrote:
> > 
> > [ ... snip ... ]
> >> The mouse has 3 green leds and one red to indicate battery status, but I think
> >> they operate autonomously.
> > 
> > It's possible that the mouse is presenting them in the report descriptor 
> > though (and maybe it's even possible to control them from the host).
> > 
> > Could you please provide contents of
> > 
> > 	/sys/kernel/debug/hid/<device>/rdesc
> 
> For the record, below. I wonder why there's two more "LED.?" lines (7) than the
> warnings I get (5)?

Because some of them get successfully (re-)mapped to named LEDs.

-- 
Jiri Kosina
SUSE Labs