Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] leds: fix /sys/class/leds/<led>/trigger and add new api
@ 2019-09-08 12:41 Akinobu Mita
  2019-09-08 12:41 ` [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Akinobu Mita @ 2019-09-08 12:41 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.

The first patch in this series converts /sys/class/leds/<led>/trigger to
bin attribute and removes the PAGE_SIZE limitation.

The rest of series provides a new /sys/class/triggers/ directory and
/sys/class/leds/<led>/current-trigger. The new api follows the "one value
per file" rule of sysfs.

Akinobu Mita (5):
  leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger
  leds: make sure leds_class is initialized before triggers are
    registered
  driver core: class: add function to create /sys/class/<class>/foo
    directory
  leds: add /sys/class/triggers/ that contains trigger sub-directories
  leds: add /sys/class/leds/<led>/current-trigger

 Documentation/ABI/testing/sysfs-class-led |  22 +++++
 drivers/base/class.c                      |   7 ++
 drivers/leds/led-class.c                  |  49 +++++++++--
 drivers/leds/led-triggers.c               | 139 +++++++++++++++++++++++++-----
 drivers/leds/leds.h                       |  12 +++
 include/linux/device.h                    |   3 +
 include/linux/leds.h                      |   6 +-
 7 files changed, 207 insertions(+), 31 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] 17+ messages in thread

* [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger
  2019-09-08 12:41 [PATCH 0/5] leds: fix /sys/class/leds/<led>/trigger and add new api Akinobu Mita
@ 2019-09-08 12:41 ` Akinobu Mita
  2019-09-08 13:10   ` Greg Kroah-Hartman
  2019-09-08 13:18   ` Greg Kroah-Hartman
  2019-09-08 12:41 ` [PATCH 2/5] leds: make sure leds_class is initialized before triggers are registered Akinobu Mita
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 17+ messages in thread
From: Akinobu Mita @ 2019-09-08 12:41 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.

This converts /sys/class/leds/<led>/trigger to bin attribute and removes
the PAGE_SIZE limitation.

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	[flat|nested] 17+ messages in thread

* [PATCH 2/5] leds: make sure leds_class is initialized before triggers are registered
  2019-09-08 12:41 [PATCH 0/5] leds: fix /sys/class/leds/<led>/trigger and add new api Akinobu Mita
  2019-09-08 12:41 ` [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita
@ 2019-09-08 12:41 ` Akinobu Mita
  2019-09-08 13:07   ` Greg Kroah-Hartman
  2019-09-08 12:41 ` [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory Akinobu Mita
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Akinobu Mita @ 2019-09-08 12:41 UTC (permalink / raw)
  To: linux-leds, linux-kernel
  Cc: Akinobu Mita, Greg Kroah-Hartman, Rafael J. Wysocki,
	Jacek Anaszewski, Pavel Machek, Dan Murphy

If the led-class and usb-common modules are built into the kernel, the
usb-common module could be initialized earlier than the led-class module.

So when the ledtrig_usb_gadget and ledtrig_usb_host LED triggers are
registered by usb-common module, the leds_class could not be initialized
yet.

We are going to populate sub-directories, each representing an LED
trigger in /sys/class/triggers/, so leds_class needs to be initialized
before any LED triggers is registered.

This makes led-class initialize earlier then usb-common by changing
initcall group.

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>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/leds/led-class.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 8b5a1d1..7d85181 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -424,7 +424,7 @@ static void __exit leds_exit(void)
 	class_destroy(leds_class);
 }
 
-subsys_initcall(leds_init);
+postcore_initcall(leds_init);
 module_exit(leds_exit);
 
 MODULE_AUTHOR("John Lenz, Richard Purdie");
-- 
2.7.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory
  2019-09-08 12:41 [PATCH 0/5] leds: fix /sys/class/leds/<led>/trigger and add new api Akinobu Mita
  2019-09-08 12:41 ` [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita
  2019-09-08 12:41 ` [PATCH 2/5] leds: make sure leds_class is initialized before triggers are registered Akinobu Mita
@ 2019-09-08 12:41 ` Akinobu Mita
  2019-09-08 13:08   ` Greg Kroah-Hartman
                     ` (2 more replies)
  2019-09-08 12:41 ` [PATCH 4/5] leds: add /sys/class/triggers/ that contains trigger sub-directories Akinobu Mita
  2019-09-08 12:41 ` [PATCH 5/5] leds: add /sys/class/leds/<led>/current-trigger Akinobu Mita
  4 siblings, 3 replies; 17+ messages in thread
From: Akinobu Mita @ 2019-09-08 12:41 UTC (permalink / raw)
  To: linux-leds, linux-kernel
  Cc: Akinobu Mita, Greg Kroah-Hartman, Rafael J. Wysocki,
	Jacek Anaszewski, Pavel Machek, Dan Murphy

This adds a new function class_kobject_create_and_add() that creates a
directory in the /sys/class/<class>.

This function is required to create the /sys/class/leds/triggers directory
that contains all available LED triggers.

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>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/base/class.c   | 7 +++++++
 include/linux/device.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index d8a6a58..f4c53e7 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -104,6 +104,13 @@ void class_remove_file_ns(struct class *cls, const struct class_attribute *attr,
 		sysfs_remove_file_ns(&cls->p->subsys.kobj, &attr->attr, ns);
 }
 
+struct kobject *class_kobject_create_and_add(const char *name,
+					     struct class *cls)
+{
+	return kobject_create_and_add(name, &cls->p->subsys.kobj);
+}
+EXPORT_SYMBOL_GPL(class_kobject_create_and_add);
+
 static struct class *class_get(struct class *cls)
 {
 	if (cls)
diff --git a/include/linux/device.h b/include/linux/device.h
index 6717ade..335e901 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -505,6 +505,9 @@ static inline void class_remove_file(struct class *class,
 	return class_remove_file_ns(class, attr, NULL);
 }
 
+struct kobject * __must_check class_kobject_create_and_add(const char *name,
+							   struct class *cls);
+
 /* Simple class attribute that is just a static string */
 struct class_attribute_string {
 	struct class_attribute attr;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 4/5] leds: add /sys/class/triggers/ that contains trigger sub-directories
  2019-09-08 12:41 [PATCH 0/5] leds: fix /sys/class/leds/<led>/trigger and add new api Akinobu Mita
                   ` (2 preceding siblings ...)
  2019-09-08 12:41 ` [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory Akinobu Mita
@ 2019-09-08 12:41 ` Akinobu Mita
  2019-09-08 13:21   ` Greg Kroah-Hartman
  2019-09-08 12:41 ` [PATCH 5/5] leds: add /sys/class/leds/<led>/current-trigger Akinobu Mita
  4 siblings, 1 reply; 17+ messages in thread
From: Akinobu Mita @ 2019-09-08 12:41 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, this violates the "one value per file" rule of sysfs.

This provides /sys/class/leds/triggers directory that contains a number of
sub-directories, each representing an LED trigger. The name of the
sub-directory matches the LED trigger name.

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>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 Documentation/ABI/testing/sysfs-class-led |  9 +++++++++
 drivers/leds/led-class.c                  | 32 +++++++++++++++++++++++++++++++
 drivers/leds/led-triggers.c               | 19 ++++++++++++++++++
 drivers/leds/leds.h                       |  1 +
 include/linux/leds.h                      |  1 +
 5 files changed, 62 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..14d91af 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,12 @@ Description:
 		gpio and backlight triggers. In case of the backlight trigger,
 		it is useful when driving a LED which is intended to indicate
 		a device in a standby like state.
+
+What:		/sys/class/leds/triggers/
+Date:		September 2019
+KernelVersion:	5.5
+Contact:	linux-leds@vger.kernel.org
+Description:
+		This directory contains a number of sub-directories, each
+		representing an LED trigger. The name of the sub-directory
+		matches the LED trigger name.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 7d85181..04e6c14 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -81,6 +81,28 @@ static struct bin_attribute *led_trigger_bin_attrs[] = {
 static const struct attribute_group led_trigger_group = {
 	.bin_attrs = led_trigger_bin_attrs,
 };
+
+static int led_triggers_kobj_create(void)
+{
+	led_triggers_kobj = class_kobject_create_and_add("triggers",
+							 leds_class);
+
+	return led_triggers_kobj ? 0 : -ENOMEM;
+}
+
+static void led_triggers_kobj_destroy(void)
+{
+	kobject_put(led_triggers_kobj);
+}
+
+#else
+static inline int led_triggers_kobj_create(void)
+{
+	return 0;
+}
+static void led_triggers_kobj_destroy(void)
+{
+}
 #endif
 
 static struct attribute *led_class_attrs[] = {
@@ -411,16 +433,26 @@ EXPORT_SYMBOL_GPL(devm_led_classdev_unregister);
 
 static int __init leds_init(void)
 {
+	int ret;
+
 	leds_class = class_create(THIS_MODULE, "leds");
 	if (IS_ERR(leds_class))
 		return PTR_ERR(leds_class);
 	leds_class->pm = &leds_class_dev_pm_ops;
 	leds_class->dev_groups = led_groups;
+
+	ret = led_triggers_kobj_create();
+	if (ret) {
+		class_unregister(leds_class);
+		return ret;
+	}
+
 	return 0;
 }
 
 static void __exit leds_exit(void)
 {
+	led_triggers_kobj_destroy();
 	class_destroy(leds_class);
 }
 
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index ed5a311..4a86964 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -268,16 +268,26 @@ void led_trigger_rename_static(const char *name, struct led_trigger *trig)
 }
 EXPORT_SYMBOL_GPL(led_trigger_rename_static);
 
+static struct kobj_type led_trigger_kobj_type = {
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+struct kobject *led_triggers_kobj;
+EXPORT_SYMBOL_GPL(led_triggers_kobj);
+
 /* LED Trigger Interface */
 
 int led_trigger_register(struct led_trigger *trig)
 {
 	struct led_classdev *led_cdev;
 	struct led_trigger *_trig;
+	int ret;
 
 	rwlock_init(&trig->leddev_list_lock);
 	INIT_LIST_HEAD(&trig->led_cdevs);
 
+	kobject_init(&trig->kobj, &led_trigger_kobj_type);
+
 	down_write(&triggers_list_lock);
 	/* Make sure the trigger's name isn't already in use */
 	list_for_each_entry(_trig, &trigger_list, next_trig) {
@@ -286,6 +296,14 @@ int led_trigger_register(struct led_trigger *trig)
 			return -EEXIST;
 		}
 	}
+
+	WARN_ON_ONCE(!led_triggers_kobj);
+	ret = kobject_add(&trig->kobj, led_triggers_kobj, "%s", trig->name);
+	if (ret) {
+		up_write(&triggers_list_lock);
+		return ret;
+	}
+
 	/* Add to the list of led triggers */
 	list_add_tail(&trig->next_trig, &trigger_list);
 	up_write(&triggers_list_lock);
@@ -316,6 +334,7 @@ void led_trigger_unregister(struct led_trigger *trig)
 
 	/* Remove from the list of led triggers */
 	down_write(&triggers_list_lock);
+	kobject_put(&trig->kobj);
 	list_del_init(&trig->next_trig);
 	up_write(&triggers_list_lock);
 
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index a0ee33c..52debe0 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -33,5 +33,6 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
 extern struct list_head trigger_list;
+extern struct kobject *led_triggers_kobj;
 
 #endif	/* __LEDS_H_INCLUDED */
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 33ae825..379f282 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -279,6 +279,7 @@ struct led_trigger {
 	struct list_head  next_trig;
 
 	const struct attribute_group **groups;
+	struct kobject kobj;
 };
 
 /*
-- 
2.7.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 5/5] leds: add /sys/class/leds/<led>/current-trigger
  2019-09-08 12:41 [PATCH 0/5] leds: fix /sys/class/leds/<led>/trigger and add new api Akinobu Mita
                   ` (3 preceding siblings ...)
  2019-09-08 12:41 ` [PATCH 4/5] leds: add /sys/class/triggers/ that contains trigger sub-directories Akinobu Mita
@ 2019-09-08 12:41 ` Akinobu Mita
  2019-09-08 13:10   ` Greg Kroah-Hartman
  4 siblings, 1 reply; 17+ messages in thread
From: Akinobu Mita @ 2019-09-08 12:41 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, this violates the "one value per file" rule of sysfs.

This provides /sys/class/leds/<led>/current-trigger which is almost
identical to /sys/class/leds/<led>/trigger.  The only difference is that
'current-trigger' only shows the current trigger name.

This new file follows the "one value per file" rule of sysfs.
We can use the /sys/class/triggers directory to get the list of available
LED triggers.

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>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 Documentation/ABI/testing/sysfs-class-led | 13 +++++++++++
 drivers/leds/led-class.c                  |  7 ++++++
 drivers/leds/led-triggers.c               | 38 +++++++++++++++++++++++++++----
 drivers/leds/leds.h                       |  5 ++++
 4 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 14d91af..1a1be10 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -70,3 +70,16 @@ Description:
 		This directory contains a number of sub-directories, each
 		representing an LED trigger. The name of the sub-directory
 		matches the LED trigger name.
+
+What:		/sys/class/leds/<led>/current-trigger
+Date:		September 2019
+KernelVersion:	5.5
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Set the trigger for this LED. A trigger is a kernel based source
+		of LED events.
+		Writing the trigger name to this file will change the current
+		trigger. Trigger specific parameters can appear in
+		/sys/class/leds/<led> once a given trigger is selected. For
+		their documentation see sysfs-class-led-trigger-*.
+		Reading this file will return the current LED trigger name.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 04e6c14..388500b 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -73,12 +73,19 @@ static ssize_t max_brightness_show(struct device *dev,
 static DEVICE_ATTR_RO(max_brightness);
 
 #ifdef CONFIG_LEDS_TRIGGERS
+static DEVICE_ATTR(current_trigger, 0644, led_current_trigger_show,
+		   led_current_trigger_store);
+static struct attribute *led_current_trigger_attrs[] = {
+	&dev_attr_current_trigger.attr,
+	NULL,
+};
 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_current_trigger_attrs,
 	.bin_attrs = led_trigger_bin_attrs,
 };
 
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 4a86964..41bcc508 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -27,11 +27,9 @@ LIST_HEAD(trigger_list);
 
  /* Used by LED Class */
 
-ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
-			  struct bin_attribute *bin_attr, char *buf,
-			  loff_t pos, size_t count)
+static ssize_t led_trigger_store(struct device *dev, const char *buf,
+				 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;
@@ -67,8 +65,25 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 	mutex_unlock(&led_cdev->led_access);
 	return ret;
 }
+
+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);
+
+	return led_trigger_store(dev, buf, count);
+}
 EXPORT_SYMBOL_GPL(led_trigger_write);
 
+ssize_t led_current_trigger_store(struct device *dev,
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	return led_trigger_store(dev, buf, count);
+}
+EXPORT_SYMBOL_GPL(led_current_trigger_store);
+
 __printf(4, 5)
 static int led_trigger_snprintf(char *buf, size_t size, bool query,
 				const char *fmt, ...)
@@ -146,6 +161,21 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(led_trigger_read);
 
+ssize_t led_current_trigger_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	int len;
+
+	down_read(&led_cdev->trigger_lock);
+	len = scnprintf(buf, PAGE_SIZE, "%s\n", led_cdev->trigger ?
+			led_cdev->trigger->name : "none");
+	up_read(&led_cdev->trigger_lock);
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(led_current_trigger_show);
+
 /* 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 52debe0..e3d04c2 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -29,6 +29,11 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
 ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 			struct bin_attribute *bin_attr, char *buf,
 			loff_t pos, size_t count);
+ssize_t led_current_trigger_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count);
+ssize_t led_current_trigger_show(struct device *dev,
+				struct device_attribute *attr, char *buf);
 
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] leds: make sure leds_class is initialized before triggers are registered
  2019-09-08 12:41 ` [PATCH 2/5] leds: make sure leds_class is initialized before triggers are registered Akinobu Mita
@ 2019-09-08 13:07   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 13:07 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-kernel, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

On Sun, Sep 08, 2019 at 09:41:09PM +0900, Akinobu Mita wrote:
> If the led-class and usb-common modules are built into the kernel, the
> usb-common module could be initialized earlier than the led-class module.
> 
> So when the ledtrig_usb_gadget and ledtrig_usb_host LED triggers are
> registered by usb-common module, the leds_class could not be initialized
> yet.
> 
> We are going to populate sub-directories, each representing an LED
> trigger in /sys/class/triggers/, so leds_class needs to be initialized
> before any LED triggers is registered.
> 
> This makes led-class initialize earlier then usb-common by changing
> initcall group.
> 
> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/leds/led-class.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 8b5a1d1..7d85181 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -424,7 +424,7 @@ static void __exit leds_exit(void)
>  	class_destroy(leds_class);
>  }
>  
> -subsys_initcall(leds_init);
> +postcore_initcall(leds_init);

This is a case of "whack a mole".

Why not just initialize everything the first time the function is
called?  That way you don't have to mess with any of the link order
stuff and everything will always continue to work if things ever change
in the future?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory
  2019-09-08 12:41 ` [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory Akinobu Mita
@ 2019-09-08 13:08   ` Greg Kroah-Hartman
  2019-09-08 13:11   ` Greg Kroah-Hartman
  2019-09-08 20:26   ` Pavel Machek
  2 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 13:08 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-kernel, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

On Sun, Sep 08, 2019 at 09:41:10PM +0900, Akinobu Mita wrote:
> This adds a new function class_kobject_create_and_add() that creates a
> directory in the /sys/class/<class>.
> 
> This function is required to create the /sys/class/leds/triggers directory
> that contains all available LED triggers.
> 
> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/base/class.c   | 7 +++++++
>  include/linux/device.h | 3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/base/class.c b/drivers/base/class.c
> index d8a6a58..f4c53e7 100644
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -104,6 +104,13 @@ void class_remove_file_ns(struct class *cls, const struct class_attribute *attr,
>  		sysfs_remove_file_ns(&cls->p->subsys.kobj, &attr->attr, ns);
>  }
>  
> +struct kobject *class_kobject_create_and_add(const char *name,
> +					     struct class *cls)
> +{
> +	return kobject_create_and_add(name, &cls->p->subsys.kobj);
> +}
> +EXPORT_SYMBOL_GPL(class_kobject_create_and_add);

Ick, why?  Are you _SURE_ you really need to do this?  Positive?  This
feels very strange...

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/5] leds: add /sys/class/leds/<led>/current-trigger
  2019-09-08 12:41 ` [PATCH 5/5] leds: add /sys/class/leds/<led>/current-trigger Akinobu Mita
@ 2019-09-08 13:10   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 13:10 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-kernel, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

On Sun, Sep 08, 2019 at 09:41:12PM +0900, Akinobu Mita wrote:
> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, this violates the "one value per file" rule of sysfs.
> 
> This provides /sys/class/leds/<led>/current-trigger which is almost
> identical to /sys/class/leds/<led>/trigger.  The only difference is that
> 'current-trigger' only shows the current trigger name.
> 
> This new file follows the "one value per file" rule of sysfs.
> We can use the /sys/class/triggers directory to get the list of available
> LED triggers.
> 
> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-class-led | 13 +++++++++++
>  drivers/leds/led-class.c                  |  7 ++++++
>  drivers/leds/led-triggers.c               | 38 +++++++++++++++++++++++++++----
>  drivers/leds/leds.h                       |  5 ++++
>  4 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 14d91af..1a1be10 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -70,3 +70,16 @@ Description:
>  		This directory contains a number of sub-directories, each
>  		representing an LED trigger. The name of the sub-directory
>  		matches the LED trigger name.
> +
> +What:		/sys/class/leds/<led>/current-trigger
> +Date:		September 2019
> +KernelVersion:	5.5
> +Contact:	linux-leds@vger.kernel.org
> +Description:
> +		Set the trigger for this LED. A trigger is a kernel based source
> +		of LED events.
> +		Writing the trigger name to this file will change the current
> +		trigger. Trigger specific parameters can appear in
> +		/sys/class/leds/<led> once a given trigger is selected. For
> +		their documentation see sysfs-class-led-trigger-*.
> +		Reading this file will return the current LED trigger name.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 04e6c14..388500b 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -73,12 +73,19 @@ static ssize_t max_brightness_show(struct device *dev,
>  static DEVICE_ATTR_RO(max_brightness);
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
> +static DEVICE_ATTR(current_trigger, 0644, led_current_trigger_show,
> +		   led_current_trigger_store);

DEVICE_ATTR_RW()?


> +static struct attribute *led_current_trigger_attrs[] = {
> +	&dev_attr_current_trigger.attr,
> +	NULL,
> +};
>  static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write, 0);

BIN_ATTR_RW()?

And no whitespace?

thanks,

gre gk-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger
  2019-09-08 12:41 ` [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita
@ 2019-09-08 13:10   ` Greg Kroah-Hartman
  2019-09-11 15:25     ` Akinobu Mita
  2019-09-08 13:18   ` Greg Kroah-Hartman
  1 sibling, 1 reply; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 13:10 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-kernel, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

On Sun, Sep 08, 2019 at 09:41:08PM +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.
> 
> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> the PAGE_SIZE limitation.
> 
> 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);

BIN_ATTR_RW()?


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory
  2019-09-08 12:41 ` [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory Akinobu Mita
  2019-09-08 13:08   ` Greg Kroah-Hartman
@ 2019-09-08 13:11   ` Greg Kroah-Hartman
  2019-09-08 20:26   ` Pavel Machek
  2 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 13:11 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-kernel, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

On Sun, Sep 08, 2019 at 09:41:10PM +0900, Akinobu Mita wrote:
> This adds a new function class_kobject_create_and_add() that creates a
> directory in the /sys/class/<class>.
> 
> This function is required to create the /sys/class/leds/triggers directory
> that contains all available LED triggers.

I don't see a patch in this series that does that, is it 4/5?  I never
got that one...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger
  2019-09-08 12:41 ` [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita
  2019-09-08 13:10   ` Greg Kroah-Hartman
@ 2019-09-08 13:18   ` Greg Kroah-Hartman
  1 sibling, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 13:18 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-kernel, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

On Sun, Sep 08, 2019 at 09:41:08PM +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.
> 
> This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> the PAGE_SIZE limitation.

Can you also put the "this really is not a good idea" type wording here
in the changelog text as well?  Just want to try to make it explicitly
clear that this should never be copied by anyone else.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] leds: add /sys/class/triggers/ that contains trigger sub-directories
  2019-09-08 12:41 ` [PATCH 4/5] leds: add /sys/class/triggers/ that contains trigger sub-directories Akinobu Mita
@ 2019-09-08 13:21   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 13:21 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-kernel, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

On Sun, Sep 08, 2019 at 09:41:11PM +0900, Akinobu Mita wrote:
> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, this violates the "one value per file" rule of sysfs.
> 
> This provides /sys/class/leds/triggers directory that contains a number of
> sub-directories, each representing an LED trigger. The name of the
> sub-directory matches the LED trigger name.
> 
> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-class-led |  9 +++++++++
>  drivers/leds/led-class.c                  | 32 +++++++++++++++++++++++++++++++
>  drivers/leds/led-triggers.c               | 19 ++++++++++++++++++
>  drivers/leds/leds.h                       |  1 +
>  include/linux/leds.h                      |  1 +
>  5 files changed, 62 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..14d91af 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,12 @@ Description:
>  		gpio and backlight triggers. In case of the backlight trigger,
>  		it is useful when driving a LED which is intended to indicate
>  		a device in a standby like state.
> +
> +What:		/sys/class/leds/triggers/
> +Date:		September 2019
> +KernelVersion:	5.5
> +Contact:	linux-leds@vger.kernel.org
> +Description:
> +		This directory contains a number of sub-directories, each
> +		representing an LED trigger. The name of the sub-directory
> +		matches the LED trigger name.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 7d85181..04e6c14 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -81,6 +81,28 @@ static struct bin_attribute *led_trigger_bin_attrs[] = {
>  static const struct attribute_group led_trigger_group = {
>  	.bin_attrs = led_trigger_bin_attrs,
>  };
> +
> +static int led_triggers_kobj_create(void)
> +{
> +	led_triggers_kobj = class_kobject_create_and_add("triggers",
> +							 leds_class);
> +
> +	return led_triggers_kobj ? 0 : -ENOMEM;
> +}
> +
> +static void led_triggers_kobj_destroy(void)
> +{
> +	kobject_put(led_triggers_kobj);
> +}
> +
> +#else
> +static inline int led_triggers_kobj_create(void)
> +{
> +	return 0;
> +}
> +static void led_triggers_kobj_destroy(void)
> +{
> +}
>  #endif
>  
>  static struct attribute *led_class_attrs[] = {
> @@ -411,16 +433,26 @@ EXPORT_SYMBOL_GPL(devm_led_classdev_unregister);
>  
>  static int __init leds_init(void)
>  {
> +	int ret;
> +
>  	leds_class = class_create(THIS_MODULE, "leds");
>  	if (IS_ERR(leds_class))
>  		return PTR_ERR(leds_class);
>  	leds_class->pm = &leds_class_dev_pm_ops;
>  	leds_class->dev_groups = led_groups;
> +
> +	ret = led_triggers_kobj_create();
> +	if (ret) {
> +		class_unregister(leds_class);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
>  static void __exit leds_exit(void)
>  {
> +	led_triggers_kobj_destroy();
>  	class_destroy(leds_class);
>  }
>  
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index ed5a311..4a86964 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -268,16 +268,26 @@ void led_trigger_rename_static(const char *name, struct led_trigger *trig)
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_rename_static);
>  
> +static struct kobj_type led_trigger_kobj_type = {
> +	.sysfs_ops = &kobj_sysfs_ops,
> +};
> +
> +struct kobject *led_triggers_kobj;
> +EXPORT_SYMBOL_GPL(led_triggers_kobj);
> +
>  /* LED Trigger Interface */
>  
>  int led_trigger_register(struct led_trigger *trig)
>  {
>  	struct led_classdev *led_cdev;
>  	struct led_trigger *_trig;
> +	int ret;
>  
>  	rwlock_init(&trig->leddev_list_lock);
>  	INIT_LIST_HEAD(&trig->led_cdevs);
>  
> +	kobject_init(&trig->kobj, &led_trigger_kobj_type);
> +
>  	down_write(&triggers_list_lock);
>  	/* Make sure the trigger's name isn't already in use */
>  	list_for_each_entry(_trig, &trigger_list, next_trig) {
> @@ -286,6 +296,14 @@ int led_trigger_register(struct led_trigger *trig)
>  			return -EEXIST;
>  		}
>  	}
> +
> +	WARN_ON_ONCE(!led_triggers_kobj);
> +	ret = kobject_add(&trig->kobj, led_triggers_kobj, "%s", trig->name);
> +	if (ret) {
> +		up_write(&triggers_list_lock);
> +		return ret;
> +	}
> +
>  	/* Add to the list of led triggers */
>  	list_add_tail(&trig->next_trig, &trigger_list);
>  	up_write(&triggers_list_lock);
> @@ -316,6 +334,7 @@ void led_trigger_unregister(struct led_trigger *trig)
>  
>  	/* Remove from the list of led triggers */
>  	down_write(&triggers_list_lock);
> +	kobject_put(&trig->kobj);
>  	list_del_init(&trig->next_trig);
>  	up_write(&triggers_list_lock);
>  
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index a0ee33c..52debe0 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -33,5 +33,6 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>  extern struct rw_semaphore leds_list_lock;
>  extern struct list_head leds_list;
>  extern struct list_head trigger_list;
> +extern struct kobject *led_triggers_kobj;
>  
>  #endif	/* __LEDS_H_INCLUDED */
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 33ae825..379f282 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -279,6 +279,7 @@ struct led_trigger {
>  	struct list_head  next_trig;
>  
>  	const struct attribute_group **groups;
> +	struct kobject kobj;

No, don't do this.  Do not put "raw" kobjects in the device tree below a
"normal" directory, it's only going to cause massive problems for
userspace tools to be notified of the attributes.

If you want to make led_triggers "real" devices, great, do that!  But
this hybrid "not quite real" is not going to work out well at all in the
end.

Make this a 'struct device' and you will be fine.  That is probably the
simplest of all.  Now if you want to lump them below the leds class or
not is up to you, personally, I'd make it a new one as the structure is
different and this makes it more obvious, but it is up to you as you
know best how userspace is going to interact with these.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory
  2019-09-08 12:41 ` [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory Akinobu Mita
  2019-09-08 13:08   ` Greg Kroah-Hartman
  2019-09-08 13:11   ` Greg Kroah-Hartman
@ 2019-09-08 20:26   ` Pavel Machek
  2 siblings, 0 replies; 17+ messages in thread
From: Pavel Machek @ 2019-09-08 20:26 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, linux-kernel, Greg Kroah-Hartman, Rafael J. Wysocki,
	Jacek Anaszewski, Dan Murphy

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

Hi!

> This adds a new function class_kobject_create_and_add() that creates a
> directory in the /sys/class/<class>.
> 
> This function is required to create the /sys/class/leds/triggers directory
> that contains all available LED triggers.

Lets not do this. With your fixes (1,2/5), we'll have reasonable
interface for triggers. We don't need a new one.

[And yes, we'll want to do something with the cpu activity trigger,
too.]

Best regards,
								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] 17+ messages in thread

* Re: [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger
  2019-09-08 13:10   ` Greg Kroah-Hartman
@ 2019-09-11 15:25     ` Akinobu Mita
  2019-09-11 15:36       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 17+ messages in thread
From: Akinobu Mita @ 2019-09-11 15:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-leds, LKML, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

2019年9月8日(日) 22:10 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>
> On Sun, Sep 08, 2019 at 09:41:08PM +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.
> >
> > This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> > the PAGE_SIZE limitation.
> >
> > 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);
>
> BIN_ATTR_RW()?

We can use BIN_ATTR_RW() by renaming led_trigger_{read,write}() to
trigger_{read,write}().  But led_trigger_{read,write}() are not static
functions.  These are defined as export symbols for led-class module.

So trigger_{read,write}() will be too generic symbol names, won't they?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger
  2019-09-11 15:25     ` Akinobu Mita
@ 2019-09-11 15:36       ` Greg Kroah-Hartman
  2019-09-11 17:25         ` Akinobu Mita
  0 siblings, 1 reply; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-11 15:36 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-leds, LKML, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

On Thu, Sep 12, 2019 at 12:25:28AM +0900, Akinobu Mita wrote:
> 2019年9月8日(日) 22:10 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >
> > On Sun, Sep 08, 2019 at 09:41:08PM +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.
> > >
> > > This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> > > the PAGE_SIZE limitation.
> > >
> > > 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);
> >
> > BIN_ATTR_RW()?
> 
> We can use BIN_ATTR_RW() by renaming led_trigger_{read,write}() to
> trigger_{read,write}().  But led_trigger_{read,write}() are not static
> functions.  These are defined as export symbols for led-class module.
> 
> So trigger_{read,write}() will be too generic symbol names, won't they?

Yes they would, sorry I didn't notice that.

Wait, why are those functions being exported?  Who is calling a sysfs
function from a different code path than sysfs?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger
  2019-09-11 15:36       ` Greg Kroah-Hartman
@ 2019-09-11 17:25         ` Akinobu Mita
  0 siblings, 0 replies; 17+ messages in thread
From: Akinobu Mita @ 2019-09-11 17:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-leds, LKML, Rafael J. Wysocki, Jacek Anaszewski,
	Pavel Machek, Dan Murphy

2019年9月12日(木) 0:36 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>
> On Thu, Sep 12, 2019 at 12:25:28AM +0900, Akinobu Mita wrote:
> > 2019年9月8日(日) 22:10 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> > >
> > > On Sun, Sep 08, 2019 at 09:41:08PM +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.
> > > >
> > > > This converts /sys/class/leds/<led>/trigger to bin attribute and removes
> > > > the PAGE_SIZE limitation.
> > > >
> > > > 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);
> > >
> > > BIN_ATTR_RW()?
> >
> > We can use BIN_ATTR_RW() by renaming led_trigger_{read,write}() to
> > trigger_{read,write}().  But led_trigger_{read,write}() are not static
> > functions.  These are defined as export symbols for led-class module.
> >
> > So trigger_{read,write}() will be too generic symbol names, won't they?
>
> Yes they would, sorry I didn't notice that.
>
> Wait, why are those functions being exported?  Who is calling a sysfs
> function from a different code path than sysfs?

led-class.c :)

led_trigger_{read,write}() are defined in led-triggers.c which is built
into the kernel. led-class.c can be built as module.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-08 12:41 [PATCH 0/5] leds: fix /sys/class/leds/<led>/trigger and add new api Akinobu Mita
2019-09-08 12:41 ` [PATCH 1/5] leds: remove PAGE_SIZE limit of /sys/class/leds/<led>/trigger Akinobu Mita
2019-09-08 13:10   ` Greg Kroah-Hartman
2019-09-11 15:25     ` Akinobu Mita
2019-09-11 15:36       ` Greg Kroah-Hartman
2019-09-11 17:25         ` Akinobu Mita
2019-09-08 13:18   ` Greg Kroah-Hartman
2019-09-08 12:41 ` [PATCH 2/5] leds: make sure leds_class is initialized before triggers are registered Akinobu Mita
2019-09-08 13:07   ` Greg Kroah-Hartman
2019-09-08 12:41 ` [PATCH 3/5] driver core: class: add function to create /sys/class/<class>/foo directory Akinobu Mita
2019-09-08 13:08   ` Greg Kroah-Hartman
2019-09-08 13:11   ` Greg Kroah-Hartman
2019-09-08 20:26   ` Pavel Machek
2019-09-08 12:41 ` [PATCH 4/5] leds: add /sys/class/triggers/ that contains trigger sub-directories Akinobu Mita
2019-09-08 13:21   ` Greg Kroah-Hartman
2019-09-08 12:41 ` [PATCH 5/5] leds: add /sys/class/leds/<led>/current-trigger Akinobu Mita
2019-09-08 13:10   ` Greg Kroah-Hartman

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org linux-leds@archiver.kernel.org
	public-inbox-index linux-leds


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/ public-inbox