Hi,
On 18-11-16 11:49, Jacek Anaszewski wrote:
> 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.
Hmm, I don't like adding a sysfs file which changes behavior of
the "brightness" file, I think that we can work around this,
see below.
> 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.
Ok, I've created a systemd-backlight patch to prefer
current_brightness when present, avoiding the problem of
systemd-backlight removing the kbd-backlight trigger on boot
when it is restoring a brightness setting of 0.
That fixes the immediate problem, without needing to break /
hack the old "brightness" ABI.
So if you want feel free to drop this patch from the series
and just apply patch 1 and 2, Once merged I'll send a v6 of
the platform driver patches to match the dropping of this
patch.
Regards,
Hans
>
>>> 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
>>
>>
>>
>
>
------------------------------------------------------------------------------
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
