Re: [ibm-acpi-devel] [PATCH v5 3/6] leds: triggers: Add support for read-only triggers

[Hans de G. <hde...@re...>]

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 <hde...@re...>
>> ---
>> 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;
>>
>
>