| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 23 Mar 2016 17:02:59 +0100
From: Jacek Anaszewski <j.anaszewski@...sung.com>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
On 03/23/2016 12:57 PM, Heiner Kallweit wrote:
> Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
>> On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
>>> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>>>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>>>> Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
>>>>>> On 03/21/2016 06:34 PM, Heiner Kallweit wrote:
>>>>>>> Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
>>>>>>>> On 03/19/2016 08:11 PM, Heiner Kallweit wrote:
>>>>>>>>> Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
>>>>>>>>>> On 03/17/2016 08:53 PM, Heiner Kallweit wrote:
>>>>>>>>>>> Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
>>>>>>>>>>>> Hi Heiner,
>>>>>>>>>>>>
>>>>>>>>>>>> On 03/13/2016 06:14 PM, Heiner Kallweit wrote:
>>>>>>>>>>>>> Add basic support for RGB triggers. Triggers with flag LED_TRIG_CAP_RGB
>>>>>>>>>>>>> set are available to RGB LED devices only.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> drivers/leds/led-triggers.c | 15 ++++++++++++---
>>>>>>>>>>>>> include/linux/leds.h | 3 +++
>>>>>>>>>>>>> 2 files changed, 15 insertions(+), 3 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>>>>>>>>>>>>> index 2181581..3ccf88b 100644
>>>>>>>>>>>>> --- a/drivers/leds/led-triggers.c
>>>>>>>>>>>>> +++ b/drivers/leds/led-triggers.c
>>>>>>>>>>>>> @@ -30,6 +30,13 @@ static LIST_HEAD(trigger_list);
>>>>>>>>>>>>>
>>>>>>>>>>>>> /* Used by LED Class */
>>>>>>>>>>>>>
>>>>>>>>>>>>> +static inline bool led_trig_check_rgb(struct led_trigger *trig,
>>>>>>>>>>>>> + struct led_classdev *led_cdev)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + return !(trig->flags & LED_TRIG_CAP_RGB) ||
>>>>>>>>>>>>> + led_cdev->flags & LED_DEV_CAP_RGB;
>>>>>>>>>>>>> +}
>>>>>>>>>>>>
>>>>>>>>>>>> Could you explain what is the purpose of this function?
>>>>>>>>>>>> What actually do we want to check here?
>>>>>>>>>>>>
>>>>>>>>>>> Triggers using RGB functionality can't be used with non-RGB LED's.
>>>>>>>>>>> This check checks for such unsupported combinations:
>>>>>>>>>>> It returns false if the trigger uses RGB functionality but LED doesn't
>>>>>>>>>>> support the RGB extension.
>>>>>>>>>>
>>>>>>>>>> We need more meaningful name for it. Maybe led_trigger_is_supported() ?
>>>>>>>>>> And let's make it no-op for !CONFIG_LEDS_CLASS_RGB case.
>>>>>>>>>>
>>>>>>>>> OK, led_trigger_is_supported() is better.
>>>>>>>>>
>>>>>>>>> Making the function a no-op in the non-RGB case would have some impact:
>>>>>>>>> We'd have to make sure that all public trigger functions are a de-facto no-op
>>>>>>>>> for RGB triggers (at least register / unregister). Means we would need
>>>>>>>>> something like this in each public trigger function:
>>>>>>>>>
>>>>>>>>> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>>>>>>>>> if (trig->flags & LED_TRIG_CAP_RGB))
>>>>>>>>> return;
>>>>>>>>> #endif
>>>>>>>>>
>>>>>>>>> I think this would add a lot of overhead and therefore IMHO it's better to
>>>>>>>>> not make the check function a no-op.
>>>>>>>>
>>>>>>>> Wouldn't it suffice to make the no-op returning true?
>>>>>>>> Preventing RGB trigger registration for non-RGB LED class configuration
>>>>>>>> seems to be different thing, also to be considered.
>>>>>>>>
>>>>>>> No, it's not sufficient. Let's say the RGB extension is disabled and we have a RGB trigger.
>>>>>>> The check is a no-op now (returns always true), therefore the RGB trigger would be displayed
>>>>>>> in the list of available triggers also for all non-RGB LED's.
>>>>>>
>>>>>> If RGB trigger was made dependent on LED RGB class, then the related
>>>>>> Kconfig symbol would remain undefined in !CONFIG_LEDS_CLASS_RGB case.
>>>>>>
>>>>> Making a RGB trigger dependent on LED RGB class would mean to enclose all calls to trigger
>>>>> functions in the RGB trigger like this:
>>>>> #if IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>>>>> trigger_function()
>>>>> #endif
>>>>
>>>> You probably think about the case when we have two triggers in
>>>> single module, like in the planned {rgb-}heartbeat case?
>>>>
>>>> If so this is an argument for having RGB triggers in separate files.
>>>>
>>> I mean the case of triggers implemented outside drivers/leds. There the trigger code
>>> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
>>> and it's not directly under our (LED core) control.
>>>
>>>>> This would apply to led_trigger_(un)register, led_trigger_event, led_trigger_blink, etc.
>>>>> And I think it wouldn't be too nice to force other kernel modules wanting to implement
>>>>> a RGB trigger to add these conditional compile statements.
>>>>
>>>> What other modules do you have on mind? LED triggers are implemented in
>>>> their own files.
>>>>
>>> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
>>> implemented in other parts of the kernel.
>>
>> In this case surrounding all the trigger implementation with
>> IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
>
> Yes, that's what would need to be done. But IMHO it's not nice to force trigger implementations
> in other parts of the kernel to guard each trigger-related call this way.
My main objection is that led_trigger_is_supported() would be redundant
in led_trigger_store() and led_trigger_show() for non-RGB LED subsystem
configuration.
> Also it might happen
> that a trigger is implemented w/o this guarding and w/o informing you.
> Then this (RGB) trigger would show up also for all non-RGB LED's.
It is likely that it wouldn't compile without led-rgb-core.o.
> I still think that not making led_trigger_is_supported() a no-op in the non-RGB case is a small
> price for preventing such potential issues.
We could avoid the issues by adding a guard in led_trigger_register(),
that would prevent RGB trigger registration in !CONFIG_LEDS_CLASS_RGB
case.
>> In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
>> IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>>
>>>>> Alternatively, as mentioned before, we would have to add this to all public trigger functions:
>>>>>
>>>>> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>>>>> if (trig->flags & LED_TRIG_CAP_RGB))
>>>>> return;
>>>>> #endif
>>>>> I think this would add significant overhead w/o gaining really something.
>>>>>
>>>>>>> We could maximum remove the "|| led_cdev->flags & LED_DEV_CAP_RGB" from the check if
>>>>>>> the RGB extension is disabled. But it's open whether this minimal gain in a non-critical
>>>>>>> code path justifies this.
>>>>>>>
>>>>>>>>>>>>> ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>>>> const char *buf, size_t count)
>>>>>>>>>>>>> {
>>>>>>>>>>>>> @@ -52,12 +59,12 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>>>> down_read(&triggers_list_lock);
>>>>>>>>>>>>> list_for_each_entry(trig, &trigger_list, next_trig) {
>>>>>>>>>>>>> if (sysfs_streq(buf, trig->name)) {
>>>>>>>>>>>>> + if (!led_trig_check_rgb(trig, led_cdev))
>>>>>>>>>>>>> + break;
>>>>>>>>>>>
>>>>>>>>>>> Check for the case that userspace wants to set a RGB trigger for a non-RGB LED via sysfs.
>>>>>>>>>>>
>>>>>>>>>>>>> down_write(&led_cdev->trigger_lock);
>>>>>>>>>>>>> led_trigger_set(led_cdev, trig);
>>>>>>>>>>>>> up_write(&led_cdev->trigger_lock);
>>>>>>>>>>>>> -
>>>>>>>>>>>>> - up_read(&triggers_list_lock);
>>>>>>>>>>>>> - goto unlock;
>>>>>>>>>>>>> + break;
>>>>>>>>>>
>>>>>>>>>> This seems to be an unrelated cleanup. Please submit it separately.
>>>>>>>>>>
>>>>>>>>> OK
>>>>>>>>>
>>>>>>>>>>>>> }
>>>>>>>>>>>>> }
>>>>>>>>>>>>> up_read(&triggers_list_lock);
>>>>>>>>>>>>> @@ -84,6 +91,8 @@ ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>>>> len += sprintf(buf+len, "none ");
>>>>>>>>>>>>>
>>>>>>>>>>>>> list_for_each_entry(trig, &trigger_list, next_trig) {
>>>>>>>>>>>>> + if (!led_trig_check_rgb(trig, led_cdev))
>>>>>>>>>>>>> + continue;
>>>>>>>>>>>
>>>>>>>>>>> Omit RGB triggers when listing the available triggers for a non-RGB LED via sysfs.
>>>>>>>>>>>
>>>>>>>>>>>>> if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
>>>>>>>>>>>>> trig->name))
>>>>>>>>>>>>> len += sprintf(buf+len, "[%s] ", trig->name);
>>>>>>>>>>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>>>>>>>>>>> index 58e22e6..07eb074 100644
>>>>>>>>>>>>> --- a/include/linux/leds.h
>>>>>>>>>>>>> +++ b/include/linux/leds.h
>>>>>>>>>>>>> @@ -248,6 +248,9 @@ enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
>>>>>>>>>>>>> struct led_trigger {
>>>>>>>>>>>>> /* Trigger Properties */
>>>>>>>>>>>>> const char *name;
>>>>>>>>>>>>> + u8 flags;
>>>>>>>>>>>>> +#define LED_TRIG_CAP_RGB BIT(0)
>>>>>>>>>>>>> +
>>>>>>>>>>>>> void (*activate)(struct led_classdev *led_cdev);
>>>>>>>>>>>>> void (*deactivate)(struct led_classdev *led_cdev);
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
Best regards,
Jacek Anaszewski
Powered by blists - more mailing lists