Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
However, the size of this file is limited to PAGE_SIZE because of the
limitation for sysfs attribute.
Enabling LED CPU trigger on systems with thousands of CPUs easily hits
PAGE_SIZE limit, and makes it impossible to see all available LED triggers
and which trigger is currently activated.
This converts /sys/class/leds/<led>/trigger to bin attribute and removes
the PAGE_SIZE limitation.
Cc: Jacek Anaszewski <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Dan Murphy <[email protected]>
Signed-off-by: Akinobu Mita <[email protected]>
---
drivers/leds/led-class.c | 8 ++---
drivers/leds/led-triggers.c | 84 ++++++++++++++++++++++++++++++++++-----------
drivers/leds/leds.h | 6 ++++
include/linux/leds.h | 5 ---
4 files changed, 74 insertions(+), 29 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 4793e77..8b5a1d1 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -73,13 +73,13 @@ static ssize_t max_brightness_show(struct device *dev,
static DEVICE_ATTR_RO(max_brightness);
#ifdef CONFIG_LEDS_TRIGGERS
-static DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
-static struct attribute *led_trigger_attrs[] = {
- &dev_attr_trigger.attr,
+static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write, 0);
+static struct bin_attribute *led_trigger_bin_attrs[] = {
+ &bin_attr_trigger,
NULL,
};
static const struct attribute_group led_trigger_group = {
- .attrs = led_trigger_attrs,
+ .bin_attrs = led_trigger_bin_attrs,
};
#endif
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 8d11a5e..4788e00 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -16,6 +16,7 @@
#include <linux/rwsem.h>
#include <linux/leds.h>
#include <linux/slab.h>
+#include <linux/mm.h>
#include "leds.h"
/*
@@ -26,9 +27,11 @@ LIST_HEAD(trigger_list);
/* Used by LED Class */
-ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
+ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t pos, size_t count)
{
+ struct device *dev = kobj_to_dev(kobj);
struct led_classdev *led_cdev = dev_get_drvdata(dev);
struct led_trigger *trig;
int ret = count;
@@ -64,39 +67,80 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
mutex_unlock(&led_cdev->led_access);
return ret;
}
-EXPORT_SYMBOL_GPL(led_trigger_store);
+EXPORT_SYMBOL_GPL(led_trigger_write);
-ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
- char *buf)
+__printf(4, 5)
+static int led_trigger_snprintf(char *buf, size_t size, bool query,
+ const char *fmt, ...)
+{
+ va_list args;
+ int i;
+
+ va_start(args, fmt);
+ if (query)
+ i = vsnprintf(NULL, 0, fmt, args);
+ else
+ i = vscnprintf(buf, size, fmt, args);
+ va_end(args);
+
+ return i;
+}
+
+static int led_trigger_format(char *buf, size_t size, bool query,
+ struct led_classdev *led_cdev)
{
- struct led_classdev *led_cdev = dev_get_drvdata(dev);
struct led_trigger *trig;
int len = 0;
+ len += led_trigger_snprintf(buf + len, size - len, query, "%s",
+ led_cdev->trigger ? "none" : "[none]");
+
+ list_for_each_entry(trig, &trigger_list, next_trig) {
+ bool hit = led_cdev->trigger &&
+ !strcmp(led_cdev->trigger->name, trig->name);
+
+ len += led_trigger_snprintf(buf + len, size - len, query,
+ " %s%s%s", hit ? "[" : "",
+ trig->name, hit ? "]" : "");
+ }
+
+ len += led_trigger_snprintf(buf + len, size - len, query, "\n");
+
+ return len;
+}
+
+ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t count)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ void *data;
+ int len;
+
down_read(&triggers_list_lock);
down_read(&led_cdev->trigger_lock);
- if (!led_cdev->trigger)
- len += scnprintf(buf+len, PAGE_SIZE - len, "[none] ");
+ len = led_trigger_format(NULL, 0, true, led_cdev);
+ data = kvmalloc(len + 1, GFP_KERNEL);
+ if (data)
+ len = led_trigger_format(data, len + 1, false, led_cdev);
else
- len += scnprintf(buf+len, PAGE_SIZE - len, "none ");
+ len = -ENOMEM;
- list_for_each_entry(trig, &trigger_list, next_trig) {
- if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
- trig->name))
- len += scnprintf(buf+len, PAGE_SIZE - len, "[%s] ",
- trig->name);
- else
- len += scnprintf(buf+len, PAGE_SIZE - len, "%s ",
- trig->name);
- }
up_read(&led_cdev->trigger_lock);
up_read(&triggers_list_lock);
- len += scnprintf(len+buf, PAGE_SIZE - len, "\n");
+ if (len < 0)
+ return len;
+
+ len = memory_read_from_buffer(buf, count, &pos, data, len);
+
+ kvfree(data);
+
return len;
}
-EXPORT_SYMBOL_GPL(led_trigger_show);
+EXPORT_SYMBOL_GPL(led_trigger_read);
/* Caller must ensure led_cdev->trigger_lock held */
int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 47b2294..a0ee33c 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -23,6 +23,12 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
enum led_brightness value);
void led_set_brightness_nosleep(struct led_classdev *led_cdev,
enum led_brightness value);
+ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t count);
+ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t pos, size_t count);
extern struct rw_semaphore leds_list_lock;
extern struct list_head leds_list;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index fd2eb7c..33ae825 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -290,11 +290,6 @@ struct led_trigger {
#define led_trigger_get_led(dev) ((struct led_classdev *)dev_get_drvdata((dev)))
#define led_trigger_get_drvdata(dev) (led_get_trigger_data(led_trigger_get_led(dev)))
-ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count);
-ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
- char *buf);
-
/* Registration functions for complex triggers */
extern int led_trigger_register(struct led_trigger *trigger);
extern void led_trigger_unregister(struct led_trigger *trigger);
--
2.7.4
Hi Akinobu,
Thank you for the patch.
I have one nit below but in general it looks good to me.
I've tested it with 2000 mtd triggers (~14kB file size)
and it worked flawlessly.
Still, I would like to have ack from Greg for it.
Adding Greg on Cc.
On 8/29/19 4:49 PM, Akinobu Mita wrote:
> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, the size of this file is limited to PAGE_SIZE because of the
> limitation for sysfs attribute.
>
> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> and which trigger is currently activated.
>
> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> the PAGE_SIZE limitation.
>
> Cc: Jacek Anaszewski <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Dan Murphy <[email protected]>
> Signed-off-by: Akinobu Mita <[email protected]>
> ---
> drivers/leds/led-class.c | 8 ++---
> drivers/leds/led-triggers.c | 84 ++++++++++++++++++++++++++++++++++-----------
> drivers/leds/leds.h | 6 ++++
> include/linux/leds.h | 5 ---
> 4 files changed, 74 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 4793e77..8b5a1d1 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -73,13 +73,13 @@ static ssize_t max_brightness_show(struct device *dev,
> static DEVICE_ATTR_RO(max_brightness);
>
> #ifdef CONFIG_LEDS_TRIGGERS
> -static DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
> -static struct attribute *led_trigger_attrs[] = {
> - &dev_attr_trigger.attr,
> +static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write, 0);
> +static struct bin_attribute *led_trigger_bin_attrs[] = {
> + &bin_attr_trigger,
> NULL,
> };
> static const struct attribute_group led_trigger_group = {
> - .attrs = led_trigger_attrs,
> + .bin_attrs = led_trigger_bin_attrs,
> };
> #endif
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 8d11a5e..4788e00 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -16,6 +16,7 @@
> #include <linux/rwsem.h>
> #include <linux/leds.h>
> #include <linux/slab.h>
> +#include <linux/mm.h>
> #include "leds.h"
>
> /*
> @@ -26,9 +27,11 @@ LIST_HEAD(trigger_list);
>
> /* Used by LED Class */
>
> -ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
> - const char *buf, size_t count)
> +ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf,
> + loff_t pos, size_t count)
> {
> + struct device *dev = kobj_to_dev(kobj);
> struct led_classdev *led_cdev = dev_get_drvdata(dev);
> struct led_trigger *trig;
> int ret = count;
> @@ -64,39 +67,80 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
> mutex_unlock(&led_cdev->led_access);
> return ret;
> }
> -EXPORT_SYMBOL_GPL(led_trigger_store);
> +EXPORT_SYMBOL_GPL(led_trigger_write);
>
> -ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
> - char *buf)
> +__printf(4, 5)
> +static int led_trigger_snprintf(char *buf, size_t size, bool query,
> + const char *fmt, ...)
> +{
> + va_list args;
> + int i;
> +
> + va_start(args, fmt);
> + if (query)
> + i = vsnprintf(NULL, 0, fmt, args);
> + else
> + i = vscnprintf(buf, size, fmt, args);
> + va_end(args);
> +
> + return i;
> +}
> +
> +static int led_trigger_format(char *buf, size_t size, bool query,
> + struct led_classdev *led_cdev)
> {
> - struct led_classdev *led_cdev = dev_get_drvdata(dev);
> struct led_trigger *trig;
> int len = 0;
>
> + len += led_trigger_snprintf(buf + len, size - len, query, "%s",
Here we know len is 0 so we can pass just buf and size.
> + led_cdev->trigger ? "none" : "[none]");
> +
> + list_for_each_entry(trig, &trigger_list, next_trig) {
> + bool hit = led_cdev->trigger &&
> + !strcmp(led_cdev->trigger->name, trig->name);
> +
> + len += led_trigger_snprintf(buf + len, size - len, query,
> + " %s%s%s", hit ? "[" : "",
> + trig->name, hit ? "]" : "");
> + }
> +
> + len += led_trigger_snprintf(buf + len, size - len, query, "\n");
> +
> + return len;
> +}
> +
> +ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf,
> + loff_t pos, size_t count)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + void *data;
> + int len;
> +
> down_read(&triggers_list_lock);
> down_read(&led_cdev->trigger_lock);
>
> - if (!led_cdev->trigger)
> - len += scnprintf(buf+len, PAGE_SIZE - len, "[none] ");
> + len = led_trigger_format(NULL, 0, true, led_cdev);
> + data = kvmalloc(len + 1, GFP_KERNEL);
> + if (data)
> + len = led_trigger_format(data, len + 1, false, led_cdev);
> else
> - len += scnprintf(buf+len, PAGE_SIZE - len, "none ");
> + len = -ENOMEM;
>
> - list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
> - trig->name))
> - len += scnprintf(buf+len, PAGE_SIZE - len, "[%s] ",
> - trig->name);
> - else
> - len += scnprintf(buf+len, PAGE_SIZE - len, "%s ",
> - trig->name);
> - }
> up_read(&led_cdev->trigger_lock);
> up_read(&triggers_list_lock);
>
> - len += scnprintf(len+buf, PAGE_SIZE - len, "\n");
> + if (len < 0)
> + return len;
> +
> + len = memory_read_from_buffer(buf, count, &pos, data, len);
> +
> + kvfree(data);
> +
> return len;
> }
> -EXPORT_SYMBOL_GPL(led_trigger_show);
> +EXPORT_SYMBOL_GPL(led_trigger_read);
>
> /* Caller must ensure led_cdev->trigger_lock held */
> int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index 47b2294..a0ee33c 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -23,6 +23,12 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
> enum led_brightness value);
> void led_set_brightness_nosleep(struct led_classdev *led_cdev,
> enum led_brightness value);
> +ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf,
> + loff_t pos, size_t count);
> +ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf,
> + loff_t pos, size_t count);
>
> extern struct rw_semaphore leds_list_lock;
> extern struct list_head leds_list;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index fd2eb7c..33ae825 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -290,11 +290,6 @@ struct led_trigger {
> #define led_trigger_get_led(dev) ((struct led_classdev *)dev_get_drvdata((dev)))
> #define led_trigger_get_drvdata(dev) (led_get_trigger_data(led_trigger_get_led(dev)))
>
> -ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
> - const char *buf, size_t count);
> -ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
> - char *buf);
> -
> /* Registration functions for complex triggers */
> extern int led_trigger_register(struct led_trigger *trigger);
> extern void led_trigger_unregister(struct led_trigger *trigger);
>
--
Best regards,
Jacek Anaszewski
Hi!
> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, the size of this file is limited to PAGE_SIZE because of the
> limitation for sysfs attribute.
>
> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> and which trigger is currently activated.
>
> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> the PAGE_SIZE limitation.
>
> Cc: Jacek Anaszewski <[email protected]>
Nothing obviously wrong from a quick look...
Acked-by: Pavel Machek <[email protected]>
Thanks a lot for doing this!
I believe we should cc Greg here.
BTW if you care about cpuled trigger... it would be cool to turn it into one trigger and pass
a cpu number as a parameter. It is good to see this limit fixed, but real solution is to
register one trigger per driver.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
> Hi Akinobu,
>
> Thank you for the patch.
>
> I have one nit below but in general it looks good to me.
> I've tested it with 2000 mtd triggers (~14kB file size)
> and it worked flawlessly.
>
> Still, I would like to have ack from Greg for it.
>
> Adding Greg on Cc.
>
> On 8/29/19 4:49 PM, Akinobu Mita wrote:
> > Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> > However, the size of this file is limited to PAGE_SIZE because of the
> > limitation for sysfs attribute.
> >
> > Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> > PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> > and which trigger is currently activated.
> >
> > This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> > the PAGE_SIZE limitation.
But this is NOT a binary file. A sysfs binary file is used for when the
kernel passes data to or from hardware without any parsing of the data
by the kernel.
You are not doing that here, you are abusing the "one value per file"
rule of sysfs so much that you are forced to work around the limitation
it put in place on purpose to keep you from doing stuff like this.
Please fix this "correctly" by creating a new api that works properly
and just live with the fact that this file will never work correctly and
move everyone to use the new api instead.
Don't keep on abusing the interface by workarounds like this, it is not
ok.
thanks,
greg k-h
On 9/2/19 8:12 PM, Greg KH wrote:
> On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
>> Hi Akinobu,
>>
>> Thank you for the patch.
>>
>> I have one nit below but in general it looks good to me.
>> I've tested it with 2000 mtd triggers (~14kB file size)
>> and it worked flawlessly.
>>
>> Still, I would like to have ack from Greg for it.
>>
>> Adding Greg on Cc.
>>
>> On 8/29/19 4:49 PM, Akinobu Mita wrote:
>>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
>>> However, the size of this file is limited to PAGE_SIZE because of the
>>> limitation for sysfs attribute.
>>>
>>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
>>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
>>> and which trigger is currently activated.
>>>
>>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
>>> the PAGE_SIZE limitation.
>
> But this is NOT a binary file. A sysfs binary file is used for when the
> kernel passes data to or from hardware without any parsing of the data
> by the kernel.
>
> You are not doing that here, you are abusing the "one value per file"
> rule of sysfs so much that you are forced to work around the limitation
> it put in place on purpose to keep you from doing stuff like this.
>
> Please fix this "correctly" by creating a new api that works properly
> and just live with the fact that this file will never work correctly and
> move everyone to use the new api instead.
>
> Don't keep on abusing the interface by workarounds like this, it is not
> ok.
In the message [0] you pledged to give us exception for that, provided
it will be properly documented in the code. I suppose you now object
because the patch does not meet that condition.
Provided that will be fixed, can we count on your ack for the
implementation of the solution you proposed? :-)
[0] https://lore.kernel.org/lkml/[email protected]/
--
Best regards,
Jacek Anaszewski
On Mon, Sep 02, 2019 at 08:47:02PM +0200, Jacek Anaszewski wrote:
> On 9/2/19 8:12 PM, Greg KH wrote:
> > On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
> >> Hi Akinobu,
> >>
> >> Thank you for the patch.
> >>
> >> I have one nit below but in general it looks good to me.
> >> I've tested it with 2000 mtd triggers (~14kB file size)
> >> and it worked flawlessly.
> >>
> >> Still, I would like to have ack from Greg for it.
> >>
> >> Adding Greg on Cc.
> >>
> >> On 8/29/19 4:49 PM, Akinobu Mita wrote:
> >>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> >>> However, the size of this file is limited to PAGE_SIZE because of the
> >>> limitation for sysfs attribute.
> >>>
> >>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> >>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> >>> and which trigger is currently activated.
> >>>
> >>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> >>> the PAGE_SIZE limitation.
> >
> > But this is NOT a binary file. A sysfs binary file is used for when the
> > kernel passes data to or from hardware without any parsing of the data
> > by the kernel.
> >
> > You are not doing that here, you are abusing the "one value per file"
> > rule of sysfs so much that you are forced to work around the limitation
> > it put in place on purpose to keep you from doing stuff like this.
> >
> > Please fix this "correctly" by creating a new api that works properly
> > and just live with the fact that this file will never work correctly and
> > move everyone to use the new api instead.
> >
> > Don't keep on abusing the interface by workarounds like this, it is not
> > ok.
>
> In the message [0] you pledged to give us exception for that, provided
> it will be properly documented in the code. I suppose you now object
> because the patch does not meet that condition.
Well, I honestly don't remember writing that email, but it was 5 months
and many thousands of emails ago :)
Also, you all didn't document the heck out of this. So no, I really do
not want to see this patch accepted as-is.
> Provided that will be fixed, can we count on your ack for the
> implementation of the solution you proposed? :-)
Let's see the patch that actually implements what I suggested first :)
thanks,
greg k-h
2019年9月3日(火) 4:08 Greg KH <[email protected]>:
>
> On Mon, Sep 02, 2019 at 08:47:02PM +0200, Jacek Anaszewski wrote:
> > On 9/2/19 8:12 PM, Greg KH wrote:
> > > On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
> > >> Hi Akinobu,
> > >>
> > >> Thank you for the patch.
> > >>
> > >> I have one nit below but in general it looks good to me.
> > >> I've tested it with 2000 mtd triggers (~14kB file size)
> > >> and it worked flawlessly.
> > >>
> > >> Still, I would like to have ack from Greg for it.
> > >>
> > >> Adding Greg on Cc.
> > >>
> > >> On 8/29/19 4:49 PM, Akinobu Mita wrote:
> > >>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> > >>> However, the size of this file is limited to PAGE_SIZE because of the
> > >>> limitation for sysfs attribute.
> > >>>
> > >>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> > >>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> > >>> and which trigger is currently activated.
> > >>>
> > >>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> > >>> the PAGE_SIZE limitation.
> > >
> > > But this is NOT a binary file. A sysfs binary file is used for when the
> > > kernel passes data to or from hardware without any parsing of the data
> > > by the kernel.
> > >
> > > You are not doing that here, you are abusing the "one value per file"
> > > rule of sysfs so much that you are forced to work around the limitation
> > > it put in place on purpose to keep you from doing stuff like this.
> > >
> > > Please fix this "correctly" by creating a new api that works properly
> > > and just live with the fact that this file will never work correctly and
> > > move everyone to use the new api instead.
> > >
> > > Don't keep on abusing the interface by workarounds like this, it is not
> > > ok.
> >
> > In the message [0] you pledged to give us exception for that, provided
> > it will be properly documented in the code. I suppose you now object
> > because the patch does not meet that condition.
>
> Well, I honestly don't remember writing that email, but it was 5 months
> and many thousands of emails ago :)
>
> Also, you all didn't document the heck out of this. So no, I really do
> not want to see this patch accepted as-is.
>
> > Provided that will be fixed, can we count on your ack for the
> > implementation of the solution you proposed? :-)
>
> Let's see the patch that actually implements what I suggested first :)
I'd propose introducing a new procfs file (/proc/led-triggers) and new
/sys/class/leds/<led>/current-trigger api.
Reading /proc/led-triggers file shows all available triggers.
This violates "one value per file", but it's a procfs file.
The /sys/class/leds/<led>/current-trigger is almost identical to
/sys/class/leds/<led>/trigger. The only difference is that
'current-trigger' only shows the current trigger name.
This file follows the "one value per file" rule of sysfs.
On Tue, Sep 03, 2019 at 10:55:40PM +0900, Akinobu Mita wrote:
> 2019年9月3日(火) 4:08 Greg KH <[email protected]>:
> >
> > On Mon, Sep 02, 2019 at 08:47:02PM +0200, Jacek Anaszewski wrote:
> > > On 9/2/19 8:12 PM, Greg KH wrote:
> > > > On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
> > > >> Hi Akinobu,
> > > >>
> > > >> Thank you for the patch.
> > > >>
> > > >> I have one nit below but in general it looks good to me.
> > > >> I've tested it with 2000 mtd triggers (~14kB file size)
> > > >> and it worked flawlessly.
> > > >>
> > > >> Still, I would like to have ack from Greg for it.
> > > >>
> > > >> Adding Greg on Cc.
> > > >>
> > > >> On 8/29/19 4:49 PM, Akinobu Mita wrote:
> > > >>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> > > >>> However, the size of this file is limited to PAGE_SIZE because of the
> > > >>> limitation for sysfs attribute.
> > > >>>
> > > >>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> > > >>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> > > >>> and which trigger is currently activated.
> > > >>>
> > > >>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> > > >>> the PAGE_SIZE limitation.
> > > >
> > > > But this is NOT a binary file. A sysfs binary file is used for when the
> > > > kernel passes data to or from hardware without any parsing of the data
> > > > by the kernel.
> > > >
> > > > You are not doing that here, you are abusing the "one value per file"
> > > > rule of sysfs so much that you are forced to work around the limitation
> > > > it put in place on purpose to keep you from doing stuff like this.
> > > >
> > > > Please fix this "correctly" by creating a new api that works properly
> > > > and just live with the fact that this file will never work correctly and
> > > > move everyone to use the new api instead.
> > > >
> > > > Don't keep on abusing the interface by workarounds like this, it is not
> > > > ok.
> > >
> > > In the message [0] you pledged to give us exception for that, provided
> > > it will be properly documented in the code. I suppose you now object
> > > because the patch does not meet that condition.
> >
> > Well, I honestly don't remember writing that email, but it was 5 months
> > and many thousands of emails ago :)
> >
> > Also, you all didn't document the heck out of this. So no, I really do
> > not want to see this patch accepted as-is.
> >
> > > Provided that will be fixed, can we count on your ack for the
> > > implementation of the solution you proposed? :-)
> >
> > Let's see the patch that actually implements what I suggested first :)
>
> I'd propose introducing a new procfs file (/proc/led-triggers) and new
> /sys/class/leds/<led>/current-trigger api.
>
> Reading /proc/led-triggers file shows all available triggers.
> This violates "one value per file", but it's a procfs file.
No, procfs files are ONLY for process-related things. Don't keep the
insanity of this file format by just moving it out of sysfs and into
procfs :)
thanks,
greg k-h
2019年9月3日(火) 23:07 Greg KH <[email protected]>:
>
> On Tue, Sep 03, 2019 at 10:55:40PM +0900, Akinobu Mita wrote:
> > 2019年9月3日(火) 4:08 Greg KH <[email protected]>:
> > >
> > > On Mon, Sep 02, 2019 at 08:47:02PM +0200, Jacek Anaszewski wrote:
> > > > On 9/2/19 8:12 PM, Greg KH wrote:
> > > > > On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
> > > > >> Hi Akinobu,
> > > > >>
> > > > >> Thank you for the patch.
> > > > >>
> > > > >> I have one nit below but in general it looks good to me.
> > > > >> I've tested it with 2000 mtd triggers (~14kB file size)
> > > > >> and it worked flawlessly.
> > > > >>
> > > > >> Still, I would like to have ack from Greg for it.
> > > > >>
> > > > >> Adding Greg on Cc.
> > > > >>
> > > > >> On 8/29/19 4:49 PM, Akinobu Mita wrote:
> > > > >>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> > > > >>> However, the size of this file is limited to PAGE_SIZE because of the
> > > > >>> limitation for sysfs attribute.
> > > > >>>
> > > > >>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> > > > >>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> > > > >>> and which trigger is currently activated.
> > > > >>>
> > > > >>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> > > > >>> the PAGE_SIZE limitation.
> > > > >
> > > > > But this is NOT a binary file. A sysfs binary file is used for when the
> > > > > kernel passes data to or from hardware without any parsing of the data
> > > > > by the kernel.
> > > > >
> > > > > You are not doing that here, you are abusing the "one value per file"
> > > > > rule of sysfs so much that you are forced to work around the limitation
> > > > > it put in place on purpose to keep you from doing stuff like this.
> > > > >
> > > > > Please fix this "correctly" by creating a new api that works properly
> > > > > and just live with the fact that this file will never work correctly and
> > > > > move everyone to use the new api instead.
> > > > >
> > > > > Don't keep on abusing the interface by workarounds like this, it is not
> > > > > ok.
> > > >
> > > > In the message [0] you pledged to give us exception for that, provided
> > > > it will be properly documented in the code. I suppose you now object
> > > > because the patch does not meet that condition.
> > >
> > > Well, I honestly don't remember writing that email, but it was 5 months
> > > and many thousands of emails ago :)
> > >
> > > Also, you all didn't document the heck out of this. So no, I really do
> > > not want to see this patch accepted as-is.
> > >
> > > > Provided that will be fixed, can we count on your ack for the
> > > > implementation of the solution you proposed? :-)
> > >
> > > Let's see the patch that actually implements what I suggested first :)
> >
> > I'd propose introducing a new procfs file (/proc/led-triggers) and new
> > /sys/class/leds/<led>/current-trigger api.
> >
> > Reading /proc/led-triggers file shows all available triggers.
> > This violates "one value per file", but it's a procfs file.
>
> No, procfs files are ONLY for process-related things. Don't keep the
> insanity of this file format by just moving it out of sysfs and into
> procfs :)
I see.
How about creating one file or directory for each led-trigger in
/sys/kernel/led-triggers directory?
e.g.
$ ls /sys/kernel/led-triggers
audio-micmute ide-disk phy0assoc
audio-mute kbd-altgrlock phy0radio
...
hidpp_battery_3-full panic
On Tue, Sep 03, 2019 at 11:21:59PM +0900, Akinobu Mita wrote:
> 2019年9月3日(火) 23:07 Greg KH <[email protected]>:
> >
> > On Tue, Sep 03, 2019 at 10:55:40PM +0900, Akinobu Mita wrote:
> > > 2019年9月3日(火) 4:08 Greg KH <[email protected]>:
> > > >
> > > > On Mon, Sep 02, 2019 at 08:47:02PM +0200, Jacek Anaszewski wrote:
> > > > > On 9/2/19 8:12 PM, Greg KH wrote:
> > > > > > On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
> > > > > >> Hi Akinobu,
> > > > > >>
> > > > > >> Thank you for the patch.
> > > > > >>
> > > > > >> I have one nit below but in general it looks good to me.
> > > > > >> I've tested it with 2000 mtd triggers (~14kB file size)
> > > > > >> and it worked flawlessly.
> > > > > >>
> > > > > >> Still, I would like to have ack from Greg for it.
> > > > > >>
> > > > > >> Adding Greg on Cc.
> > > > > >>
> > > > > >> On 8/29/19 4:49 PM, Akinobu Mita wrote:
> > > > > >>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> > > > > >>> However, the size of this file is limited to PAGE_SIZE because of the
> > > > > >>> limitation for sysfs attribute.
> > > > > >>>
> > > > > >>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> > > > > >>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> > > > > >>> and which trigger is currently activated.
> > > > > >>>
> > > > > >>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> > > > > >>> the PAGE_SIZE limitation.
> > > > > >
> > > > > > But this is NOT a binary file. A sysfs binary file is used for when the
> > > > > > kernel passes data to or from hardware without any parsing of the data
> > > > > > by the kernel.
> > > > > >
> > > > > > You are not doing that here, you are abusing the "one value per file"
> > > > > > rule of sysfs so much that you are forced to work around the limitation
> > > > > > it put in place on purpose to keep you from doing stuff like this.
> > > > > >
> > > > > > Please fix this "correctly" by creating a new api that works properly
> > > > > > and just live with the fact that this file will never work correctly and
> > > > > > move everyone to use the new api instead.
> > > > > >
> > > > > > Don't keep on abusing the interface by workarounds like this, it is not
> > > > > > ok.
> > > > >
> > > > > In the message [0] you pledged to give us exception for that, provided
> > > > > it will be properly documented in the code. I suppose you now object
> > > > > because the patch does not meet that condition.
> > > >
> > > > Well, I honestly don't remember writing that email, but it was 5 months
> > > > and many thousands of emails ago :)
> > > >
> > > > Also, you all didn't document the heck out of this. So no, I really do
> > > > not want to see this patch accepted as-is.
> > > >
> > > > > Provided that will be fixed, can we count on your ack for the
> > > > > implementation of the solution you proposed? :-)
> > > >
> > > > Let's see the patch that actually implements what I suggested first :)
> > >
> > > I'd propose introducing a new procfs file (/proc/led-triggers) and new
> > > /sys/class/leds/<led>/current-trigger api.
> > >
> > > Reading /proc/led-triggers file shows all available triggers.
> > > This violates "one value per file", but it's a procfs file.
> >
> > No, procfs files are ONLY for process-related things. Don't keep the
> > insanity of this file format by just moving it out of sysfs and into
> > procfs :)
>
> I see.
>
> How about creating one file or directory for each led-trigger in
> /sys/kernel/led-triggers directory?
>
> e.g.
>
> $ ls /sys/kernel/led-triggers
> audio-micmute ide-disk phy0assoc
> audio-mute kbd-altgrlock phy0radio
> ...
> hidpp_battery_3-full panic
Fine with me if you think that will work.
On 9/3/19 4:21 PM, Akinobu Mita wrote:
> 2019年9月3日(火) 23:07 Greg KH <[email protected]>:
>>
>> On Tue, Sep 03, 2019 at 10:55:40PM +0900, Akinobu Mita wrote:
>>> 2019年9月3日(火) 4:08 Greg KH <[email protected]>:
>>>>
>>>> On Mon, Sep 02, 2019 at 08:47:02PM +0200, Jacek Anaszewski wrote:
>>>>> On 9/2/19 8:12 PM, Greg KH wrote:
>>>>>> On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
>>>>>>> Hi Akinobu,
>>>>>>>
>>>>>>> Thank you for the patch.
>>>>>>>
>>>>>>> I have one nit below but in general it looks good to me.
>>>>>>> I've tested it with 2000 mtd triggers (~14kB file size)
>>>>>>> and it worked flawlessly.
>>>>>>>
>>>>>>> Still, I would like to have ack from Greg for it.
>>>>>>>
>>>>>>> Adding Greg on Cc.
>>>>>>>
>>>>>>> On 8/29/19 4:49 PM, Akinobu Mita wrote:
>>>>>>>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
>>>>>>>> However, the size of this file is limited to PAGE_SIZE because of the
>>>>>>>> limitation for sysfs attribute.
>>>>>>>>
>>>>>>>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
>>>>>>>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
>>>>>>>> and which trigger is currently activated.
>>>>>>>>
>>>>>>>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
>>>>>>>> the PAGE_SIZE limitation.
>>>>>>
>>>>>> But this is NOT a binary file. A sysfs binary file is used for when the
>>>>>> kernel passes data to or from hardware without any parsing of the data
>>>>>> by the kernel.
>>>>>>
>>>>>> You are not doing that here, you are abusing the "one value per file"
>>>>>> rule of sysfs so much that you are forced to work around the limitation
>>>>>> it put in place on purpose to keep you from doing stuff like this.
>>>>>>
>>>>>> Please fix this "correctly" by creating a new api that works properly
>>>>>> and just live with the fact that this file will never work correctly and
>>>>>> move everyone to use the new api instead.
>>>>>>
>>>>>> Don't keep on abusing the interface by workarounds like this, it is not
>>>>>> ok.
>>>>>
>>>>> In the message [0] you pledged to give us exception for that, provided
>>>>> it will be properly documented in the code. I suppose you now object
>>>>> because the patch does not meet that condition.
>>>>
>>>> Well, I honestly don't remember writing that email, but it was 5 months
>>>> and many thousands of emails ago :)
>>>>
>>>> Also, you all didn't document the heck out of this. So no, I really do
>>>> not want to see this patch accepted as-is.
>>>>
>>>>> Provided that will be fixed, can we count on your ack for the
>>>>> implementation of the solution you proposed? :-)
>>>>
>>>> Let's see the patch that actually implements what I suggested first :)
>>>
>>> I'd propose introducing a new procfs file (/proc/led-triggers) and new
>>> /sys/class/leds/<led>/current-trigger api.
>>>
>>> Reading /proc/led-triggers file shows all available triggers.
>>> This violates "one value per file", but it's a procfs file.
>>
>> No, procfs files are ONLY for process-related things. Don't keep the
>> insanity of this file format by just moving it out of sysfs and into
>> procfs :)
>
> I see.
>
> How about creating one file or directory for each led-trigger in
> /sys/kernel/led-triggers directory?
>
> e.g.
>
> $ ls /sys/kernel/led-triggers
> audio-micmute ide-disk phy0assoc
> audio-mute kbd-altgrlock phy0radio
> ...
> hidpp_battery_3-full panic
I think that /sys/class/leds/triggers would better reflect the reality.
After all LED Trigger core belongs to LED subsystem.
--
Best regards,
Jacek Anaszewski
On Tue, Sep 03, 2019 at 08:18:36PM +0200, Jacek Anaszewski wrote:
> On 9/3/19 4:21 PM, Akinobu Mita wrote:
> > 2019年9月3日(火) 23:07 Greg KH <[email protected]>:
> >>
> >> On Tue, Sep 03, 2019 at 10:55:40PM +0900, Akinobu Mita wrote:
> >>> 2019年9月3日(火) 4:08 Greg KH <[email protected]>:
> >>>>
> >>>> On Mon, Sep 02, 2019 at 08:47:02PM +0200, Jacek Anaszewski wrote:
> >>>>> On 9/2/19 8:12 PM, Greg KH wrote:
> >>>>>> On Sun, Sep 01, 2019 at 06:53:34PM +0200, Jacek Anaszewski wrote:
> >>>>>>> Hi Akinobu,
> >>>>>>>
> >>>>>>> Thank you for the patch.
> >>>>>>>
> >>>>>>> I have one nit below but in general it looks good to me.
> >>>>>>> I've tested it with 2000 mtd triggers (~14kB file size)
> >>>>>>> and it worked flawlessly.
> >>>>>>>
> >>>>>>> Still, I would like to have ack from Greg for it.
> >>>>>>>
> >>>>>>> Adding Greg on Cc.
> >>>>>>>
> >>>>>>> On 8/29/19 4:49 PM, Akinobu Mita wrote:
> >>>>>>>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> >>>>>>>> However, the size of this file is limited to PAGE_SIZE because of the
> >>>>>>>> limitation for sysfs attribute.
> >>>>>>>>
> >>>>>>>> Enabling LED CPU trigger on systems with thousands of CPUs easily hits
> >>>>>>>> PAGE_SIZE limit, and makes it impossible to see all available LED triggers
> >>>>>>>> and which trigger is currently activated.
> >>>>>>>>
> >>>>>>>> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> >>>>>>>> the PAGE_SIZE limitation.
> >>>>>>
> >>>>>> But this is NOT a binary file. A sysfs binary file is used for when the
> >>>>>> kernel passes data to or from hardware without any parsing of the data
> >>>>>> by the kernel.
> >>>>>>
> >>>>>> You are not doing that here, you are abusing the "one value per file"
> >>>>>> rule of sysfs so much that you are forced to work around the limitation
> >>>>>> it put in place on purpose to keep you from doing stuff like this.
> >>>>>>
> >>>>>> Please fix this "correctly" by creating a new api that works properly
> >>>>>> and just live with the fact that this file will never work correctly and
> >>>>>> move everyone to use the new api instead.
> >>>>>>
> >>>>>> Don't keep on abusing the interface by workarounds like this, it is not
> >>>>>> ok.
> >>>>>
> >>>>> In the message [0] you pledged to give us exception for that, provided
> >>>>> it will be properly documented in the code. I suppose you now object
> >>>>> because the patch does not meet that condition.
> >>>>
> >>>> Well, I honestly don't remember writing that email, but it was 5 months
> >>>> and many thousands of emails ago :)
> >>>>
> >>>> Also, you all didn't document the heck out of this. So no, I really do
> >>>> not want to see this patch accepted as-is.
> >>>>
> >>>>> Provided that will be fixed, can we count on your ack for the
> >>>>> implementation of the solution you proposed? :-)
> >>>>
> >>>> Let's see the patch that actually implements what I suggested first :)
> >>>
> >>> I'd propose introducing a new procfs file (/proc/led-triggers) and new
> >>> /sys/class/leds/<led>/current-trigger api.
> >>>
> >>> Reading /proc/led-triggers file shows all available triggers.
> >>> This violates "one value per file", but it's a procfs file.
> >>
> >> No, procfs files are ONLY for process-related things. Don't keep the
> >> insanity of this file format by just moving it out of sysfs and into
> >> procfs :)
> >
> > I see.
> >
> > How about creating one file or directory for each led-trigger in
> > /sys/kernel/led-triggers directory?
> >
> > e.g.
> >
> > $ ls /sys/kernel/led-triggers
> > audio-micmute ide-disk phy0assoc
> > audio-mute kbd-altgrlock phy0radio
> > ...
> > hidpp_battery_3-full panic
> I think that /sys/class/leds/triggers would better reflect the reality.
> After all LED Trigger core belongs to LED subsystem.
Yes, sorry, I missed that "kernel" directory, that's a non-starter, use
the class directory as that is what it is for here.
greg k-h
Hi!
> > Also, you all didn't document the heck out of this. So no, I really do
> > not want to see this patch accepted as-is.
> >
> > > Provided that will be fixed, can we count on your ack for the
> > > implementation of the solution you proposed? :-)
> >
> > Let's see the patch that actually implements what I suggested first :)
>
> I'd propose introducing a new procfs file (/proc/led-triggers) and new
> /sys/class/leds/<led>/current-trigger api.
No.
Your patch is good, it just needs adding comment:
/* It was stupid to create 10000 cpu triggers, but we are
stuck with it now. Don't make that mistake again. We work around it here by creating binary
attribute, which is not limited by length. This is _not_ good design, do not copy it. */
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html