* [PATCH v2 0/1] leds: fix /sys/class/leds/<led>/trigger @ 2019-09-13 15:03 Akinobu Mita 2019-09-13 15:03 ` [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita 0 siblings, 1 reply; 9+ messages in thread From: Akinobu Mita @ 2019-09-13 15:03 UTC (permalink / raw) To: linux-leds, linux-kernel Cc: Akinobu Mita, Greg Kroah-Hartman, Rafael J. Wysocki, Jacek Anaszewski, Pavel Machek, Dan Murphy (Resending with the version tag in the subject) 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 patch converts /sys/class/leds/<led>/trigger to bin attribute and removes the PAGE_SIZE limitation. The first version of this seris provided the new api that follows the "one value per file" rule of sysfs. This second version dropped it because there have been a number of problems and it turns out that the new api should be submitted separately. * v2 - Update commit message - Drop patches for new api Akinobu Mita (1): leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger drivers/leds/led-class.c | 8 ++-- drivers/leds/led-triggers.c | 90 ++++++++++++++++++++++++++++++++++----------- drivers/leds/leds.h | 6 +++ include/linux/leds.h | 5 --- 4 files changed, 79 insertions(+), 30 deletions(-) Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com> Cc: Pavel Machek <pavel@ucw.cz> Cc: Dan Murphy <dmurphy@ti.com> -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger 2019-09-13 15:03 [PATCH v2 0/1] leds: fix /sys/class/leds/<led>/trigger Akinobu Mita @ 2019-09-13 15:03 ` Akinobu Mita 2019-09-24 21:30 ` Jacek Anaszewski 2019-09-27 6:35 ` Greg Kroah-Hartman 0 siblings, 2 replies; 9+ messages in thread From: Akinobu Mita @ 2019-09-13 15:03 UTC (permalink / raw) To: linux-leds, linux-kernel Cc: Akinobu Mita, Greg Kroah-Hartman, Rafael J. Wysocki, Jacek Anaszewski, Pavel Machek, Dan Murphy 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. We work around it here by converting /sys/class/leds/<led>/trigger to binary attribute, which is not limited by length. This is _not_ good design, do not copy it. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com> Cc: Pavel Machek <pavel@ucw.cz> Cc: Dan Murphy <dmurphy@ti.com> Acked-by: Pavel Machek <pavel@ucw.cz> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> --- drivers/leds/led-class.c | 8 ++-- drivers/leds/led-triggers.c | 90 ++++++++++++++++++++++++++++++++++----------- drivers/leds/leds.h | 6 +++ include/linux/leds.h | 5 --- 4 files changed, 79 insertions(+), 30 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..ed5a311 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,84 @@ 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; + int len = led_trigger_snprintf(buf, size, 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; +} + +/* + * 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. + */ +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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger 2019-09-13 15:03 ` [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita @ 2019-09-24 21:30 ` Jacek Anaszewski 2019-09-27 6:27 ` Greg Kroah-Hartman 2019-09-27 6:35 ` Greg Kroah-Hartman 1 sibling, 1 reply; 9+ messages in thread From: Jacek Anaszewski @ 2019-09-24 21:30 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Akinobu Mita, linux-leds, linux-kernel, Rafael J. Wysocki, Pavel Machek, Dan Murphy Hi Greg, Akinobu seems to have addressed all issues that have been raised regarding this patch. I'd be happy to have your ack before applying it. Best regards, Jacek Anaszewski On 9/13/19 5:03 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. > > We work around it here by converting /sys/class/leds/<led>/trigger to > binary attribute, which is not limited by length. This is _not_ good > design, do not copy it. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Dan Murphy <dmurphy@ti.com> > Acked-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > --- > drivers/leds/led-class.c | 8 ++-- > drivers/leds/led-triggers.c | 90 ++++++++++++++++++++++++++++++++++----------- > drivers/leds/leds.h | 6 +++ > include/linux/leds.h | 5 --- > 4 files changed, 79 insertions(+), 30 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..ed5a311 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,84 @@ 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; > + int len = led_trigger_snprintf(buf, size, 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; > +} > + > +/* > + * 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. > + */ > +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); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger 2019-09-24 21:30 ` Jacek Anaszewski @ 2019-09-27 6:27 ` Greg Kroah-Hartman 0 siblings, 0 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2019-09-27 6:27 UTC (permalink / raw) To: Jacek Anaszewski Cc: Akinobu Mita, linux-leds, linux-kernel, Rafael J. Wysocki, Pavel Machek, Dan Murphy On Tue, Sep 24, 2019 at 11:30:08PM +0200, Jacek Anaszewski wrote: > Hi Greg, > > Akinobu seems to have addressed all issues that have been > raised regarding this patch. I'd be happy to have your ack before > applying it. You have to wait until after -rc1 is out before doing anything, so there's no rush :) I'll go review it now... greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger 2019-09-13 15:03 ` [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita 2019-09-24 21:30 ` Jacek Anaszewski @ 2019-09-27 6:35 ` Greg Kroah-Hartman 2019-09-27 16:47 ` Akinobu Mita 1 sibling, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2019-09-27 6:35 UTC (permalink / raw) To: Akinobu Mita Cc: linux-leds, linux-kernel, Rafael J. Wysocki, Jacek Anaszewski, Pavel Machek, Dan Murphy On Sat, Sep 14, 2019 at 12:03:24AM +0900, 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. > > We work around it here by converting /sys/class/leds/<led>/trigger to > binary attribute, which is not limited by length. This is _not_ good > design, do not copy it. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Dan Murphy <dmurphy@ti.com> > Acked-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > --- > drivers/leds/led-class.c | 8 ++-- > drivers/leds/led-triggers.c | 90 ++++++++++++++++++++++++++++++++++----------- > drivers/leds/leds.h | 6 +++ > include/linux/leds.h | 5 --- > 4 files changed, 79 insertions(+), 30 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..ed5a311 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,84 @@ 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; > +} You only call this in one place, why is it needed like this? The "old" code open-coded this, what is this helping with here? And what does "query" mean here? I have no idea how that variable matters, or what it does. Why not just test if buf is NULL or not if you don't want to use it? Ah, you are trying to see how "long" the buffer is going to be. That makes more sense, but just trigger off of the NULL buffer or not, making this a bit more "obvious" what you are doing and not tieing two parameters to each other (meaning one always reflects the other one). > +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; > + int len = led_trigger_snprintf(buf, size, 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; > +} > + > +/* > + * 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. > + */ > +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); Why kvmalloc() and not just kmalloc()? How big is this buffer you are expecting to have here? > + 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); Two locks? Why not 3? 5? How about just 1? :) > > - len += scnprintf(len+buf, PAGE_SIZE - len, "\n"); > + if (len < 0) > + return len; You just leaked data if led_trigger_format() returned an error :( Just return -ENOMEM above if !data, which makes this much simpler. thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger 2019-09-27 6:35 ` Greg Kroah-Hartman @ 2019-09-27 16:47 ` Akinobu Mita 2019-09-27 16:59 ` Pavel Machek 2019-09-27 17:46 ` Greg Kroah-Hartman 0 siblings, 2 replies; 9+ messages in thread From: Akinobu Mita @ 2019-09-27 16:47 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-leds, LKML, Rafael J. Wysocki, Jacek Anaszewski, Pavel Machek, Dan Murphy 2019年9月27日(金) 15:39 Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > > On Sat, Sep 14, 2019 at 12:03:24AM +0900, 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. > > > > We work around it here by converting /sys/class/leds/<led>/trigger to > > binary attribute, which is not limited by length. This is _not_ good > > design, do not copy it. > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com> > > Cc: Pavel Machek <pavel@ucw.cz> > > Cc: Dan Murphy <dmurphy@ti.com> > > Acked-by: Pavel Machek <pavel@ucw.cz> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > --- > > drivers/leds/led-class.c | 8 ++-- > > drivers/leds/led-triggers.c | 90 ++++++++++++++++++++++++++++++++++----------- > > drivers/leds/leds.h | 6 +++ > > include/linux/leds.h | 5 --- > > 4 files changed, 79 insertions(+), 30 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..ed5a311 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,84 @@ 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; > > +} > > You only call this in one place, why is it needed like this? The "old" > code open-coded this, what is this helping with here? > > And what does "query" mean here? I have no idea how that variable > matters, or what it does. Why not just test if buf is NULL or not if > you don't want to use it? > > Ah, you are trying to see how "long" the buffer is going to be. That > makes more sense, but just trigger off of the NULL buffer or not, making > this a bit more "obvious" what you are doing and not tieing two > parameters to each other (meaning one always reflects the other one). We cannot simply replace the "query" by checking the buffer is NULL or not. Because led_trigger_snprintf() is repeatedly called by led_trigger_format() while increasing 'buf' and decreasing 'size' by the return value of led_trigger_snprintf() every time. > > +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; > > + int len = led_trigger_snprintf(buf, size, 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; > > +} > > + > > +/* > > + * 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. > > + */ > > +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); > > Why kvmalloc() and not just kmalloc()? How big is this buffer you are > expecting to have here? The ledtrig-cpu supports upto 9999 cpus. If all these cpus were available, the buffer size would be 78,890 bytes. (for i in `seq 0 9999`;do echo -n " cpu$i"; done | wc -c) The intention of this kvmalloc() allocation is to avoid costly allocation if possible. > > + 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); > > Two locks? Why not 3? 5? How about just 1? :) I don't touch these locks in this patch :) Locking both locks seems to be necessary to prevent someone from changing trigger_list or led_cdev->trigger. > > > > - len += scnprintf(len+buf, PAGE_SIZE - len, "\n"); > > + if (len < 0) > > + return len; > > You just leaked data if led_trigger_format() returned an error :( > > Just return -ENOMEM above if !data, which makes this much simpler. We are holding the two locks, so we need to release them before return. Which one do you prefer? ... data = kvmalloc(len + 1, GFP_KERNEL); if (data) len = led_trigger_format(data, len + 1, false, led_cdev); else len = -ENOMEM; up_read(&led_cdev->trigger_lock); up_read(&triggers_list_lock); if (len < 0) return len; vs. ... data = kvmalloc(len + 1, GFP_KERNEL); if (!data) { up_read(&led_cdev->trigger_lock); up_read(&triggers_list_lock); return -ENOMEM; } len = led_trigger_format(data, len + 1, false, led_cdev); up_read(&led_cdev->trigger_lock); up_read(&triggers_list_lock); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger 2019-09-27 16:47 ` Akinobu Mita @ 2019-09-27 16:59 ` Pavel Machek 2019-09-27 17:46 ` Greg Kroah-Hartman 1 sibling, 0 replies; 9+ messages in thread From: Pavel Machek @ 2019-09-27 16:59 UTC (permalink / raw) To: Akinobu Mita Cc: Greg Kroah-Hartman, linux-leds, LKML, Rafael J. Wysocki, Jacek Anaszewski, Dan Murphy [-- Attachment #1: Type: text/plain, Size: 2370 bytes --] Hi! > > > 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); > > > > Why kvmalloc() and not just kmalloc()? How big is this buffer you are > > expecting to have here? > > The ledtrig-cpu supports upto 9999 cpus. If all these cpus were available, > the buffer size would be 78,890 bytes. > (for i in `seq 0 9999`;do echo -n " cpu$i"; done | wc -c) > > The intention of this kvmalloc() allocation is to avoid costly allocation > if possible. Sounds good. > > > - else > > > - len += scnprintf(buf+len, PAGE_SIZE - len, "%s ", > > > - trig->name); > > > - } > > > up_read(&led_cdev->trigger_lock); > > > up_read(&triggers_list_lock); > > > > Two locks? Why not 3? 5? How about just 1? :) > > I don't touch these locks in this patch :) Nor should you :-). > > Just return -ENOMEM above if !data, which makes this much simpler. > > We are holding the two locks, so we need to release them before return. > Which one do you prefer? > > ... > data = kvmalloc(len + 1, GFP_KERNEL); > if (data) > len = led_trigger_format(data, len + 1, false, led_cdev); > else > len = -ENOMEM; > > up_read(&led_cdev->trigger_lock); > up_read(&triggers_list_lock); > > if (len < 0) > return len; > > vs. > > ... > data = kvmalloc(len + 1, GFP_KERNEL); > if (!data) { > up_read(&led_cdev->trigger_lock); > up_read(&triggers_list_lock); > return -ENOMEM; > } > len = led_trigger_format(data, len + 1, false, led_cdev); > > up_read(&led_cdev->trigger_lock); > up_read(&triggers_list_lock); Second one is better. Using goto to jump to error path doing cleanups is also acceptable. Thanks, Pavel -- (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] 9+ messages in thread
* Re: [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger 2019-09-27 16:47 ` Akinobu Mita 2019-09-27 16:59 ` Pavel Machek @ 2019-09-27 17:46 ` Greg Kroah-Hartman 2019-09-28 4:08 ` Akinobu Mita 1 sibling, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2019-09-27 17:46 UTC (permalink / raw) To: Akinobu Mita Cc: linux-leds, LKML, Rafael J. Wysocki, Jacek Anaszewski, Pavel Machek, Dan Murphy On Sat, Sep 28, 2019 at 01:47:21AM +0900, Akinobu Mita wrote: > 2019年9月27日(金) 15:39 Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > > > > On Sat, Sep 14, 2019 at 12:03:24AM +0900, 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. > > > > > > We work around it here by converting /sys/class/leds/<led>/trigger to > > > binary attribute, which is not limited by length. This is _not_ good > > > design, do not copy it. > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com> > > > Cc: Pavel Machek <pavel@ucw.cz> > > > Cc: Dan Murphy <dmurphy@ti.com> > > > Acked-by: Pavel Machek <pavel@ucw.cz> > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > > --- > > > drivers/leds/led-class.c | 8 ++-- > > > drivers/leds/led-triggers.c | 90 ++++++++++++++++++++++++++++++++++----------- > > > drivers/leds/leds.h | 6 +++ > > > include/linux/leds.h | 5 --- > > > 4 files changed, 79 insertions(+), 30 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..ed5a311 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,84 @@ 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; > > > +} > > > > You only call this in one place, why is it needed like this? The "old" > > code open-coded this, what is this helping with here? > > > > And what does "query" mean here? I have no idea how that variable > > matters, or what it does. Why not just test if buf is NULL or not if > > you don't want to use it? > > > > Ah, you are trying to see how "long" the buffer is going to be. That > > makes more sense, but just trigger off of the NULL buffer or not, making > > this a bit more "obvious" what you are doing and not tieing two > > parameters to each other (meaning one always reflects the other one). > > We cannot simply replace the "query" by checking the buffer is NULL or not. > Because led_trigger_snprintf() is repeatedly called by led_trigger_format() > while increasing 'buf' and decreasing 'size' by the return value of > led_trigger_snprintf() every time. Odd, but ok, I'll trust you, this looks like very odd code :( > > > +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; > > > + int len = led_trigger_snprintf(buf, size, 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; > > > +} > > > + > > > +/* > > > + * 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. > > > + */ > > > +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); > > > > Why kvmalloc() and not just kmalloc()? How big is this buffer you are > > expecting to have here? > > The ledtrig-cpu supports upto 9999 cpus. If all these cpus were available, > the buffer size would be 78,890 bytes. > (for i in `seq 0 9999`;do echo -n " cpu$i"; done | wc -c) > > The intention of this kvmalloc() allocation is to avoid costly allocation > if possible. Ah, forgot it could get that big. > > > + 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); > > > > Two locks? Why not 3? 5? How about just 1? :) > > I don't touch these locks in this patch :) > > Locking both locks seems to be necessary to prevent someone from changing > trigger_list or led_cdev->trigger. Ok, it just looked odd to me. > > > - len += scnprintf(len+buf, PAGE_SIZE - len, "\n"); > > > + if (len < 0) > > > + return len; > > > > You just leaked data if led_trigger_format() returned an error :( > > > > Just return -ENOMEM above if !data, which makes this much simpler. > > We are holding the two locks, so we need to release them before return. > Which one do you prefer? > > ... > data = kvmalloc(len + 1, GFP_KERNEL); > if (data) > len = led_trigger_format(data, len + 1, false, led_cdev); > else > len = -ENOMEM; > > up_read(&led_cdev->trigger_lock); > up_read(&triggers_list_lock); > > if (len < 0) > return len; > > vs. > > ... > data = kvmalloc(len + 1, GFP_KERNEL); > if (!data) { > up_read(&led_cdev->trigger_lock); > up_read(&triggers_list_lock); > return -ENOMEM; > } > len = led_trigger_format(data, len + 1, false, led_cdev); > > up_read(&led_cdev->trigger_lock); > up_read(&triggers_list_lock); I don't care, as long as you do not leak memory I'm happy :) thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger 2019-09-27 17:46 ` Greg Kroah-Hartman @ 2019-09-28 4:08 ` Akinobu Mita 0 siblings, 0 replies; 9+ messages in thread From: Akinobu Mita @ 2019-09-28 4:08 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-leds, LKML, Rafael J. Wysocki, Jacek Anaszewski, Pavel Machek, Dan Murphy 2019年9月28日(土) 2:46 Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > > On Sat, Sep 28, 2019 at 01:47:21AM +0900, Akinobu Mita wrote: > > 2019年9月27日(金) 15:39 Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > > > > > > On Sat, Sep 14, 2019 at 12:03:24AM +0900, 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. > > > > > > > > We work around it here by converting /sys/class/leds/<led>/trigger to > > > > binary attribute, which is not limited by length. This is _not_ good > > > > design, do not copy it. > > > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > > > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com> > > > > Cc: Pavel Machek <pavel@ucw.cz> > > > > Cc: Dan Murphy <dmurphy@ti.com> > > > > Acked-by: Pavel Machek <pavel@ucw.cz> > > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > > > --- > > > > drivers/leds/led-class.c | 8 ++-- > > > > drivers/leds/led-triggers.c | 90 ++++++++++++++++++++++++++++++++++----------- > > > > drivers/leds/leds.h | 6 +++ > > > > include/linux/leds.h | 5 --- > > > > 4 files changed, 79 insertions(+), 30 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..ed5a311 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,84 @@ 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; > > > > +} > > > > > > You only call this in one place, why is it needed like this? The "old" > > > code open-coded this, what is this helping with here? > > > > > > And what does "query" mean here? I have no idea how that variable > > > matters, or what it does. Why not just test if buf is NULL or not if > > > you don't want to use it? > > > > > > Ah, you are trying to see how "long" the buffer is going to be. That > > > makes more sense, but just trigger off of the NULL buffer or not, making > > > this a bit more "obvious" what you are doing and not tieing two > > > parameters to each other (meaning one always reflects the other one). > > > > We cannot simply replace the "query" by checking the buffer is NULL or not. > > Because led_trigger_snprintf() is repeatedly called by led_trigger_format() > > while increasing 'buf' and decreasing 'size' by the return value of > > led_trigger_snprintf() every time. > > Odd, but ok, I'll trust you, this looks like very odd code :( We can remove the "query" by checking if "size" is less than or equal to zero in led_trigger_snprintf(), instead of checking "buf". Because the "size" keeps decreasing every time led_trigger_snprintf() is called by led_trigger_format(). ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-09-28 4:08 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-13 15:03 [PATCH v2 0/1] leds: fix /sys/class/leds/<led>/trigger Akinobu Mita 2019-09-13 15:03 ` [PATCH v2 1/1] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita 2019-09-24 21:30 ` Jacek Anaszewski 2019-09-27 6:27 ` Greg Kroah-Hartman 2019-09-27 6:35 ` Greg Kroah-Hartman 2019-09-27 16:47 ` Akinobu Mita 2019-09-27 16:59 ` Pavel Machek 2019-09-27 17:46 ` Greg Kroah-Hartman 2019-09-28 4:08 ` Akinobu Mita
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.