Re: [PATCH 1/3] leds: triggers: add support for RGB triggers

From: Jacek Anaszewski
Date: Thu Mar 24 2016 - 09:23:56 EST

Next message: Adrian Hunter: "Re: Warnings for invalid VDD (sdhci-s3c)"
Previous message: Jisheng Zhang: "Re: [PATCH 0/2] arm64: cpuidle: make arm_cpuidle_suspend() more efficient"
Next in thread: Heiner Kallweit: "Re: [PATCH 1/3] leds: triggers: add support for RGB triggers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 03/23/2016 05:36 PM, Heiner Kallweit wrote:

Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:

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@xxxxxxxxx>
---
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.

Yes, it's redundant for non-RGB configurations. But it affects sysfs access only
and overhead / impact should be minimal to negligible

I agree.

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.

It would compile because the only relevant difference between a RGB and a non-RGB trigger is a flag
being set in struct led_trigger.

RGB trigger would probably need to use some led-rgb-core API, e.g. as
in case of led_trigger_range_event() from your patch - we've already
agreed about moving most of its internals to led-rgb-core.c

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.

With "preventing registration" most likely you mean registering being a no-op.

Actually I mean checking if trigger is supported by current LED
subsystem configuration, i.e. we will need to use
led_is_trigger_supported() in led_trigger_register(). This is another
argument for this API to be no-op only if !CONFIG_LEDS_TRIGGERS.
To conclude - I agree that it shouldn't be no-op in
!CONFIG_LEDS_CLASS_RGB case.

I'm afraid we'd need the same also in all other public trigger functions, because it may cause
problems if registering is a no-op and we call e.g. led_trigger_event then (not being a no-op).
That's what I meant when I wrote earlier in this thread that we might need something like this
in all exported trigger functions:

#if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
if (trig->flags & LED_TRIG_CAP_RGB))
return;
#endif

And this seems to be much more overhead than the one check in sysfs access not being a no-op
in the non-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

Next message: Adrian Hunter: "Re: Warnings for invalid VDD (sdhci-s3c)"
Previous message: Jisheng Zhang: "Re: [PATCH 0/2] arm64: cpuidle: make arm_cpuidle_suspend() more efficient"
Next in thread: Heiner Kallweit: "Re: [PATCH 1/3] leds: triggers: add support for RGB triggers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]