Hi,
On 11/18/2016 10:04 AM, Hans de Goede wrote:
> 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.
So we will need to fix that. Maybe we should make it configurable.
E.g. a sysfs file could define whether writing 0 to brightness file
clears a trigger.
Trigger can already be disabled by writing "none" to triggers file.
>> 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.
Having this type of hardware trigger reflected in the LED class
device configuration all the time is not something critical I think.
More important is leaving a possibility of applying other existing
sources of kernel events to the LED controller.
>> 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 linux-leds" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
--
Best regards,
Jacek Anaszewski
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
