Hi, On 18-11-16 09:52, Jacek Anaszewski wrote:
Hi Hans, Thanks for the new patch set. On 11/17/2016 11:24 PM, Hans de Goede wrote:In some cases an LED is controlled through a hardwired (taken care of in firmware outside of the kernels control) trigger. Add an LED_TRIGGER_READ_ONLY flag for this and disallow user-space changing the trigger when this flag is set. Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> --- Changes in v5: -This is a new patch in v5 of this patch-set --- drivers/leds/led-class.c | 2 +- drivers/leds/led-triggers.c | 5 +++++ include/linux/leds.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 326ee6e..56f32cc 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -54,7 +54,7 @@ static ssize_t brightness_store(struct device *dev, if (ret) goto unlock; - if (state == LED_OFF) + if (state == LED_OFF && !(led_cdev->flags & LED_TRIGGER_READ_ONLY)) led_trigger_remove(led_cdev); led_set_brightness(led_cdev, state); diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c index d2ed9c2..9669104 100644 --- a/drivers/leds/led-triggers.c +++ b/drivers/leds/led-triggers.c @@ -37,6 +37,11 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, struct led_trigger *trig; int ret = count; + if (led_cdev->flags & LED_TRIGGER_READ_ONLY) { + dev_err(led_cdev->dev, "Error this led triggers is hardwired and cannot be changed\n"); + return -EINVAL;It means that after a trigger is once assigned to a LED class device it will be impossible to deactivate it.
No this flag is not set by the trigger code, it is set by the LED driver itself, to indicate there is a hardwired trigger.
Why not leaving it to the user's decision whether they want to have hw brightness changes notifications?
The user cannot disable the hotkey -> keyboard-backlight-led link and the trigger represents this link. More over, if we allow changing the trigger, then writing 0 to the brightness attribute will remove the trigger and make the device no longer poll-able, and writing 0 is exactly what the systemd-backlight service does when restoring backlight settings on boot and the kbd-backlight was off at the last shutdown. This again shows how poorly thought out the old "brightness" file API is, all systemd-backlight want to do is set the brightness to 0, but as a side effect it will also unlink the trigger, because writing 0 has 2 effects for one system call. Anyways this is not something we can fix.
This way we disable the possibility to set different trigger like e.g. timer, after this one is set, which is not a non-realistic scenario.
Then we would have 2 triggers active, as the hotkey trigger is part of the firmware of the laptop and is never going away.
Generally it is quite odd to add a functionality that once set is latched. If one will set such a trigger by mistake, then system restart will be required to reset this (unless the driver is built as a module).
This is not under user-control, the is controlled by the LED driver by setting the flag before registering the LED and this is on purpose, because the trigger is hardwired. TL;DR: 1) We've decided to model the hotkey -> kbd-backlight control link as a trigger 2) This is hardwired in the laptop's firmware therefor the trigger cannot be changed 3) Thus we need support for read-only triggers Regards, Hans
+ } + mutex_lock(&led_cdev->led_access); if (led_sysfs_is_disabled(led_cdev)) { diff --git a/include/linux/leds.h b/include/linux/leds.h index 870b8c2..e076b74 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -47,6 +47,7 @@ struct led_classdev { #define LED_DEV_CAP_FLASH (1 << 18) #define LED_HW_PLUGGABLE (1 << 19) #define LED_PANIC_INDICATOR (1 << 20) +#define LED_TRIGGER_READ_ONLY (1 << 21) /* set_brightness_work / blink_timer flags, atomic, private. */ unsigned long work_flags;
-- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
