* [PATCH v7] leds: class: Add new optional brightness_hw_changed attribute
@ 2017-01-27 7:41 Hans de Goede
2017-01-27 11:58 ` Pavel Machek
0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2017-01-27 7:41 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek, Darren Hart,
Henrique de Moraes Holschuh, Pali Rohár
Cc: linux-leds, platform-driver-x86, Hans de Goede
Some LEDs may have their brightness level changed autonomously
(outside of kernel control) by hardware / firmware. This commit
adds support for an optional brightness_hw_changed attribute to
signal such changes to userspace (if a driver can detect them):
What: /sys/class/leds/<led>/brightness_hw_changed
Date: January 2017
KernelVersion: 4.11
Description:
Last hardware set brightness level for this LED. Some LEDs
may be changed autonomously by hardware/firmware. Only LEDs
where this happens and the driver can detect this, will
have this file.
This file supports poll() to detect when the hardware
changes the brightness.
Reading this file will return the last brightness level set
by the hardware, this may be different from the current
brightness.
Drivers which want to support this, simply add LED_BRIGHT_HW_CHANGED to
their flags field and call led_classdev_notify_brightness_hw_changed()
with the hardware set brightness when they detect a hardware / firmware
triggered brightness change.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
-New patch in v6 of this set, replacing previous trigger based API
Changes in v7
-Return -ENODATA when brightness_hw_changed gets read before any
hw brightness change event as happened
-Make the hw_brightness_changed attr presence configurable through Kconfig
---
Documentation/ABI/testing/sysfs-class-led | 17 +++++++
drivers/leds/Kconfig | 9 ++++
drivers/leds/led-class.c | 73 +++++++++++++++++++++++++++++++
include/linux/leds.h | 15 +++++++
4 files changed, 114 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 491cdee..5f67f7a 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -23,6 +23,23 @@ Description:
If the LED does not support different brightness levels, this
should be 1.
+What: /sys/class/leds/<led>/brightness_hw_changed
+Date: January 2017
+KernelVersion: 4.11
+Description:
+ Last hardware set brightness level for this LED. Some LEDs
+ may be changed autonomously by hardware/firmware. Only LEDs
+ where this happens and the driver can detect this, will have
+ this file.
+
+ This file supports poll() to detect when the hardware changes
+ the brightness.
+
+ Reading this file will return the last brightness level set
+ by the hardware, this may be different from the current
+ brightness. Reading this file when no hw brightness change
+ event has happened will return an ENODATA error.
+
What: /sys/class/leds/<led>/trigger
Date: March 2006
KernelVersion: 2.6.17
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index c621cbb..275f467 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -29,6 +29,15 @@ config LEDS_CLASS_FLASH
for the flash related features of a LED device. It can be built
as a module.
+config LEDS_BRIGHTNESS_HW_CHANGED
+ bool "LED Class brightness_hw_changed attribute support"
+ depends on LEDS_CLASS
+ help
+ This option enables support for the brightness_hw_changed attribute
+ for led sysfs class devices under /sys/class/leds.
+
+ See Documentation/ABI/testing/sysfs-class-led for details.
+
comment "LED drivers"
config LEDS_88PM860X
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 326ee6e..2f8c843 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -103,6 +103,65 @@ static const struct attribute_group *led_groups[] = {
NULL,
};
+#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
+static ssize_t brightness_hw_changed_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+ if (led_cdev->brightness_hw_changed == -1)
+ return -ENODATA;
+
+ return sprintf(buf, "%u\n", led_cdev->brightness_hw_changed);
+}
+
+static DEVICE_ATTR_RO(brightness_hw_changed);
+
+static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
+{
+ struct device *dev = led_cdev->dev;
+ int ret;
+
+ ret = device_create_file(dev, &dev_attr_brightness_hw_changed);
+ if (ret) {
+ dev_err(dev, "Error creating brightness_hw_changed\n");
+ return ret;
+ }
+
+ led_cdev->brightness_hw_changed_kn =
+ sysfs_get_dirent(dev->kobj.sd, "brightness_hw_changed");
+ if (!led_cdev->brightness_hw_changed_kn) {
+ dev_err(dev, "Error getting brightness_hw_changed kn\n");
+ device_remove_file(dev, &dev_attr_brightness_hw_changed);
+ return -ENXIO;
+ }
+
+ return 0;
+}
+
+static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
+{
+ sysfs_put(led_cdev->brightness_hw_changed_kn);
+ device_remove_file(led_cdev->dev, &dev_attr_brightness_hw_changed);
+}
+
+void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ led_cdev->brightness_hw_changed = brightness;
+ sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn);
+}
+EXPORT_SYMBOL_GPL(led_classdev_notify_brightness_hw_changed);
+#else
+static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
+{
+ return 0;
+}
+static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
+{
+}
+#endif
+
/**
* led_classdev_suspend - suspend an led_classdev.
* @led_cdev: the led_classdev to suspend.
@@ -204,10 +263,21 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
dev_warn(parent, "Led %s renamed to %s due to name collision",
led_cdev->name, dev_name(led_cdev->dev));
+ if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
+ ret = led_add_brightness_hw_changed(led_cdev);
+ if (ret) {
+ device_unregister(led_cdev->dev);
+ return ret;
+ }
+ }
+
led_cdev->work_flags = 0;
#ifdef CONFIG_LEDS_TRIGGERS
init_rwsem(&led_cdev->trigger_lock);
#endif
+#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
+ led_cdev->brightness_hw_changed = -1;
+#endif
mutex_init(&led_cdev->led_access);
/* add to the list of leds */
down_write(&leds_list_lock);
@@ -256,6 +326,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
flush_work(&led_cdev->set_brightness_work);
+ if (led_cdev->flags & LED_BRIGHT_HW_CHANGED)
+ led_remove_brightness_hw_changed(led_cdev);
+
device_unregister(led_cdev->dev);
down_write(&leds_list_lock);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 569cb53..c771153 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -13,6 +13,7 @@
#define __LINUX_LEDS_H_INCLUDED
#include <linux/device.h>
+#include <linux/kernfs.h>
#include <linux/list.h>
#include <linux/mutex.h>
#include <linux/rwsem.h>
@@ -46,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_BRIGHT_HW_CHANGED (1 << 21)
/* set_brightness_work / blink_timer flags, atomic, private. */
unsigned long work_flags;
@@ -110,6 +112,11 @@ struct led_classdev {
bool activated;
#endif
+#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
+ int brightness_hw_changed;
+ struct kernfs_node *brightness_hw_changed_kn;
+#endif
+
/* Ensures consistent access to the LED Flash Class device */
struct mutex led_access;
};
@@ -422,4 +429,12 @@ static inline void ledtrig_cpu(enum cpu_led_event evt)
}
#endif
+#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
+extern void led_classdev_notify_brightness_hw_changed(
+ struct led_classdev *led_cdev, enum led_brightness brightness);
+#else
+static inline void led_classdev_notify_brightness_hw_changed(
+ struct led_classdev *led_cdev, enum led_brightness brightness) { }
+#endif
+
#endif /* __LINUX_LEDS_H_INCLUDED */
--
2.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v7] leds: class: Add new optional brightness_hw_changed attribute
2017-01-27 7:41 [PATCH v7] leds: class: Add new optional brightness_hw_changed attribute Hans de Goede
@ 2017-01-27 11:58 ` Pavel Machek
2017-01-29 11:48 ` Jacek Anaszewski
0 siblings, 1 reply; 4+ messages in thread
From: Pavel Machek @ 2017-01-27 11:58 UTC (permalink / raw)
To: Hans de Goede
Cc: Jacek Anaszewski, Darren Hart, Henrique de Moraes Holschuh,
Pali Rohár, linux-leds, platform-driver-x86
[-- Attachment #1: Type: text/plain, Size: 8648 bytes --]
On Fri 2017-01-27 08:41:35, Hans de Goede wrote:
> Some LEDs may have their brightness level changed autonomously
> (outside of kernel control) by hardware / firmware. This commit
> adds support for an optional brightness_hw_changed attribute to
> signal such changes to userspace (if a driver can detect them):
>
> What: /sys/class/leds/<led>/brightness_hw_changed
> Date: January 2017
> KernelVersion: 4.11
> Description:
> Last hardware set brightness level for this LED. Some LEDs
> may be changed autonomously by hardware/firmware. Only LEDs
> where this happens and the driver can detect this, will
> have this file.
>
> This file supports poll() to detect when the hardware
> changes the brightness.
>
> Reading this file will return the last brightness level set
> by the hardware, this may be different from the current
> brightness.
>
> Drivers which want to support this, simply add LED_BRIGHT_HW_CHANGED to
> their flags field and call led_classdev_notify_brightness_hw_changed()
> with the hardware set brightness when they detect a hardware / firmware
> triggered brightness change.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
> ---
> Changes in v6:
> -New patch in v6 of this set, replacing previous trigger based API
> Changes in v7
> -Return -ENODATA when brightness_hw_changed gets read before any
> hw brightness change event as happened
> -Make the hw_brightness_changed attr presence configurable through Kconfig
> ---
> Documentation/ABI/testing/sysfs-class-led | 17 +++++++
> drivers/leds/Kconfig | 9 ++++
> drivers/leds/led-class.c | 73 +++++++++++++++++++++++++++++++
> include/linux/leds.h | 15 +++++++
> 4 files changed, 114 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 491cdee..5f67f7a 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -23,6 +23,23 @@ Description:
> If the LED does not support different brightness levels, this
> should be 1.
>
> +What: /sys/class/leds/<led>/brightness_hw_changed
> +Date: January 2017
> +KernelVersion: 4.11
> +Description:
> + Last hardware set brightness level for this LED. Some LEDs
> + may be changed autonomously by hardware/firmware. Only LEDs
> + where this happens and the driver can detect this, will have
> + this file.
> +
> + This file supports poll() to detect when the hardware changes
> + the brightness.
> +
> + Reading this file will return the last brightness level set
> + by the hardware, this may be different from the current
> + brightness. Reading this file when no hw brightness change
> + event has happened will return an ENODATA error.
> +
> What: /sys/class/leds/<led>/trigger
> Date: March 2006
> KernelVersion: 2.6.17
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index c621cbb..275f467 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -29,6 +29,15 @@ config LEDS_CLASS_FLASH
> for the flash related features of a LED device. It can be built
> as a module.
>
> +config LEDS_BRIGHTNESS_HW_CHANGED
> + bool "LED Class brightness_hw_changed attribute support"
> + depends on LEDS_CLASS
> + help
> + This option enables support for the brightness_hw_changed attribute
> + for led sysfs class devices under /sys/class/leds.
> +
> + See Documentation/ABI/testing/sysfs-class-led for details.
> +
> comment "LED drivers"
>
> config LEDS_88PM860X
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 326ee6e..2f8c843 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -103,6 +103,65 @@ static const struct attribute_group *led_groups[] = {
> NULL,
> };
>
> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
> +static ssize_t brightness_hw_changed_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> + if (led_cdev->brightness_hw_changed == -1)
> + return -ENODATA;
> +
> + return sprintf(buf, "%u\n", led_cdev->brightness_hw_changed);
> +}
> +
> +static DEVICE_ATTR_RO(brightness_hw_changed);
> +
> +static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
> +{
> + struct device *dev = led_cdev->dev;
> + int ret;
> +
> + ret = device_create_file(dev, &dev_attr_brightness_hw_changed);
> + if (ret) {
> + dev_err(dev, "Error creating brightness_hw_changed\n");
> + return ret;
> + }
> +
> + led_cdev->brightness_hw_changed_kn =
> + sysfs_get_dirent(dev->kobj.sd, "brightness_hw_changed");
> + if (!led_cdev->brightness_hw_changed_kn) {
> + dev_err(dev, "Error getting brightness_hw_changed kn\n");
> + device_remove_file(dev, &dev_attr_brightness_hw_changed);
> + return -ENXIO;
> + }
> +
> + return 0;
> +}
> +
> +static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
> +{
> + sysfs_put(led_cdev->brightness_hw_changed_kn);
> + device_remove_file(led_cdev->dev, &dev_attr_brightness_hw_changed);
> +}
> +
> +void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + led_cdev->brightness_hw_changed = brightness;
> + sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn);
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_notify_brightness_hw_changed);
> +#else
> +static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
> +{
> + return 0;
> +}
> +static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
> +{
> +}
> +#endif
> +
> /**
> * led_classdev_suspend - suspend an led_classdev.
> * @led_cdev: the led_classdev to suspend.
> @@ -204,10 +263,21 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
> dev_warn(parent, "Led %s renamed to %s due to name collision",
> led_cdev->name, dev_name(led_cdev->dev));
>
> + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
> + ret = led_add_brightness_hw_changed(led_cdev);
> + if (ret) {
> + device_unregister(led_cdev->dev);
> + return ret;
> + }
> + }
> +
> led_cdev->work_flags = 0;
> #ifdef CONFIG_LEDS_TRIGGERS
> init_rwsem(&led_cdev->trigger_lock);
> #endif
> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
> + led_cdev->brightness_hw_changed = -1;
> +#endif
> mutex_init(&led_cdev->led_access);
> /* add to the list of leds */
> down_write(&leds_list_lock);
> @@ -256,6 +326,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>
> flush_work(&led_cdev->set_brightness_work);
>
> + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED)
> + led_remove_brightness_hw_changed(led_cdev);
> +
> device_unregister(led_cdev->dev);
>
> down_write(&leds_list_lock);
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 569cb53..c771153 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -13,6 +13,7 @@
> #define __LINUX_LEDS_H_INCLUDED
>
> #include <linux/device.h>
> +#include <linux/kernfs.h>
> #include <linux/list.h>
> #include <linux/mutex.h>
> #include <linux/rwsem.h>
> @@ -46,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_BRIGHT_HW_CHANGED (1 << 21)
>
> /* set_brightness_work / blink_timer flags, atomic, private. */
> unsigned long work_flags;
> @@ -110,6 +112,11 @@ struct led_classdev {
> bool activated;
> #endif
>
> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
> + int brightness_hw_changed;
> + struct kernfs_node *brightness_hw_changed_kn;
> +#endif
> +
> /* Ensures consistent access to the LED Flash Class device */
> struct mutex led_access;
> };
> @@ -422,4 +429,12 @@ static inline void ledtrig_cpu(enum cpu_led_event evt)
> }
> #endif
>
> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
> +extern void led_classdev_notify_brightness_hw_changed(
> + struct led_classdev *led_cdev, enum led_brightness brightness);
> +#else
> +static inline void led_classdev_notify_brightness_hw_changed(
> + struct led_classdev *led_cdev, enum led_brightness brightness) { }
> +#endif
> +
> #endif /* __LINUX_LEDS_H_INCLUDED */
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v7] leds: class: Add new optional brightness_hw_changed attribute
2017-01-27 11:58 ` Pavel Machek
@ 2017-01-29 11:48 ` Jacek Anaszewski
2017-01-29 13:21 ` Hans de Goede
0 siblings, 1 reply; 4+ messages in thread
From: Jacek Anaszewski @ 2017-01-29 11:48 UTC (permalink / raw)
To: Pavel Machek, Hans de Goede
Cc: Darren Hart, Henrique de Moraes Holschuh, Pali Rohár,
linux-leds, platform-driver-x86
Hi Hans,
Thanks for addressing my remarks.
While testing the feature I came across one issue, please refer below.
On 01/27/2017 12:58 PM, Pavel Machek wrote:
> On Fri 2017-01-27 08:41:35, Hans de Goede wrote:
>> Some LEDs may have their brightness level changed autonomously
>> (outside of kernel control) by hardware / firmware. This commit
>> adds support for an optional brightness_hw_changed attribute to
>> signal such changes to userspace (if a driver can detect them):
>>
>> What: /sys/class/leds/<led>/brightness_hw_changed
>> Date: January 2017
>> KernelVersion: 4.11
>> Description:
>> Last hardware set brightness level for this LED. Some LEDs
>> may be changed autonomously by hardware/firmware. Only LEDs
>> where this happens and the driver can detect this, will
>> have this file.
>>
>> This file supports poll() to detect when the hardware
>> changes the brightness.
>>
>> Reading this file will return the last brightness level set
>> by the hardware, this may be different from the current
>> brightness.
>>
>> Drivers which want to support this, simply add LED_BRIGHT_HW_CHANGED to
>> their flags field and call led_classdev_notify_brightness_hw_changed()
>> with the hardware set brightness when they detect a hardware / firmware
>> triggered brightness change.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
>> ---
>> Changes in v6:
>> -New patch in v6 of this set, replacing previous trigger based API
>> Changes in v7
>> -Return -ENODATA when brightness_hw_changed gets read before any
>> hw brightness change event as happened
>> -Make the hw_brightness_changed attr presence configurable through Kconfig
>> ---
>> Documentation/ABI/testing/sysfs-class-led | 17 +++++++
>> drivers/leds/Kconfig | 9 ++++
>> drivers/leds/led-class.c | 73 +++++++++++++++++++++++++++++++
>> include/linux/leds.h | 15 +++++++
>> 4 files changed, 114 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
>> index 491cdee..5f67f7a 100644
>> --- a/Documentation/ABI/testing/sysfs-class-led
>> +++ b/Documentation/ABI/testing/sysfs-class-led
>> @@ -23,6 +23,23 @@ Description:
>> If the LED does not support different brightness levels, this
>> should be 1.
>>
>> +What: /sys/class/leds/<led>/brightness_hw_changed
>> +Date: January 2017
>> +KernelVersion: 4.11
>> +Description:
>> + Last hardware set brightness level for this LED. Some LEDs
>> + may be changed autonomously by hardware/firmware. Only LEDs
>> + where this happens and the driver can detect this, will have
>> + this file.
>> +
>> + This file supports poll() to detect when the hardware changes
>> + the brightness.
>> +
>> + Reading this file will return the last brightness level set
>> + by the hardware, this may be different from the current
>> + brightness. Reading this file when no hw brightness change
>> + event has happened will return an ENODATA error.
>> +
>> What: /sys/class/leds/<led>/trigger
>> Date: March 2006
>> KernelVersion: 2.6.17
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index c621cbb..275f467 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -29,6 +29,15 @@ config LEDS_CLASS_FLASH
>> for the flash related features of a LED device. It can be built
>> as a module.
>>
>> +config LEDS_BRIGHTNESS_HW_CHANGED
>> + bool "LED Class brightness_hw_changed attribute support"
>> + depends on LEDS_CLASS
>> + help
>> + This option enables support for the brightness_hw_changed attribute
>> + for led sysfs class devices under /sys/class/leds.
>> +
>> + See Documentation/ABI/testing/sysfs-class-led for details.
>> +
>> comment "LED drivers"
>>
>> config LEDS_88PM860X
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 326ee6e..2f8c843 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -103,6 +103,65 @@ static const struct attribute_group *led_groups[] = {
>> NULL,
>> };
>>
>> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>> +static ssize_t brightness_hw_changed_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +
>> + if (led_cdev->brightness_hw_changed == -1)
>> + return -ENODATA;
>> +
>> + return sprintf(buf, "%u\n", led_cdev->brightness_hw_changed);
>> +}
>> +
>> +static DEVICE_ATTR_RO(brightness_hw_changed);
>> +
>> +static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
>> +{
>> + struct device *dev = led_cdev->dev;
>> + int ret;
>> +
>> + ret = device_create_file(dev, &dev_attr_brightness_hw_changed);
>> + if (ret) {
>> + dev_err(dev, "Error creating brightness_hw_changed\n");
>> + return ret;
>> + }
>> +
>> + led_cdev->brightness_hw_changed_kn =
>> + sysfs_get_dirent(dev->kobj.sd, "brightness_hw_changed");
>> + if (!led_cdev->brightness_hw_changed_kn) {
>> + dev_err(dev, "Error getting brightness_hw_changed kn\n");
>> + device_remove_file(dev, &dev_attr_brightness_hw_changed);
>> + return -ENXIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
>> +{
>> + sysfs_put(led_cdev->brightness_hw_changed_kn);
>> + device_remove_file(led_cdev->dev, &dev_attr_brightness_hw_changed);
>> +}
>> +void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev,
>> + enum led_brightness brightness)
>> +{
>> + led_cdev->brightness_hw_changed = brightness;
>> + sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn);
>> +}
>> +EXPORT_SYMBOL_GPL(led_classdev_notify_brightness_hw_changed);
If you forget to set LED_BRIGHT_HW_CHANGED in the driver calling this
API, then you get NULL pointer dereference here due to uninitialized
led_cdev->brightness_hw_changed_kn.
In order to avoid that let's make the
led_classdev_notify_brightness_hw_changed() return int and return
-EINVAL from it if LED_BRIGHT_HW_CHANGED flag is not set.
Moreover, Documentation/leds/leds-class.txt should provide
information that the flag should be set to make the
brightness_hw_changed available. Currently only the commit message
seems to mention that.
>> +#else
>> +static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
>> +{
>> + return 0;
>> +}
>> +static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
>> +{
>> +}
>> +#endif
>> +
>> /**
>> * led_classdev_suspend - suspend an led_classdev.
>> * @led_cdev: the led_classdev to suspend.
>> @@ -204,10 +263,21 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>> dev_warn(parent, "Led %s renamed to %s due to name collision",
>> led_cdev->name, dev_name(led_cdev->dev));
>>
>> + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>> + ret = led_add_brightness_hw_changed(led_cdev);
>> + if (ret) {
>> + device_unregister(led_cdev->dev);
>> + return ret;
>> + }
>> + }
>> +
>> led_cdev->work_flags = 0;
>> #ifdef CONFIG_LEDS_TRIGGERS
>> init_rwsem(&led_cdev->trigger_lock);
>> #endif
>> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>> + led_cdev->brightness_hw_changed = -1;
>> +#endif
>> mutex_init(&led_cdev->led_access);
>> /* add to the list of leds */
>> down_write(&leds_list_lock);
>> @@ -256,6 +326,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>>
>> flush_work(&led_cdev->set_brightness_work);
>>
>> + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED)
>> + led_remove_brightness_hw_changed(led_cdev);
>> +
>> device_unregister(led_cdev->dev);
>>
>> down_write(&leds_list_lock);
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 569cb53..c771153 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -13,6 +13,7 @@
>> #define __LINUX_LEDS_H_INCLUDED
>>
>> #include <linux/device.h>
>> +#include <linux/kernfs.h>
>> #include <linux/list.h>
>> #include <linux/mutex.h>
>> #include <linux/rwsem.h>
>> @@ -46,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_BRIGHT_HW_CHANGED (1 << 21)
>>
>> /* set_brightness_work / blink_timer flags, atomic, private. */
>> unsigned long work_flags;
>> @@ -110,6 +112,11 @@ struct led_classdev {
>> bool activated;
>> #endif
>>
>> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>> + int brightness_hw_changed;
>> + struct kernfs_node *brightness_hw_changed_kn;
>> +#endif
>> +
>> /* Ensures consistent access to the LED Flash Class device */
>> struct mutex led_access;
>> };
>> @@ -422,4 +429,12 @@ static inline void ledtrig_cpu(enum cpu_led_event evt)
>> }
>> #endif
>>
>> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>> +extern void led_classdev_notify_brightness_hw_changed(
>> + struct led_classdev *led_cdev, enum led_brightness brightness);
>> +#else
>> +static inline void led_classdev_notify_brightness_hw_changed(
>> + struct led_classdev *led_cdev, enum led_brightness brightness) { }
>> +#endif
>> +
>> #endif /* __LINUX_LEDS_H_INCLUDED */
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v7] leds: class: Add new optional brightness_hw_changed attribute
2017-01-29 11:48 ` Jacek Anaszewski
@ 2017-01-29 13:21 ` Hans de Goede
0 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2017-01-29 13:21 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek
Cc: Darren Hart, Henrique de Moraes Holschuh, Pali Rohár,
linux-leds, platform-driver-x86
Hi,
On 01/29/2017 12:48 PM, Jacek Anaszewski wrote:
> Hi Hans,
>
> Thanks for addressing my remarks.
> While testing the feature I came across one issue, please refer below.
>
> On 01/27/2017 12:58 PM, Pavel Machek wrote:
>> On Fri 2017-01-27 08:41:35, Hans de Goede wrote:
>>> Some LEDs may have their brightness level changed autonomously
>>> (outside of kernel control) by hardware / firmware. This commit
>>> adds support for an optional brightness_hw_changed attribute to
>>> signal such changes to userspace (if a driver can detect them):
>>>
>>> What: /sys/class/leds/<led>/brightness_hw_changed
>>> Date: January 2017
>>> KernelVersion: 4.11
>>> Description:
>>> Last hardware set brightness level for this LED. Some LEDs
>>> may be changed autonomously by hardware/firmware. Only LEDs
>>> where this happens and the driver can detect this, will
>>> have this file.
>>>
>>> This file supports poll() to detect when the hardware
>>> changes the brightness.
>>>
>>> Reading this file will return the last brightness level set
>>> by the hardware, this may be different from the current
>>> brightness.
>>>
>>> Drivers which want to support this, simply add LED_BRIGHT_HW_CHANGED to
>>> their flags field and call led_classdev_notify_brightness_hw_changed()
>>> with the hardware set brightness when they detect a hardware / firmware
>>> triggered brightness change.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>
>>> ---
>>> Changes in v6:
>>> -New patch in v6 of this set, replacing previous trigger based API
>>> Changes in v7
>>> -Return -ENODATA when brightness_hw_changed gets read before any
>>> hw brightness change event as happened
>>> -Make the hw_brightness_changed attr presence configurable through Kconfig
>>> ---
>>> Documentation/ABI/testing/sysfs-class-led | 17 +++++++
>>> drivers/leds/Kconfig | 9 ++++
>>> drivers/leds/led-class.c | 73 +++++++++++++++++++++++++++++++
>>> include/linux/leds.h | 15 +++++++
>>> 4 files changed, 114 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
>>> index 491cdee..5f67f7a 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>> @@ -23,6 +23,23 @@ Description:
>>> If the LED does not support different brightness levels, this
>>> should be 1.
>>>
>>> +What: /sys/class/leds/<led>/brightness_hw_changed
>>> +Date: January 2017
>>> +KernelVersion: 4.11
>>> +Description:
>>> + Last hardware set brightness level for this LED. Some LEDs
>>> + may be changed autonomously by hardware/firmware. Only LEDs
>>> + where this happens and the driver can detect this, will have
>>> + this file.
>>> +
>>> + This file supports poll() to detect when the hardware changes
>>> + the brightness.
>>> +
>>> + Reading this file will return the last brightness level set
>>> + by the hardware, this may be different from the current
>>> + brightness. Reading this file when no hw brightness change
>>> + event has happened will return an ENODATA error.
>>> +
>>> What: /sys/class/leds/<led>/trigger
>>> Date: March 2006
>>> KernelVersion: 2.6.17
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index c621cbb..275f467 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -29,6 +29,15 @@ config LEDS_CLASS_FLASH
>>> for the flash related features of a LED device. It can be built
>>> as a module.
>>>
>>> +config LEDS_BRIGHTNESS_HW_CHANGED
>>> + bool "LED Class brightness_hw_changed attribute support"
>>> + depends on LEDS_CLASS
>>> + help
>>> + This option enables support for the brightness_hw_changed attribute
>>> + for led sysfs class devices under /sys/class/leds.
>>> +
>>> + See Documentation/ABI/testing/sysfs-class-led for details.
>>> +
>>> comment "LED drivers"
>>>
>>> config LEDS_88PM860X
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 326ee6e..2f8c843 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -103,6 +103,65 @@ static const struct attribute_group *led_groups[] = {
>>> NULL,
>>> };
>>>
>>> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>>> +static ssize_t brightness_hw_changed_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> +
>>> + if (led_cdev->brightness_hw_changed == -1)
>>> + return -ENODATA;
>>> +
>>> + return sprintf(buf, "%u\n", led_cdev->brightness_hw_changed);
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(brightness_hw_changed);
>>> +
>>> +static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
>>> +{
>>> + struct device *dev = led_cdev->dev;
>>> + int ret;
>>> +
>>> + ret = device_create_file(dev, &dev_attr_brightness_hw_changed);
>>> + if (ret) {
>>> + dev_err(dev, "Error creating brightness_hw_changed\n");
>>> + return ret;
>>> + }
>>> +
>>> + led_cdev->brightness_hw_changed_kn =
>>> + sysfs_get_dirent(dev->kobj.sd, "brightness_hw_changed");
>>> + if (!led_cdev->brightness_hw_changed_kn) {
>>> + dev_err(dev, "Error getting brightness_hw_changed kn\n");
>>> + device_remove_file(dev, &dev_attr_brightness_hw_changed);
>>> + return -ENXIO;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
>>> +{
>>> + sysfs_put(led_cdev->brightness_hw_changed_kn);
>>> + device_remove_file(led_cdev->dev, &dev_attr_brightness_hw_changed);
>>> +}
>>> +void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev,
>>> + enum led_brightness brightness)
>>> +{
>>> + led_cdev->brightness_hw_changed = brightness;
>>> + sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn);
>>> +}
>>> +EXPORT_SYMBOL_GPL(led_classdev_notify_brightness_hw_changed);
>
> If you forget to set LED_BRIGHT_HW_CHANGED in the driver calling this
> API, then you get NULL pointer dereference here due to uninitialized
> led_cdev->brightness_hw_changed_kn.
>
> In order to avoid that let's make the
> led_classdev_notify_brightness_hw_changed() return int and return
> -EINVAL from it if LED_BRIGHT_HW_CHANGED flag is not set.
That requires adding error checking to all callers, since this
error will only happen if there is a bug in the kernel / driver
code I've instead done this:
void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
if (WARN_ON(!led_cdev->brightness_hw_changed_kn))
return;
led_cdev->brightness_hw_changed = brightness;
sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn);
}
With avoids the need for error checks in all callers, while still catching
the error.
> Moreover, Documentation/leds/leds-class.txt should provide
> information that the flag should be set to make the
> brightness_hw_changed available. Currently only the commit message
> seems to mention that.
Ok, I will update Documentation/leds/leds-class.txt for v8.
Regards,
Hans
>
>>> +#else
>>> +static int led_add_brightness_hw_changed(struct led_classdev *led_cdev)
>>> +{
>>> + return 0;
>>> +}
>>> +static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
>>> +{
>>> +}
>>> +#endif
>>> +
>>> /**
>>> * led_classdev_suspend - suspend an led_classdev.
>>> * @led_cdev: the led_classdev to suspend.
>>> @@ -204,10 +263,21 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>>> dev_warn(parent, "Led %s renamed to %s due to name collision",
>>> led_cdev->name, dev_name(led_cdev->dev));
>>>
>>> + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>>> + ret = led_add_brightness_hw_changed(led_cdev);
>>> + if (ret) {
>>> + device_unregister(led_cdev->dev);
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> led_cdev->work_flags = 0;
>>> #ifdef CONFIG_LEDS_TRIGGERS
>>> init_rwsem(&led_cdev->trigger_lock);
>>> #endif
>>> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>>> + led_cdev->brightness_hw_changed = -1;
>>> +#endif
>>> mutex_init(&led_cdev->led_access);
>>> /* add to the list of leds */
>>> down_write(&leds_list_lock);
>>> @@ -256,6 +326,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>>>
>>> flush_work(&led_cdev->set_brightness_work);
>>>
>>> + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED)
>>> + led_remove_brightness_hw_changed(led_cdev);
>>> +
>>> device_unregister(led_cdev->dev);
>>>
>>> down_write(&leds_list_lock);
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index 569cb53..c771153 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -13,6 +13,7 @@
>>> #define __LINUX_LEDS_H_INCLUDED
>>>
>>> #include <linux/device.h>
>>> +#include <linux/kernfs.h>
>>> #include <linux/list.h>
>>> #include <linux/mutex.h>
>>> #include <linux/rwsem.h>
>>> @@ -46,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_BRIGHT_HW_CHANGED (1 << 21)
>>>
>>> /* set_brightness_work / blink_timer flags, atomic, private. */
>>> unsigned long work_flags;
>>> @@ -110,6 +112,11 @@ struct led_classdev {
>>> bool activated;
>>> #endif
>>>
>>> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>>> + int brightness_hw_changed;
>>> + struct kernfs_node *brightness_hw_changed_kn;
>>> +#endif
>>> +
>>> /* Ensures consistent access to the LED Flash Class device */
>>> struct mutex led_access;
>>> };
>>> @@ -422,4 +429,12 @@ static inline void ledtrig_cpu(enum cpu_led_event evt)
>>> }
>>> #endif
>>>
>>> +#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>>> +extern void led_classdev_notify_brightness_hw_changed(
>>> + struct led_classdev *led_cdev, enum led_brightness brightness);
>>> +#else
>>> +static inline void led_classdev_notify_brightness_hw_changed(
>>> + struct led_classdev *led_cdev, enum led_brightness brightness) { }
>>> +#endif
>>> +
>>> #endif /* __LINUX_LEDS_H_INCLUDED */
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-29 13:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-27 7:41 [PATCH v7] leds: class: Add new optional brightness_hw_changed attribute Hans de Goede
2017-01-27 11:58 ` Pavel Machek
2017-01-29 11:48 ` Jacek Anaszewski
2017-01-29 13:21 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.