| 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
| ||
|
Message-id: <56F0FCAD.7060709@samsung.com>
Date: Tue, 22 Mar 2016 09:05:01 +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/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.
> 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