[PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Hans de Goede]

Hi All,

Here is v8 of the platform drivers changes matching / using the new
v8 LED subsys led_classdev_notify_brightness_hw_changed() functionality
queued in -next.

There have been some changes (and preparation patches added) compared
to the previous versions since the new LED api requires the driver to
only call led_classdev_notify_brightness_hw_changed() on hotkey triggered
changes and the ACPI events indicating brightness changes also get
triggered when setting the brightness to led_set_brightness (or sysfs).

This series depends on the patch adding
led_classdev_notify_brightness_hw_changed() to the LED subsystem,
Jacek can you create a stable branch with just that patch which the
platform/x86 platform maintainers can merge, so that they can apply
this series ?

Regards,

Hans

[PATCH v8 1/7] platform/x86/thinkpad_acpi: Stop setting led_classdev brightness directly

[Hans de Goede]

There is no need to set the led_classdev's brightness value from
its set_brightness callback, this is taken care of by the led-core and
thinkpad_acpi really should not be mucking with it.

Note that kbdlight_set_level_and_update() is still used by the old
thinpad_acpi specific sysfs interface for the led, so we cannot
remove it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v8:
-New patch in v8 of this patch-set
---
 drivers/platform/x86/thinkpad_acpi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index cacb43f..0680bb3 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5095,8 +5095,6 @@ static int kbdlight_set_level(int level)
 	return 0;
 }
 
-static int kbdlight_set_level_and_update(int level);
-
 static int kbdlight_get_level(void)
 {
 	int status = 0;
@@ -5164,7 +5162,7 @@ static void kbdlight_set_worker(struct work_struct *work)
 			container_of(work, struct tpacpi_led_classdev, work);
 
 	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
-		kbdlight_set_level_and_update(data->new_state);
+		kbdlight_set_level(data->new_state);
 }
 
 static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
-- 
2.9.3

[PATCH v8 2/7] platform/x86/thinkpad_acpi: Use brightness_set_blocking callback for LEDs

[Hans de Goede]

Now a days the LED core can take care of executing brightness_set from
a workqueue if it needs to sleep, make use of this and remove a bunch
of DIY code for this.

Since this commit removes the workqueue usage for LEDs, the
led_sysfs_blink_set callback may now also sleep, this is fine.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v8:
-New patch in v8 of this patch-set
---
 drivers/platform/x86/thinkpad_acpi.c | 80 ++++++++----------------------------
 1 file changed, 16 insertions(+), 64 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0680bb3..f51833f 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -372,11 +372,9 @@ enum led_status_t {
 	TPACPI_LED_BLINK,
 };
 
-/* Special LED class that can defer work */
+/* tpacpi LED class */
 struct tpacpi_led_classdev {
 	struct led_classdev led_classdev;
-	struct work_struct work;
-	enum led_status_t new_state;
 	int led;
 };
 
@@ -5156,24 +5154,10 @@ static bool kbdlight_is_supported(void)
 	return status & BIT(9);
 }
 
-static void kbdlight_set_worker(struct work_struct *work)
-{
-	struct tpacpi_led_classdev *data =
-			container_of(work, struct tpacpi_led_classdev, work);
-
-	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
-		kbdlight_set_level(data->new_state);
-}
-
-static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
+static int kbdlight_sysfs_set(struct led_classdev *led_cdev,
 			enum led_brightness brightness)
 {
-	struct tpacpi_led_classdev *data =
-			container_of(led_cdev,
-				     struct tpacpi_led_classdev,
-				     led_classdev);
-	data->new_state = brightness;
-	queue_work(tpacpi_wq, &data->work);
+	return kbdlight_set_level(brightness);
 }
 
 static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
@@ -5191,7 +5175,7 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
 	.led_classdev = {
 		.name		= "tpacpi::kbd_backlight",
 		.max_brightness	= 2,
-		.brightness_set	= &kbdlight_sysfs_set,
+		.brightness_set_blocking = &kbdlight_sysfs_set,
 		.brightness_get	= &kbdlight_sysfs_get,
 	}
 };
@@ -5203,7 +5187,6 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
 	vdbg_printk(TPACPI_DBG_INIT, "initializing kbdlight subdriver\n");
 
 	TPACPI_ACPIHANDLE_INIT(hkey);
-	INIT_WORK(&tpacpi_led_kbdlight.work, kbdlight_set_worker);
 
 	if (!kbdlight_is_supported()) {
 		tp_features.kbdlight = 0;
@@ -5227,7 +5210,6 @@ static void kbdlight_exit(void)
 {
 	if (tp_features.kbdlight)
 		led_classdev_unregister(&tpacpi_led_kbdlight.led_classdev);
-	flush_workqueue(tpacpi_wq);
 }
 
 static int kbdlight_set_level_and_update(int level)
@@ -5356,25 +5338,11 @@ static int light_set_status(int status)
 	return -ENXIO;
 }
 
-static void light_set_status_worker(struct work_struct *work)
-{
-	struct tpacpi_led_classdev *data =
-			container_of(work, struct tpacpi_led_classdev, work);
-
-	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
-		light_set_status((data->new_state != TPACPI_LED_OFF));
-}
-
-static void light_sysfs_set(struct led_classdev *led_cdev,
+static int light_sysfs_set(struct led_classdev *led_cdev,
 			enum led_brightness brightness)
 {
-	struct tpacpi_led_classdev *data =
-		container_of(led_cdev,
-			     struct tpacpi_led_classdev,
-			     led_classdev);
-	data->new_state = (brightness != LED_OFF) ?
-				TPACPI_LED_ON : TPACPI_LED_OFF;
-	queue_work(tpacpi_wq, &data->work);
+	return light_set_status((brightness != LED_OFF) ?
+				TPACPI_LED_ON : TPACPI_LED_OFF);
 }
 
 static enum led_brightness light_sysfs_get(struct led_classdev *led_cdev)
@@ -5385,7 +5353,7 @@ static enum led_brightness light_sysfs_get(struct led_classdev *led_cdev)
 static struct tpacpi_led_classdev tpacpi_led_thinklight = {
 	.led_classdev = {
 		.name		= "tpacpi::thinklight",
-		.brightness_set	= &light_sysfs_set,
+		.brightness_set_blocking = &light_sysfs_set,
 		.brightness_get	= &light_sysfs_get,
 	}
 };
@@ -5401,7 +5369,6 @@ static int __init light_init(struct ibm_init_struct *iibm)
 		TPACPI_ACPIHANDLE_INIT(lght);
 	}
 	TPACPI_ACPIHANDLE_INIT(cmos);
-	INIT_WORK(&tpacpi_led_thinklight.work, light_set_status_worker);
 
 	/* light not supported on 570, 600e/x, 770e, 770x, G4x, R30, R31 */
 	tp_features.light = (cmos_handle || lght_handle) && !ledb_handle;
@@ -5435,7 +5402,6 @@ static int __init light_init(struct ibm_init_struct *iibm)
 static void light_exit(void)
 {
 	led_classdev_unregister(&tpacpi_led_thinklight.led_classdev);
-	flush_workqueue(tpacpi_wq);
 }
 
 static int light_read(struct seq_file *m)
@@ -5702,29 +5668,21 @@ static int led_set_status(const unsigned int led,
 	return rc;
 }
 
-static void led_set_status_worker(struct work_struct *work)
-{
-	struct tpacpi_led_classdev *data =
-		container_of(work, struct tpacpi_led_classdev, work);
-
-	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
-		led_set_status(data->led, data->new_state);
-}
-
-static void led_sysfs_set(struct led_classdev *led_cdev,
+static int led_sysfs_set(struct led_classdev *led_cdev,
 			enum led_brightness brightness)
 {
 	struct tpacpi_led_classdev *data = container_of(led_cdev,
 			     struct tpacpi_led_classdev, led_classdev);
+	enum led_status_t new_state;
 
 	if (brightness == LED_OFF)
-		data->new_state = TPACPI_LED_OFF;
+		new_state = TPACPI_LED_OFF;
 	else if (tpacpi_led_state_cache[data->led] != TPACPI_LED_BLINK)
-		data->new_state = TPACPI_LED_ON;
+		new_state = TPACPI_LED_ON;
 	else
-		data->new_state = TPACPI_LED_BLINK;
+		new_state = TPACPI_LED_BLINK;
 
-	queue_work(tpacpi_wq, &data->work);
+	return led_set_status(data->led, new_state);
 }
 
 static int led_sysfs_blink_set(struct led_classdev *led_cdev,
@@ -5741,10 +5699,7 @@ static int led_sysfs_blink_set(struct led_classdev *led_cdev,
 	} else if ((*delay_on != 500) || (*delay_off != 500))
 		return -EINVAL;
 
-	data->new_state = TPACPI_LED_BLINK;
-	queue_work(tpacpi_wq, &data->work);
-
-	return 0;
+	return led_set_status(data->led, TPACPI_LED_BLINK);
 }
 
 static enum led_brightness led_sysfs_get(struct led_classdev *led_cdev)
@@ -5773,7 +5728,6 @@ static void led_exit(void)
 			led_classdev_unregister(&tpacpi_leds[i].led_classdev);
 	}
 
-	flush_workqueue(tpacpi_wq);
 	kfree(tpacpi_leds);
 }
 
@@ -5787,7 +5741,7 @@ static int __init tpacpi_init_led(unsigned int led)
 	if (!tpacpi_led_names[led])
 		return 0;
 
-	tpacpi_leds[led].led_classdev.brightness_set = &led_sysfs_set;
+	tpacpi_leds[led].led_classdev.brightness_set_blocking = &led_sysfs_set;
 	tpacpi_leds[led].led_classdev.blink_set = &led_sysfs_blink_set;
 	if (led_supported == TPACPI_LED_570)
 		tpacpi_leds[led].led_classdev.brightness_get =
@@ -5795,8 +5749,6 @@ static int __init tpacpi_init_led(unsigned int led)
 
 	tpacpi_leds[led].led_classdev.name = tpacpi_led_names[led];
 
-	INIT_WORK(&tpacpi_leds[led].work, led_set_status_worker);
-
 	rc = led_classdev_register(&tpacpi_pdev->dev,
 				&tpacpi_leds[led].led_classdev);
 	if (rc < 0)
-- 
2.9.3

[PATCH v8 3/7] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Make thinkpad_acpi call led_classdev_notify_brightness_hw_changed on the
kbd_led led_classdev registered by thinkpad_acpi when the kbd backlight
brightness is changed through the hotkey.

This will allow userspace to monitor (poll) for brightness changes on
these LEDs caused by the hotkey.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
-This is a new patch in v3 of this patch-set
Changes in v4:
-No changes
Changes in v5:
-Switch to new led-trigger based API for notifying userspace about
 keyboard backlight brightness changes.
-Also call ledtrig_kbd_backlight() for laptops with a thinklight
-Rename the hotkey defines from THINKLIGHT to KBD_LIGHT since they
 are shared between the thinklight and the keyboard backlight
Changes in v6:
-Switch to new brightness_hw_changed LED class API
Changes in v8:
-Cache last set brightness value and only call
 led_classdev_notify_brightness_hw_changed if the brightness has changed,
 this is necessary because the TP_HKEY_EV_KBD_LIGHT events get triggered
 when we set the brightness ourselves too
---
 drivers/platform/x86/thinkpad_acpi.c | 42 ++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index f51833f..1d18b32 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -163,6 +163,7 @@ enum tpacpi_hkey_event_t {
 	TP_HKEY_EV_HOTKEY_BASE		= 0x1001, /* first hotkey (FN+F1) */
 	TP_HKEY_EV_BRGHT_UP		= 0x1010, /* Brightness up */
 	TP_HKEY_EV_BRGHT_DOWN		= 0x1011, /* Brightness down */
+	TP_HKEY_EV_KBD_LIGHT		= 0x1012, /* Thinklight/kbd backlight */
 	TP_HKEY_EV_VOL_UP		= 0x1015, /* Volume up or unmute */
 	TP_HKEY_EV_VOL_DOWN		= 0x1016, /* Volume down or unmute */
 	TP_HKEY_EV_VOL_MUTE		= 0x1017, /* Mixer output mute */
@@ -1957,7 +1958,7 @@ enum {	/* Positions of some of the keys in hotkey masks */
 	TP_ACPI_HKEY_HIBERNATE_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNF12,
 	TP_ACPI_HKEY_BRGHTUP_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNHOME,
 	TP_ACPI_HKEY_BRGHTDWN_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNEND,
-	TP_ACPI_HKEY_THNKLGHT_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNPAGEUP,
+	TP_ACPI_HKEY_KBD_LIGHT_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNPAGEUP,
 	TP_ACPI_HKEY_ZOOM_MASK		= 1 << TP_ACPI_HOTKEYSCAN_FNSPACE,
 	TP_ACPI_HKEY_VOLUP_MASK		= 1 << TP_ACPI_HOTKEYSCAN_VOLUMEUP,
 	TP_ACPI_HKEY_VOLDWN_MASK	= 1 << TP_ACPI_HOTKEYSCAN_VOLUMEDOWN,
@@ -2342,7 +2343,7 @@ static void hotkey_read_nvram(struct tp_nvram_state *n, const u32 m)
 		n->display_toggle = !!(d & TP_NVRAM_MASK_HKT_DISPLAY);
 		n->hibernate_toggle = !!(d & TP_NVRAM_MASK_HKT_HIBERNATE);
 	}
-	if (m & TP_ACPI_HKEY_THNKLGHT_MASK) {
+	if (m & TP_ACPI_HKEY_KBD_LIGHT_MASK) {
 		d = nvram_read_byte(TP_NVRAM_ADDR_THINKLIGHT);
 		n->thinklight_toggle = !!(d & TP_NVRAM_MASK_THINKLIGHT);
 	}
@@ -5082,15 +5083,26 @@ static struct ibm_struct video_driver_data = {
  * Keyboard backlight subdriver
  */
 
+static enum led_brightness kbdlight_brightness;
+static DEFINE_MUTEX(kbdlight_mutex);
+
 static int kbdlight_set_level(int level)
 {
+	int ret = 0;
+
 	if (!hkey_handle)
 		return -ENXIO;
 
+	mutex_lock(&kbdlight_mutex);
+
 	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
-		return -EIO;
+		ret = -EIO;
+	else
+		kbdlight_brightness = level;
 
-	return 0;
+	mutex_unlock(&kbdlight_mutex);
+
+	return ret;
 }
 
 static int kbdlight_get_level(void)
@@ -5175,6 +5187,7 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
 	.led_classdev = {
 		.name		= "tpacpi::kbd_backlight",
 		.max_brightness	= 2,
+		.flags		= LED_BRIGHT_HW_CHANGED,
 		.brightness_set_blocking = &kbdlight_sysfs_set,
 		.brightness_get	= &kbdlight_sysfs_get,
 	}
@@ -5194,6 +5207,7 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
 		return 1;
 	}
 
+	kbdlight_brightness = kbdlight_sysfs_get(NULL);
 	tp_features.kbdlight = 1;
 
 	rc = led_classdev_register(&tpacpi_pdev->dev,
@@ -5203,6 +5217,8 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
 		return rc;
 	}
 
+	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
+				      TP_ACPI_HKEY_KBD_LIGHT_MASK);
 	return 0;
 }
 
@@ -9119,6 +9135,24 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 			volume_alsa_notify_change();
 		}
 	}
+	if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_KBD_LIGHT) {
+		enum led_brightness brightness;
+
+		mutex_lock(&kbdlight_mutex);
+
+		/*
+		 * Check the brightness actually changed, setting the brightness
+		 * through kbdlight_set_level() also triggers this event.
+		 */
+		brightness = kbdlight_sysfs_get(NULL);
+		if (kbdlight_brightness != brightness) {
+			kbdlight_brightness = brightness;
+			led_classdev_notify_brightness_hw_changed(
+				&tpacpi_led_kbdlight.led_classdev, brightness);
+		}
+
+		mutex_unlock(&kbdlight_mutex);
+	}
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
-- 
2.9.3

[PATCH v8 4/7] platform/x86/dell-*: Add a generic dell-laptop notifier chain

[Hans de Goede]

There are several cases where events handled in one of the dell-* drivers
need to be propagated to another dell-* driver.

This commits add 3 generic functions:
dell_laptop_register_notifier()
dell_laptop_unregister_notifier()
dell_laptop_call_notifier()

It currently only defines 1 action:
DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED

Which is intended to propagate kbd_backlight_brightness_changed wmi
events from dell-wmi to dell-laptop (which contains the actual kbd
backlight driver).

These functions are put in dell-smbios as both dell-wmi and dell-laptop
use smbios functions and I do not want to put the notifier head in
either driver, as that will make the 2 drivers depend on each other.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
Changes in v3:
-No changes
Changes in v4:
-Rename functions from dell_smbios_*_notifier to dell_laptop_*_notifier
Changes in v5:
-Dropped due to new led-trigger based approach making this unnecessary
Changes in v6:
-Re-introduced as led-trigger based approach got nacked
Changes in v8:
-Change from atomic to blocking notifier as some of the called notifier
 callbacks may take a mutex
---
 drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h | 11 +++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index d2412ab..0a57234 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -105,6 +105,26 @@ struct calling_interface_token *dell_smbios_find_token(int tokenid)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_find_token);
 
+static BLOCKING_NOTIFIER_HEAD(dell_laptop_chain_head);
+
+int dell_laptop_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&dell_laptop_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_register_notifier);
+
+int dell_laptop_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&dell_laptop_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_unregister_notifier);
+
+void dell_laptop_call_notifier(unsigned long action, void *data)
+{
+	blocking_notifier_call_chain(&dell_laptop_chain_head, action, data);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_call_notifier);
+
 static void __init parse_da_table(const struct dmi_header *dm)
 {
 	/* Final token is a terminator, so we don't want to copy it */
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ec7d40a..45cbc22 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -16,6 +16,8 @@
 #ifndef _DELL_SMBIOS_H_
 #define _DELL_SMBIOS_H_
 
+struct notifier_block;
+
 /* This structure will be modified by the firmware when we enter
  * system management mode, hence the volatiles */
 
@@ -43,4 +45,13 @@ void dell_smbios_release_buffer(void);
 void dell_smbios_send_request(int class, int select);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
+
+enum dell_laptop_notifier_actions {
+	DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED,
+};
+
+int dell_laptop_register_notifier(struct notifier_block *nb);
+int dell_laptop_unregister_notifier(struct notifier_block *nb);
+void dell_laptop_call_notifier(unsigned long action, void *data);
+
 #endif
-- 
2.9.3

[PATCH v8 5/7] platform/x86/dell-laptop: Refactor kbd_led_triggers_store()

[Hans de Goede]

Return -EINVAL immediately on invalid input, rather then doing
the straight path in an if block and returning -EINVAL at the end
of the function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v8:
-New patch in v8 of this patch-set
---
 drivers/platform/x86/dell-laptop.c | 63 +++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 14392a0..a2913a5 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1660,38 +1660,39 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
 		}
 	}
 
-	if (trigger_bit != -1) {
-		new_state = state;
-		if (trigger[0] == '+')
-			new_state.triggers |= BIT(trigger_bit);
-		else {
-			new_state.triggers &= ~BIT(trigger_bit);
-			/* NOTE: trackstick bit (2) must be disabled when
-			 *       disabling touchpad bit (1), otherwise touchpad
-			 *       bit (1) will not be disabled */
-			if (trigger_bit == 1)
-				new_state.triggers &= ~BIT(2);
-		}
-		if ((kbd_info.triggers & new_state.triggers) !=
-		    new_state.triggers)
-			return -EINVAL;
-		if (new_state.triggers && !triggers_enabled) {
-			new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
-			kbd_set_level(&new_state, kbd_previous_level);
-		} else if (new_state.triggers == 0) {
-			kbd_set_level(&new_state, 0);
-		}
-		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
-			return -EINVAL;
-		ret = kbd_set_state_safe(&new_state, &state);
-		if (ret)
-			return ret;
-		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
-			kbd_previous_mode_bit = new_state.mode_bit;
-		return count;
-	}
+	if (trigger_bit == -1)
+		return -EINVAL;
 
-	return -EINVAL;
+	new_state = state;
+	if (trigger[0] == '+')
+		new_state.triggers |= BIT(trigger_bit);
+	else {
+		new_state.triggers &= ~BIT(trigger_bit);
+		/*
+		 * NOTE: trackstick bit (2) must be disabled when
+		 *       disabling touchpad bit (1), otherwise touchpad
+		 *       bit (1) will not be disabled
+		 */
+		if (trigger_bit == 1)
+			new_state.triggers &= ~BIT(2);
+	}
+	if ((kbd_info.triggers & new_state.triggers) !=
+	    new_state.triggers)
+		return -EINVAL;
+	if (new_state.triggers && !triggers_enabled) {
+		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+		kbd_set_level(&new_state, kbd_previous_level);
+	} else if (new_state.triggers == 0) {
+		kbd_set_level(&new_state, 0);
+	}
+	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+		return -EINVAL;
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
+		kbd_previous_mode_bit = new_state.mode_bit;
+	return count;
 }
 
 static ssize_t kbd_led_triggers_show(struct device *dev,
-- 
2.9.3

[PATCH v8 6/7] platform/x86/dell-laptop: Protect kbd_state against races

[Hans de Goede]

The kbd led code has multiple entry points each of which modifies the
kbd_state by reading it, modifying a copy, writing the copy and on
error setting the modified copy writing back the original state.

This is racy, so add a mutex protection the read-modify-write cycle
on each of the entry points.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v8:
-New patch in v8 of this patch-set
---
 drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
 1 file changed, 76 insertions(+), 36 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index a2913a5..70951f3 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
 static u8 kbd_previous_mode_bit;
 
 static bool kbd_led_present;
+static DEFINE_MUTEX(kbd_led_mutex);
 
 /*
  * NOTE: there are three ways to set the keyboard backlight level.
@@ -1562,9 +1563,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
 		}
 	}
 
+	mutex_lock(&kbd_led_mutex);
+
 	ret = kbd_get_state(&state);
 	if (ret)
-		return ret;
+		goto out;
 
 	new_state = state;
 	new_state.timeout_value = value;
@@ -1572,9 +1575,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
 
 	ret = kbd_set_state_safe(&new_state, &state);
 	if (ret)
-		return ret;
+		goto out;
 
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static ssize_t kbd_led_timeout_show(struct device *dev,
@@ -1634,9 +1640,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
 	if (trigger[0] != '+' && trigger[0] != '-')
 		return -EINVAL;
 
+	mutex_lock(&kbd_led_mutex);
+
 	ret = kbd_get_state(&state);
 	if (ret)
-		return ret;
+		goto out;
 
 	if (kbd_triggers_supported)
 		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
@@ -1650,18 +1658,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
 			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
 				continue;
 			if (trigger[0] == '+' &&
-			    triggers_enabled && (state.triggers & BIT(i)))
-				return count;
+			    triggers_enabled && (state.triggers & BIT(i))) {
+				ret = count;
+				goto out;
+			}
 			if (trigger[0] == '-' &&
-			    (!triggers_enabled || !(state.triggers & BIT(i))))
-				return count;
+			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
+				ret = count;
+				goto out;
+			}
 			trigger_bit = i;
 			break;
 		}
 	}
 
-	if (trigger_bit == -1)
-		return -EINVAL;
+	if (trigger_bit == -1) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	new_state = state;
 	if (trigger[0] == '+')
@@ -1677,22 +1691,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
 			new_state.triggers &= ~BIT(2);
 	}
 	if ((kbd_info.triggers & new_state.triggers) !=
-	    new_state.triggers)
-		return -EINVAL;
+	    new_state.triggers) {
+		ret = -EINVAL;
+		goto out;
+	}
 	if (new_state.triggers && !triggers_enabled) {
 		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
 		kbd_set_level(&new_state, kbd_previous_level);
 	} else if (new_state.triggers == 0) {
 		kbd_set_level(&new_state, 0);
 	}
-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
-		return -EINVAL;
+	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
+		ret = -EINVAL;
+		goto out;
+	}
 	ret = kbd_set_state_safe(&new_state, &state);
 	if (ret)
-		return ret;
+		goto out;
 	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
 		kbd_previous_mode_bit = new_state.mode_bit;
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static ssize_t kbd_led_triggers_show(struct device *dev,
@@ -1749,12 +1770,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	mutex_lock(&kbd_led_mutex);
+
 	ret = kbd_get_state(&state);
 	if (ret)
-		return ret;
+		goto out;
 
-	if (enable == kbd_is_als_mode_bit(state.mode_bit))
-		return count;
+	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
+		ret = count;
+		goto out;
+	}
 
 	new_state = state;
 
@@ -1774,15 +1799,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
 			new_state.mode_bit = KBD_MODE_BIT_ON;
 		}
 	}
-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
-		return -EINVAL;
+	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = kbd_set_state_safe(&new_state, &state);
 	if (ret)
-		return ret;
+		goto out;
 	kbd_previous_mode_bit = new_state.mode_bit;
 
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static ssize_t kbd_led_als_enabled_show(struct device *dev,
@@ -1817,18 +1847,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	mutex_lock(&kbd_led_mutex);
+
 	ret = kbd_get_state(&state);
 	if (ret)
-		return ret;
+		goto out;
 
 	new_state = state;
 	new_state.als_setting = setting;
 
 	ret = kbd_set_state_safe(&new_state, &state);
 	if (ret)
-		return ret;
+		goto out;
 
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static ssize_t kbd_led_als_setting_show(struct device *dev,
@@ -1913,27 +1948,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
 	u16 num;
 	int ret;
 
+	mutex_lock(&kbd_led_mutex);
+
 	if (kbd_get_max_level()) {
 		ret = kbd_get_state(&state);
 		if (ret)
-			return ret;
+			goto out;
 		new_state = state;
 		ret = kbd_set_level(&new_state, value);
 		if (ret)
-			return ret;
-		return kbd_set_state_safe(&new_state, &state);
-	}
-
-	if (kbd_get_valid_token_counts()) {
+			goto out;
+		ret = kbd_set_state_safe(&new_state, &state);
+	} else if (kbd_get_valid_token_counts()) {
 		for (num = kbd_token_bits; num != 0 && value > 0; --value)
 			num &= num - 1; /* clear the first bit set */
 		if (num == 0)
-			return 0;
-		return kbd_set_token_bit(ffs(num) - 1);
+			ret = 0;
+		else
+			ret = kbd_set_token_bit(ffs(num) - 1);
+	} else {
+		pr_warn("Keyboard brightness level control not supported\n");
+		ret = -ENXIO;
 	}
 
-	pr_warn("Keyboard brightness level control not supported\n");
-	return -ENXIO;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static struct led_classdev kbd_led = {
-- 
2.9.3

[PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Make dell-wmi notify on hotkey kbd brightness changes, listen for this
in dell-laptop and call led_classdev_notify_brightness_hw_changed.

This will allow userspace to monitor (poll) for brightness changes on
these LEDs caused by the hotkey.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Changes in v2:
-Use the new dell_smbios*notify functionality
Changes in v3:
-Simplify the if condition for calling led_notify_brightness_change
Changes in v4:
-Adjust for dell_smbios_*_notifier to dell_laptop_*_notifier rename
Changes in v5:
-Switch to new led-trigger based API for notifying userspace about
 keyboard backlight brightness changes.
Changes in v6:
-Switch to new brightness_hw_changed LED class API
Changes in v8:
-Cache last set brightness value and only call
 led_classdev_notify_brightness_hw_changed if the brightness has changed,
 this is necessary because the WMI events get triggered when we set the
 brightness ourselves too
---
 drivers/platform/x86/dell-laptop.c | 47 +++++++++++++++++++++++++++++++++++++-
 drivers/platform/x86/dell-wmi.c    | 14 ++++++++----
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 70951f3..f30938b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
 static u8 kbd_previous_mode_bit;
 
 static bool kbd_led_present;
+static enum led_brightness kbd_led_brightness;
 static DEFINE_MUTEX(kbd_led_mutex);
 
 /*
@@ -1943,6 +1944,7 @@ static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
 static int kbd_led_level_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
+	enum led_brightness new_brightness = value;
 	struct kbd_state state;
 	struct kbd_state new_state;
 	u16 num;
@@ -1971,6 +1973,8 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
 		ret = -ENXIO;
 	}
 
+	if (ret == 0)
+		kbd_led_brightness = new_brightness;
 out:
 	mutex_unlock(&kbd_led_mutex);
 	return ret;
@@ -1978,6 +1982,7 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
 
 static struct led_classdev kbd_led = {
 	.name           = "dell::kbd_backlight",
+	.flags		= LED_BRIGHT_HW_CHANGED,
 	.brightness_set_blocking = kbd_led_level_set,
 	.brightness_get = kbd_led_level_get,
 	.groups         = kbd_led_groups,
@@ -1985,6 +1990,8 @@ static struct led_classdev kbd_led = {
 
 static int __init kbd_led_init(struct device *dev)
 {
+	int ret;
+
 	kbd_init();
 	if (!kbd_led_present)
 		return -ENODEV;
@@ -1996,7 +2003,12 @@ static int __init kbd_led_init(struct device *dev)
 		if (kbd_led.max_brightness)
 			kbd_led.max_brightness--;
 	}
-	return led_classdev_register(dev, &kbd_led);
+	kbd_led_brightness = kbd_led_level_get(&kbd_led);
+	ret = led_classdev_register(dev, &kbd_led);
+	if (ret)
+		kbd_led_present = false;
+
+	return ret;
 }
 
 static void brightness_set_exit(struct led_classdev *led_cdev,
@@ -2013,6 +2025,36 @@ static void kbd_led_exit(void)
 	led_classdev_unregister(&kbd_led);
 }
 
+static int dell_laptop_notifier_call(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	enum led_brightness brightness;
+
+	switch (action) {
+	case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
+		if (!kbd_led_present)
+			break;
+
+		mutex_lock(&kbd_led_mutex);
+
+		brightness = kbd_led_level_get(&kbd_led);
+		if (kbd_led_brightness != brightness) {
+			kbd_led_brightness = brightness;
+			led_classdev_notify_brightness_hw_changed(&kbd_led,
+								  brightness);
+		}
+
+		mutex_unlock(&kbd_led_mutex);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block dell_laptop_notifier = {
+	.notifier_call = dell_laptop_notifier_call,
+};
+
 static int __init dell_init(void)
 {
 	struct calling_interface_buffer *buffer;
@@ -2056,6 +2098,8 @@ static int __init dell_init(void)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
 				    &dell_debugfs_fops);
 
+	dell_laptop_register_notifier(&dell_laptop_notifier);
+
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
 
@@ -2107,6 +2151,7 @@ static int __init dell_init(void)
 
 static void __exit dell_exit(void)
 {
+	dell_laptop_unregister_notifier(&dell_laptop_notifier);
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 75e6370..15740b5 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -297,11 +297,11 @@ static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
 	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
 
 	/* Keyboard backlight level changed */
-	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x01e1, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02ea, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02eb, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02ec, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02f6, { KEY_KBDILLUMTOGGLE } },
 };
 
 static struct input_dev *dell_wmi_input_dev;
@@ -329,6 +329,10 @@ static void dell_wmi_process_key(int type, int code)
 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
 		return;
 
+	if (key->keycode == KEY_KBDILLUMTOGGLE)
+		dell_laptop_call_notifier(
+			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
2.9.3

Re: [PATCH v8 2/7] platform/x86/thinkpad_acpi: Use brightness_set_blocking callback for LEDs

[Henrique de Moraes Holschuh]

On Thu, Feb 9, 2017, at 13:44, Hans de Goede wrote:
> Now a days the LED core can take care of executing brightness_set from
> a workqueue if it needs to sleep, make use of this and remove a bunch
> of DIY code for this.
> 
> Since this commit removes the workqueue usage for LEDs, the
> led_sysfs_blink_set callback may now also sleep, this is fine.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

For the thinkpad-acpi bits:
Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

BTW, one sure-fire way to test this is to attach a trigger fired from
atomic context.  If it doesn't defer to a workqueue, the machine *will*
crash.  Thinkpad-acpi LEDs need to sleep (and will trigger an SMI, etc,
etc).

-- 
  Henrique Holschuh

Re: [PATCH v8 3/7] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Henrique de Moraes Holschuh]

On Thu, Feb 9, 2017, at 13:44, Hans de Goede wrote:
> Make thinkpad_acpi call led_classdev_notify_brightness_hw_changed on the
> kbd_led led_classdev registered by thinkpad_acpi when the kbd backlight
> brightness is changed through the hotkey.
> 
> This will allow userspace to monitor (poll) for brightness changes on
> these LEDs caused by the hotkey.

It will *often* do so, you assume the thinkpad firmware is far more well
behaved than it really is :-(

It is model/bios-revision specific.  Fortunately, it should works on
most models, and there really isn't a better way of doing this (other
than adding the event to the NVRAM poll set of some very old models,
which is also not worth doing unless you can actually test it).

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
  Henrique Holschuh

Re: [PATCH v8 1/7] platform/x86/thinkpad_acpi: Stop setting led_classdev brightness directly

[Henrique de Moraes Holschuh]

On Thu, Feb 9, 2017, at 13:44, Hans de Goede wrote:
> There is no need to set the led_classdev's brightness value from
> its set_brightness callback, this is taken care of by the led-core and
> thinkpad_acpi really should not be mucking with it.
> 
> Note that kbdlight_set_level_and_update() is still used by the old
> thinpad_acpi specific sysfs interface for the led, so we cannot
> remove it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>


-- 
  Henrique Holschuh

Re: [PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Jacek Anaszewski]

On 02/09/2017 04:44 PM, Hans de Goede wrote:
> Hi All,
> 
> Here is v8 of the platform drivers changes matching / using the new
> v8 LED subsys led_classdev_notify_brightness_hw_changed() functionality
> queued in -next.
> 
> There have been some changes (and preparation patches added) compared
> to the previous versions since the new LED api requires the driver to
> only call led_classdev_notify_brightness_hw_changed() on hotkey triggered
> changes and the ACPI events indicating brightness changes also get
> triggered when setting the brightness to led_set_brightness (or sysfs).
> 
> This series depends on the patch adding
> led_classdev_notify_brightness_hw_changed() to the LED subsystem,
> Jacek can you create a stable branch with just that patch which the
> platform/x86 platform maintainers can merge, so that they can apply
> this series ?

Created branch led_brightness_hw_changed on
git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On Thu 2017-02-09 16:44:10, Hans de Goede wrote:
> Hi All,
> 
> Here is v8 of the platform drivers changes matching / using the new
> v8 LED subsys led_classdev_notify_brightness_hw_changed() functionality
> queued in -next.
> 
> There have been some changes (and preparation patches added) compared
> to the previous versions since the new LED api requires the driver to
> only call led_classdev_notify_brightness_hw_changed() on hotkey triggered
> changes and the ACPI events indicating brightness changes also get
> triggered when setting the brightness to led_set_brightness (or sysfs).
> 
> This series depends on the patch adding
> led_classdev_notify_brightness_hw_changed() to the LED subsystem,
> Jacek can you create a stable branch with just that patch which the
> platform/x86 platform maintainers can merge, so that they can apply
> this series ?

Series looks good to me.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v8 3/7] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Andy Shevchenko]

On Thu, Feb 9, 2017 at 8:08 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Thu, Feb 9, 2017, at 13:44, Hans de Goede wrote:
>> Make thinkpad_acpi call led_classdev_notify_brightness_hw_changed on the
>> kbd_led led_classdev registered by thinkpad_acpi when the kbd backlight
>> brightness is changed through the hotkey.
>>
>> This will allow userspace to monitor (poll) for brightness changes on
>> these LEDs caused by the hotkey.
>
> It will *often* do so, you assume the thinkpad firmware is far more well
> behaved than it really is :-(
>
> It is model/bios-revision specific.  Fortunately, it should works on
> most models, and there really isn't a better way of doing this (other
> than adding the event to the NVRAM poll set of some very old models,
> which is also not worth doing unless you can actually test it).
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

Thanks, thinkpad_acpi bits are applied to testing

>
> --
>   Henrique Holschuh



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/7] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 13-02-17 23:52, Andy Shevchenko wrote:
> On Thu, Feb 9, 2017 at 8:08 PM, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
>> On Thu, Feb 9, 2017, at 13:44, Hans de Goede wrote:
>>> Make thinkpad_acpi call led_classdev_notify_brightness_hw_changed on the
>>> kbd_led led_classdev registered by thinkpad_acpi when the kbd backlight
>>> brightness is changed through the hotkey.
>>>
>>> This will allow userspace to monitor (poll) for brightness changes on
>>> these LEDs caused by the hotkey.
>>
>> It will *often* do so, you assume the thinkpad firmware is far more well
>> behaved than it really is :-(
>>
>> It is model/bios-revision specific.  Fortunately, it should works on
>> most models, and there really isn't a better way of doing this (other
>> than adding the event to the NVRAM poll set of some very old models,
>> which is also not worth doing unless you can actually test it).
>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>
> Thanks, thinkpad_acpi bits are applied to testing

Thank you, so the Dell bits are waiting for Pali's ack ?

Note the Dell bits have been tested on actual hardware too.

Regards,

Hans

Re: [PATCH v8 3/7] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Andy Shevchenko]

On Tue, Feb 14, 2017 at 11:25 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> On 13-02-17 23:52, Andy Shevchenko wrote:
>> On Thu, Feb 9, 2017 at 8:08 PM, Henrique de Moraes Holschuh
>> <hmh@hmh.eng.br> wrote:
>>> On Thu, Feb 9, 2017, at 13:44, Hans de Goede wrote:

>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

>> Thanks, thinkpad_acpi bits are applied to testing

> Thank you, so the Dell bits are waiting for Pali's ack ?
> Note the Dell bits have been tested on actual hardware too.

This cycle shows a flaw in my understanding of process. I've
mistakenly pushed couple of patches without maintainers' review. So, I
guess from now on we highly recommend to have a tag from actual
maintainer before we are going to proceed.

Darren, if you think this is too strong, what would be better approach?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/7] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Tuesday 14 February 2017 10:25:24 Hans de Goede wrote:
> Thank you, so the Dell bits are waiting for Pali's ack ?

Please give me some time for review, currently I'm busy.

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

Re: [PATCH v8 3/7] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Darren Hart]

On Tue, Feb 14, 2017 at 11:33:49AM +0200, Andy Shevchenko wrote:
> On Tue, Feb 14, 2017 at 11:25 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > On 13-02-17 23:52, Andy Shevchenko wrote:
> >> On Thu, Feb 9, 2017 at 8:08 PM, Henrique de Moraes Holschuh
> >> <hmh@hmh.eng.br> wrote:
> >>> On Thu, Feb 9, 2017, at 13:44, Hans de Goede wrote:
> 
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> 
> >> Thanks, thinkpad_acpi bits are applied to testing
> 
> > Thank you, so the Dell bits are waiting for Pali's ack ?
> > Note the Dell bits have been tested on actual hardware too.
> 
> This cycle shows a flaw in my understanding of process. I've
> mistakenly pushed couple of patches without maintainers' review. So, I
> guess from now on we highly recommend to have a tag from actual
> maintainer before we are going to proceed.
> 
> Darren, if you think this is too strong, what would be better approach?

What I've discussed with the various driver maintainers is they should reply
within a week, and after that, I (now Andy and/or I) will make the decision.

I try to stick to this especially for functional changes. For DMI matches and
new HIDs and other trivial things, I usually just queue to testing and ask the
maintainer if they have any objections, and take silence for accent :)

I do think there is value in getting the patch into testing as soon as possible,
even while waiting for the driver maintainer to get to it. The risk would be
accidentally committing a patch a maintainer objects to. I don't think this has
happened yet as an email from the maintainer can trigger dropping the patch. We
can discuss how to manage the corner cases offline.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v8 5/7] platform/x86/dell-laptop: Refactor kbd_led_triggers_store()

[Pali Rohár]

On Thursday 09 February 2017 16:44:15 Hans de Goede wrote:
> Return -EINVAL immediately on invalid input, rather then doing
> the straight path in an if block and returning -EINVAL at the end
> of the function.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Ok, you can add my Reviewed-by.

> ---
> Changes in v8:
> -New patch in v8 of this patch-set
> ---
>  drivers/platform/x86/dell-laptop.c | 63 +++++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 14392a0..a2913a5 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1660,38 +1660,39 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>  		}
>  	}
>  
> -	if (trigger_bit != -1) {
> -		new_state = state;
> -		if (trigger[0] == '+')
> -			new_state.triggers |= BIT(trigger_bit);
> -		else {
> -			new_state.triggers &= ~BIT(trigger_bit);
> -			/* NOTE: trackstick bit (2) must be disabled when
> -			 *       disabling touchpad bit (1), otherwise touchpad
> -			 *       bit (1) will not be disabled */
> -			if (trigger_bit == 1)
> -				new_state.triggers &= ~BIT(2);
> -		}
> -		if ((kbd_info.triggers & new_state.triggers) !=
> -		    new_state.triggers)
> -			return -EINVAL;
> -		if (new_state.triggers && !triggers_enabled) {
> -			new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> -			kbd_set_level(&new_state, kbd_previous_level);
> -		} else if (new_state.triggers == 0) {
> -			kbd_set_level(&new_state, 0);
> -		}
> -		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> -			return -EINVAL;
> -		ret = kbd_set_state_safe(&new_state, &state);
> -		if (ret)
> -			return ret;
> -		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
> -			kbd_previous_mode_bit = new_state.mode_bit;
> -		return count;
> -	}
> +	if (trigger_bit == -1)
> +		return -EINVAL;
>  
> -	return -EINVAL;
> +	new_state = state;
> +	if (trigger[0] == '+')
> +		new_state.triggers |= BIT(trigger_bit);
> +	else {
> +		new_state.triggers &= ~BIT(trigger_bit);
> +		/*
> +		 * NOTE: trackstick bit (2) must be disabled when
> +		 *       disabling touchpad bit (1), otherwise touchpad
> +		 *       bit (1) will not be disabled
> +		 */
> +		if (trigger_bit == 1)
> +			new_state.triggers &= ~BIT(2);
> +	}
> +	if ((kbd_info.triggers & new_state.triggers) !=
> +	    new_state.triggers)
> +		return -EINVAL;
> +	if (new_state.triggers && !triggers_enabled) {
> +		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> +		kbd_set_level(&new_state, kbd_previous_level);
> +	} else if (new_state.triggers == 0) {
> +		kbd_set_level(&new_state, 0);
> +	}
> +	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> +		return -EINVAL;
> +	ret = kbd_set_state_safe(&new_state, &state);
> +	if (ret)
> +		return ret;
> +	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
> +		kbd_previous_mode_bit = new_state.mode_bit;
> +	return count;
>  }
>  
>  static ssize_t kbd_led_triggers_show(struct device *dev,

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

Re: [PATCH v8 6/7] platform/x86/dell-laptop: Protect kbd_state against races

[Pali Rohár]

On Thursday 09 February 2017 16:44:16 Hans de Goede wrote:
> The kbd led code has multiple entry points each of which modifies the
> kbd_state by reading it, modifying a copy, writing the copy and on
> error setting the modified copy writing back the original state.
> 
> This is racy, so add a mutex protection the read-modify-write cycle
> on each of the entry points.

Is this mutex really needed? kbd_get_state and kbd_set_state are already
locked by mutex. Which situation is trying this patch fix?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v8:
> -New patch in v8 of this patch-set
> ---
>  drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
>  1 file changed, 76 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index a2913a5..70951f3 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
>  static u8 kbd_previous_mode_bit;
>  
>  static bool kbd_led_present;
> +static DEFINE_MUTEX(kbd_led_mutex);
>  
>  /*
>   * NOTE: there are three ways to set the keyboard backlight level.
> @@ -1562,9 +1563,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>  		}
>  	}
>  
> +	mutex_lock(&kbd_led_mutex);
> +
>  	ret = kbd_get_state(&state);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	new_state = state;
>  	new_state.timeout_value = value;
> @@ -1572,9 +1575,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>  
>  	ret = kbd_set_state_safe(&new_state, &state);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
> -	return count;
> +	ret = count;
> +out:
> +	mutex_unlock(&kbd_led_mutex);
> +	return ret;
>  }
>  
>  static ssize_t kbd_led_timeout_show(struct device *dev,
> @@ -1634,9 +1640,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>  	if (trigger[0] != '+' && trigger[0] != '-')
>  		return -EINVAL;
>  
> +	mutex_lock(&kbd_led_mutex);
> +
>  	ret = kbd_get_state(&state);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	if (kbd_triggers_supported)
>  		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
> @@ -1650,18 +1658,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>  			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
>  				continue;
>  			if (trigger[0] == '+' &&
> -			    triggers_enabled && (state.triggers & BIT(i)))
> -				return count;
> +			    triggers_enabled && (state.triggers & BIT(i))) {
> +				ret = count;
> +				goto out;
> +			}
>  			if (trigger[0] == '-' &&
> -			    (!triggers_enabled || !(state.triggers & BIT(i))))
> -				return count;
> +			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
> +				ret = count;
> +				goto out;
> +			}
>  			trigger_bit = i;
>  			break;
>  		}
>  	}
>  
> -	if (trigger_bit == -1)
> -		return -EINVAL;
> +	if (trigger_bit == -1) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	new_state = state;
>  	if (trigger[0] == '+')
> @@ -1677,22 +1691,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>  			new_state.triggers &= ~BIT(2);
>  	}
>  	if ((kbd_info.triggers & new_state.triggers) !=
> -	    new_state.triggers)
> -		return -EINVAL;
> +	    new_state.triggers) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  	if (new_state.triggers && !triggers_enabled) {
>  		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
>  		kbd_set_level(&new_state, kbd_previous_level);
>  	} else if (new_state.triggers == 0) {
>  		kbd_set_level(&new_state, 0);
>  	}
> -	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> -		return -EINVAL;
> +	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  	ret = kbd_set_state_safe(&new_state, &state);
>  	if (ret)
> -		return ret;
> +		goto out;
>  	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
>  		kbd_previous_mode_bit = new_state.mode_bit;
> -	return count;
> +	ret = count;
> +out:
> +	mutex_unlock(&kbd_led_mutex);
> +	return ret;
>  }
>  
>  static ssize_t kbd_led_triggers_show(struct device *dev,
> @@ -1749,12 +1770,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> +	mutex_lock(&kbd_led_mutex);
> +
>  	ret = kbd_get_state(&state);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
> -	if (enable == kbd_is_als_mode_bit(state.mode_bit))
> -		return count;
> +	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
> +		ret = count;
> +		goto out;
> +	}
>  
>  	new_state = state;
>  
> @@ -1774,15 +1799,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
>  			new_state.mode_bit = KBD_MODE_BIT_ON;
>  		}
>  	}
> -	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> -		return -EINVAL;
> +	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	ret = kbd_set_state_safe(&new_state, &state);
>  	if (ret)
> -		return ret;
> +		goto out;
>  	kbd_previous_mode_bit = new_state.mode_bit;
>  
> -	return count;
> +	ret = count;
> +out:
> +	mutex_unlock(&kbd_led_mutex);
> +	return ret;
>  }
>  
>  static ssize_t kbd_led_als_enabled_show(struct device *dev,
> @@ -1817,18 +1847,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> +	mutex_lock(&kbd_led_mutex);
> +
>  	ret = kbd_get_state(&state);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	new_state = state;
>  	new_state.als_setting = setting;
>  
>  	ret = kbd_set_state_safe(&new_state, &state);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
> -	return count;
> +	ret = count;
> +out:
> +	mutex_unlock(&kbd_led_mutex);
> +	return ret;
>  }
>  
>  static ssize_t kbd_led_als_setting_show(struct device *dev,
> @@ -1913,27 +1948,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>  	u16 num;
>  	int ret;
>  
> +	mutex_lock(&kbd_led_mutex);
> +
>  	if (kbd_get_max_level()) {
>  		ret = kbd_get_state(&state);
>  		if (ret)
> -			return ret;
> +			goto out;
>  		new_state = state;
>  		ret = kbd_set_level(&new_state, value);
>  		if (ret)
> -			return ret;
> -		return kbd_set_state_safe(&new_state, &state);
> -	}
> -
> -	if (kbd_get_valid_token_counts()) {
> +			goto out;
> +		ret = kbd_set_state_safe(&new_state, &state);
> +	} else if (kbd_get_valid_token_counts()) {
>  		for (num = kbd_token_bits; num != 0 && value > 0; --value)
>  			num &= num - 1; /* clear the first bit set */
>  		if (num == 0)
> -			return 0;
> -		return kbd_set_token_bit(ffs(num) - 1);
> +			ret = 0;
> +		else
> +			ret = kbd_set_token_bit(ffs(num) - 1);
> +	} else {
> +		pr_warn("Keyboard brightness level control not supported\n");
> +		ret = -ENXIO;
>  	}
>  
> -	pr_warn("Keyboard brightness level control not supported\n");
> -	return -ENXIO;
> +out:
> +	mutex_unlock(&kbd_led_mutex);
> +	return ret;
>  }
>  
>  static struct led_classdev kbd_led = {

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Thursday 09 February 2017 16:44:17 Hans de Goede wrote:
> Make dell-wmi notify on hotkey kbd brightness changes, listen for this
> in dell-laptop and call led_classdev_notify_brightness_hw_changed.
> 
> This will allow userspace to monitor (poll) for brightness changes on
> these LEDs caused by the hotkey.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Changes in v2:
> -Use the new dell_smbios*notify functionality
> Changes in v3:
> -Simplify the if condition for calling led_notify_brightness_change
> Changes in v4:
> -Adjust for dell_smbios_*_notifier to dell_laptop_*_notifier rename
> Changes in v5:
> -Switch to new led-trigger based API for notifying userspace about
>  keyboard backlight brightness changes.
> Changes in v6:
> -Switch to new brightness_hw_changed LED class API
> Changes in v8:
> -Cache last set brightness value and only call
>  led_classdev_notify_brightness_hw_changed if the brightness has changed,
>  this is necessary because the WMI events get triggered when we set the
>  brightness ourselves too

NAK. This does not work. Brightness can and already is handled also by
userspace application from Dell libsmbios package. Not every brightness
change is going via kernel driver and so new variable kbd_led_brightness
contains just random value.

> ---
>  drivers/platform/x86/dell-laptop.c | 47 +++++++++++++++++++++++++++++++++++++-
>  drivers/platform/x86/dell-wmi.c    | 14 ++++++++----
>  2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 70951f3..f30938b 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
>  static u8 kbd_previous_mode_bit;
>  
>  static bool kbd_led_present;
> +static enum led_brightness kbd_led_brightness;
>  static DEFINE_MUTEX(kbd_led_mutex);
>  
>  /*
> @@ -1943,6 +1944,7 @@ static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
>  static int kbd_led_level_set(struct led_classdev *led_cdev,
>  			     enum led_brightness value)
>  {
> +	enum led_brightness new_brightness = value;
>  	struct kbd_state state;
>  	struct kbd_state new_state;
>  	u16 num;
> @@ -1971,6 +1973,8 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>  		ret = -ENXIO;
>  	}
>  
> +	if (ret == 0)
> +		kbd_led_brightness = new_brightness;
>  out:
>  	mutex_unlock(&kbd_led_mutex);
>  	return ret;
> @@ -1978,6 +1982,7 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>  
>  static struct led_classdev kbd_led = {
>  	.name           = "dell::kbd_backlight",
> +	.flags		= LED_BRIGHT_HW_CHANGED,
>  	.brightness_set_blocking = kbd_led_level_set,
>  	.brightness_get = kbd_led_level_get,
>  	.groups         = kbd_led_groups,
> @@ -1985,6 +1990,8 @@ static struct led_classdev kbd_led = {
>  
>  static int __init kbd_led_init(struct device *dev)
>  {
> +	int ret;
> +
>  	kbd_init();
>  	if (!kbd_led_present)
>  		return -ENODEV;
> @@ -1996,7 +2003,12 @@ static int __init kbd_led_init(struct device *dev)
>  		if (kbd_led.max_brightness)
>  			kbd_led.max_brightness--;
>  	}
> -	return led_classdev_register(dev, &kbd_led);
> +	kbd_led_brightness = kbd_led_level_get(&kbd_led);
> +	ret = led_classdev_register(dev, &kbd_led);
> +	if (ret)
> +		kbd_led_present = false;
> +
> +	return ret;
>  }
>  
>  static void brightness_set_exit(struct led_classdev *led_cdev,
> @@ -2013,6 +2025,36 @@ static void kbd_led_exit(void)
>  	led_classdev_unregister(&kbd_led);
>  }
>  
> +static int dell_laptop_notifier_call(struct notifier_block *nb,
> +				     unsigned long action, void *data)
> +{
> +	enum led_brightness brightness;
> +
> +	switch (action) {
> +	case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
> +		if (!kbd_led_present)
> +			break;
> +
> +		mutex_lock(&kbd_led_mutex);
> +
> +		brightness = kbd_led_level_get(&kbd_led);
> +		if (kbd_led_brightness != brightness) {
> +			kbd_led_brightness = brightness;
> +			led_classdev_notify_brightness_hw_changed(&kbd_led,
> +								  brightness);
> +		}
> +
> +		mutex_unlock(&kbd_led_mutex);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block dell_laptop_notifier = {
> +	.notifier_call = dell_laptop_notifier_call,
> +};
> +
>  static int __init dell_init(void)
>  {
>  	struct calling_interface_buffer *buffer;
> @@ -2056,6 +2098,8 @@ static int __init dell_init(void)
>  		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
>  				    &dell_debugfs_fops);
>  
> +	dell_laptop_register_notifier(&dell_laptop_notifier);
> +
>  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>  		return 0;
>  
> @@ -2107,6 +2151,7 @@ static int __init dell_init(void)
>  
>  static void __exit dell_exit(void)
>  {
> +	dell_laptop_unregister_notifier(&dell_laptop_notifier);
>  	debugfs_remove_recursive(dell_laptop_dir);
>  	if (quirks && quirks->touchpad_led)
>  		touchpad_led_exit();
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 75e6370..15740b5 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -297,11 +297,11 @@ static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
>  	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
>  
>  	/* Keyboard backlight level changed */
> -	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
> -	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
> -	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
> -	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
> -	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
> +	{ KE_IGNORE, 0x01e1, { KEY_KBDILLUMTOGGLE } },
> +	{ KE_IGNORE, 0x02ea, { KEY_KBDILLUMTOGGLE } },
> +	{ KE_IGNORE, 0x02eb, { KEY_KBDILLUMTOGGLE } },
> +	{ KE_IGNORE, 0x02ec, { KEY_KBDILLUMTOGGLE } },
> +	{ KE_IGNORE, 0x02f6, { KEY_KBDILLUMTOGGLE } },
>  };
>  
>  static struct input_dev *dell_wmi_input_dev;
> @@ -329,6 +329,10 @@ static void dell_wmi_process_key(int type, int code)
>  	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
>  		return;
>  
> +	if (key->keycode == KEY_KBDILLUMTOGGLE)
> +		dell_laptop_call_notifier(
> +			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
> +
>  	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>  }
>  

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

Re: [PATCH v8 4/7] platform/x86/dell-*: Add a generic dell-laptop notifier chain

[Pali Rohár]

On Thursday 09 February 2017 16:44:14 Hans de Goede wrote:
> There are several cases where events handled in one of the dell-* drivers
> need to be propagated to another dell-* driver.
> 
> This commits add 3 generic functions:
> dell_laptop_register_notifier()
> dell_laptop_unregister_notifier()
> dell_laptop_call_notifier()
> 
> It currently only defines 1 action:
> DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED
> 
> Which is intended to propagate kbd_backlight_brightness_changed wmi
> events from dell-wmi to dell-laptop (which contains the actual kbd
> backlight driver).
> 
> These functions are put in dell-smbios as both dell-wmi and dell-laptop
> use smbios functions and I do not want to put the notifier head in
> either driver, as that will make the 2 drivers depend on each other.

As wrote in previous emails I do not like adding some totally
SMBIOS-unrelated functions to SMBIOS driver. Basically dell-smbios.ko is
driver for sending smbios calls and this new laptop notifier is for
sending events between other dell drivers.

But I remember that we have not find any clean or better solution yet.

So I will let this patch open for other people...

If somebody has better idea how to solve this problem let us know.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> Changes in v3:
> -No changes
> Changes in v4:
> -Rename functions from dell_smbios_*_notifier to dell_laptop_*_notifier
> Changes in v5:
> -Dropped due to new led-trigger based approach making this unnecessary
> Changes in v6:
> -Re-introduced as led-trigger based approach got nacked
> Changes in v8:
> -Change from atomic to blocking notifier as some of the called notifier
>  callbacks may take a mutex
> ---
>  drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
>  drivers/platform/x86/dell-smbios.h | 11 +++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index d2412ab..0a57234 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -105,6 +105,26 @@ struct calling_interface_token *dell_smbios_find_token(int tokenid)
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_find_token);
>  
> +static BLOCKING_NOTIFIER_HEAD(dell_laptop_chain_head);
> +
> +int dell_laptop_register_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&dell_laptop_chain_head, nb);
> +}
> +EXPORT_SYMBOL_GPL(dell_laptop_register_notifier);
> +
> +int dell_laptop_unregister_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&dell_laptop_chain_head, nb);
> +}
> +EXPORT_SYMBOL_GPL(dell_laptop_unregister_notifier);
> +
> +void dell_laptop_call_notifier(unsigned long action, void *data)
> +{
> +	blocking_notifier_call_chain(&dell_laptop_chain_head, action, data);
> +}
> +EXPORT_SYMBOL_GPL(dell_laptop_call_notifier);
> +
>  static void __init parse_da_table(const struct dmi_header *dm)
>  {
>  	/* Final token is a terminator, so we don't want to copy it */
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> index ec7d40a..45cbc22 100644
> --- a/drivers/platform/x86/dell-smbios.h
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -16,6 +16,8 @@
>  #ifndef _DELL_SMBIOS_H_
>  #define _DELL_SMBIOS_H_
>  
> +struct notifier_block;
> +
>  /* This structure will be modified by the firmware when we enter
>   * system management mode, hence the volatiles */
>  
> @@ -43,4 +45,13 @@ void dell_smbios_release_buffer(void);
>  void dell_smbios_send_request(int class, int select);
>  
>  struct calling_interface_token *dell_smbios_find_token(int tokenid);
> +
> +enum dell_laptop_notifier_actions {
> +	DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED,
> +};
> +
> +int dell_laptop_register_notifier(struct notifier_block *nb);
> +int dell_laptop_unregister_notifier(struct notifier_block *nb);
> +void dell_laptop_call_notifier(unsigned long action, void *data);
> +
>  #endif

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

Re: [PATCH v8 6/7] platform/x86/dell-laptop: Protect kbd_state against races

[Hans de Goede]

Hi,

On 21-02-17 15:06, Pali Rohár wrote:
> On Thursday 09 February 2017 16:44:16 Hans de Goede wrote:
>> The kbd led code has multiple entry points each of which modifies the
>> kbd_state by reading it, modifying a copy, writing the copy and on
>> error setting the modified copy writing back the original state.
>>
>> This is racy, so add a mutex protection the read-modify-write cycle
>> on each of the entry points.
>
> Is this mutex really needed? kbd_get_state and kbd_set_state are already
> locked by mutex. Which situation is trying this patch fix?

Yes this is really necessary, between getting the state
and storing it writing to another sysfs attribute
may cause a change to the state which will then get
overwritten by the write of the earlier gotten state.

This is a classic read-modify-write race and as such
needs protection.

Regards,

Hans


>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v8:
>> -New patch in v8 of this patch-set
>> ---
>>  drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
>>  1 file changed, 76 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
>> index a2913a5..70951f3 100644
>> --- a/drivers/platform/x86/dell-laptop.c
>> +++ b/drivers/platform/x86/dell-laptop.c
>> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
>>  static u8 kbd_previous_mode_bit;
>>
>>  static bool kbd_led_present;
>> +static DEFINE_MUTEX(kbd_led_mutex);
>>
>>  /*
>>   * NOTE: there are three ways to set the keyboard backlight level.
>> @@ -1562,9 +1563,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>>  		}
>>  	}
>>
>> +	mutex_lock(&kbd_led_mutex);
>> +
>>  	ret = kbd_get_state(&state);
>>  	if (ret)
>> -		return ret;
>> +		goto out;
>>
>>  	new_state = state;
>>  	new_state.timeout_value = value;
>> @@ -1572,9 +1575,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>>
>>  	ret = kbd_set_state_safe(&new_state, &state);
>>  	if (ret)
>> -		return ret;
>> +		goto out;
>>
>> -	return count;
>> +	ret = count;
>> +out:
>> +	mutex_unlock(&kbd_led_mutex);
>> +	return ret;
>>  }
>>
>>  static ssize_t kbd_led_timeout_show(struct device *dev,
>> @@ -1634,9 +1640,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>>  	if (trigger[0] != '+' && trigger[0] != '-')
>>  		return -EINVAL;
>>
>> +	mutex_lock(&kbd_led_mutex);
>> +
>>  	ret = kbd_get_state(&state);
>>  	if (ret)
>> -		return ret;
>> +		goto out;
>>
>>  	if (kbd_triggers_supported)
>>  		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
>> @@ -1650,18 +1658,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>>  			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
>>  				continue;
>>  			if (trigger[0] == '+' &&
>> -			    triggers_enabled && (state.triggers & BIT(i)))
>> -				return count;
>> +			    triggers_enabled && (state.triggers & BIT(i))) {
>> +				ret = count;
>> +				goto out;
>> +			}
>>  			if (trigger[0] == '-' &&
>> -			    (!triggers_enabled || !(state.triggers & BIT(i))))
>> -				return count;
>> +			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
>> +				ret = count;
>> +				goto out;
>> +			}
>>  			trigger_bit = i;
>>  			break;
>>  		}
>>  	}
>>
>> -	if (trigger_bit == -1)
>> -		return -EINVAL;
>> +	if (trigger_bit == -1) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>>
>>  	new_state = state;
>>  	if (trigger[0] == '+')
>> @@ -1677,22 +1691,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>>  			new_state.triggers &= ~BIT(2);
>>  	}
>>  	if ((kbd_info.triggers & new_state.triggers) !=
>> -	    new_state.triggers)
>> -		return -EINVAL;
>> +	    new_state.triggers) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>>  	if (new_state.triggers && !triggers_enabled) {
>>  		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
>>  		kbd_set_level(&new_state, kbd_previous_level);
>>  	} else if (new_state.triggers == 0) {
>>  		kbd_set_level(&new_state, 0);
>>  	}
>> -	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
>> -		return -EINVAL;
>> +	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>>  	ret = kbd_set_state_safe(&new_state, &state);
>>  	if (ret)
>> -		return ret;
>> +		goto out;
>>  	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
>>  		kbd_previous_mode_bit = new_state.mode_bit;
>> -	return count;
>> +	ret = count;
>> +out:
>> +	mutex_unlock(&kbd_led_mutex);
>> +	return ret;
>>  }
>>
>>  static ssize_t kbd_led_triggers_show(struct device *dev,
>> @@ -1749,12 +1770,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
>>  	if (ret)
>>  		return ret;
>>
>> +	mutex_lock(&kbd_led_mutex);
>> +
>>  	ret = kbd_get_state(&state);
>>  	if (ret)
>> -		return ret;
>> +		goto out;
>>
>> -	if (enable == kbd_is_als_mode_bit(state.mode_bit))
>> -		return count;
>> +	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
>> +		ret = count;
>> +		goto out;
>> +	}
>>
>>  	new_state = state;
>>
>> @@ -1774,15 +1799,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
>>  			new_state.mode_bit = KBD_MODE_BIT_ON;
>>  		}
>>  	}
>> -	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
>> -		return -EINVAL;
>> +	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>>
>>  	ret = kbd_set_state_safe(&new_state, &state);
>>  	if (ret)
>> -		return ret;
>> +		goto out;
>>  	kbd_previous_mode_bit = new_state.mode_bit;
>>
>> -	return count;
>> +	ret = count;
>> +out:
>> +	mutex_unlock(&kbd_led_mutex);
>> +	return ret;
>>  }
>>
>>  static ssize_t kbd_led_als_enabled_show(struct device *dev,
>> @@ -1817,18 +1847,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
>>  	if (ret)
>>  		return ret;
>>
>> +	mutex_lock(&kbd_led_mutex);
>> +
>>  	ret = kbd_get_state(&state);
>>  	if (ret)
>> -		return ret;
>> +		goto out;
>>
>>  	new_state = state;
>>  	new_state.als_setting = setting;
>>
>>  	ret = kbd_set_state_safe(&new_state, &state);
>>  	if (ret)
>> -		return ret;
>> +		goto out;
>>
>> -	return count;
>> +	ret = count;
>> +out:
>> +	mutex_unlock(&kbd_led_mutex);
>> +	return ret;
>>  }
>>
>>  static ssize_t kbd_led_als_setting_show(struct device *dev,
>> @@ -1913,27 +1948,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>>  	u16 num;
>>  	int ret;
>>
>> +	mutex_lock(&kbd_led_mutex);
>> +
>>  	if (kbd_get_max_level()) {
>>  		ret = kbd_get_state(&state);
>>  		if (ret)
>> -			return ret;
>> +			goto out;
>>  		new_state = state;
>>  		ret = kbd_set_level(&new_state, value);
>>  		if (ret)
>> -			return ret;
>> -		return kbd_set_state_safe(&new_state, &state);
>> -	}
>> -
>> -	if (kbd_get_valid_token_counts()) {
>> +			goto out;
>> +		ret = kbd_set_state_safe(&new_state, &state);
>> +	} else if (kbd_get_valid_token_counts()) {
>>  		for (num = kbd_token_bits; num != 0 && value > 0; --value)
>>  			num &= num - 1; /* clear the first bit set */
>>  		if (num == 0)
>> -			return 0;
>> -		return kbd_set_token_bit(ffs(num) - 1);
>> +			ret = 0;
>> +		else
>> +			ret = kbd_set_token_bit(ffs(num) - 1);
>> +	} else {
>> +		pr_warn("Keyboard brightness level control not supported\n");
>> +		ret = -ENXIO;
>>  	}
>>
>> -	pr_warn("Keyboard brightness level control not supported\n");
>> -	return -ENXIO;
>> +out:
>> +	mutex_unlock(&kbd_led_mutex);
>> +	return ret;
>>  }
>>
>>  static struct led_classdev kbd_led = {
>

Re: [PATCH v8 6/7] platform/x86/dell-laptop: Protect kbd_state against races

[Pali Rohár]

On Tuesday 21 February 2017 15:18:14 Hans de Goede wrote:
> Hi,
> 
> On 21-02-17 15:06, Pali Rohár wrote:
> >On Thursday 09 February 2017 16:44:16 Hans de Goede wrote:
> >>The kbd led code has multiple entry points each of which modifies the
> >>kbd_state by reading it, modifying a copy, writing the copy and on
> >>error setting the modified copy writing back the original state.
> >>
> >>This is racy, so add a mutex protection the read-modify-write cycle
> >>on each of the entry points.
> >
> >Is this mutex really needed? kbd_get_state and kbd_set_state are already
> >locked by mutex. Which situation is trying this patch fix?
> 
> Yes this is really necessary, between getting the state
> and storing it writing to another sysfs attribute
> may cause a change to the state which will then get
> overwritten by the write of the earlier gotten state.
> 
> This is a classic read-modify-write race and as such
> needs protection.

Right, to preserve all changes (by all concurrent modifications) it is
needed to do that kbd_get_state() + modify + kbd_set_state() atomically.

So this patch fix all concurrent modifications by kernel.

But does not fix race condition when both userspace and kernel want to
change keyboard brightness settings.

> Regards,
> 
> Hans
> 
> 
> >
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>Changes in v8:
> >>-New patch in v8 of this patch-set
> >>---
> >> drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
> >> 1 file changed, 76 insertions(+), 36 deletions(-)
> >>
> >>diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> >>index a2913a5..70951f3 100644
> >>--- a/drivers/platform/x86/dell-laptop.c
> >>+++ b/drivers/platform/x86/dell-laptop.c
> >>@@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
> >> static u8 kbd_previous_mode_bit;
> >>
> >> static bool kbd_led_present;
> >>+static DEFINE_MUTEX(kbd_led_mutex);
> >>
> >> /*
> >>  * NOTE: there are three ways to set the keyboard backlight level.
> >>@@ -1562,9 +1563,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
> >> 		}
> >> 	}
> >>
> >>+	mutex_lock(&kbd_led_mutex);
> >>+
> >> 	ret = kbd_get_state(&state);
> >> 	if (ret)
> >>-		return ret;
> >>+		goto out;
> >>
> >> 	new_state = state;
> >> 	new_state.timeout_value = value;
> >>@@ -1572,9 +1575,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
> >>
> >> 	ret = kbd_set_state_safe(&new_state, &state);
> >> 	if (ret)
> >>-		return ret;
> >>+		goto out;
> >>
> >>-	return count;
> >>+	ret = count;
> >>+out:
> >>+	mutex_unlock(&kbd_led_mutex);
> >>+	return ret;
> >> }
> >>
> >> static ssize_t kbd_led_timeout_show(struct device *dev,
> >>@@ -1634,9 +1640,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> >> 	if (trigger[0] != '+' && trigger[0] != '-')
> >> 		return -EINVAL;
> >>
> >>+	mutex_lock(&kbd_led_mutex);
> >>+
> >> 	ret = kbd_get_state(&state);
> >> 	if (ret)
> >>-		return ret;
> >>+		goto out;
> >>
> >> 	if (kbd_triggers_supported)
> >> 		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
> >>@@ -1650,18 +1658,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> >> 			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
> >> 				continue;
> >> 			if (trigger[0] == '+' &&
> >>-			    triggers_enabled && (state.triggers & BIT(i)))
> >>-				return count;
> >>+			    triggers_enabled && (state.triggers & BIT(i))) {
> >>+				ret = count;
> >>+				goto out;
> >>+			}
> >> 			if (trigger[0] == '-' &&
> >>-			    (!triggers_enabled || !(state.triggers & BIT(i))))
> >>-				return count;
> >>+			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
> >>+				ret = count;
> >>+				goto out;
> >>+			}
> >> 			trigger_bit = i;
> >> 			break;
> >> 		}
> >> 	}
> >>
> >>-	if (trigger_bit == -1)
> >>-		return -EINVAL;
> >>+	if (trigger_bit == -1) {
> >>+		ret = -EINVAL;
> >>+		goto out;
> >>+	}
> >>
> >> 	new_state = state;
> >> 	if (trigger[0] == '+')
> >>@@ -1677,22 +1691,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> >> 			new_state.triggers &= ~BIT(2);
> >> 	}
> >> 	if ((kbd_info.triggers & new_state.triggers) !=
> >>-	    new_state.triggers)
> >>-		return -EINVAL;
> >>+	    new_state.triggers) {
> >>+		ret = -EINVAL;
> >>+		goto out;
> >>+	}
> >> 	if (new_state.triggers && !triggers_enabled) {
> >> 		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> >> 		kbd_set_level(&new_state, kbd_previous_level);
> >> 	} else if (new_state.triggers == 0) {
> >> 		kbd_set_level(&new_state, 0);
> >> 	}
> >>-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> >>-		return -EINVAL;
> >>+	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
> >>+		ret = -EINVAL;
> >>+		goto out;
> >>+	}
> >> 	ret = kbd_set_state_safe(&new_state, &state);
> >> 	if (ret)
> >>-		return ret;
> >>+		goto out;
> >> 	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
> >> 		kbd_previous_mode_bit = new_state.mode_bit;
> >>-	return count;
> >>+	ret = count;
> >>+out:
> >>+	mutex_unlock(&kbd_led_mutex);
> >>+	return ret;
> >> }
> >>
> >> static ssize_t kbd_led_triggers_show(struct device *dev,
> >>@@ -1749,12 +1770,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
> >> 	if (ret)
> >> 		return ret;
> >>
> >>+	mutex_lock(&kbd_led_mutex);
> >>+
> >> 	ret = kbd_get_state(&state);
> >> 	if (ret)
> >>-		return ret;
> >>+		goto out;
> >>
> >>-	if (enable == kbd_is_als_mode_bit(state.mode_bit))
> >>-		return count;
> >>+	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
> >>+		ret = count;
> >>+		goto out;
> >>+	}
> >>
> >> 	new_state = state;
> >>
> >>@@ -1774,15 +1799,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
> >> 			new_state.mode_bit = KBD_MODE_BIT_ON;
> >> 		}
> >> 	}
> >>-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> >>-		return -EINVAL;
> >>+	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
> >>+		ret = -EINVAL;
> >>+		goto out;
> >>+	}
> >>
> >> 	ret = kbd_set_state_safe(&new_state, &state);
> >> 	if (ret)
> >>-		return ret;
> >>+		goto out;
> >> 	kbd_previous_mode_bit = new_state.mode_bit;
> >>
> >>-	return count;
> >>+	ret = count;
> >>+out:
> >>+	mutex_unlock(&kbd_led_mutex);
> >>+	return ret;
> >> }
> >>
> >> static ssize_t kbd_led_als_enabled_show(struct device *dev,
> >>@@ -1817,18 +1847,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
> >> 	if (ret)
> >> 		return ret;
> >>
> >>+	mutex_lock(&kbd_led_mutex);
> >>+
> >> 	ret = kbd_get_state(&state);
> >> 	if (ret)
> >>-		return ret;
> >>+		goto out;
> >>
> >> 	new_state = state;
> >> 	new_state.als_setting = setting;
> >>
> >> 	ret = kbd_set_state_safe(&new_state, &state);
> >> 	if (ret)
> >>-		return ret;
> >>+		goto out;
> >>
> >>-	return count;
> >>+	ret = count;
> >>+out:
> >>+	mutex_unlock(&kbd_led_mutex);
> >>+	return ret;
> >> }
> >>
> >> static ssize_t kbd_led_als_setting_show(struct device *dev,
> >>@@ -1913,27 +1948,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
> >> 	u16 num;
> >> 	int ret;
> >>
> >>+	mutex_lock(&kbd_led_mutex);
> >>+
> >> 	if (kbd_get_max_level()) {
> >> 		ret = kbd_get_state(&state);
> >> 		if (ret)
> >>-			return ret;
> >>+			goto out;
> >> 		new_state = state;
> >> 		ret = kbd_set_level(&new_state, value);
> >> 		if (ret)
> >>-			return ret;
> >>-		return kbd_set_state_safe(&new_state, &state);
> >>-	}
> >>-
> >>-	if (kbd_get_valid_token_counts()) {
> >>+			goto out;
> >>+		ret = kbd_set_state_safe(&new_state, &state);
> >>+	} else if (kbd_get_valid_token_counts()) {
> >> 		for (num = kbd_token_bits; num != 0 && value > 0; --value)
> >> 			num &= num - 1; /* clear the first bit set */
> >> 		if (num == 0)
> >>-			return 0;
> >>-		return kbd_set_token_bit(ffs(num) - 1);
> >>+			ret = 0;
> >>+		else
> >>+			ret = kbd_set_token_bit(ffs(num) - 1);
> >>+	} else {
> >>+		pr_warn("Keyboard brightness level control not supported\n");
> >>+		ret = -ENXIO;
> >> 	}
> >>
> >>-	pr_warn("Keyboard brightness level control not supported\n");
> >>-	return -ENXIO;
> >>+out:
> >>+	mutex_unlock(&kbd_led_mutex);
> >>+	return ret;
> >> }
> >>
> >> static struct led_classdev kbd_led = {
> >

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 21-02-17 15:11, Pali Rohár wrote:
> On Thursday 09 February 2017 16:44:17 Hans de Goede wrote:
>> Make dell-wmi notify on hotkey kbd brightness changes, listen for this
>> in dell-laptop and call led_classdev_notify_brightness_hw_changed.
>>
>> This will allow userspace to monitor (poll) for brightness changes on
>> these LEDs caused by the hotkey.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Changes in v2:
>> -Use the new dell_smbios*notify functionality
>> Changes in v3:
>> -Simplify the if condition for calling led_notify_brightness_change
>> Changes in v4:
>> -Adjust for dell_smbios_*_notifier to dell_laptop_*_notifier rename
>> Changes in v5:
>> -Switch to new led-trigger based API for notifying userspace about
>>  keyboard backlight brightness changes.
>> Changes in v6:
>> -Switch to new brightness_hw_changed LED class API
>> Changes in v8:
>> -Cache last set brightness value and only call
>>  led_classdev_notify_brightness_hw_changed if the brightness has changed,
>>  this is necessary because the WMI events get triggered when we set the
>>  brightness ourselves too
>
> NAK. This does not work. Brightness can and already is handled also by
> userspace application from Dell libsmbios package. Not every brightness
> change is going via kernel driver and so new variable kbd_led_brightness
> contains just random value.

This shows why it is a bad idea to have userspace code directly poking the
hardware. There is a reason why we've been moving away from the xserver
directly poking gpu-s to doing everything in the kernel.

To me libsmbios is a relic, something of ages long gone by and a normal
user should never use it.

You're right that it can be used to change things underneath the kernel,
but as said normally a user should never do that.

What will happen if a user does that regardless is that an acpi event
will trigger and the following code will execute:

+		mutex_lock(&kbd_led_mutex);
+
+		brightness = kbd_led_level_get(&kbd_led);
+		if (kbd_led_brightness != brightness) {
+			kbd_led_brightness = brightness;
+			led_classdev_notify_brightness_hw_changed(&kbd_led,
+								  brightness);
+		}
+
+		mutex_unlock(&kbd_led_mutex);

After which the kbd_led_brightness will be in sync again, so basically
the only side effect of the user changing the keyboard brightness
through libsmbios will be led_classdev_notify_brightness_hw_changed()
getting called, which is not unsurprising if you change something outside
of the kernel, so I really see no problem here.

Currently the only listener for led_classdev_notify_brightness_hw_changed()
is the gnome3 desktop stack, and having the keyboard brightness
slider in the control panel update when setting the brightness outside
of the normal ways actually is a good thing.

Regards,

Hans

>> ---
>>  drivers/platform/x86/dell-laptop.c | 47 +++++++++++++++++++++++++++++++++++++-
>>  drivers/platform/x86/dell-wmi.c    | 14 ++++++++----
>>  2 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
>> index 70951f3..f30938b 100644
>> --- a/drivers/platform/x86/dell-laptop.c
>> +++ b/drivers/platform/x86/dell-laptop.c
>> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
>>  static u8 kbd_previous_mode_bit;
>>
>>  static bool kbd_led_present;
>> +static enum led_brightness kbd_led_brightness;
>>  static DEFINE_MUTEX(kbd_led_mutex);
>>
>>  /*
>> @@ -1943,6 +1944,7 @@ static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
>>  static int kbd_led_level_set(struct led_classdev *led_cdev,
>>  			     enum led_brightness value)
>>  {
>> +	enum led_brightness new_brightness = value;
>>  	struct kbd_state state;
>>  	struct kbd_state new_state;
>>  	u16 num;
>> @@ -1971,6 +1973,8 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>>  		ret = -ENXIO;
>>  	}
>>
>> +	if (ret == 0)
>> +		kbd_led_brightness = new_brightness;
>>  out:
>>  	mutex_unlock(&kbd_led_mutex);
>>  	return ret;
>> @@ -1978,6 +1982,7 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>>
>>  static struct led_classdev kbd_led = {
>>  	.name           = "dell::kbd_backlight",
>> +	.flags		= LED_BRIGHT_HW_CHANGED,
>>  	.brightness_set_blocking = kbd_led_level_set,
>>  	.brightness_get = kbd_led_level_get,
>>  	.groups         = kbd_led_groups,
>> @@ -1985,6 +1990,8 @@ static struct led_classdev kbd_led = {
>>
>>  static int __init kbd_led_init(struct device *dev)
>>  {
>> +	int ret;
>> +
>>  	kbd_init();
>>  	if (!kbd_led_present)
>>  		return -ENODEV;
>> @@ -1996,7 +2003,12 @@ static int __init kbd_led_init(struct device *dev)
>>  		if (kbd_led.max_brightness)
>>  			kbd_led.max_brightness--;
>>  	}
>> -	return led_classdev_register(dev, &kbd_led);
>> +	kbd_led_brightness = kbd_led_level_get(&kbd_led);
>> +	ret = led_classdev_register(dev, &kbd_led);
>> +	if (ret)
>> +		kbd_led_present = false;
>> +
>> +	return ret;
>>  }
>>
>>  static void brightness_set_exit(struct led_classdev *led_cdev,
>> @@ -2013,6 +2025,36 @@ static void kbd_led_exit(void)
>>  	led_classdev_unregister(&kbd_led);
>>  }
>>
>> +static int dell_laptop_notifier_call(struct notifier_block *nb,
>> +				     unsigned long action, void *data)
>> +{
>> +	enum led_brightness brightness;
>> +
>> +	switch (action) {
>> +	case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
>> +		if (!kbd_led_present)
>> +			break;
>> +
>> +		mutex_lock(&kbd_led_mutex);
>> +
>> +		brightness = kbd_led_level_get(&kbd_led);
>> +		if (kbd_led_brightness != brightness) {
>> +			kbd_led_brightness = brightness;
>> +			led_classdev_notify_brightness_hw_changed(&kbd_led,
>> +								  brightness);
>> +		}
>> +
>> +		mutex_unlock(&kbd_led_mutex);
>> +		break;
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block dell_laptop_notifier = {
>> +	.notifier_call = dell_laptop_notifier_call,
>> +};
>> +
>>  static int __init dell_init(void)
>>  {
>>  	struct calling_interface_buffer *buffer;
>> @@ -2056,6 +2098,8 @@ static int __init dell_init(void)
>>  		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
>>  				    &dell_debugfs_fops);
>>
>> +	dell_laptop_register_notifier(&dell_laptop_notifier);
>> +
>>  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>>  		return 0;
>>
>> @@ -2107,6 +2151,7 @@ static int __init dell_init(void)
>>
>>  static void __exit dell_exit(void)
>>  {
>> +	dell_laptop_unregister_notifier(&dell_laptop_notifier);
>>  	debugfs_remove_recursive(dell_laptop_dir);
>>  	if (quirks && quirks->touchpad_led)
>>  		touchpad_led_exit();
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index 75e6370..15740b5 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -297,11 +297,11 @@ static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
>>  	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
>>
>>  	/* Keyboard backlight level changed */
>> -	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
>> -	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
>> -	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
>> -	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
>> -	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
>> +	{ KE_IGNORE, 0x01e1, { KEY_KBDILLUMTOGGLE } },
>> +	{ KE_IGNORE, 0x02ea, { KEY_KBDILLUMTOGGLE } },
>> +	{ KE_IGNORE, 0x02eb, { KEY_KBDILLUMTOGGLE } },
>> +	{ KE_IGNORE, 0x02ec, { KEY_KBDILLUMTOGGLE } },
>> +	{ KE_IGNORE, 0x02f6, { KEY_KBDILLUMTOGGLE } },
>>  };
>>
>>  static struct input_dev *dell_wmi_input_dev;
>> @@ -329,6 +329,10 @@ static void dell_wmi_process_key(int type, int code)
>>  	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
>>  		return;
>>
>> +	if (key->keycode == KEY_KBDILLUMTOGGLE)
>> +		dell_laptop_call_notifier(
>> +			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
>> +
>>  	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>>  }
>>
>

Re: [PATCH v8 6/7] platform/x86/dell-laptop: Protect kbd_state against races

[Hans de Goede]

Hi,

On 21-02-17 15:25, Pali Rohár wrote:
> On Tuesday 21 February 2017 15:18:14 Hans de Goede wrote:
>> Hi,
>>
>> On 21-02-17 15:06, Pali Rohár wrote:
>>> On Thursday 09 February 2017 16:44:16 Hans de Goede wrote:
>>>> The kbd led code has multiple entry points each of which modifies the
>>>> kbd_state by reading it, modifying a copy, writing the copy and on
>>>> error setting the modified copy writing back the original state.
>>>>
>>>> This is racy, so add a mutex protection the read-modify-write cycle
>>>> on each of the entry points.
>>>
>>> Is this mutex really needed? kbd_get_state and kbd_set_state are already
>>> locked by mutex. Which situation is trying this patch fix?
>>
>> Yes this is really necessary, between getting the state
>> and storing it writing to another sysfs attribute
>> may cause a change to the state which will then get
>> overwritten by the write of the earlier gotten state.
>>
>> This is a classic read-modify-write race and as such
>> needs protection.
>
> Right, to preserve all changes (by all concurrent modifications) it is
> needed to do that kbd_get_state() + modify + kbd_set_state() atomically.
>
> So this patch fix all concurrent modifications by kernel.
>
> But does not fix race condition when both userspace and kernel want to
> change keyboard brightness settings.

Yes using libsmbios is ALWAYS racy, as said in my previous mails,
people really should use the proper kernel interfaces rather then
directly poking hw from userspace, but just because one path is
racy is not a good reason to not fix the races in another path,
esp, when that other path is the preferred way to do things
and is actually the path which all modern desktop environments use.

Regards,

Hans



>
>> Regards,
>>
>> Hans
>>
>>
>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v8:
>>>> -New patch in v8 of this patch-set
>>>> ---
>>>> drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
>>>> 1 file changed, 76 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
>>>> index a2913a5..70951f3 100644
>>>> --- a/drivers/platform/x86/dell-laptop.c
>>>> +++ b/drivers/platform/x86/dell-laptop.c
>>>> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
>>>> static u8 kbd_previous_mode_bit;
>>>>
>>>> static bool kbd_led_present;
>>>> +static DEFINE_MUTEX(kbd_led_mutex);
>>>>
>>>> /*
>>>>  * NOTE: there are three ways to set the keyboard backlight level.
>>>> @@ -1562,9 +1563,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>>>> 		}
>>>> 	}
>>>>
>>>> +	mutex_lock(&kbd_led_mutex);
>>>> +
>>>> 	ret = kbd_get_state(&state);
>>>> 	if (ret)
>>>> -		return ret;
>>>> +		goto out;
>>>>
>>>> 	new_state = state;
>>>> 	new_state.timeout_value = value;
>>>> @@ -1572,9 +1575,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>>>>
>>>> 	ret = kbd_set_state_safe(&new_state, &state);
>>>> 	if (ret)
>>>> -		return ret;
>>>> +		goto out;
>>>>
>>>> -	return count;
>>>> +	ret = count;
>>>> +out:
>>>> +	mutex_unlock(&kbd_led_mutex);
>>>> +	return ret;
>>>> }
>>>>
>>>> static ssize_t kbd_led_timeout_show(struct device *dev,
>>>> @@ -1634,9 +1640,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>>>> 	if (trigger[0] != '+' && trigger[0] != '-')
>>>> 		return -EINVAL;
>>>>
>>>> +	mutex_lock(&kbd_led_mutex);
>>>> +
>>>> 	ret = kbd_get_state(&state);
>>>> 	if (ret)
>>>> -		return ret;
>>>> +		goto out;
>>>>
>>>> 	if (kbd_triggers_supported)
>>>> 		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
>>>> @@ -1650,18 +1658,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>>>> 			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
>>>> 				continue;
>>>> 			if (trigger[0] == '+' &&
>>>> -			    triggers_enabled && (state.triggers & BIT(i)))
>>>> -				return count;
>>>> +			    triggers_enabled && (state.triggers & BIT(i))) {
>>>> +				ret = count;
>>>> +				goto out;
>>>> +			}
>>>> 			if (trigger[0] == '-' &&
>>>> -			    (!triggers_enabled || !(state.triggers & BIT(i))))
>>>> -				return count;
>>>> +			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
>>>> +				ret = count;
>>>> +				goto out;
>>>> +			}
>>>> 			trigger_bit = i;
>>>> 			break;
>>>> 		}
>>>> 	}
>>>>
>>>> -	if (trigger_bit == -1)
>>>> -		return -EINVAL;
>>>> +	if (trigger_bit == -1) {
>>>> +		ret = -EINVAL;
>>>> +		goto out;
>>>> +	}
>>>>
>>>> 	new_state = state;
>>>> 	if (trigger[0] == '+')
>>>> @@ -1677,22 +1691,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>>>> 			new_state.triggers &= ~BIT(2);
>>>> 	}
>>>> 	if ((kbd_info.triggers & new_state.triggers) !=
>>>> -	    new_state.triggers)
>>>> -		return -EINVAL;
>>>> +	    new_state.triggers) {
>>>> +		ret = -EINVAL;
>>>> +		goto out;
>>>> +	}
>>>> 	if (new_state.triggers && !triggers_enabled) {
>>>> 		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
>>>> 		kbd_set_level(&new_state, kbd_previous_level);
>>>> 	} else if (new_state.triggers == 0) {
>>>> 		kbd_set_level(&new_state, 0);
>>>> 	}
>>>> -	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
>>>> -		return -EINVAL;
>>>> +	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
>>>> +		ret = -EINVAL;
>>>> +		goto out;
>>>> +	}
>>>> 	ret = kbd_set_state_safe(&new_state, &state);
>>>> 	if (ret)
>>>> -		return ret;
>>>> +		goto out;
>>>> 	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
>>>> 		kbd_previous_mode_bit = new_state.mode_bit;
>>>> -	return count;
>>>> +	ret = count;
>>>> +out:
>>>> +	mutex_unlock(&kbd_led_mutex);
>>>> +	return ret;
>>>> }
>>>>
>>>> static ssize_t kbd_led_triggers_show(struct device *dev,
>>>> @@ -1749,12 +1770,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
>>>> 	if (ret)
>>>> 		return ret;
>>>>
>>>> +	mutex_lock(&kbd_led_mutex);
>>>> +
>>>> 	ret = kbd_get_state(&state);
>>>> 	if (ret)
>>>> -		return ret;
>>>> +		goto out;
>>>>
>>>> -	if (enable == kbd_is_als_mode_bit(state.mode_bit))
>>>> -		return count;
>>>> +	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
>>>> +		ret = count;
>>>> +		goto out;
>>>> +	}
>>>>
>>>> 	new_state = state;
>>>>
>>>> @@ -1774,15 +1799,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
>>>> 			new_state.mode_bit = KBD_MODE_BIT_ON;
>>>> 		}
>>>> 	}
>>>> -	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
>>>> -		return -EINVAL;
>>>> +	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
>>>> +		ret = -EINVAL;
>>>> +		goto out;
>>>> +	}
>>>>
>>>> 	ret = kbd_set_state_safe(&new_state, &state);
>>>> 	if (ret)
>>>> -		return ret;
>>>> +		goto out;
>>>> 	kbd_previous_mode_bit = new_state.mode_bit;
>>>>
>>>> -	return count;
>>>> +	ret = count;
>>>> +out:
>>>> +	mutex_unlock(&kbd_led_mutex);
>>>> +	return ret;
>>>> }
>>>>
>>>> static ssize_t kbd_led_als_enabled_show(struct device *dev,
>>>> @@ -1817,18 +1847,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
>>>> 	if (ret)
>>>> 		return ret;
>>>>
>>>> +	mutex_lock(&kbd_led_mutex);
>>>> +
>>>> 	ret = kbd_get_state(&state);
>>>> 	if (ret)
>>>> -		return ret;
>>>> +		goto out;
>>>>
>>>> 	new_state = state;
>>>> 	new_state.als_setting = setting;
>>>>
>>>> 	ret = kbd_set_state_safe(&new_state, &state);
>>>> 	if (ret)
>>>> -		return ret;
>>>> +		goto out;
>>>>
>>>> -	return count;
>>>> +	ret = count;
>>>> +out:
>>>> +	mutex_unlock(&kbd_led_mutex);
>>>> +	return ret;
>>>> }
>>>>
>>>> static ssize_t kbd_led_als_setting_show(struct device *dev,
>>>> @@ -1913,27 +1948,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>>>> 	u16 num;
>>>> 	int ret;
>>>>
>>>> +	mutex_lock(&kbd_led_mutex);
>>>> +
>>>> 	if (kbd_get_max_level()) {
>>>> 		ret = kbd_get_state(&state);
>>>> 		if (ret)
>>>> -			return ret;
>>>> +			goto out;
>>>> 		new_state = state;
>>>> 		ret = kbd_set_level(&new_state, value);
>>>> 		if (ret)
>>>> -			return ret;
>>>> -		return kbd_set_state_safe(&new_state, &state);
>>>> -	}
>>>> -
>>>> -	if (kbd_get_valid_token_counts()) {
>>>> +			goto out;
>>>> +		ret = kbd_set_state_safe(&new_state, &state);
>>>> +	} else if (kbd_get_valid_token_counts()) {
>>>> 		for (num = kbd_token_bits; num != 0 && value > 0; --value)
>>>> 			num &= num - 1; /* clear the first bit set */
>>>> 		if (num == 0)
>>>> -			return 0;
>>>> -		return kbd_set_token_bit(ffs(num) - 1);
>>>> +			ret = 0;
>>>> +		else
>>>> +			ret = kbd_set_token_bit(ffs(num) - 1);
>>>> +	} else {
>>>> +		pr_warn("Keyboard brightness level control not supported\n");
>>>> +		ret = -ENXIO;
>>>> 	}
>>>>
>>>> -	pr_warn("Keyboard brightness level control not supported\n");
>>>> -	return -ENXIO;
>>>> +out:
>>>> +	mutex_unlock(&kbd_led_mutex);
>>>> +	return ret;
>>>> }
>>>>
>>>> static struct led_classdev kbd_led = {
>>>
>

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Tuesday 21 February 2017 15:40:16 Hans de Goede wrote:
> Hi,
> 
> On 21-02-17 15:11, Pali Rohár wrote:
> >On Thursday 09 February 2017 16:44:17 Hans de Goede wrote:
> >>Make dell-wmi notify on hotkey kbd brightness changes, listen for this
> >>in dell-laptop and call led_classdev_notify_brightness_hw_changed.
> >>
> >>This will allow userspace to monitor (poll) for brightness changes on
> >>these LEDs caused by the hotkey.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>Changes in v2:
> >>-Use the new dell_smbios*notify functionality
> >>Changes in v3:
> >>-Simplify the if condition for calling led_notify_brightness_change
> >>Changes in v4:
> >>-Adjust for dell_smbios_*_notifier to dell_laptop_*_notifier rename
> >>Changes in v5:
> >>-Switch to new led-trigger based API for notifying userspace about
> >> keyboard backlight brightness changes.
> >>Changes in v6:
> >>-Switch to new brightness_hw_changed LED class API
> >>Changes in v8:
> >>-Cache last set brightness value and only call
> >> led_classdev_notify_brightness_hw_changed if the brightness has changed,
> >> this is necessary because the WMI events get triggered when we set the
> >> brightness ourselves too
> >
> >NAK. This does not work. Brightness can and already is handled also by
> >userspace application from Dell libsmbios package. Not every brightness
> >change is going via kernel driver and so new variable kbd_led_brightness
> >contains just random value.
> 
> This shows why it is a bad idea to have userspace code directly poking the
> hardware. There is a reason why we've been moving away from the xserver
> directly poking gpu-s to doing everything in the kernel.

I agree.

> To me libsmbios is a relic, something of ages long gone by and a normal
> user should never use it.

Do not remember that libsmbios was there first and kernel code is
reimplementation from that userspace. Without libsmbios no kernel
support would be there.

And we can expect in future if Dell again decide to release some SMBIOS
code it will be in libsmbios...

> You're right that it can be used to change things underneath the kernel,
> but as said normally a user should never do that.

I used it and often, because kernel does not have support for keyboard
backlight. And other people probably too.

So if we agree or not, we need to deal with fact that userspace can and
it is already calling smbios API. And started it doing earlier as kernel.

> What will happen if a user does that regardless is that an acpi event
> will trigger and the following code will execute:
> 
> +		mutex_lock(&kbd_led_mutex);
> +
> +		brightness = kbd_led_level_get(&kbd_led);
> +		if (kbd_led_brightness != brightness) {
> +			kbd_led_brightness = brightness;
> +			led_classdev_notify_brightness_hw_changed(&kbd_led,
> +								  brightness);
> +		}
> +
> +		mutex_unlock(&kbd_led_mutex);
> 
> After which the kbd_led_brightness will be in sync again, so basically
> the only side effect of the user changing the keyboard brightness
> through libsmbios will be led_classdev_notify_brightness_hw_changed()
> getting called, which is not unsurprising if you change something outside
> of the kernel, so I really see no problem here.
> 
> Currently the only listener for led_classdev_notify_brightness_hw_changed()
> is the gnome3 desktop stack, and having the keyboard brightness
> slider in the control panel update when setting the brightness outside
> of the normal ways actually is a good thing.

So do we really need this code which prevents update?

> Regards,
> 
> Hans
> 
> >>---
> >> drivers/platform/x86/dell-laptop.c | 47 +++++++++++++++++++++++++++++++++++++-
> >> drivers/platform/x86/dell-wmi.c    | 14 ++++++++----
> >> 2 files changed, 55 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> >>index 70951f3..f30938b 100644
> >>--- a/drivers/platform/x86/dell-laptop.c
> >>+++ b/drivers/platform/x86/dell-laptop.c
> >>@@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
> >> static u8 kbd_previous_mode_bit;
> >>
> >> static bool kbd_led_present;
> >>+static enum led_brightness kbd_led_brightness;
> >> static DEFINE_MUTEX(kbd_led_mutex);
> >>
> >> /*
> >>@@ -1943,6 +1944,7 @@ static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
> >> static int kbd_led_level_set(struct led_classdev *led_cdev,
> >> 			     enum led_brightness value)
> >> {
> >>+	enum led_brightness new_brightness = value;
> >> 	struct kbd_state state;
> >> 	struct kbd_state new_state;
> >> 	u16 num;
> >>@@ -1971,6 +1973,8 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
> >> 		ret = -ENXIO;
> >> 	}
> >>
> >>+	if (ret == 0)
> >>+		kbd_led_brightness = new_brightness;
> >> out:
> >> 	mutex_unlock(&kbd_led_mutex);
> >> 	return ret;
> >>@@ -1978,6 +1982,7 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
> >>
> >> static struct led_classdev kbd_led = {
> >> 	.name           = "dell::kbd_backlight",
> >>+	.flags		= LED_BRIGHT_HW_CHANGED,
> >> 	.brightness_set_blocking = kbd_led_level_set,
> >> 	.brightness_get = kbd_led_level_get,
> >> 	.groups         = kbd_led_groups,
> >>@@ -1985,6 +1990,8 @@ static struct led_classdev kbd_led = {
> >>
> >> static int __init kbd_led_init(struct device *dev)
> >> {
> >>+	int ret;
> >>+
> >> 	kbd_init();
> >> 	if (!kbd_led_present)
> >> 		return -ENODEV;
> >>@@ -1996,7 +2003,12 @@ static int __init kbd_led_init(struct device *dev)
> >> 		if (kbd_led.max_brightness)
> >> 			kbd_led.max_brightness--;
> >> 	}
> >>-	return led_classdev_register(dev, &kbd_led);
> >>+	kbd_led_brightness = kbd_led_level_get(&kbd_led);
> >>+	ret = led_classdev_register(dev, &kbd_led);
> >>+	if (ret)
> >>+		kbd_led_present = false;
> >>+
> >>+	return ret;
> >> }
> >>
> >> static void brightness_set_exit(struct led_classdev *led_cdev,
> >>@@ -2013,6 +2025,36 @@ static void kbd_led_exit(void)
> >> 	led_classdev_unregister(&kbd_led);
> >> }
> >>
> >>+static int dell_laptop_notifier_call(struct notifier_block *nb,
> >>+				     unsigned long action, void *data)
> >>+{
> >>+	enum led_brightness brightness;
> >>+
> >>+	switch (action) {
> >>+	case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
> >>+		if (!kbd_led_present)
> >>+			break;
> >>+
> >>+		mutex_lock(&kbd_led_mutex);
> >>+
> >>+		brightness = kbd_led_level_get(&kbd_led);
> >>+		if (kbd_led_brightness != brightness) {
> >>+			kbd_led_brightness = brightness;
> >>+			led_classdev_notify_brightness_hw_changed(&kbd_led,
> >>+								  brightness);
> >>+		}
> >>+
> >>+		mutex_unlock(&kbd_led_mutex);
> >>+		break;
> >>+	}
> >>+
> >>+	return NOTIFY_OK;
> >>+}
> >>+
> >>+static struct notifier_block dell_laptop_notifier = {
> >>+	.notifier_call = dell_laptop_notifier_call,
> >>+};
> >>+
> >> static int __init dell_init(void)
> >> {
> >> 	struct calling_interface_buffer *buffer;
> >>@@ -2056,6 +2098,8 @@ static int __init dell_init(void)
> >> 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
> >> 				    &dell_debugfs_fops);
> >>
> >>+	dell_laptop_register_notifier(&dell_laptop_notifier);
> >>+
> >> 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> >> 		return 0;
> >>
> >>@@ -2107,6 +2151,7 @@ static int __init dell_init(void)
> >>
> >> static void __exit dell_exit(void)
> >> {
> >>+	dell_laptop_unregister_notifier(&dell_laptop_notifier);
> >> 	debugfs_remove_recursive(dell_laptop_dir);
> >> 	if (quirks && quirks->touchpad_led)
> >> 		touchpad_led_exit();
> >>diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> >>index 75e6370..15740b5 100644
> >>--- a/drivers/platform/x86/dell-wmi.c
> >>+++ b/drivers/platform/x86/dell-wmi.c
> >>@@ -297,11 +297,11 @@ static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
> >> 	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
> >>
> >> 	/* Keyboard backlight level changed */
> >>-	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
> >>-	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
> >>-	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
> >>-	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
> >>-	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
> >>+	{ KE_IGNORE, 0x01e1, { KEY_KBDILLUMTOGGLE } },
> >>+	{ KE_IGNORE, 0x02ea, { KEY_KBDILLUMTOGGLE } },
> >>+	{ KE_IGNORE, 0x02eb, { KEY_KBDILLUMTOGGLE } },
> >>+	{ KE_IGNORE, 0x02ec, { KEY_KBDILLUMTOGGLE } },
> >>+	{ KE_IGNORE, 0x02f6, { KEY_KBDILLUMTOGGLE } },
> >> };
> >>
> >> static struct input_dev *dell_wmi_input_dev;
> >>@@ -329,6 +329,10 @@ static void dell_wmi_process_key(int type, int code)
> >> 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
> >> 		return;
> >>
> >>+	if (key->keycode == KEY_KBDILLUMTOGGLE)
> >>+		dell_laptop_call_notifier(
> >>+			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
> >>+
> >> 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
> >> }
> >>
> >

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

Re: [PATCH v8 6/7] platform/x86/dell-laptop: Protect kbd_state against races

[Pali Rohár]

On Tuesday 21 February 2017 15:42:02 Hans de Goede wrote:
> Hi,
> 
> On 21-02-17 15:25, Pali Rohár wrote:
> >On Tuesday 21 February 2017 15:18:14 Hans de Goede wrote:
> >>Hi,
> >>
> >>On 21-02-17 15:06, Pali Rohár wrote:
> >>>On Thursday 09 February 2017 16:44:16 Hans de Goede wrote:
> >>>>The kbd led code has multiple entry points each of which modifies the
> >>>>kbd_state by reading it, modifying a copy, writing the copy and on
> >>>>error setting the modified copy writing back the original state.
> >>>>
> >>>>This is racy, so add a mutex protection the read-modify-write cycle
> >>>>on each of the entry points.
> >>>
> >>>Is this mutex really needed? kbd_get_state and kbd_set_state are already
> >>>locked by mutex. Which situation is trying this patch fix?
> >>
> >>Yes this is really necessary, between getting the state
> >>and storing it writing to another sysfs attribute
> >>may cause a change to the state which will then get
> >>overwritten by the write of the earlier gotten state.
> >>
> >>This is a classic read-modify-write race and as such
> >>needs protection.
> >
> >Right, to preserve all changes (by all concurrent modifications) it is
> >needed to do that kbd_get_state() + modify + kbd_set_state() atomically.
> >
> >So this patch fix all concurrent modifications by kernel.
> >
> >But does not fix race condition when both userspace and kernel want to
> >change keyboard brightness settings.
> 
> Yes using libsmbios is ALWAYS racy, as said in my previous mails,
> people really should use the proper kernel interfaces rather then
> directly poking hw from userspace, but just because one path is
> racy is not a good reason to not fix the races in another path,
> esp, when that other path is the preferred way to do things
> and is actually the path which all modern desktop environments use.

Yes, I'm not against this kernel fix. I just pointed that it does not
fix race condition between userspace and kernel...

Anyway, patch seems good so add my Reviewed-by.

> Regards,
> 
> Hans
> 
> 
> 
> >
> >>Regards,
> >>
> >>Hans
> >>
> >>
> >>>
> >>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>---
> >>>>Changes in v8:
> >>>>-New patch in v8 of this patch-set
> >>>>---
> >>>>drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
> >>>>1 file changed, 76 insertions(+), 36 deletions(-)
> >>>>
> >>>>diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> >>>>index a2913a5..70951f3 100644
> >>>>--- a/drivers/platform/x86/dell-laptop.c
> >>>>+++ b/drivers/platform/x86/dell-laptop.c
> >>>>@@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
> >>>>static u8 kbd_previous_mode_bit;
> >>>>
> >>>>static bool kbd_led_present;
> >>>>+static DEFINE_MUTEX(kbd_led_mutex);
> >>>>
> >>>>/*
> >>>> * NOTE: there are three ways to set the keyboard backlight level.
> >>>>@@ -1562,9 +1563,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
> >>>>		}
> >>>>	}
> >>>>
> >>>>+	mutex_lock(&kbd_led_mutex);
> >>>>+
> >>>>	ret = kbd_get_state(&state);
> >>>>	if (ret)
> >>>>-		return ret;
> >>>>+		goto out;
> >>>>
> >>>>	new_state = state;
> >>>>	new_state.timeout_value = value;
> >>>>@@ -1572,9 +1575,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
> >>>>
> >>>>	ret = kbd_set_state_safe(&new_state, &state);
> >>>>	if (ret)
> >>>>-		return ret;
> >>>>+		goto out;
> >>>>
> >>>>-	return count;
> >>>>+	ret = count;
> >>>>+out:
> >>>>+	mutex_unlock(&kbd_led_mutex);
> >>>>+	return ret;
> >>>>}
> >>>>
> >>>>static ssize_t kbd_led_timeout_show(struct device *dev,
> >>>>@@ -1634,9 +1640,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> >>>>	if (trigger[0] != '+' && trigger[0] != '-')
> >>>>		return -EINVAL;
> >>>>
> >>>>+	mutex_lock(&kbd_led_mutex);
> >>>>+
> >>>>	ret = kbd_get_state(&state);
> >>>>	if (ret)
> >>>>-		return ret;
> >>>>+		goto out;
> >>>>
> >>>>	if (kbd_triggers_supported)
> >>>>		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
> >>>>@@ -1650,18 +1658,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> >>>>			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
> >>>>				continue;
> >>>>			if (trigger[0] == '+' &&
> >>>>-			    triggers_enabled && (state.triggers & BIT(i)))
> >>>>-				return count;
> >>>>+			    triggers_enabled && (state.triggers & BIT(i))) {
> >>>>+				ret = count;
> >>>>+				goto out;
> >>>>+			}
> >>>>			if (trigger[0] == '-' &&
> >>>>-			    (!triggers_enabled || !(state.triggers & BIT(i))))
> >>>>-				return count;
> >>>>+			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
> >>>>+				ret = count;
> >>>>+				goto out;
> >>>>+			}
> >>>>			trigger_bit = i;
> >>>>			break;
> >>>>		}
> >>>>	}
> >>>>
> >>>>-	if (trigger_bit == -1)
> >>>>-		return -EINVAL;
> >>>>+	if (trigger_bit == -1) {
> >>>>+		ret = -EINVAL;
> >>>>+		goto out;
> >>>>+	}
> >>>>
> >>>>	new_state = state;
> >>>>	if (trigger[0] == '+')
> >>>>@@ -1677,22 +1691,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> >>>>			new_state.triggers &= ~BIT(2);
> >>>>	}
> >>>>	if ((kbd_info.triggers & new_state.triggers) !=
> >>>>-	    new_state.triggers)
> >>>>-		return -EINVAL;
> >>>>+	    new_state.triggers) {
> >>>>+		ret = -EINVAL;
> >>>>+		goto out;
> >>>>+	}
> >>>>	if (new_state.triggers && !triggers_enabled) {
> >>>>		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> >>>>		kbd_set_level(&new_state, kbd_previous_level);
> >>>>	} else if (new_state.triggers == 0) {
> >>>>		kbd_set_level(&new_state, 0);
> >>>>	}
> >>>>-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> >>>>-		return -EINVAL;
> >>>>+	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
> >>>>+		ret = -EINVAL;
> >>>>+		goto out;
> >>>>+	}
> >>>>	ret = kbd_set_state_safe(&new_state, &state);
> >>>>	if (ret)
> >>>>-		return ret;
> >>>>+		goto out;
> >>>>	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
> >>>>		kbd_previous_mode_bit = new_state.mode_bit;
> >>>>-	return count;
> >>>>+	ret = count;
> >>>>+out:
> >>>>+	mutex_unlock(&kbd_led_mutex);
> >>>>+	return ret;
> >>>>}
> >>>>
> >>>>static ssize_t kbd_led_triggers_show(struct device *dev,
> >>>>@@ -1749,12 +1770,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
> >>>>	if (ret)
> >>>>		return ret;
> >>>>
> >>>>+	mutex_lock(&kbd_led_mutex);
> >>>>+
> >>>>	ret = kbd_get_state(&state);
> >>>>	if (ret)
> >>>>-		return ret;
> >>>>+		goto out;
> >>>>
> >>>>-	if (enable == kbd_is_als_mode_bit(state.mode_bit))
> >>>>-		return count;
> >>>>+	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
> >>>>+		ret = count;
> >>>>+		goto out;
> >>>>+	}
> >>>>
> >>>>	new_state = state;
> >>>>
> >>>>@@ -1774,15 +1799,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
> >>>>			new_state.mode_bit = KBD_MODE_BIT_ON;
> >>>>		}
> >>>>	}
> >>>>-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> >>>>-		return -EINVAL;
> >>>>+	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
> >>>>+		ret = -EINVAL;
> >>>>+		goto out;
> >>>>+	}
> >>>>
> >>>>	ret = kbd_set_state_safe(&new_state, &state);
> >>>>	if (ret)
> >>>>-		return ret;
> >>>>+		goto out;
> >>>>	kbd_previous_mode_bit = new_state.mode_bit;
> >>>>
> >>>>-	return count;
> >>>>+	ret = count;
> >>>>+out:
> >>>>+	mutex_unlock(&kbd_led_mutex);
> >>>>+	return ret;
> >>>>}
> >>>>
> >>>>static ssize_t kbd_led_als_enabled_show(struct device *dev,
> >>>>@@ -1817,18 +1847,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
> >>>>	if (ret)
> >>>>		return ret;
> >>>>
> >>>>+	mutex_lock(&kbd_led_mutex);
> >>>>+
> >>>>	ret = kbd_get_state(&state);
> >>>>	if (ret)
> >>>>-		return ret;
> >>>>+		goto out;
> >>>>
> >>>>	new_state = state;
> >>>>	new_state.als_setting = setting;
> >>>>
> >>>>	ret = kbd_set_state_safe(&new_state, &state);
> >>>>	if (ret)
> >>>>-		return ret;
> >>>>+		goto out;
> >>>>
> >>>>-	return count;
> >>>>+	ret = count;
> >>>>+out:
> >>>>+	mutex_unlock(&kbd_led_mutex);
> >>>>+	return ret;
> >>>>}
> >>>>
> >>>>static ssize_t kbd_led_als_setting_show(struct device *dev,
> >>>>@@ -1913,27 +1948,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
> >>>>	u16 num;
> >>>>	int ret;
> >>>>
> >>>>+	mutex_lock(&kbd_led_mutex);
> >>>>+
> >>>>	if (kbd_get_max_level()) {
> >>>>		ret = kbd_get_state(&state);
> >>>>		if (ret)
> >>>>-			return ret;
> >>>>+			goto out;
> >>>>		new_state = state;
> >>>>		ret = kbd_set_level(&new_state, value);
> >>>>		if (ret)
> >>>>-			return ret;
> >>>>-		return kbd_set_state_safe(&new_state, &state);
> >>>>-	}
> >>>>-
> >>>>-	if (kbd_get_valid_token_counts()) {
> >>>>+			goto out;
> >>>>+		ret = kbd_set_state_safe(&new_state, &state);
> >>>>+	} else if (kbd_get_valid_token_counts()) {
> >>>>		for (num = kbd_token_bits; num != 0 && value > 0; --value)
> >>>>			num &= num - 1; /* clear the first bit set */
> >>>>		if (num == 0)
> >>>>-			return 0;
> >>>>-		return kbd_set_token_bit(ffs(num) - 1);
> >>>>+			ret = 0;
> >>>>+		else
> >>>>+			ret = kbd_set_token_bit(ffs(num) - 1);
> >>>>+	} else {
> >>>>+		pr_warn("Keyboard brightness level control not supported\n");
> >>>>+		ret = -ENXIO;
> >>>>	}
> >>>>
> >>>>-	pr_warn("Keyboard brightness level control not supported\n");
> >>>>-	return -ENXIO;
> >>>>+out:
> >>>>+	mutex_unlock(&kbd_led_mutex);
> >>>>+	return ret;
> >>>>}
> >>>>
> >>>>static struct led_classdev kbd_led = {
> >>>
> >

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 21-02-17 15:50, Pali Rohár wrote:
> On Tuesday 21 February 2017 15:40:16 Hans de Goede wrote:
>> Hi,
>>
>> On 21-02-17 15:11, Pali Rohár wrote:
>>> On Thursday 09 February 2017 16:44:17 Hans de Goede wrote:
>>>> Make dell-wmi notify on hotkey kbd brightness changes, listen for this
>>>> in dell-laptop and call led_classdev_notify_brightness_hw_changed.
>>>>
>>>> This will allow userspace to monitor (poll) for brightness changes on
>>>> these LEDs caused by the hotkey.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> Changes in v2:
>>>> -Use the new dell_smbios*notify functionality
>>>> Changes in v3:
>>>> -Simplify the if condition for calling led_notify_brightness_change
>>>> Changes in v4:
>>>> -Adjust for dell_smbios_*_notifier to dell_laptop_*_notifier rename
>>>> Changes in v5:
>>>> -Switch to new led-trigger based API for notifying userspace about
>>>> keyboard backlight brightness changes.
>>>> Changes in v6:
>>>> -Switch to new brightness_hw_changed LED class API
>>>> Changes in v8:
>>>> -Cache last set brightness value and only call
>>>> led_classdev_notify_brightness_hw_changed if the brightness has changed,
>>>> this is necessary because the WMI events get triggered when we set the
>>>> brightness ourselves too
>>>
>>> NAK. This does not work. Brightness can and already is handled also by
>>> userspace application from Dell libsmbios package. Not every brightness
>>> change is going via kernel driver and so new variable kbd_led_brightness
>>> contains just random value.
>>
>> This shows why it is a bad idea to have userspace code directly poking the
>> hardware. There is a reason why we've been moving away from the xserver
>> directly poking gpu-s to doing everything in the kernel.
>
> I agree.
>
>> To me libsmbios is a relic, something of ages long gone by and a normal
>> user should never use it.
>
> Do not remember that libsmbios was there first and kernel code is
> reimplementation from that userspace. Without libsmbios no kernel
> support would be there.
>
> And we can expect in future if Dell again decide to release some SMBIOS
> code it will be in libsmbios...
>
>> You're right that it can be used to change things underneath the kernel,
>> but as said normally a user should never do that.
>
> I used it and often, because kernel does not have support for keyboard
> backlight. And other people probably too.
>
> So if we agree or not, we need to deal with fact that userspace can and
> it is already calling smbios API. And started it doing earlier as kernel.
>
>> What will happen if a user does that regardless is that an acpi event
>> will trigger and the following code will execute:
>>
>> +		mutex_lock(&kbd_led_mutex);
>> +
>> +		brightness = kbd_led_level_get(&kbd_led);
>> +		if (kbd_led_brightness != brightness) {
>> +			kbd_led_brightness = brightness;
>> +			led_classdev_notify_brightness_hw_changed(&kbd_led,
>> +								  brightness);
>> +		}
>> +
>> +		mutex_unlock(&kbd_led_mutex);
>>
>> After which the kbd_led_brightness will be in sync again, so basically
>> the only side effect of the user changing the keyboard brightness
>> through libsmbios will be led_classdev_notify_brightness_hw_changed()
>> getting called, which is not unsurprising if you change something outside
>> of the kernel, so I really see no problem here.
>>
>> Currently the only listener for led_classdev_notify_brightness_hw_changed()
>> is the gnome3 desktop stack, and having the keyboard brightness
>> slider in the control panel update when setting the brightness outside
>> of the normal ways actually is a good thing.
>
> So do we really need this code which prevents update?

Yes, because the ABI specification for the new brightness_hw_changed says
that poll() listeners will only be woken up if the brightness is changed
outside of the kernel and not when the kernel changes it itself.

Regards,

Hans



>
>> Regards,
>>
>> Hans
>>
>>>> ---
>>>> drivers/platform/x86/dell-laptop.c | 47 +++++++++++++++++++++++++++++++++++++-
>>>> drivers/platform/x86/dell-wmi.c    | 14 ++++++++----
>>>> 2 files changed, 55 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
>>>> index 70951f3..f30938b 100644
>>>> --- a/drivers/platform/x86/dell-laptop.c
>>>> +++ b/drivers/platform/x86/dell-laptop.c
>>>> @@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
>>>> static u8 kbd_previous_mode_bit;
>>>>
>>>> static bool kbd_led_present;
>>>> +static enum led_brightness kbd_led_brightness;
>>>> static DEFINE_MUTEX(kbd_led_mutex);
>>>>
>>>> /*
>>>> @@ -1943,6 +1944,7 @@ static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
>>>> static int kbd_led_level_set(struct led_classdev *led_cdev,
>>>> 			     enum led_brightness value)
>>>> {
>>>> +	enum led_brightness new_brightness = value;
>>>> 	struct kbd_state state;
>>>> 	struct kbd_state new_state;
>>>> 	u16 num;
>>>> @@ -1971,6 +1973,8 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>>>> 		ret = -ENXIO;
>>>> 	}
>>>>
>>>> +	if (ret == 0)
>>>> +		kbd_led_brightness = new_brightness;
>>>> out:
>>>> 	mutex_unlock(&kbd_led_mutex);
>>>> 	return ret;
>>>> @@ -1978,6 +1982,7 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
>>>>
>>>> static struct led_classdev kbd_led = {
>>>> 	.name           = "dell::kbd_backlight",
>>>> +	.flags		= LED_BRIGHT_HW_CHANGED,
>>>> 	.brightness_set_blocking = kbd_led_level_set,
>>>> 	.brightness_get = kbd_led_level_get,
>>>> 	.groups         = kbd_led_groups,
>>>> @@ -1985,6 +1990,8 @@ static struct led_classdev kbd_led = {
>>>>
>>>> static int __init kbd_led_init(struct device *dev)
>>>> {
>>>> +	int ret;
>>>> +
>>>> 	kbd_init();
>>>> 	if (!kbd_led_present)
>>>> 		return -ENODEV;
>>>> @@ -1996,7 +2003,12 @@ static int __init kbd_led_init(struct device *dev)
>>>> 		if (kbd_led.max_brightness)
>>>> 			kbd_led.max_brightness--;
>>>> 	}
>>>> -	return led_classdev_register(dev, &kbd_led);
>>>> +	kbd_led_brightness = kbd_led_level_get(&kbd_led);
>>>> +	ret = led_classdev_register(dev, &kbd_led);
>>>> +	if (ret)
>>>> +		kbd_led_present = false;
>>>> +
>>>> +	return ret;
>>>> }
>>>>
>>>> static void brightness_set_exit(struct led_classdev *led_cdev,
>>>> @@ -2013,6 +2025,36 @@ static void kbd_led_exit(void)
>>>> 	led_classdev_unregister(&kbd_led);
>>>> }
>>>>
>>>> +static int dell_laptop_notifier_call(struct notifier_block *nb,
>>>> +				     unsigned long action, void *data)
>>>> +{
>>>> +	enum led_brightness brightness;
>>>> +
>>>> +	switch (action) {
>>>> +	case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
>>>> +		if (!kbd_led_present)
>>>> +			break;
>>>> +
>>>> +		mutex_lock(&kbd_led_mutex);
>>>> +
>>>> +		brightness = kbd_led_level_get(&kbd_led);
>>>> +		if (kbd_led_brightness != brightness) {
>>>> +			kbd_led_brightness = brightness;
>>>> +			led_classdev_notify_brightness_hw_changed(&kbd_led,
>>>> +								  brightness);
>>>> +		}
>>>> +
>>>> +		mutex_unlock(&kbd_led_mutex);
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return NOTIFY_OK;
>>>> +}
>>>> +
>>>> +static struct notifier_block dell_laptop_notifier = {
>>>> +	.notifier_call = dell_laptop_notifier_call,
>>>> +};
>>>> +
>>>> static int __init dell_init(void)
>>>> {
>>>> 	struct calling_interface_buffer *buffer;
>>>> @@ -2056,6 +2098,8 @@ static int __init dell_init(void)
>>>> 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
>>>> 				    &dell_debugfs_fops);
>>>>
>>>> +	dell_laptop_register_notifier(&dell_laptop_notifier);
>>>> +
>>>> 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>>>> 		return 0;
>>>>
>>>> @@ -2107,6 +2151,7 @@ static int __init dell_init(void)
>>>>
>>>> static void __exit dell_exit(void)
>>>> {
>>>> +	dell_laptop_unregister_notifier(&dell_laptop_notifier);
>>>> 	debugfs_remove_recursive(dell_laptop_dir);
>>>> 	if (quirks && quirks->touchpad_led)
>>>> 		touchpad_led_exit();
>>>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>>>> index 75e6370..15740b5 100644
>>>> --- a/drivers/platform/x86/dell-wmi.c
>>>> +++ b/drivers/platform/x86/dell-wmi.c
>>>> @@ -297,11 +297,11 @@ static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
>>>> 	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
>>>>
>>>> 	/* Keyboard backlight level changed */
>>>> -	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
>>>> -	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
>>>> -	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
>>>> -	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
>>>> -	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
>>>> +	{ KE_IGNORE, 0x01e1, { KEY_KBDILLUMTOGGLE } },
>>>> +	{ KE_IGNORE, 0x02ea, { KEY_KBDILLUMTOGGLE } },
>>>> +	{ KE_IGNORE, 0x02eb, { KEY_KBDILLUMTOGGLE } },
>>>> +	{ KE_IGNORE, 0x02ec, { KEY_KBDILLUMTOGGLE } },
>>>> +	{ KE_IGNORE, 0x02f6, { KEY_KBDILLUMTOGGLE } },
>>>> };
>>>>
>>>> static struct input_dev *dell_wmi_input_dev;
>>>> @@ -329,6 +329,10 @@ static void dell_wmi_process_key(int type, int code)
>>>> 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
>>>> 		return;
>>>>
>>>> +	if (key->keycode == KEY_KBDILLUMTOGGLE)
>>>> +		dell_laptop_call_notifier(
>>>> +			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
>>>> +
>>>> 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>>>> }
>>>>
>>>
>

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
> >So do we really need this code which prevents update?
> 
> Yes, because the ABI specification for the new brightness_hw_changed says
> that poll() listeners will only be woken up if the brightness is changed
> outside of the kernel and not when the kernel changes it itself.

So in case there are two applications in userspace which want to monitor
brightness change and both of those application could change brightness
(via sysfs) then these two applications would not know about every
brightness change and would be out-of-sync of reality what is really
configured by kernel.

This is one part which I did not liked in proposed ABI as it force
userspace to choose and use only one brightness monitoring application
at same time.

Plus if ABI was specified that poll() could be used only for hardware
change (and not by software e.g. kernel/userspace) then such
functionality is not possible to implement for Dell machines. As it
looks like Dell firmware send event about every change of brightness and
we cannot distinguish if change was done by software (kernel) or by
hardware itself.

I do not know now if you already accepted that ABI, I'm just saying that
I do not like it due to requirement for one userspace application as
listener and also because it is not what can be used for Dell machines.

Jacek, was this requirement in ABI already accepted into stable?

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 21-02-17 16:13, Pali Rohár wrote:
> On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
>>> So do we really need this code which prevents update?
>>
>> Yes, because the ABI specification for the new brightness_hw_changed says
>> that poll() listeners will only be woken up if the brightness is changed
>> outside of the kernel and not when the kernel changes it itself.
>
> So in case there are two applications in userspace which want to monitor
> brightness change and both of those application could change brightness
> (via sysfs) then these two applications would not know about every
> brightness change and would be out-of-sync of reality what is really
> configured by kernel.

Yes, because with triggers and blinking etc. it is impossible for
userspace to continuously monitor brigthness in a way which does not
cause a high system load.

> This is one part which I did not liked in proposed ABI as it force
> userspace to choose and use only one brightness monitoring application
> at same time.

Oh please, we are not going to have this whole ABI discussion again.
I already spend months on this, this ship has long sailed.

> Plus if ABI was specified that poll() could be used only for hardware
> change (and not by software e.g. kernel/userspace) then such
> functionality is not possible to implement for Dell machines. As it
> looks like Dell firmware send event about every change of brightness and
> we cannot distinguish if change was done by software (kernel) or by
> hardware itself.

It is possible just fine on Dell machines, except that libsmbios changes
will get seen as hardware triggered changes, which in a way they are
as libsmbios is making hardware changes behind the kernels back.

> I do not know now if you already accepted that ABI

Yes the ABI is in current next, and given that the 4.11 merge-window has
opened this ship has really sailed, also you're making a problem where
there really is none. Now can we please move forward with this ?

Regards,

Hans

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote:
> Hi,
> 
> On 21-02-17 16:13, Pali Rohár wrote:
> >On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
> >>>So do we really need this code which prevents update?
> >>
> >>Yes, because the ABI specification for the new brightness_hw_changed says
> >>that poll() listeners will only be woken up if the brightness is changed
> >>outside of the kernel and not when the kernel changes it itself.
> >
> >So in case there are two applications in userspace which want to monitor
> >brightness change and both of those application could change brightness
> >(via sysfs) then these two applications would not know about every
> >brightness change and would be out-of-sync of reality what is really
> >configured by kernel.
> 
> Yes, because with triggers and blinking etc. it is impossible for
> userspace to continuously monitor brigthness in a way which does not
> cause a high system load.

Triggers and blinking features are out due to high cpu load. Fine.

But why also manual writes to /sys/class/leds/... by userspace
applications is filtered and not reported via poll()?

> >This is one part which I did not liked in proposed ABI as it force
> >userspace to choose and use only one brightness monitoring application
> >at same time.
> 
> Oh please, we are not going to have this whole ABI discussion again.
> I already spend months on this, this ship has long sailed.

Ok, I just want to know reason for above one question.

> >Plus if ABI was specified that poll() could be used only for hardware
> >change (and not by software e.g. kernel/userspace) then such
> >functionality is not possible to implement for Dell machines. As it
> >looks like Dell firmware send event about every change of brightness and
> >we cannot distinguish if change was done by software (kernel) or by
> >hardware itself.
> 
> It is possible just fine on Dell machines, except that libsmbios changes
> will get seen as hardware triggered changes, which in a way they are
> as libsmbios is making hardware changes behind the kernels back.

Tool smbios-keyboard-ctl is valid and will be there. If you like it or
not we need to deal with fact that libsmbios exists and is used...

Due to fundamental reasons we ignore all race condition between
libsmbios and kernel (they are basically not possible to solve). I'm
fine with this.

But why should setting keyboard backlight via smbios-keyboard-ctl and
via echo > /sys/class/leds/ behave differently?

Only just because we say that some userspace application should not be
used and because we invented new kernel ABI which is not fully designed
for this case for existing Dell machines?

For me it looks like a bad design somewhere...

And also I do not like implementation in this dell driver that for every
one received event we need to query smbios with another SMM call to get
current brightness level. And after few seconds userspace ask again for
current level (because it received event that brightness was changed)
and we need to do another SMM call.

> >I do not know now if you already accepted that ABI
> 
> Yes the ABI is in current next, and given that the 4.11 merge-window has
> opened this ship has really sailed, also you're making a problem where
> there really is none. Now can we please move forward with this ?
> 
> Regards,
> 
> Hans

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Jacek Anaszewski]

On 02/21/2017 04:13 PM, Pali Rohár wrote:
> On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
>>> So do we really need this code which prevents update?
>>
>> Yes, because the ABI specification for the new brightness_hw_changed says
>> that poll() listeners will only be woken up if the brightness is changed
>> outside of the kernel and not when the kernel changes it itself.
> 
> So in case there are two applications in userspace which want to monitor
> brightness change and both of those application could change brightness
> (via sysfs) then these two applications would not know about every
> brightness change and would be out-of-sync of reality what is really
> configured by kernel.
> 
> This is one part which I did not liked in proposed ABI as it force
> userspace to choose and use only one brightness monitoring application
> at same time.
> 
> Plus if ABI was specified that poll() could be used only for hardware
> change (and not by software e.g. kernel/userspace) then such
> functionality is not possible to implement for Dell machines. As it
> looks like Dell firmware send event about every change of brightness and
> we cannot distinguish if change was done by software (kernel) or by
> hardware itself.
> 
> I do not know now if you already accepted that ABI, I'm just saying that
> I do not like it due to requirement for one userspace application as
> listener and also because it is not what can be used for Dell machines.
> 
> Jacek, was this requirement in ABI already accepted into stable?

Yes, Linus today pulled that and it is in mainline now. From LED
subsystem point of view I don't see any problem - the file is called
brightness_hw_changed and its purpose is to report brightness changes
occurred in the hardware, outside of kernel control. It also returns
last brightness as was set by the hardware, which can be different from
what old brightness file reports in the same time.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 21-02-17 18:08, Pali Rohár wrote:
> On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote:
>> Hi,
>>
>> On 21-02-17 16:13, Pali Rohár wrote:
>>> On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
>>>>> So do we really need this code which prevents update?
>>>>
>>>> Yes, because the ABI specification for the new brightness_hw_changed says
>>>> that poll() listeners will only be woken up if the brightness is changed
>>>> outside of the kernel and not when the kernel changes it itself.
>>>
>>> So in case there are two applications in userspace which want to monitor
>>> brightness change and both of those application could change brightness
>>> (via sysfs) then these two applications would not know about every
>>> brightness change and would be out-of-sync of reality what is really
>>> configured by kernel.
>>
>> Yes, because with triggers and blinking etc. it is impossible for
>> userspace to continuously monitor brigthness in a way which does not
>> cause a high system load.
>
> Triggers and blinking features are out due to high cpu load. Fine.
>
> But why also manual writes to /sys/class/leds/... by userspace
> applications is filtered and not reported via poll()?

I agree that having a way for interested userspace to detect those
would be good, but that would need to be another API, may poll()
on the brightness attribute itself while excluding triggers / blinking
changes from wakeup ?

Anyways that is something to discuss in a thread specific to the
LED subsystem and somewhat orthogonal to this patch.

One disadvantage of poll() is that it does not give the source of
the change, so in retrospect I actually like the new brightness_hw_changed
attribute as that does give the source, which is something which we need
to know in userspace. In previous versions of the ABI I had to do
the same brightness comparison I'm doing in the dell-laptop driver
now in userspace, where it can never be done safely as userspace does
not know about other userspace.

Since the Dell smbios events don't provide us with a source of the
change, we need to compare the brightness to the last set brightness
somewhere and IMHO the kernel is the best (least bad) place to do
that.

> Due to fundamental reasons we ignore all race condition between
> libsmbios and kernel (they are basically not possible to solve). I'm
> fine with this.
>
> But why should setting keyboard backlight via smbios-keyboard-ctl and
> via echo > /sys/class/leds/ behave differently?

Because we cannot solve the smbios-keyboard-ctl case, but we can solve
the echo case, as said we could probably use a new kernel ABI to allow
userspace to detect changes caused by the echo example, but that
really is a whole new discussion.

Regards,

Hans

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote:
> Hi,
> 
> On 21-02-17 18:08, Pali Rohár wrote:
> >On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote:
> >>Hi,
> >>
> >>On 21-02-17 16:13, Pali Rohár wrote:
> >>>On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
> >>>>>So do we really need this code which prevents update?
> >>>>
> >>>>Yes, because the ABI specification for the new brightness_hw_changed says
> >>>>that poll() listeners will only be woken up if the brightness is changed
> >>>>outside of the kernel and not when the kernel changes it itself.
> >>>
> >>>So in case there are two applications in userspace which want to monitor
> >>>brightness change and both of those application could change brightness
> >>>(via sysfs) then these two applications would not know about every
> >>>brightness change and would be out-of-sync of reality what is really
> >>>configured by kernel.
> >>
> >>Yes, because with triggers and blinking etc. it is impossible for
> >>userspace to continuously monitor brigthness in a way which does not
> >>cause a high system load.
> >
> >Triggers and blinking features are out due to high cpu load. Fine.
> >
> >But why also manual writes to /sys/class/leds/... by userspace
> >applications is filtered and not reported via poll()?
> 
> I agree that having a way for interested userspace to detect those
> would be good, but that would need to be another API, may poll()
> on the brightness attribute itself while excluding triggers / blinking
> changes from wakeup ?
> 
> Anyways that is something to discuss in a thread specific to the
> LED subsystem and somewhat orthogonal to this patch.

Ok, lets start discussion about it in new separate thread. I was in
impression that this was already part of discussion and in proposed ABI.
Now I see that it was just in original cpu consuming ABI which was
rejected.

> One disadvantage of poll() is that it does not give the source of
> the change, so in retrospect I actually like the new brightness_hw_changed
> attribute as that does give the source, which is something which we need
> to know in userspace.

Do you really in userspace need to know source of change? And to
distinguish between change from hardware and change from userspace done
by echo > /sys/class/leds?

I though that this is for informing userspace application that led
status was changed and application should update some bar or number
which show current state of backlight level.

> In previous versions of the ABI I had to do
> the same brightness comparison I'm doing in the dell-laptop driver
> now in userspace, where it can never be done safely as userspace does
> not know about other userspace.
> 
> Since the Dell smbios events don't provide us with a source of the
> change, we need to compare the brightness to the last set brightness
> somewhere and IMHO the kernel is the best (least bad) place to do
> that.

Maybe kernel place is the least bad place, but still it is unreliable
for Dell machines.

You do not know latency and delay how long it will take Dell firmware to
deliver event to kernel. It also implies that there is race condition
between delivering event and reading new backlight level. Basically it
is different and similar race condition as you are fixing by another
patch in this series.

> >Due to fundamental reasons we ignore all race condition between
> >libsmbios and kernel (they are basically not possible to solve). I'm
> >fine with this.
> >
> >But why should setting keyboard backlight via smbios-keyboard-ctl and
> >via echo > /sys/class/leds/ behave differently?
> 
> Because we cannot solve the smbios-keyboard-ctl case, but we can solve
> the echo case, as said we could probably use a new kernel ABI to allow
> userspace to detect changes caused by the echo example, but that
> really is a whole new discussion.
> 
> Regards,
> 
> Hans
> 

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

HI,

On 22-02-17 09:49, Pali Rohár wrote:
> On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote:
>> Hi,
>>
>> On 21-02-17 18:08, Pali Rohár wrote:
>>> On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 21-02-17 16:13, Pali Rohár wrote:
>>>>> On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
>>>>>>> So do we really need this code which prevents update?
>>>>>>
>>>>>> Yes, because the ABI specification for the new brightness_hw_changed says
>>>>>> that poll() listeners will only be woken up if the brightness is changed
>>>>>> outside of the kernel and not when the kernel changes it itself.
>>>>>
>>>>> So in case there are two applications in userspace which want to monitor
>>>>> brightness change and both of those application could change brightness
>>>>> (via sysfs) then these two applications would not know about every
>>>>> brightness change and would be out-of-sync of reality what is really
>>>>> configured by kernel.
>>>>
>>>> Yes, because with triggers and blinking etc. it is impossible for
>>>> userspace to continuously monitor brigthness in a way which does not
>>>> cause a high system load.
>>>
>>> Triggers and blinking features are out due to high cpu load. Fine.
>>>
>>> But why also manual writes to /sys/class/leds/... by userspace
>>> applications is filtered and not reported via poll()?
>>
>> I agree that having a way for interested userspace to detect those
>> would be good, but that would need to be another API, may poll()
>> on the brightness attribute itself while excluding triggers / blinking
>> changes from wakeup ?
>>
>> Anyways that is something to discuss in a thread specific to the
>> LED subsystem and somewhat orthogonal to this patch.
>
> Ok, lets start discussion about it in new separate thread. I was in
> impression that this was already part of discussion and in proposed ABI.
> Now I see that it was just in original cpu consuming ABI which was
> rejected.
>
>> One disadvantage of poll() is that it does not give the source of
>> the change, so in retrospect I actually like the new brightness_hw_changed
>> attribute as that does give the source, which is something which we need
>> to know in userspace.
>
> Do you really in userspace need to know source of change? And to
> distinguish between change from hardware and change from userspace done
> by echo > /sys/class/leds?

Yes, a change done through hardware (through the firmware handled
hotkeys) will make the desktop environment show an osd overlay
with the new kbd backlight brightness, where as a change done
through e.g. the brightness slider in gnome-control-panel should
not show that same osd.

> I though that this is for informing userspace application that led
> status was changed and application should update some bar or number
> which show current state of backlight level.

That is only part of it, we also want the OSD but ONLY when changed
through the hotkeys (on other models the hotkeys are handled in
userspace software, which will then also show the OSD, we want this
to be consistent whether the keys are handled in firmware or in
userspace).

>> In previous versions of the ABI I had to do
>> the same brightness comparison I'm doing in the dell-laptop driver
>> now in userspace, where it can never be done safely as userspace does
>> not know about other userspace.
>>
>> Since the Dell smbios events don't provide us with a source of the
>> change, we need to compare the brightness to the last set brightness
>> somewhere and IMHO the kernel is the best (least bad) place to do
>> that.
>
> Maybe kernel place is the least bad place, but still it is unreliable
> for Dell machines.
>
> You do not know latency and delay how long it will take Dell firmware to
> deliver event to kernel. It also implies that there is race condition
> between delivering event and reading new backlight level.

A race condition which will always exists if userspace polls the backlight
periodically say once a minute then it can always end up polling just
before the hotkey press is done. Which is exactly why we need the event,
so we don't need to poll  and when the event is delivered we know the new
backlight level.

Regards,

Hans

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Wednesday 22 February 2017 11:24:13 Hans de Goede wrote:
> HI,
> 
> On 22-02-17 09:49, Pali Rohár wrote:
> >On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote:
> >>Hi,
> >>
> >>On 21-02-17 18:08, Pali Rohár wrote:
> >>>On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote:
> >>>>Hi,
> >>>>
> >>>>On 21-02-17 16:13, Pali Rohár wrote:
> >>>>>On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
> >>>>>>>So do we really need this code which prevents update?
> >>>>>>
> >>>>>>Yes, because the ABI specification for the new brightness_hw_changed says
> >>>>>>that poll() listeners will only be woken up if the brightness is changed
> >>>>>>outside of the kernel and not when the kernel changes it itself.
> >>>>>
> >>>>>So in case there are two applications in userspace which want to monitor
> >>>>>brightness change and both of those application could change brightness
> >>>>>(via sysfs) then these two applications would not know about every
> >>>>>brightness change and would be out-of-sync of reality what is really
> >>>>>configured by kernel.
> >>>>
> >>>>Yes, because with triggers and blinking etc. it is impossible for
> >>>>userspace to continuously monitor brigthness in a way which does not
> >>>>cause a high system load.
> >>>
> >>>Triggers and blinking features are out due to high cpu load. Fine.
> >>>
> >>>But why also manual writes to /sys/class/leds/... by userspace
> >>>applications is filtered and not reported via poll()?
> >>
> >>I agree that having a way for interested userspace to detect those
> >>would be good, but that would need to be another API, may poll()
> >>on the brightness attribute itself while excluding triggers / blinking
> >>changes from wakeup ?
> >>
> >>Anyways that is something to discuss in a thread specific to the
> >>LED subsystem and somewhat orthogonal to this patch.
> >
> >Ok, lets start discussion about it in new separate thread. I was in
> >impression that this was already part of discussion and in proposed ABI.
> >Now I see that it was just in original cpu consuming ABI which was
> >rejected.
> >
> >>One disadvantage of poll() is that it does not give the source of
> >>the change, so in retrospect I actually like the new brightness_hw_changed
> >>attribute as that does give the source, which is something which we need
> >>to know in userspace.
> >
> >Do you really in userspace need to know source of change? And to
> >distinguish between change from hardware and change from userspace done
> >by echo > /sys/class/leds?
> 
> Yes, a change done through hardware (through the firmware handled
> hotkeys) will make the desktop environment show an osd overlay
> with the new kbd backlight brightness, where as a change done
> through e.g. the brightness slider in gnome-control-panel should
> not show that same osd.
> 
> >I though that this is for informing userspace application that led
> >status was changed and application should update some bar or number
> >which show current state of backlight level.
> 
> That is only part of it, we also want the OSD but ONLY when changed
> through the hotkeys (on other models the hotkeys are handled in
> userspace software, which will then also show the OSD, we want this
> to be consistent whether the keys are handled in firmware or in
> userspace).

Ok, so userspace application wants to distinguish between these types of
events:

1) autonomous hardware decided to change brightness
2) application itself changed brightness
3) other application changed brightness
4) pressed keyboard hotkey changed brightness
  4.1) managed by firmware
  4.2) managed by some application

And for 4) and 1) you want to show notification to user, right?

> >>In previous versions of the ABI I had to do
> >>the same brightness comparison I'm doing in the dell-laptop driver
> >>now in userspace, where it can never be done safely as userspace does
> >>not know about other userspace.
> >>
> >>Since the Dell smbios events don't provide us with a source of the
> >>change, we need to compare the brightness to the last set brightness
> >>somewhere and IMHO the kernel is the best (least bad) place to do
> >>that.
> >
> >Maybe kernel place is the least bad place, but still it is unreliable
> >for Dell machines.
> >
> >You do not know latency and delay how long it will take Dell firmware to
> >deliver event to kernel. It also implies that there is race condition
> >between delivering event and reading new backlight level.
> 
> A race condition which will always exists if userspace polls the backlight
> periodically say once a minute then it can always end up polling just
> before the hotkey press is done. Which is exactly why we need the event,
> so we don't need to poll  and when the event is delivered we know the new
> backlight level.

So instead userspace polling you will ask for backlight level after
receiving firmware event. This is same race as in userspace. Returned
backlight level does not have to be one which caused firmware event. If
firmware delays that event (and due to ACPI slowness it can be really
delayed) then another backlight change could happen... And you will just
use incorrect level for comparison.

Basically on Dell machine you cannot distinguish between events done by
1), 2), 3) and 4). You are trying to hide this fact by bringing some
logic which on first look can fix it...

Also calling another SMBIOS call via SMM for every received event (even
if userspace is not interested in it) does not look like good
implementation. And another one is issued after userspace receive that
event (via poll()) and try to know current value.

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 22-02-17 13:01, Pali Rohár wrote:
> On Wednesday 22 February 2017 11:24:13 Hans de Goede wrote:
>> HI,
>>
>> On 22-02-17 09:49, Pali Rohár wrote:
>>> On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 21-02-17 18:08, Pali Rohár wrote:
>>>>> On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 21-02-17 16:13, Pali Rohár wrote:
>>>>>>> On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
>>>>>>>>> So do we really need this code which prevents update?
>>>>>>>>
>>>>>>>> Yes, because the ABI specification for the new brightness_hw_changed says
>>>>>>>> that poll() listeners will only be woken up if the brightness is changed
>>>>>>>> outside of the kernel and not when the kernel changes it itself.
>>>>>>>
>>>>>>> So in case there are two applications in userspace which want to monitor
>>>>>>> brightness change and both of those application could change brightness
>>>>>>> (via sysfs) then these two applications would not know about every
>>>>>>> brightness change and would be out-of-sync of reality what is really
>>>>>>> configured by kernel.
>>>>>>
>>>>>> Yes, because with triggers and blinking etc. it is impossible for
>>>>>> userspace to continuously monitor brigthness in a way which does not
>>>>>> cause a high system load.
>>>>>
>>>>> Triggers and blinking features are out due to high cpu load. Fine.
>>>>>
>>>>> But why also manual writes to /sys/class/leds/... by userspace
>>>>> applications is filtered and not reported via poll()?
>>>>
>>>> I agree that having a way for interested userspace to detect those
>>>> would be good, but that would need to be another API, may poll()
>>>> on the brightness attribute itself while excluding triggers / blinking
>>>> changes from wakeup ?
>>>>
>>>> Anyways that is something to discuss in a thread specific to the
>>>> LED subsystem and somewhat orthogonal to this patch.
>>>
>>> Ok, lets start discussion about it in new separate thread. I was in
>>> impression that this was already part of discussion and in proposed ABI.
>>> Now I see that it was just in original cpu consuming ABI which was
>>> rejected.
>>>
>>>> One disadvantage of poll() is that it does not give the source of
>>>> the change, so in retrospect I actually like the new brightness_hw_changed
>>>> attribute as that does give the source, which is something which we need
>>>> to know in userspace.
>>>
>>> Do you really in userspace need to know source of change? And to
>>> distinguish between change from hardware and change from userspace done
>>> by echo > /sys/class/leds?
>>
>> Yes, a change done through hardware (through the firmware handled
>> hotkeys) will make the desktop environment show an osd overlay
>> with the new kbd backlight brightness, where as a change done
>> through e.g. the brightness slider in gnome-control-panel should
>> not show that same osd.
>>
>>> I though that this is for informing userspace application that led
>>> status was changed and application should update some bar or number
>>> which show current state of backlight level.
>>
>> That is only part of it, we also want the OSD but ONLY when changed
>> through the hotkeys (on other models the hotkeys are handled in
>> userspace software, which will then also show the OSD, we want this
>> to be consistent whether the keys are handled in firmware or in
>> userspace).
>
> Ok, so userspace application wants to distinguish between these types of
> events:
>
> 1) autonomous hardware decided to change brightness
> 2) application itself changed brightness
> 3) other application changed brightness
> 4) pressed keyboard hotkey changed brightness
>   4.1) managed by firmware
>   4.2) managed by some application
>
> And for 4) and 1) you want to show notification to user, right?

Yes.

>>>> In previous versions of the ABI I had to do
>>>> the same brightness comparison I'm doing in the dell-laptop driver
>>>> now in userspace, where it can never be done safely as userspace does
>>>> not know about other userspace.
>>>>
>>>> Since the Dell smbios events don't provide us with a source of the
>>>> change, we need to compare the brightness to the last set brightness
>>>> somewhere and IMHO the kernel is the best (least bad) place to do
>>>> that.
>>>
>>> Maybe kernel place is the least bad place, but still it is unreliable
>>> for Dell machines.
>>>
>>> You do not know latency and delay how long it will take Dell firmware to
>>> deliver event to kernel. It also implies that there is race condition
>>> between delivering event and reading new backlight level.
>>
>> A race condition which will always exists if userspace polls the backlight
>> periodically say once a minute then it can always end up polling just
>> before the hotkey press is done. Which is exactly why we need the event,
>> so we don't need to poll  and when the event is delivered we know the new
>> backlight level.
>
> So instead userspace polling you will ask for backlight level after
> receiving firmware event. This is same race as in userspace. Returned
> backlight level does not have to be one which caused firmware event.

1) These events do not happen 100s of times per second, they happen
almost never

2) This is the best we can do given the firmware interface we have

> If firmware delays that event (and due to ACPI slowness it can be really
> delayed) then another backlight change could happen... And you will just
> use incorrect level for comparison.

Again given the frequency of these events this is not really an issue.

> Basically on Dell machine you cannot distinguish between events done by
> 1), 2), 3) and 4). You are trying to hide this fact by bringing some
> logic which on first look can fix it...

Sure we can distuingish 2, 3 and 4.2 will always go through the kernel
and 1 and 4.1 will not, so we can just compare the last written value to
the value after the event, this works fine. There really is no problem here.

Granted using libsmbios to change things will cause the change to be
seen as an autonomous / firmware change rahter then done by userspace,
as you wrote yourself in a previous mail:

"Due to fundamental reasons we ignore all race condition between
libsmbios and kernel (they are basically not possible to solve). I'm
fine with this."

This is unsolvable, but the end result is that other userspace bits
(if present) may mistake the change for an autonomous change and
show the OSD, so the only bad side-effect is the OSD being shown
in that case. And I really doubt this will ever happen, either the
user uses something like GNOME and he will likely never use libsmbios
directly, or he runs some more DIY desktop environment and does
use smbiosctl in which case no one will be listening for the
autonomous changes.

> Also calling another SMBIOS call via SMM for every received event (even
> if userspace is not interested in it) does not look like good
> implementation. And another one is issued after userspace receive that
> event (via poll()) and try to know current value.

That is not true, the hw_brightness_changed attribute returns the last
brightness passed to led_classdev_notify_brightness_hw_changed(), this
is the whole reason why it has a brightness argument, this is actually
done to avoid races, so that we get the brightness asap after receiving
the event and then even if userspace takes it sweet time we actually
report the brightness resulting from the event and not some later
change (this is part of the ABI definition of brightness_hw_changed).

So no we do not end up doing 2 SMBIOS calls and even if we were
given the frequency of these events the performance impact would be
absolutely negligible.

You are really seeing a lot of problems where there are none,
remember Linus' motto: perfect is the enemy of good.

Now can we please move forward with this ?

Regards,

Hans

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote:
> 1) These events do not happen 100s of times per second, they happen
> almost never

100 events per second is probably not happening, but on my Latitude I
see that sometimes more events are delayed and delivered at once.

> 2) This is the best we can do given the firmware interface we have

What about just dropping upcoming one event in interval of next 2-3
second? Instead of trying to remember last state in kernel and then
trying to mach if received event was that which was caused by change?

This would allow us to reduce doing one SMM call for each received
event and I think it would be same reliable as your approach.

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

Re: [PATCH v8 1/7] platform/x86/thinkpad_acpi: Stop setting led_classdev brightness directly

[Pali Rohár]

On Thursday 09 February 2017 16:44:11 Hans de Goede wrote:
> There is no need to set the led_classdev's brightness value from
> its set_brightness callback, this is taken care of by the led-core and
> thinkpad_acpi really should not be mucking with it.
> 
> Note that kbdlight_set_level_and_update() is still used by the old
> thinpad_acpi specific sysfs interface for the led, so we cannot
> remove it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v8:
> -New patch in v8 of this patch-set
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index cacb43f..0680bb3 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5095,8 +5095,6 @@ static int kbdlight_set_level(int level)
>  	return 0;
>  }
>  
> -static int kbdlight_set_level_and_update(int level);
> -
>  static int kbdlight_get_level(void)
>  {
>  	int status = 0;
> @@ -5164,7 +5162,7 @@ static void kbdlight_set_worker(struct work_struct *work)
>  			container_of(work, struct tpacpi_led_classdev, work);
>  
>  	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
> -		kbdlight_set_level_and_update(data->new_state);
> +		kbdlight_set_level(data->new_state);
>  }
>  
>  static void kbdlight_sysfs_set(struct led_classdev *led_cdev,

Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save
kbdlight state on suspend and restore it on resume). It is really OK to
revert that change?

CCing Marco who is author of that commit.

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

Re: [PATCH v8 1/7] platform/x86/thinkpad_acpi: Stop setting led_classdev brightness directly

[Hans de Goede]

Hi,

On 01-03-17 12:27, Pali Rohár wrote:
> On Thursday 09 February 2017 16:44:11 Hans de Goede wrote:
>> There is no need to set the led_classdev's brightness value from
>> its set_brightness callback, this is taken care of by the led-core and
>> thinkpad_acpi really should not be mucking with it.
>>
>> Note that kbdlight_set_level_and_update() is still used by the old
>> thinpad_acpi specific sysfs interface for the led, so we cannot
>> remove it.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v8:
>> -New patch in v8 of this patch-set
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index cacb43f..0680bb3 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -5095,8 +5095,6 @@ static int kbdlight_set_level(int level)
>>  	return 0;
>>  }
>>
>> -static int kbdlight_set_level_and_update(int level);
>> -
>>  static int kbdlight_get_level(void)
>>  {
>>  	int status = 0;
>> @@ -5164,7 +5162,7 @@ static void kbdlight_set_worker(struct work_struct *work)
>>  			container_of(work, struct tpacpi_led_classdev, work);
>>
>>  	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
>> -		kbdlight_set_level_and_update(data->new_state);
>> +		kbdlight_set_level(data->new_state);
>>  }
>>
>>  static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
>
> Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save
> kbdlight state on suspend and restore it on resume). It is really OK to
> revert that change?

Yes, as explained on the commit msg, kbdlight_set_worker is only used
from ledclass callbacks and the led core already update led_cdev->brightness
when calling those.

Regards,

Hans

Re: [PATCH v8 1/7] platform/x86/thinkpad_acpi: Stop setting led_classdev brightness directly

[Pali Rohár]

On Wednesday 01 March 2017 12:57:42 Hans de Goede wrote:
> Hi,
> 
> On 01-03-17 12:27, Pali Rohár wrote:
> >On Thursday 09 February 2017 16:44:11 Hans de Goede wrote:
> >>There is no need to set the led_classdev's brightness value from
> >>its set_brightness callback, this is taken care of by the led-core and
> >>thinkpad_acpi really should not be mucking with it.
> >>
> >>Note that kbdlight_set_level_and_update() is still used by the old
> >>thinpad_acpi specific sysfs interface for the led, so we cannot
> >>remove it.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>Changes in v8:
> >>-New patch in v8 of this patch-set
> >>---
> >> drivers/platform/x86/thinkpad_acpi.c | 4 +---
> >> 1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> >>index cacb43f..0680bb3 100644
> >>--- a/drivers/platform/x86/thinkpad_acpi.c
> >>+++ b/drivers/platform/x86/thinkpad_acpi.c
> >>@@ -5095,8 +5095,6 @@ static int kbdlight_set_level(int level)
> >> 	return 0;
> >> }
> >>
> >>-static int kbdlight_set_level_and_update(int level);
> >>-
> >> static int kbdlight_get_level(void)
> >> {
> >> 	int status = 0;
> >>@@ -5164,7 +5162,7 @@ static void kbdlight_set_worker(struct work_struct *work)
> >> 			container_of(work, struct tpacpi_led_classdev, work);
> >>
> >> 	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
> >>-		kbdlight_set_level_and_update(data->new_state);
> >>+		kbdlight_set_level(data->new_state);
> >> }
> >>
> >> static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
> >
> >Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save
> >kbdlight state on suspend and restore it on resume). It is really OK to
> >revert that change?
> 
> Yes, as explained on the commit msg, kbdlight_set_worker is only used
> from ledclass callbacks and the led core already update led_cdev->brightness
> when calling those.

Ok, I wrote it because of adding Marco to discussion so he has chance to
comment this change -- as it is basically revert of his commit...

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 01-03-17 12:15, Pali Rohár wrote:
> On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote:
>> 1) These events do not happen 100s of times per second, they happen
>> almost never
>
> 100 events per second is probably not happening, but on my Latitude I
> see that sometimes more events are delayed and delivered at once.
>
>> 2) This is the best we can do given the firmware interface we have
>
> What about just dropping upcoming one event in interval of next 2-3
> second? Instead of trying to remember last state in kernel and then
> trying to mach if received event was that which was caused by change?

That is way more racy then the solution with the kernel remembering
the brightness. With my proposed solution there is basically no race,
the only downside is the driver seeing a brightness change caused by
libsmbios as one caused by the hotkeys, but it will do so 100%
reliably and that is something which I believe we can live with
just fine.

> This would allow us to reduce doing one SMM call for each received
> event and I think it would be same reliable as your approach.

As I explained *already* we already have only one SMM call for reach
received event, we pass the read-back brightness into
led_classdev_notify_brightness_hw_changed and when userspace
reads the new brightness_hw_changed event in response to the wakeup
the led-core will use the value passed through
led_classdev_notify_brightness_hw_changed so only 1 SMM call happens
per event.

Also again this does not happen 100 times per second you're really
trying to optimize something which does not need any optimization
at all here.

Regards,

Hans

Re: [PATCH v8 1/7] platform/x86/thinkpad_acpi: Stop setting led_classdev brightness directly

[Hans de Goede]

HI,

On 01-03-17 13:00, Pali Rohár wrote:
> On Wednesday 01 March 2017 12:57:42 Hans de Goede wrote:
>> Hi,
>>
>> On 01-03-17 12:27, Pali Rohár wrote:
>>> On Thursday 09 February 2017 16:44:11 Hans de Goede wrote:
>>>> There is no need to set the led_classdev's brightness value from
>>>> its set_brightness callback, this is taken care of by the led-core and
>>>> thinkpad_acpi really should not be mucking with it.
>>>>
>>>> Note that kbdlight_set_level_and_update() is still used by the old
>>>> thinpad_acpi specific sysfs interface for the led, so we cannot
>>>> remove it.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v8:
>>>> -New patch in v8 of this patch-set
>>>> ---
>>>> drivers/platform/x86/thinkpad_acpi.c | 4 +---
>>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>>> index cacb43f..0680bb3 100644
>>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>>> @@ -5095,8 +5095,6 @@ static int kbdlight_set_level(int level)
>>>> 	return 0;
>>>> }
>>>>
>>>> -static int kbdlight_set_level_and_update(int level);
>>>> -
>>>> static int kbdlight_get_level(void)
>>>> {
>>>> 	int status = 0;
>>>> @@ -5164,7 +5162,7 @@ static void kbdlight_set_worker(struct work_struct *work)
>>>> 			container_of(work, struct tpacpi_led_classdev, work);
>>>>
>>>> 	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
>>>> -		kbdlight_set_level_and_update(data->new_state);
>>>> +		kbdlight_set_level(data->new_state);
>>>> }
>>>>
>>>> static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
>>>
>>> Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save
>>> kbdlight state on suspend and restore it on resume). It is really OK to
>>> revert that change?
>>
>> Yes, as explained on the commit msg, kbdlight_set_worker is only used
>> from ledclass callbacks and the led core already update led_cdev->brightness
>> when calling those.
>
> Ok, I wrote it because of adding Marco to discussion so he has chance to
> comment this change -- as it is basically revert of his commit...

No it is not, it removes a one line change he made amongst many others,
the important parts of his commit are in other parts of it.

Regards,

Hans

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Wednesday 01 March 2017 13:02:58 Hans de Goede wrote:
> Hi,
> 
> On 01-03-17 12:15, Pali Rohár wrote:
> >On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote:
> >>1) These events do not happen 100s of times per second, they happen
> >>almost never
> >
> >100 events per second is probably not happening, but on my Latitude I
> >see that sometimes more events are delayed and delivered at once.
> >
> >>2) This is the best we can do given the firmware interface we have
> >
> >What about just dropping upcoming one event in interval of next 2-3
> >second? Instead of trying to remember last state in kernel and then
> >trying to mach if received event was that which was caused by change?
> 
> That is way more racy then the solution with the kernel remembering
> the brightness. With my proposed solution there is basically no race,

Is is racy. When you change keyboard backlight via LED sysfs and also in
that time kernel receive hardware event about changed backlight, kernel
do not know if that event comes as reaction to change via LED sysfs or
as autonomous firmware decision or as reaction to pressing key for
changing keyboard backlight (managed by firmware).

I'm not talking here about userspace libsmbios. All is just by
autonomous firmware, keyboard usage and LED sysfs API.

And how is my idea about dropping exactly one event for one request for
changing backlight via LED sysfs API more racy as yours?

Received event from firmware has only the timestamp, no more information
yet (*). Therefore state of events does is not ordered and any two
events are basically same.

In your provides patch you are adding additional information to event.
It is currently read brightness level in time when was event delivered,
not state of time when event was created. Which means you cannot swap
any two received events anymore as they are part of current driver
state.

In my idea I do not add any additional information to event. Which means
all received events are same in current driver state and I can swap
them. So I can decide to drop one event when driver is changing
backlight level. And because I can swap received events it does not
matter if I drop "correct" one (that which was really generated by my
backlight level change) or any other before or after (as they are same).
The only important is that kernel use correct number of firmware events.

Idea for time interval of 2-3 seconds is that if event is delivered
later as after 3 seconds it is probably useless as driver application
code can be already out-of-sync. But this time interval is just another
idea and we do not need to use it.

(*) Looks like for some machines in dell-wmi.c we know also which
brightness level was set by firmware, but it is not used now and we
still have machines without this information.

> the only downside is the driver seeing a brightness change caused by
> libsmbios as one caused by the hotkeys, but it will do so 100%
> reliably and that is something which I believe we can live with
> just fine.
> 
> >This would allow us to reduce doing one SMM call for each received
> >event and I think it would be same reliable as your approach.
> 
> As I explained *already* we already have only one SMM call for reach
> received event, we pass the read-back brightness into
> led_classdev_notify_brightness_hw_changed and when userspace
> reads the new brightness_hw_changed event in response to the wakeup
> the led-core will use the value passed through
> led_classdev_notify_brightness_hw_changed so only 1 SMM call happens
> per event.
> 
> Also again this does not happen 100 times per second you're really
> trying to optimize something which does not need any optimization
> at all here.

We have reports that on some Dell notebooks is issuing SMM call causing
freezing kernel for about 1-3 seconds (as firmware stay in SMM mode for
a longer time...).

> Regards,
> 
> Hans
> 

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 01-03-17 13:55, Pali Rohár wrote:
> On Wednesday 01 March 2017 13:02:58 Hans de Goede wrote:
>> Hi,
>>
>> On 01-03-17 12:15, Pali Rohár wrote:
>>> On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote:
>>>> 1) These events do not happen 100s of times per second, they happen
>>>> almost never
>>>
>>> 100 events per second is probably not happening, but on my Latitude I
>>> see that sometimes more events are delayed and delivered at once.
>>>
>>>> 2) This is the best we can do given the firmware interface we have
>>>
>>> What about just dropping upcoming one event in interval of next 2-3
>>> second? Instead of trying to remember last state in kernel and then
>>> trying to mach if received event was that which was caused by change?
>>
>> That is way more racy then the solution with the kernel remembering
>> the brightness. With my proposed solution there is basically no race,
>
> Is is racy. When you change keyboard backlight via LED sysfs and also in
> that time kernel receive hardware event about changed backlight, kernel
> do not know if that event comes as reaction to change via LED sysfs or
> as autonomous firmware decision or as reaction to pressing key for
> changing keyboard backlight (managed by firmware).

In this case there are only a limited number of scenarios:

1. The hotkey event gets the mutex before the sysfs write
In this case there is no race as the event is processed before the
sysfs write can be proceed.

2. The sysfs write gets the mutex before the hotkey event:
There are 2 sub scenarios here:

2a) The sysfs writes the same value as the hotkey press just set:
In this case the cached brightness will get updated to this new
value before processing either event can be processed, when both
events get processed they will fail the new_brightness != brightness
check and no event will be generated. Arguably this is a bug but
I believe if the user just set the brightness to the same value,
making the hotkey change a nop that this behavior is fine. The sysfs
write won the race after all, so from a kernel pov it really was
first and the hotkey event really is a nop.

2b) The sysfs write writes a different value then the hotkey,
in this case a question becomes which of the 2 we actually get,
but this is up to the BIOS not up to the kernel, basically this
depends on who won the race from the BIOS pov. If the sysfs
write was seen last from the BIOS pov then our SMM call to get
the current value will report the already cached value for both
events and no events get generated. This make sense since the
hotkey change got cancelled by the sysfs write. If otoh the hotkey
gets handled last, then the first event will read back
a different value, after which an events gets send to userspace
and the cached value gets updated, changing the second event
into a NOP. So in this case things just work as if there was
a significant amount of time between the sysfs write and the
hotkey press.

TL;DR: the very worst then can happen is loosing a hotkey
event because the exact same value was written to sysfs after
the press was handled by the BIOS but before we get to process
the event.

> I'm not talking here about userspace libsmbios. All is just by
> autonomous firmware, keyboard usage and LED sysfs API.
>
> And how is my idea about dropping exactly one event for one request for
> changing backlight via LED sysfs API more racy as yours?

You wrote: "What about just dropping upcoming one event in interval
of next 2-3 second?"

The time window you added makes things more razy. If we just increment
an atomic_t on sysfs_write, and decrement_and_test on an event then
that would be fine and would fix your sysfs write vs hotkey press write
as we would get 2 events in that case.

> Received event from firmware has only the timestamp, no more information
> yet (*). Therefore state of events does is not ordered and any two
> events are basically same.
>
> In your provides patch you are adding additional information to event.
> It is currently read brightness level in time when was event delivered,

Correct, that is part of the ABI definition of the brightness_hw_changed
sysfs attribute, we must pass the brightness at the time of the hardware
initiated change.

> not state of time when event was created. Which means you cannot swap
> any two received events anymore as they are part of current driver
> state.
>
> In my idea I do not add any additional information to event. Which means
> all received events are same in current driver state and I can swap
> them. So I can decide to drop one event when driver is changing
> backlight level. And because I can swap received events it does not
> matter if I drop "correct" one (that which was really generated by my
> backlight level change) or any other before or after (as they are same).
> The only important is that kernel use correct number of firmware events.
>
> Idea for time interval of 2-3 seconds is that if event is delivered
> later as after 3 seconds it is probably useless as driver application
> code can be already out-of-sync. But this time interval is just another
> idea and we do not need to use it.

I really don't like the 2-3 seconds stuff, but I'm fine with going
with your idea of just swallowing one event after a sysfs-write
without the timeout stuff.

> (*) Looks like for some machines in dell-wmi.c we know also which
> brightness level was set by firmware, but it is not used now and we
> still have machines without this information.
>
>> the only downside is the driver seeing a brightness change caused by
>> libsmbios as one caused by the hotkeys, but it will do so 100%
>> reliably and that is something which I believe we can live with
>> just fine.
>>
>>> This would allow us to reduce doing one SMM call for each received
>>> event and I think it would be same reliable as your approach.
>>
>> As I explained *already* we already have only one SMM call for reach
>> received event, we pass the read-back brightness into
>> led_classdev_notify_brightness_hw_changed and when userspace
>> reads the new brightness_hw_changed event in response to the wakeup
>> the led-core will use the value passed through
>> led_classdev_notify_brightness_hw_changed so only 1 SMM call happens
>> per event.
>>
>> Also again this does not happen 100 times per second you're really
>> trying to optimize something which does not need any optimization
>> at all here.
>
> We have reports that on some Dell notebooks is issuing SMM call causing
> freezing kernel for about 1-3 seconds (as firmware stay in SMM mode for
> a longer time...).

I doubt those are notebooks with backlight keyboards, but anyways if
we swallow 1 event per sysfs write we are only doing SMM calls when
actually reporting a change to userspace at which point we need to
make that smm call sooner or later anyways...

Regards,

Hans

Re: [PATCH v8 1/7] platform/x86/thinkpad_acpi: Stop setting led_classdev brightness directly

[Marco Trevisan (Treviño)]

2017-03-01 12:27 GMT+01:00 Pali Rohár <pali.rohar@gmail.com>:
>> @@ -5164,7 +5162,7 @@ static void kbdlight_set_worker(struct work_struct *work)
>>                       container_of(work, struct tpacpi_led_classdev, work);
>>
>>       if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
>> -             kbdlight_set_level_and_update(data->new_state);
>> +             kbdlight_set_level(data->new_state);
>>  }
>>
>>  static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
>
> Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save
> kbdlight state on suspend and restore it on resume). It is really OK to
> revert that change?
>
> CCing Marco who is author of that commit.


I think that as per commit c685e20df this is actually not needed
anymore, as it will be userspace to handle that.
So it's fine to me.

Thanks for asking, though

Re: [PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Andy Shevchenko]

On Thu, Feb 9, 2017 at 5:44 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Here is v8 of the platform drivers changes matching / using the new
> v8 LED subsys led_classdev_notify_brightness_hw_changed() functionality
> queued in -next.
>
> There have been some changes (and preparation patches added) compared
> to the previous versions since the new LED api requires the driver to
> only call led_classdev_notify_brightness_hw_changed() on hotkey triggered
> changes and the ACPI events indicating brightness changes also get
> triggered when setting the brightness to led_set_brightness (or sysfs).
>
> This series depends on the patch adding
> led_classdev_notify_brightness_hw_changed() to the LED subsystem,
> Jacek can you create a stable branch with just that patch which the
> platform/x86 platform maintainers can merge, so that they can apply
> this series ?

I briefly read the discussion and looks like I'll wait for v9 after
merge window is closed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Hans de Goede]

Hi,

On 02-03-17 00:10, Andy Shevchenko wrote:
> On Thu, Feb 9, 2017 at 5:44 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Here is v8 of the platform drivers changes matching / using the new
>> v8 LED subsys led_classdev_notify_brightness_hw_changed() functionality
>> queued in -next.
>>
>> There have been some changes (and preparation patches added) compared
>> to the previous versions since the new LED api requires the driver to
>> only call led_classdev_notify_brightness_hw_changed() on hotkey triggered
>> changes and the ACPI events indicating brightness changes also get
>> triggered when setting the brightness to led_set_brightness (or sysfs).
>>
>> This series depends on the patch adding
>> led_classdev_notify_brightness_hw_changed() to the LED subsystem,
>> Jacek can you create a stable branch with just that patch which the
>> platform/x86 platform maintainers can merge, so that they can apply
>> this series ?
>
> I briefly read the discussion and looks like I'll wait for v9 after
> merge window is closed.

Ok.

Note the 3 acpi-thinkpad patches have been in
linux-platform-drivers-x86.git/testing for a while now and there has been
no discussion surrounding those. Do you want me to send a v9 of those too,
or just the Dell driver patches ?

Regards,

Hans

Re: [PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Pali Rohár]

On Thursday 02 March 2017 15:12:52 Hans de Goede wrote:
> Hi,
> 
> On 02-03-17 00:10, Andy Shevchenko wrote:
> >On Thu, Feb 9, 2017 at 5:44 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> >
> >>Here is v8 of the platform drivers changes matching / using the new
> >>v8 LED subsys led_classdev_notify_brightness_hw_changed() functionality
> >>queued in -next.
> >>
> >>There have been some changes (and preparation patches added) compared
> >>to the previous versions since the new LED api requires the driver to
> >>only call led_classdev_notify_brightness_hw_changed() on hotkey triggered
> >>changes and the ACPI events indicating brightness changes also get
> >>triggered when setting the brightness to led_set_brightness (or sysfs).
> >>
> >>This series depends on the patch adding
> >>led_classdev_notify_brightness_hw_changed() to the LED subsystem,
> >>Jacek can you create a stable branch with just that patch which the
> >>platform/x86 platform maintainers can merge, so that they can apply
> >>this series ?
> >
> >I briefly read the discussion and looks like I'll wait for v9 after
> >merge window is closed.
> 
> Ok.
> 
> Note the 3 acpi-thinkpad patches have been in
> linux-platform-drivers-x86.git/testing for a while now and there has been
> no discussion surrounding those. Do you want me to send a v9 of those too,
> or just the Dell driver patches ?

If we all agree on change for Dell driver to just drop one event, maybe
we can discuss if similar approach should be used also for Thinkpad
driver...

> Regards,
> 
> Hans

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

Re: [PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Hans de Goede]

HI,

On 02-03-17 15:22, Pali Rohár wrote:
> On Thursday 02 March 2017 15:12:52 Hans de Goede wrote:
>> Hi,
>>
>> On 02-03-17 00:10, Andy Shevchenko wrote:
>>> On Thu, Feb 9, 2017 at 5:44 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>>> Here is v8 of the platform drivers changes matching / using the new
>>>> v8 LED subsys led_classdev_notify_brightness_hw_changed() functionality
>>>> queued in -next.
>>>>
>>>> There have been some changes (and preparation patches added) compared
>>>> to the previous versions since the new LED api requires the driver to
>>>> only call led_classdev_notify_brightness_hw_changed() on hotkey triggered
>>>> changes and the ACPI events indicating brightness changes also get
>>>> triggered when setting the brightness to led_set_brightness (or sysfs).
>>>>
>>>> This series depends on the patch adding
>>>> led_classdev_notify_brightness_hw_changed() to the LED subsystem,
>>>> Jacek can you create a stable branch with just that patch which the
>>>> platform/x86 platform maintainers can merge, so that they can apply
>>>> this series ?
>>>
>>> I briefly read the discussion and looks like I'll wait for v9 after
>>> merge window is closed.
>>
>> Ok.
>>
>> Note the 3 acpi-thinkpad patches have been in
>> linux-platform-drivers-x86.git/testing for a while now and there has been
>> no discussion surrounding those. Do you want me to send a v9 of those too,
>> or just the Dell driver patches ?
>
> If we all agree on change for Dell driver to just drop one event, maybe
> we can discuss if similar approach should be used also for Thinkpad
> driver...

As mentioned during the review of the thinkpad patches already, there is no
guarantee that all generation thinkpads actually generate events when setting
the brightness through calling into the BIOS, so doing this may cause actual
hotkey events to get swallowed. So lets just keep this as is.

Note I still need to test that a similar problem does not exist for the Dell
case.

Regards,

Hans

Re: [PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Pali Rohár]

On Thursday 02 March 2017 15:30:23 Hans de Goede wrote:
> HI,
> 
> On 02-03-17 15:22, Pali Rohár wrote:
> >On Thursday 02 March 2017 15:12:52 Hans de Goede wrote:
> >>Hi,
> >>
> >>On 02-03-17 00:10, Andy Shevchenko wrote:
> >>>On Thu, Feb 9, 2017 at 5:44 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>
> >>>>Here is v8 of the platform drivers changes matching / using the new
> >>>>v8 LED subsys led_classdev_notify_brightness_hw_changed() functionality
> >>>>queued in -next.
> >>>>
> >>>>There have been some changes (and preparation patches added) compared
> >>>>to the previous versions since the new LED api requires the driver to
> >>>>only call led_classdev_notify_brightness_hw_changed() on hotkey triggered
> >>>>changes and the ACPI events indicating brightness changes also get
> >>>>triggered when setting the brightness to led_set_brightness (or sysfs).
> >>>>
> >>>>This series depends on the patch adding
> >>>>led_classdev_notify_brightness_hw_changed() to the LED subsystem,
> >>>>Jacek can you create a stable branch with just that patch which the
> >>>>platform/x86 platform maintainers can merge, so that they can apply
> >>>>this series ?
> >>>
> >>>I briefly read the discussion and looks like I'll wait for v9 after
> >>>merge window is closed.
> >>
> >>Ok.
> >>
> >>Note the 3 acpi-thinkpad patches have been in
> >>linux-platform-drivers-x86.git/testing for a while now and there has been
> >>no discussion surrounding those. Do you want me to send a v9 of those too,
> >>or just the Dell driver patches ?
> >
> >If we all agree on change for Dell driver to just drop one event, maybe
> >we can discuss if similar approach should be used also for Thinkpad
> >driver...
> 
> As mentioned during the review of the thinkpad patches already, there is no
> guarantee that all generation thinkpads actually generate events when setting
> the brightness through calling into the BIOS, so doing this may cause actual
> hotkey events to get swallowed. So lets just keep this as is.

Ok, in this case, there is no option and thinkpad patches can go...

> Note I still need to test that a similar problem does not exist for the Dell
> case.

If you have access to more Dell machines which looks like could be
problematic then please check this.

> Regards,
> 
> Hans

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

Re: [PATCH v8 0/7] platform/x86: Notify userspace about hotkeys changing kbd-backlight brightness

[Andy Shevchenko]

On Thu, Mar 2, 2017 at 4:12 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> On 02-03-17 00:10, Andy Shevchenko wrote:
>> On Thu, Feb 9, 2017 at 5:44 PM, Hans de Goede <hdegoede@redhat.com> wrote:

>> I briefly read the discussion and looks like I'll wait for v9 after
>> merge window is closed.

> Note the 3 acpi-thinkpad patches have been in
> linux-platform-drivers-x86.git/testing for a while now and there has been
> no discussion surrounding those. Do you want me to send a v9 of those too,
> or just the Dell driver patches ?

Thinkpad patches have nothing to prevent them going for-next.
Thus, just Dell related.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Pali Rohár]

On Wednesday 01 March 2017 14:58:45 Hans de Goede wrote:
> >In my idea I do not add any additional information to event. Which means
> >all received events are same in current driver state and I can swap
> >them. So I can decide to drop one event when driver is changing
> >backlight level. And because I can swap received events it does not
> >matter if I drop "correct" one (that which was really generated by my
> >backlight level change) or any other before or after (as they are same).
> >The only important is that kernel use correct number of firmware events.
> >
> >Idea for time interval of 2-3 seconds is that if event is delivered
> >later as after 3 seconds it is probably useless as driver application
> >code can be already out-of-sync. But this time interval is just another
> >idea and we do not need to use it.
> 
> I really don't like the 2-3 seconds stuff, but I'm fine with going
> with your idea of just swallowing one event after a sysfs-write
> without the timeout stuff.

Ok. I'm fine with just dropping exactly one event after setting new
value via sysfs. It is OK for you?

> >(*) Looks like for some machines in dell-wmi.c we know also which
> >brightness level was set by firmware, but it is not used now and we
> >still have machines without this information.
> >
> >>the only downside is the driver seeing a brightness change caused by
> >>libsmbios as one caused by the hotkeys, but it will do so 100%
> >>reliably and that is something which I believe we can live with
> >>just fine.
> >>
> >>>This would allow us to reduce doing one SMM call for each received
> >>>event and I think it would be same reliable as your approach.
> >>
> >>As I explained *already* we already have only one SMM call for reach
> >>received event, we pass the read-back brightness into
> >>led_classdev_notify_brightness_hw_changed and when userspace
> >>reads the new brightness_hw_changed event in response to the wakeup
> >>the led-core will use the value passed through
> >>led_classdev_notify_brightness_hw_changed so only 1 SMM call happens
> >>per event.
> >>
> >>Also again this does not happen 100 times per second you're really
> >>trying to optimize something which does not need any optimization
> >>at all here.
> >
> >We have reports that on some Dell notebooks is issuing SMM call causing
> >freezing kernel for about 1-3 seconds (as firmware stay in SMM mode for
> >a longer time...).
> 
> I doubt those are notebooks with backlight keyboards,

It is possible and also that no notebooks with backlight keyboard is
affected. I just wanted to show that there can be a real problem as we
got reports about such freezes in SMM.

> but anyways if
> we swallow 1 event per sysfs write we are only doing SMM calls when
> actually reporting a change to userspace at which point we need to
> make that smm call sooner or later anyways...

Yes. But I think that we do not have to penalize users who are not using
new brightness_hw_changed attribute.

> Regards,
> 
> Hans

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

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 03-03-17 13:00, Pali Rohár wrote:
> On Wednesday 01 March 2017 14:58:45 Hans de Goede wrote:
>>> In my idea I do not add any additional information to event. Which means
>>> all received events are same in current driver state and I can swap
>>> them. So I can decide to drop one event when driver is changing
>>> backlight level. And because I can swap received events it does not
>>> matter if I drop "correct" one (that which was really generated by my
>>> backlight level change) or any other before or after (as they are same).
>>> The only important is that kernel use correct number of firmware events.
>>>
>>> Idea for time interval of 2-3 seconds is that if event is delivered
>>> later as after 3 seconds it is probably useless as driver application
>>> code can be already out-of-sync. But this time interval is just another
>>> idea and we do not need to use it.
>>
>> I really don't like the 2-3 seconds stuff, but I'm fine with going
>> with your idea of just swallowing one event after a sysfs-write
>> without the timeout stuff.
>
> Ok. I'm fine with just dropping exactly one event after setting new
> value via sysfs. It is OK for you?

Yes that will work for me. I will prepare a new version. Note I probably
will not have time for this till the end of this week.

One thing I was wondering, is what will happen if we write the
same value to the sysfs brightness file twice, do we get events
for both writes?  One way to avoid this problem would be to compare
the written value to the last known value and discard the write
(avoiding an expensive SMM call) if they are equal.

Discarding double writes does introduce a race between hotkey changes
vs sysfs writes, if a hotkey change has been made but the event has
not yet been handled then a sysfs write may be seen as being double
and get discarded even though it is not double. OTOH if the hotkey
press wins the race then the result would have been the write being
discarded anyways, so I don't think this is a real problem.

You're input on this would be appreciated. If writing the same value
twice does not generate events then we must filter out the double
writes. If it does drop events we can choose, the simplest would be
to not filter in that case, avoiding any races (and userspace normally
should not write the same value twice, but we cannot assume that).

Regards,

Hans

Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

HI,

On 06-03-17 14:39, Hans de Goede wrote:
> Hi,
>
> On 03-03-17 13:00, Pali Rohár wrote:
>> On Wednesday 01 March 2017 14:58:45 Hans de Goede wrote:
>>>> In my idea I do not add any additional information to event. Which means
>>>> all received events are same in current driver state and I can swap
>>>> them. So I can decide to drop one event when driver is changing
>>>> backlight level. And because I can swap received events it does not
>>>> matter if I drop "correct" one (that which was really generated by my
>>>> backlight level change) or any other before or after (as they are same).
>>>> The only important is that kernel use correct number of firmware events.
>>>>
>>>> Idea for time interval of 2-3 seconds is that if event is delivered
>>>> later as after 3 seconds it is probably useless as driver application
>>>> code can be already out-of-sync. But this time interval is just another
>>>> idea and we do not need to use it.
>>>
>>> I really don't like the 2-3 seconds stuff, but I'm fine with going
>>> with your idea of just swallowing one event after a sysfs-write
>>> without the timeout stuff.
>>
>> Ok. I'm fine with just dropping exactly one event after setting new
>> value via sysfs. It is OK for you?
>
> Yes that will work for me. I will prepare a new version. Note I probably
> will not have time for this till the end of this week.
>
> One thing I was wondering, is what will happen if we write the
> same value to the sysfs brightness file twice, do we get events
> for both writes?  One way to avoid this problem would be to compare
> the written value to the last known value and discard the write
> (avoiding an expensive SMM call) if they are equal.
>
> Discarding double writes does introduce a race between hotkey changes
> vs sysfs writes, if a hotkey change has been made but the event has
> not yet been handled then a sysfs write may be seen as being double
> and get discarded even though it is not double. OTOH if the hotkey
> press wins the race then the result would have been the write being
> discarded anyways, so I don't think this is a real problem.
>
> You're input on this would be appreciated. If writing the same value
> twice does not generate events then we must filter out the double
> writes. If it does drop events we can choose, the simplest would be
> to not filter in that case, avoiding any races (and userspace normally
> should not write the same value twice, but we cannot assume that).

Ok, I've spend some time on this today. As I was afraid no new wmi
events are generated when writing the same value to sysfs twice,
making the ignore one event after write approach tricky.

But luckily I've found a better fix, the kbd led related wmi events
with a type of 0x0010 are only send when using the hotkeys.

So if we ignore the events with a type of 0x0011, by dropping the
part of the patch which was mapping those to KEY_KBDILLUMTOGGLE
instead of to KEY_RESERVED, we only end up calling
dell_laptop_call_notifier from dell-wmi.c on hotkey presses and
we can drop the current vs new brightness comparison which
you disliked from dell-laptop.c.

So we can fix this by making the patch smaller :)

I will prepare a v9 with these changes and send that out later
today.

Regards,

Hans