[PATCH 0/3] leds: add device trigger

[PATCH 0/3] leds: add device trigger

[Maciek Borzecki]

A series of patches that add yet another LED trigger, a device
activity trigger.

The motivation is to have a LED trigger that is associated with a
device and can be fired from cetrain points in the code that have been
chosen by the developer. With this this device activity trigger it is
possible for instance to easily hook up a tty driver for a console to
blink one LED, yet another serial port to blink a second LED and
writes to a block device to trigger a third LED.

The patches have been tested on Wandboard Quad.

The first patch adds the actual trigger. Each device wishing to use
the trigger has to be explicitly registered by calling
ledtrig_dev_add(), and passing it's dev_t. The intention is that the
trigger will be used in scenarios that are impossible to foresee at
this moment, and are likely to be approach in a case by case manner
anyway.

The second patch adds couple of debugfs helpers.

The third patch adds documentation and notes on debugfs interface.

Maciek Borzecki (3):
  leds: add device activity LED triggers
  leds: add debugfs to device trigger
  Documentation: leds: document ledtrig-device trigger

 Documentation/leds/00-INDEX           |   3 +
 Documentation/leds/ledtrig-device.txt |  32 ++++
 drivers/leds/trigger/Kconfig          |   8 +
 drivers/leds/trigger/Makefile         |   1 +
 drivers/leds/trigger/ledtrig-device.c | 289 ++++++++++++++++++++++++++++++++++
 include/linux/leds.h                  |  10 ++
 6 files changed, 343 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-device.txt
 create mode 100644 drivers/leds/trigger/ledtrig-device.c

-- 
2.5.3

[PATCH 1/3] leds: add device activity LED triggers

[Maciek Borzecki]

The patch adds LED triggers for indicating an activity on a selected
device. The drivers that intend to use triggers need to register
respective devices using ledtrig_dev_add(). Triggers are generated by
explicitly calling ledtrig_dev_activity().

Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
---
 drivers/leds/trigger/Kconfig          |   8 ++
 drivers/leds/trigger/Makefile         |   1 +
 drivers/leds/trigger/ledtrig-device.c | 185 ++++++++++++++++++++++++++++++++++
 include/linux/leds.h                  |  10 ++
 4 files changed, 204 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-device.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 5bda6a9b56bbd90b4a3749f87bc0c6fda8dd5034..c6ecfbd7c0876f21688140aa9afc7eb5b9fec3a2 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -108,4 +108,12 @@ config LEDS_TRIGGER_CAMERA
 	  This enables direct flash/torch on/off by the driver, kernel space.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_DEVICE
+	bool "LED Device Activity Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to be triggered by an actvity on a selected
+          device.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 1abf48dacf7ebfcfb8208f7ae7bdf29d7c11ba32..86beeacd5403afc163873d7c3f817ee082b64e04 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
+obj-$(CONFIG_LEDS_TRIGGER_DEVICE)	+= ledtrig-device.o
diff --git a/drivers/leds/trigger/ledtrig-device.c b/drivers/leds/trigger/ledtrig-device.c
new file mode 100644
index 0000000000000000000000000000000000000000..dbb8d7d2b4a0258149c581a040c416d412d9ceeb
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-device.c
@@ -0,0 +1,185 @@
+/*
+ * LED Device Activity Trigger
+ *
+ * Copyright 2015 Maciej Borzecki <maciek.borzecki@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/rwsem.h>
+#include <linux/kdev_t.h>
+
+#define BLINK_DELAY 30
+static unsigned long blink_delay = BLINK_DELAY;
+
+static DECLARE_RWSEM(devs_list_lock);
+static LIST_HEAD(devs_list);
+
+#define MAX_NAME_LEN 20
+
+struct ledtrig_dev_data {
+	char name[MAX_NAME_LEN];
+	dev_t dev;
+	struct led_trigger *trig;
+	struct list_head node;
+};
+
+/**
+ * ledtrig_dev_activity - signal activity on device
+ * @dev: device
+ *
+ * Fires a trigger assigned to @dev device.
+ */
+void ledtrig_dev_activity(dev_t dev)
+{
+	struct ledtrig_dev_data *dev_trig;
+
+	if (!down_read_trylock(&devs_list_lock))
+		return;
+
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		if (dev_trig->dev == dev) {
+			led_trigger_blink_oneshot(dev_trig->trig,
+						  &blink_delay,
+						  &blink_delay,
+						  0);
+			break;
+		}
+	}
+	up_read(&devs_list_lock);
+}
+EXPORT_SYMBOL(ledtrig_dev_activity);
+
+static struct ledtrig_dev_data *ledtrig_dev_new(dev_t dev)
+{
+	struct ledtrig_dev_data *dev_trig;
+
+	dev_trig = kzalloc(sizeof(*dev_trig), GFP_KERNEL);
+	if (!dev_trig)
+		return NULL;
+
+	INIT_LIST_HEAD(&dev_trig->node);
+	dev_trig->dev = dev;
+	snprintf(dev_trig->name, sizeof(dev_trig->name),
+		 "dev-%u:%u", MAJOR(dev), MINOR(dev));
+
+	return dev_trig;
+}
+
+static void ledtrig_dev_release(struct ledtrig_dev_data *dev_trig)
+{
+	led_trigger_unregister_simple(dev_trig->trig);
+
+	kfree(dev_trig);
+}
+
+/**
+ * ledtrig_dev_add - add a trigger for device
+ * @dev: device for which the trigger is to be added
+ *
+ * Create and register a new trigger for device @dev. The trigger will
+ * show up as dev-<major>:<minor> in the list of avaialble LED
+ * triggers.
+ */
+void ledtrig_dev_add(dev_t dev)
+{
+	int found = 0;
+	struct ledtrig_dev_data *new_dev_trig;
+	struct ledtrig_dev_data *dev_trig;
+
+	new_dev_trig = ledtrig_dev_new(dev);
+	if (!new_dev_trig)
+		return;
+
+	down_write(&devs_list_lock);
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		if (dev_trig->dev == dev) {
+			found = 1;
+			break;
+		}
+	}
+	if (!found)
+		list_add(&new_dev_trig->node, &devs_list);
+	up_write(&devs_list_lock);
+
+	if (!found)
+		/* register with led triggers */
+		led_trigger_register_simple(new_dev_trig->name,
+					    &new_dev_trig->trig);
+	else
+		kfree(new_dev_trig);
+}
+EXPORT_SYMBOL(ledtrig_dev_add);
+
+/**
+ * ledtrig_dev_del - delete a trigger
+ * @dev: device for which to delete a trigger
+ */
+void ledtrig_dev_del(dev_t dev)
+{
+
+	struct ledtrig_dev_data *dev_trig;
+
+	down_write(&devs_list_lock);
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		if (dev_trig->dev == dev) {
+			/* remove from devs list */
+			list_del(&dev_trig->node);
+
+			/* unregister & release data */
+			ledtrig_dev_release(dev_trig);
+			break;
+		}
+	}
+	up_write(&devs_list_lock);
+
+}
+EXPORT_SYMBOL(ledtrig_dev_del);
+
+static void ledtrig_dev_remove_all(void)
+{
+	struct list_head *en;
+
+	down_write(&devs_list_lock);
+	list_for_each(en, &devs_list) {
+		struct list_head *prev = en->prev;
+		struct ledtrig_dev_data *dev_trig;
+
+		dev_trig = list_entry(en, struct ledtrig_dev_data,
+				      node);
+		/* remove from list */
+		list_del(en);
+
+		/* unregister & release data */
+		ledtrig_dev_release(dev_trig);
+
+		/* and go back */
+		en = prev;
+	}
+	up_write(&devs_list_lock);
+}
+
+static int __init ledtrig_dev_init(void)
+{
+	return 0;
+}
+
+static void __exit ledtrig_dev_exit(void)
+{
+	ledtrig_dev_remove_all();
+}
+
+module_init(ledtrig_dev_init);
+module_exit(ledtrig_dev_exit);
+
+MODULE_AUTHOR("Maciej Borzecki <maciek.borzecki@gmail.com>");
+MODULE_DESCRIPTION("LED Device Activity Trigger");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc17b8a8b1a1852dc1c420ecf0f8d2..e487d5b2ac556bdb2f1525d8b5e84df0c245d4c9 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -369,4 +369,14 @@ static inline void ledtrig_cpu(enum cpu_led_event evt)
 }
 #endif
 
+#ifdef CONFIG_LEDS_TRIGGER_DEVICE
+extern void ledtrig_dev_add(dev_t dev);
+extern void ledtrig_dev_del(dev_t dev);
+extern void ledtrig_dev_activity(dev_t dev);
+#else
+static inline void ledtrig_dev_add(dev_t dev) {}
+static inline void ledtrig_dev_del(dev_t dev) {}
+static inline void ledtrig_dev_activity(dev_t dev) {}
+#endif
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
2.5.3

[PATCH 2/3] leds: add debugfs to device trigger

[Maciek Borzecki]

Add debugfs entries for device activity trigger. Three entries are
created under /sys/kernel/debug/ledtrig-dev when the driver gets
loaded. These are:

  devices - cat'ing will produce a list of currently registered devices
            in <major>:<minor> format, a line for each device.

  register - echo'ing <major>:<minor> will create a trigger for this
             device

  trigger - echo'ing <major>:<minor> will fire a trigger for this device

Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
---
 drivers/leds/trigger/ledtrig-device.c | 113 +++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-device.c b/drivers/leds/trigger/ledtrig-device.c
index dbb8d7d2b4a0258149c581a040c416d412d9ceeb..6814db5e34d76974ae095d2e1c8f1f2e23dea79e 100644
--- a/drivers/leds/trigger/ledtrig-device.c
+++ b/drivers/leds/trigger/ledtrig-device.c
@@ -16,15 +16,18 @@
 #include <linux/list.h>
 #include <linux/rwsem.h>
 #include <linux/kdev_t.h>
+#include <linux/fs.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #define BLINK_DELAY 30
 static unsigned long blink_delay = BLINK_DELAY;
 
 static DECLARE_RWSEM(devs_list_lock);
 static LIST_HEAD(devs_list);
+static struct dentry *debug_root;
 
 #define MAX_NAME_LEN 20
-
 struct ledtrig_dev_data {
 	char name[MAX_NAME_LEN];
 	dev_t dev;
@@ -114,8 +117,11 @@ void ledtrig_dev_add(dev_t dev)
 		/* register with led triggers */
 		led_trigger_register_simple(new_dev_trig->name,
 					    &new_dev_trig->trig);
-	else
+	else {
+		pr_warn("device %u:%u already registered\n",
+			MAJOR(dev), MINOR(dev));
 		kfree(new_dev_trig);
+	}
 }
 EXPORT_SYMBOL(ledtrig_dev_add);
 
@@ -167,13 +173,116 @@ static void ledtrig_dev_remove_all(void)
 	up_write(&devs_list_lock);
 }
 
+static int ledtrig_dev_devices_show(struct seq_file *s, void *unused)
+{
+	struct ledtrig_dev_data *dev_trig;
+
+	down_read(&devs_list_lock);
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		seq_printf(s, "%u:%u\n", MAJOR(dev_trig->dev),
+			MINOR(dev_trig->dev));
+	}
+	up_read(&devs_list_lock);
+	return 0;
+}
+
+static int ledtrig_dev_devices_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, ledtrig_dev_devices_show,
+			   &inode->i_private);
+}
+
+static const struct file_operations debug_devices_ops = {
+	.owner = THIS_MODULE,
+	.open = ledtrig_dev_devices_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
+static int get_dev_from_user(const char __user *buf, size_t size,
+			dev_t *dev)
+{
+	unsigned int major;
+	unsigned int minor;
+	int ret;
+
+	WARN_ON(dev == NULL);
+	if (dev == NULL)
+		return -EINVAL;
+
+	ret = sscanf(buf, "%u:%u", &major, &minor);
+	if (ret < 2)
+		return -EINVAL;
+
+	*dev = MKDEV(major, minor);
+	return 0;
+}
+
+static ssize_t ledtrig_dev_register_write(struct file *filp,
+					const char __user *buf,
+					size_t size, loff_t *off)
+{
+	dev_t dev;
+	int ret;
+
+	ret = get_dev_from_user(buf, size, &dev);
+	if (ret < 0)
+		return ret;
+
+	pr_debug("got device %u:%u\n", MAJOR(dev), MINOR(dev));
+	ledtrig_dev_add(dev);
+
+	return size;
+}
+
+static const struct file_operations debug_register_ops = {
+	.owner = THIS_MODULE,
+	.write = ledtrig_dev_register_write,
+};
+
+static ssize_t ledtrig_dev_trigger_write(struct file *filp,
+					const char __user *buf,
+					size_t size, loff_t *off)
+{
+	dev_t dev;
+	int ret;
+
+	ret = get_dev_from_user(buf, size, &dev);
+	if (ret < 0)
+		return ret;
+
+	pr_debug("trigger device %u:%u\n", MAJOR(dev), MINOR(dev));
+	ledtrig_dev_activity(dev);
+
+	return size;
+}
+
+static const struct file_operations debug_trigger_ops = {
+	.owner = THIS_MODULE,
+	.write = ledtrig_dev_trigger_write,
+};
+
 static int __init ledtrig_dev_init(void)
 {
+	debug_root = debugfs_create_dir("ledtrig-dev", NULL);
+
+	if (debug_root) {
+		debugfs_create_file("devices", 0444, debug_root, NULL,
+				&debug_devices_ops);
+		debugfs_create_file("register", 0200, debug_root, NULL,
+				&debug_register_ops);
+		debugfs_create_file("trigger", 0200, debug_root, NULL,
+				&debug_trigger_ops);
+	}
+
 	return 0;
 }
 
 static void __exit ledtrig_dev_exit(void)
 {
+	debugfs_remove_recursive(debug_root);
+
 	ledtrig_dev_remove_all();
 }
 
-- 
2.5.3

[PATCH 3/3] Documentation: leds: document ledtrig-device trigger

[Maciek Borzecki]

This patch adds documentation for ledtrig-device trigger.

Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
---
 Documentation/leds/00-INDEX           |  3 +++
 Documentation/leds/ledtrig-device.txt | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-device.txt

diff --git a/Documentation/leds/00-INDEX b/Documentation/leds/00-INDEX
index b4ef1f34e25faafe544971f619019d748538e4de..7f18adaadc04756a6b52157a71f86bd131dd915c 100644
--- a/Documentation/leds/00-INDEX
+++ b/Documentation/leds/00-INDEX
@@ -20,3 +20,6 @@ ledtrig-oneshot.txt
 	- One-shot LED trigger for both sporadic and dense events.
 ledtrig-transient.txt
 	- LED Transient Trigger, one shot timer activation.
+ledtrig-device.txt
+        - LED Device Activity Trigger for indicating an activity
+          on a device
\ No newline at end of file
diff --git a/Documentation/leds/ledtrig-device.txt b/Documentation/leds/ledtrig-device.txt
new file mode 100644
index 0000000000000000000000000000000000000000..b319d5da9df513a15b5c8094c6a5923682ff4579
--- /dev/null
+++ b/Documentation/leds/ledtrig-device.txt
@@ -0,0 +1,32 @@
+Device Activity LED Trigger
+==========================
+
+This is a LED trigger useful for signaling an activity on a given
+device identified by it's MKDEV(major, minor). A typical use case
+would be debugging of a device or a driver on an embedded board,
+patching up trivial electronic hardware design deficiencies in
+software or just plain desire to indicate activity on a certain
+device.
+
+Internally, a instance of a trigger is created and registered with LED
+triggers for every device added via ledtrig_dev_add() call. The
+triggers are named in the following manner:
+
+         `dev-<major>:<minor>`
+
+And can be selected from under /sys/class/leds/*/trigger. A device
+activity is explicitly indicated by calling ledtrig_dev_activity() for
+a registered device.
+
+The driver also creates a set of debugfs entries under
+/sys/kernel/debug/ledtrig-dev, these are:
+
+  devices - cat'ing will produce a list of currently registered
+            devices
+
+  register - echo'ing <major>:<minor> will create a trigger for this
+             device (note, you still need to add at least
+             ledtrig_dev_activity() hook)
+
+  trigger - echo'ing <major>:<minor> will fire the trigger for this
+            device
-- 
2.5.3

Re: [PATCH 2/3] leds: add debugfs to device trigger

[kbuild test robot]

Hi Maciek,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

reproduce:
  # apt-get install sparse
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/leds/trigger/ledtrig-device.c:214:22: sparse: incorrect type in argument 1 (different address spaces)
   drivers/leds/trigger/ledtrig-device.c:214:22:    expected char const *<noident>
   drivers/leds/trigger/ledtrig-device.c:214:22:    got char const [noderef] <asn:1>*buf

vim +214 drivers/leds/trigger/ledtrig-device.c

   198		.read = seq_read,
   199		.llseek = seq_lseek,
   200		.release = single_release
   201	};
   202	
   203	static int get_dev_from_user(const char __user *buf, size_t size,
   204				dev_t *dev)
   205	{
   206		unsigned int major;
   207		unsigned int minor;
   208		int ret;
   209	
   210		WARN_ON(dev == NULL);
   211		if (dev == NULL)
   212			return -EINVAL;
   213	
 > 214		ret = sscanf(buf, "%u:%u", &major, &minor);
   215		if (ret < 2)
   216			return -EINVAL;
   217	
   218		*dev = MKDEV(major, minor);
   219		return 0;
   220	}
   221	
   222	static ssize_t ledtrig_dev_register_write(struct file *filp,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 2/3] leds: add debugfs to device trigger

[Maciek Borzecki]

On 09/29 11:41, kbuild test robot wrote:
> Hi Maciek,
>
> [auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
>
> reproduce:
>   # apt-get install sparse
>   make ARCH=x86_64 allmodconfig
>   make C=1 CF=-D__CHECK_ENDIAN__
>
>
> sparse warnings: (new ones prefixed by >>)
>
> >> drivers/leds/trigger/ledtrig-device.c:214:22: sparse: incorrect type in argument 1 (different address spaces)
>    drivers/leds/trigger/ledtrig-device.c:214:22:    expected char const *<noident>
>    drivers/leds/trigger/ledtrig-device.c:214:22:    got char const [noderef] <asn:1>*buf

Right, noted right after sending the patch. Will fix in v2.

>
> vim +214 drivers/leds/trigger/ledtrig-device.c
>
>    198		.read = seq_read,
>    199		.llseek = seq_lseek,
>    200		.release = single_release
>    201	};
>    202
>    203	static int get_dev_from_user(const char __user *buf, size_t size,
>    204				dev_t *dev)
>    205	{
>    206		unsigned int major;
>    207		unsigned int minor;
>    208		int ret;
>    209
>    210		WARN_ON(dev == NULL);
>    211		if (dev == NULL)
>    212			return -EINVAL;
>    213
>  > 214		ret = sscanf(buf, "%u:%u", &major, &minor);
>    215		if (ret < 2)
>    216			return -EINVAL;
>    217
>    218		*dev = MKDEV(major, minor);
>    219		return 0;
>    220	}
>    221
>    222	static ssize_t ledtrig_dev_register_write(struct file *filp,
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

--
Maciek Borzecki

Re: [PATCH 2/3] leds: add debugfs to device trigger

[Jacek Anaszewski]

Hi Maciek,

Please test your solution thoroughly before submitting
the next version. Writing to debugfs register attribute
fails due to lack of proper copying from user memory,
which makes testing impossible.

Best Regards,
Jacek Anaszewski

On 09/28/2015 10:43 PM, Maciek Borzecki wrote:
> Add debugfs entries for device activity trigger. Three entries are
> created under /sys/kernel/debug/ledtrig-dev when the driver gets
> loaded. These are:
>
>    devices - cat'ing will produce a list of currently registered devices
>              in <major>:<minor> format, a line for each device.
>
>    register - echo'ing <major>:<minor> will create a trigger for this
>               device
>
>    trigger - echo'ing <major>:<minor> will fire a trigger for this device
>
> Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
> ---
>   drivers/leds/trigger/ledtrig-device.c | 113 +++++++++++++++++++++++++++++++++-
>   1 file changed, 111 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-device.c b/drivers/leds/trigger/ledtrig-device.c
> index dbb8d7d2b4a0258149c581a040c416d412d9ceeb..6814db5e34d76974ae095d2e1c8f1f2e23dea79e 100644
> --- a/drivers/leds/trigger/ledtrig-device.c
> +++ b/drivers/leds/trigger/ledtrig-device.c
> @@ -16,15 +16,18 @@
>   #include <linux/list.h>
>   #include <linux/rwsem.h>
>   #include <linux/kdev_t.h>
> +#include <linux/fs.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
>
>   #define BLINK_DELAY 30
>   static unsigned long blink_delay = BLINK_DELAY;
>
>   static DECLARE_RWSEM(devs_list_lock);
>   static LIST_HEAD(devs_list);
> +static struct dentry *debug_root;
>
>   #define MAX_NAME_LEN 20
> -
>   struct ledtrig_dev_data {
>   	char name[MAX_NAME_LEN];
>   	dev_t dev;
> @@ -114,8 +117,11 @@ void ledtrig_dev_add(dev_t dev)
>   		/* register with led triggers */
>   		led_trigger_register_simple(new_dev_trig->name,
>   					    &new_dev_trig->trig);
> -	else
> +	else {
> +		pr_warn("device %u:%u already registered\n",
> +			MAJOR(dev), MINOR(dev));
>   		kfree(new_dev_trig);
> +	}
>   }
>   EXPORT_SYMBOL(ledtrig_dev_add);
>
> @@ -167,13 +173,116 @@ static void ledtrig_dev_remove_all(void)
>   	up_write(&devs_list_lock);
>   }
>
> +static int ledtrig_dev_devices_show(struct seq_file *s, void *unused)
> +{
> +	struct ledtrig_dev_data *dev_trig;
> +
> +	down_read(&devs_list_lock);
> +	list_for_each_entry(dev_trig, &devs_list, node) {
> +		seq_printf(s, "%u:%u\n", MAJOR(dev_trig->dev),
> +			MINOR(dev_trig->dev));
> +	}
> +	up_read(&devs_list_lock);
> +	return 0;
> +}
> +
> +static int ledtrig_dev_devices_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, ledtrig_dev_devices_show,
> +			   &inode->i_private);
> +}
> +
> +static const struct file_operations debug_devices_ops = {
> +	.owner = THIS_MODULE,
> +	.open = ledtrig_dev_devices_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release
> +};
> +
> +static int get_dev_from_user(const char __user *buf, size_t size,
> +			dev_t *dev)
> +{
> +	unsigned int major;
> +	unsigned int minor;
> +	int ret;
> +
> +	WARN_ON(dev == NULL);
> +	if (dev == NULL)
> +		return -EINVAL;
> +
> +	ret = sscanf(buf, "%u:%u", &major, &minor);
> +	if (ret < 2)
> +		return -EINVAL;
> +
> +	*dev = MKDEV(major, minor);
> +	return 0;
> +}
> +
> +static ssize_t ledtrig_dev_register_write(struct file *filp,
> +					const char __user *buf,
> +					size_t size, loff_t *off)
> +{
> +	dev_t dev;
> +	int ret;
> +
> +	ret = get_dev_from_user(buf, size, &dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	pr_debug("got device %u:%u\n", MAJOR(dev), MINOR(dev));
> +	ledtrig_dev_add(dev);
> +
> +	return size;
> +}
> +
> +static const struct file_operations debug_register_ops = {
> +	.owner = THIS_MODULE,
> +	.write = ledtrig_dev_register_write,
> +};
> +
> +static ssize_t ledtrig_dev_trigger_write(struct file *filp,
> +					const char __user *buf,
> +					size_t size, loff_t *off)
> +{
> +	dev_t dev;
> +	int ret;
> +
> +	ret = get_dev_from_user(buf, size, &dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	pr_debug("trigger device %u:%u\n", MAJOR(dev), MINOR(dev));
> +	ledtrig_dev_activity(dev);
> +
> +	return size;
> +}
> +
> +static const struct file_operations debug_trigger_ops = {
> +	.owner = THIS_MODULE,
> +	.write = ledtrig_dev_trigger_write,
> +};
> +
>   static int __init ledtrig_dev_init(void)
>   {
> +	debug_root = debugfs_create_dir("ledtrig-dev", NULL);
> +
> +	if (debug_root) {
> +		debugfs_create_file("devices", 0444, debug_root, NULL,
> +				&debug_devices_ops);
> +		debugfs_create_file("register", 0200, debug_root, NULL,
> +				&debug_register_ops);
> +		debugfs_create_file("trigger", 0200, debug_root, NULL,
> +				&debug_trigger_ops);
> +	}
> +
>   	return 0;
>   }
>
>   static void __exit ledtrig_dev_exit(void)
>   {
> +	debugfs_remove_recursive(debug_root);
> +
>   	ledtrig_dev_remove_all();
>   }
>
>

Re: [PATCH 2/3] leds: add debugfs to device trigger

[Maciek Borzecki]

On 09/30 16:08, Jacek Anaszewski wrote:
> Hi Maciek,
>
> Please test your solution thoroughly before submitting
> the next version. Writing to debugfs register attribute
> fails due to lack of proper copying from user memory,
> which makes testing impossible.

Hi, thanks for the comment. Indeed, I noticed the problem just after
sending the patch. Surprisingly this didn't fail on iMX6 for some
reason.

Aside from the __user access problem, is the approach, in general, ok for
you or would you suggest that I changed something?

Regards,
--
Maciek Borzecki

Re: [PATCH 2/3] leds: add debugfs to device trigger

[Jacek Anaszewski]

On 09/30/2015 05:47 PM, Maciek Borzecki wrote:
> On 09/30 16:08, Jacek Anaszewski wrote:
>> Hi Maciek,
>>
>> Please test your solution thoroughly before submitting
>> the next version. Writing to debugfs register attribute
>> fails due to lack of proper copying from user memory,
>> which makes testing impossible.
>
> Hi, thanks for the comment. Indeed, I noticed the problem just after
> sending the patch. Surprisingly this didn't fail on iMX6 for some
> reason.
>
> Aside from the __user access problem, is the approach, in general, ok for
> you or would you suggest that I changed something?

At first glance the feature looks nice. I'll be able to say more
after testing working version.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 2/3] leds: add debugfs to device trigger

[Pavel Machek]

On Mon 2015-09-28 22:43:04, Maciek Borzecki wrote:
> Add debugfs entries for device activity trigger. Three entries are
> created under /sys/kernel/debug/ledtrig-dev when the driver gets
> loaded. These are:
> 
>   devices - cat'ing will produce a list of currently registered devices
>             in <major>:<minor> format, a line for each device.
> 
>   register - echo'ing <major>:<minor> will create a trigger for this
>              device
> 
>   trigger - echo'ing <major>:<minor> will fire a trigger for this device

Debugfs? Please don't. This is not debugging infrastructure.