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;
>>
>
>
------------------------------------------------------------------------------
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
