[RFC PATCH 0/2] leds: trigger: introduce block trigger

[RFC PATCH 0/2] leds: trigger: introduce block trigger

[Enzo Matsumiya]

Inspired by https://lore.kernel.org/linux-leds/20190817145509.GA18381@amd/T/
and the ledtrig-usbport implementation.
    
This patch series introduce a new LED trigger for block devices.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>

Enzo Matsumiya (2):
  block: export block_class and disk_type symbols
  leds: trigger: implement block trigger

 block/genhd.c                        |   2 +
 drivers/leds/trigger/Kconfig         |  10 +
 drivers/leds/trigger/Makefile        |   1 +
 drivers/leds/trigger/ledtrig-block.c | 293 +++++++++++++++++++++++++++
 4 files changed, 306 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-block.c

-- 
2.31.1

[RFC PATCH 1/2] block: export block_class and disk_type symbols

[Enzo Matsumiya]

Export symbols to be used by _for_each_blk() helper in LED block
trigger.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 block/genhd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index 8c8f543572e6..516495179230 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1218,6 +1218,7 @@ static void disk_release(struct device *dev)
 struct class block_class = {
 	.name		= "block",
 };
+EXPORT_SYMBOL(block_class);
 
 static char *block_devnode(struct device *dev, umode_t *mode,
 			   kuid_t *uid, kgid_t *gid)
@@ -1235,6 +1236,7 @@ const struct device_type disk_type = {
 	.release	= disk_release,
 	.devnode	= block_devnode,
 };
+EXPORT_SYMBOL(disk_type);
 
 #ifdef CONFIG_PROC_FS
 /*
-- 
2.31.1

Re: [RFC PATCH 1/2] block: export block_class and disk_type symbols

[Greg Kroah-Hartman]

On Fri, Apr 30, 2021 at 03:32:10PM -0300, Enzo Matsumiya wrote:
> Export symbols to be used by _for_each_blk() helper in LED block
> trigger.
> 
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
>  block/genhd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 8c8f543572e6..516495179230 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1218,6 +1218,7 @@ static void disk_release(struct device *dev)
>  struct class block_class = {
>  	.name		= "block",
>  };
> +EXPORT_SYMBOL(block_class);
>  
>  static char *block_devnode(struct device *dev, umode_t *mode,
>  			   kuid_t *uid, kgid_t *gid)
> @@ -1235,6 +1236,7 @@ const struct device_type disk_type = {
>  	.release	= disk_release,
>  	.devnode	= block_devnode,
>  };
> +EXPORT_SYMBOL(disk_type);
>  
>  #ifdef CONFIG_PROC_FS
>  /*

Please please no.  These should not be needed by anything.

And if they really do, they must be EXPORT_SYMBOL_GPL().

thanks,

greg k-h

Re: [RFC PATCH 1/2] block: export block_class and disk_type symbols

[Enzo Matsumiya]

On 05/01, Greg Kroah-Hartman wrote:
>On Fri, Apr 30, 2021 at 03:32:10PM -0300, Enzo Matsumiya wrote:
>> Export symbols to be used by _for_each_blk() helper in LED block
>> trigger.
>>
>> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
>> ---
>>  block/genhd.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/block/genhd.c b/block/genhd.c
>> index 8c8f543572e6..516495179230 100644
>> --- a/block/genhd.c
>> +++ b/block/genhd.c
>> @@ -1218,6 +1218,7 @@ static void disk_release(struct device *dev)
>>  struct class block_class = {
>>  	.name		= "block",
>>  };
>> +EXPORT_SYMBOL(block_class);
>>
>>  static char *block_devnode(struct device *dev, umode_t *mode,
>>  			   kuid_t *uid, kgid_t *gid)
>> @@ -1235,6 +1236,7 @@ const struct device_type disk_type = {
>>  	.release	= disk_release,
>>  	.devnode	= block_devnode,
>>  };
>> +EXPORT_SYMBOL(disk_type);
>>
>>  #ifdef CONFIG_PROC_FS
>>  /*
>
>Please please no.  These should not be needed by anything.
>
>And if they really do, they must be EXPORT_SYMBOL_GPL().
>
>thanks,
>
>greg k-h

Thanks. I was indeed skeptical about submitting this particular change.

Do you think it's more acceptable if I implement a for_each_blk() helper
(cf. patch 2 on this series) on block code?

I couldn't find any other way to do this (get all block devices on the
system), so please let me know if I missed something.


Cheers,

Enzo

Re: [RFC PATCH 1/2] block: export block_class and disk_type symbols

[Greg Kroah-Hartman]

On Sun, May 02, 2021 at 11:37:35PM -0300, Enzo Matsumiya wrote:
> On 05/01, Greg Kroah-Hartman wrote:
> > On Fri, Apr 30, 2021 at 03:32:10PM -0300, Enzo Matsumiya wrote:
> > > Export symbols to be used by _for_each_blk() helper in LED block
> > > trigger.
> > > 
> > > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> > > ---
> > >  block/genhd.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index 8c8f543572e6..516495179230 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -1218,6 +1218,7 @@ static void disk_release(struct device *dev)
> > >  struct class block_class = {
> > >  	.name		= "block",
> > >  };
> > > +EXPORT_SYMBOL(block_class);
> > > 
> > >  static char *block_devnode(struct device *dev, umode_t *mode,
> > >  			   kuid_t *uid, kgid_t *gid)
> > > @@ -1235,6 +1236,7 @@ const struct device_type disk_type = {
> > >  	.release	= disk_release,
> > >  	.devnode	= block_devnode,
> > >  };
> > > +EXPORT_SYMBOL(disk_type);
> > > 
> > >  #ifdef CONFIG_PROC_FS
> > >  /*
> > 
> > Please please no.  These should not be needed by anything.
> > 
> > And if they really do, they must be EXPORT_SYMBOL_GPL().
> > 
> > thanks,
> > 
> > greg k-h
> 
> Thanks. I was indeed skeptical about submitting this particular change.
> 
> Do you think it's more acceptable if I implement a for_each_blk() helper
> (cf. patch 2 on this series) on block code?
> 
> I couldn't find any other way to do this (get all block devices on the
> system), so please let me know if I missed something.

Why would you want a list of all block devices in the system?  What are
you going to do with such a thing?  How does the block core handle other
random drivers getting references to the things it thinks it controls?

thanks,

greg k-h

Re: [RFC PATCH 1/2] block: export block_class and disk_type symbols

[Christoph Hellwig]

On Fri, Apr 30, 2021 at 03:32:10PM -0300, Enzo Matsumiya wrote:
> Export symbols to be used by _for_each_blk() helper in LED block
> trigger.

No way.

Re: [RFC PATCH 1/2] block: export block_class and disk_type symbols

[Enzo Matsumiya]

On 05/03, Christoph Hellwig wrote:
>On Fri, Apr 30, 2021 at 03:32:10PM -0300, Enzo Matsumiya wrote:
>> Export symbols to be used by _for_each_blk() helper in LED block
>> trigger.
>
>No way.

Thanks everyone, I'll fix this in the next patch version.


Cheers,

Enzo

[RFC PATCH 2/2] leds: trigger: implement block trigger

[Enzo Matsumiya]

Usage (using capslock LED as example/test):

    openSUSE-tw:/sys/class/leds/input0::capslock # echo block > trigger
    openSUSE-tw:/sys/class/leds/input0::capslock # cat trigger
    none usb-gadget ... ... [block]

A single "block" trigger is created, with each non-empty block device
being available to have its stats polled.

    openSUSE-tw:/sys/class/leds/input0::capslock # ls
    block_devices  brightness  device  max_brightness  power  subsystem  trigger  uevent
    openSUSE-tw:/sys/class/leds/input0::capslock # ls block_devices/
    sda  sr0
    openSUSE-tw:/sys/class/leds/input0::capslock # cat block_devices/sda
    1
    openSUSE-tw:/sys/class/leds/input0::capslock # echo 0 > block_devices/sr0

Activity is then represented in an accumulated manner (part_read_stat_accum()),
with a fixed blinking interval of 50ms.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 drivers/leds/trigger/Kconfig         |  10 +
 drivers/leds/trigger/Makefile        |   1 +
 drivers/leds/trigger/ledtrig-block.c | 293 +++++++++++++++++++++++++++
 3 files changed, 304 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-block.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index b77a01bd27f4..bead31a19148 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -153,4 +153,14 @@ config LEDS_TRIGGER_TTY
 
 	  When build as a module this driver will be called ledtrig-tty.
 
+config LEDS_TRIGGER_BLOCK
+	tristate "LED Block Device Trigger"
+	depends on BLOCK
+	default m
+	help
+	  This allows LEDs to be controlled by block device activity.
+	  This trigger doesn't require the lower level drivers to have any
+	  instrumentation. The activity is collected by polling the disk stats.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 25c4db97cdd4..cadc77d95802 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
 obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
+obj-$(CONFIG_LEDS_TRIGGER_BLOCK)	+= ledtrig-block.o
diff --git a/drivers/leds/trigger/ledtrig-block.c b/drivers/leds/trigger/ledtrig-block.c
new file mode 100644
index 000000000000..b00dbf916876
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-block.c
@@ -0,0 +1,293 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LED block trigger
+ *
+ * Copyright (C) 2021 Enzo Matsumiya <ematsumiya@suse.de>
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/genhd.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+#include <linux/part_stat.h>
+
+#include "../leds.h"
+
+extern struct class block_class;
+extern const struct device_type disk_type;
+
+struct ledtrig_blk_data {
+	struct led_classdev *led_cdev;
+	struct list_head block_devices;
+};
+
+struct ledtrig_blk_device {
+	struct list_head list;
+
+	struct ledtrig_blk_data *data;
+
+	struct gendisk *disk;
+	struct device_attribute attr;
+	struct delayed_work work;
+	struct mutex lock;
+	u64 last_activity;
+	bool observed;
+};
+
+/*
+ * Blink interval in msecs
+ */
+#define BLINK_INTERVAL 50
+
+
+/*
+ * Helpers
+ */
+
+static int _for_each_blk(void *data,
+			  int (*fn)(void *, struct gendisk *))
+{
+	struct class_dev_iter iter;
+	struct device *dev;
+	int err;
+
+	/* iterate through all block devices on the system */
+	class_dev_iter_init(&iter, &block_class, NULL, &disk_type);
+	while ((dev = class_dev_iter_next(&iter))) {
+	        struct gendisk *disk = dev_to_disk(dev);
+
+	        err = fn(data, disk);
+
+	        if (err) {
+	                pr_err("error running fn() on disk %s\n", disk->disk_name);
+	                return err;
+	        }
+	}
+	class_dev_iter_exit(&iter);
+
+	return 0;
+}
+
+
+/*
+ * Device attr
+ */
+
+static ssize_t ledtrig_blk_device_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct ledtrig_blk_device *device = container_of(attr,
+							 struct ledtrig_blk_device,
+							 attr);
+	bool observed;
+
+	mutex_lock(&device->lock);
+	observed = device->observed;
+	mutex_unlock(&device->lock);
+
+	return sprintf(buf, "%d\n", observed) + 1;
+}
+
+static ssize_t ledtrig_blk_device_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t size)
+{
+	struct ledtrig_blk_device *device = container_of(attr,
+							 struct ledtrig_blk_device,
+							 attr);
+	int err = -EINVAL;
+
+	mutex_lock(&device->lock);
+	if (!strcmp(buf, "0") || !strcmp(buf, "0\n"))
+		device->observed = 0;
+	else if (!strcmp(buf, "1") || !strcmp(buf, "1\n"))
+		device->observed = 1;
+	else
+		goto out_unlock;
+
+	err = size;
+
+out_unlock:
+	mutex_unlock(&device->lock);
+
+	return err;
+}
+
+static struct attribute *devices_attrs[] = {
+	NULL,
+};
+
+static const struct attribute_group devices_group = {
+	.name = "block_devices",
+	.attrs = devices_attrs,
+};
+
+
+/*
+ * Work
+ */
+
+static void ledtrig_blk_work(struct work_struct *work)
+{
+	struct ledtrig_blk_device *device = container_of(work, struct ledtrig_blk_device, work.work);
+	struct gendisk *disk;
+	unsigned long interval = BLINK_INTERVAL;
+	u64 activity;
+
+	if (!device->observed)
+		goto out;
+
+	disk = device->disk;
+	activity = part_stat_read_accum(disk->part0, ios);
+
+	if (device->last_activity != activity) {
+		led_stop_software_blink(device->data->led_cdev);
+		led_blink_set_oneshot(device->data->led_cdev, &interval, &interval, 0);
+
+		device->last_activity = activity;
+	}
+
+out:
+	schedule_delayed_work(&device->work, interval * 2);
+}
+
+
+/*
+ * Adding & removing block devices
+ */
+
+static int ledtrig_blk_add_device(void *data,
+				  struct gendisk *disk)
+{
+	struct ledtrig_blk_data *led_blk_data = (struct ledtrig_blk_data *) data;
+	struct led_classdev *led_cdev = led_blk_data->led_cdev;
+	struct ledtrig_blk_device *device;
+	int err;
+
+	device = kzalloc(sizeof(*device), GFP_KERNEL);
+	if (!device) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	device->data = led_blk_data;
+	device->observed = true;
+
+	sysfs_attr_init(&device->attr.attr);
+	device->attr.attr.name = disk->disk_name;
+	device->attr.attr.mode = S_IRUSR | S_IWUSR;
+	device->attr.show = ledtrig_blk_device_show;
+	device->attr.store = ledtrig_blk_device_store;
+	device->disk = disk;
+	device->last_activity = 0;
+
+	INIT_DELAYED_WORK(&device->work, ledtrig_blk_work);
+	mutex_init(&device->lock);
+
+	list_add_tail(&device->list, &led_blk_data->block_devices);
+
+	err = sysfs_add_file_to_group(&led_cdev->dev->kobj, &device->attr.attr,
+				      devices_group.name);
+	if (err)
+		goto err_free;
+
+	schedule_delayed_work(&device->work, BLINK_INTERVAL * 2);
+
+	return 0;
+
+err_free:
+	kfree(device);
+err_out:
+	return err;
+}
+
+static int ledtrig_blk_add_all_devices(struct ledtrig_blk_data *led_blk_data)
+{
+	(void)_for_each_blk(led_blk_data, ledtrig_blk_add_device);
+
+	return 0;
+}
+
+static void ledtrig_blk_remove_device(struct ledtrig_blk_data *led_blk_data,
+				      struct ledtrig_blk_device *device)
+{
+	struct led_classdev *led_cdev = led_blk_data->led_cdev;
+
+	list_del(&device->list);
+	sysfs_remove_file_from_group(&led_cdev->dev->kobj, &device->attr.attr,
+				     devices_group.name);
+	kfree(device);
+}
+
+
+/*
+ * Init, exit, etc
+ */
+
+static int ledtrig_blk_activate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_blk_data *led_blk_data;
+	int err;
+
+	led_blk_data = kzalloc(sizeof(*led_blk_data), GFP_KERNEL);
+	if (!led_blk_data)
+		return -ENOMEM;
+
+	led_blk_data->led_cdev = led_cdev;
+
+	/* List of devices */
+	INIT_LIST_HEAD(&led_blk_data->block_devices);
+	err = sysfs_create_group(&led_cdev->dev->kobj, &devices_group);
+	if (err)
+		goto err_free;
+
+	ledtrig_blk_add_all_devices(led_blk_data);
+	led_set_trigger_data(led_cdev, led_blk_data);
+
+	return 0;
+
+err_free:
+	kfree(led_blk_data);
+	return err;
+}
+
+static void ledtrig_blk_deactivate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_blk_data *led_blk_data = led_get_trigger_data(led_cdev);
+	struct ledtrig_blk_device *device, *tmp;
+
+	list_for_each_entry_safe(device, tmp, &led_blk_data->block_devices, list) {
+		cancel_delayed_work_sync(&device->work);
+		ledtrig_blk_remove_device(led_blk_data, device);
+	}
+
+	sysfs_remove_group(&led_cdev->dev->kobj, &devices_group);
+
+	kfree(led_blk_data);
+}
+
+static struct led_trigger ledtrig_blk_trigger = {
+	.name = "block",
+	.activate = ledtrig_blk_activate,
+	.deactivate = ledtrig_blk_deactivate,
+};
+
+static int __init ledtrig_blk_init(void)
+{
+	return led_trigger_register(&ledtrig_blk_trigger);
+}
+
+static void __exit ledtrig_blk_exit(void)
+{
+	led_trigger_unregister(&ledtrig_blk_trigger);
+}
+
+module_init(ledtrig_blk_init);
+module_exit(ledtrig_blk_exit);
+
+MODULE_AUTHOR("Enzo Matsumiya <ematsumiya@suse.de>");
+MODULE_DESCRIPTION("LED block trigger");
+MODULE_LICENSE("GPL v2");
+
-- 
2.31.1

Re: [RFC PATCH 2/2] leds: trigger: implement block trigger

[Randy Dunlap]

On 4/30/21 11:32 AM, Enzo Matsumiya wrote:
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index b77a01bd27f4..bead31a19148 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -153,4 +153,14 @@ config LEDS_TRIGGER_TTY
>  
>  	  When build as a module this driver will be called ledtrig-tty.
>  
> +config LEDS_TRIGGER_BLOCK
> +	tristate "LED Block Device Trigger"
> +	depends on BLOCK
> +	default m

Drop the "default m". We don't enable drivers (even to build modules)
unless they are necessary, e.g., for booting.

> +	help
> +	  This allows LEDs to be controlled by block device activity.
> +	  This trigger doesn't require the lower level drivers to have any
> +	  instrumentation. The activity is collected by polling the disk stats.
> +	  If unsure, say Y.
> +
>  endif # LEDS_TRIGGERS

thanks.
-- 
~Randy

Re: [RFC PATCH 2/2] leds: trigger: implement block trigger

[Enzo Matsumiya]

On 04/30, Randy Dunlap wrote:
>On 4/30/21 11:32 AM, Enzo Matsumiya wrote:
>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>> index b77a01bd27f4..bead31a19148 100644
>> --- a/drivers/leds/trigger/Kconfig
>> +++ b/drivers/leds/trigger/Kconfig
>> @@ -153,4 +153,14 @@ config LEDS_TRIGGER_TTY
>>
>>  	  When build as a module this driver will be called ledtrig-tty.
>>
>> +config LEDS_TRIGGER_BLOCK
>> +	tristate "LED Block Device Trigger"
>> +	depends on BLOCK
>> +	default m
>
>Drop the "default m". We don't enable drivers (even to build modules)
>unless they are necessary, e.g., for booting.
>
>> +	help
>> +	  This allows LEDs to be controlled by block device activity.
>> +	  This trigger doesn't require the lower level drivers to have any
>> +	  instrumentation. The activity is collected by polling the disk stats.
>> +	  If unsure, say Y.
>> +
>>  endif # LEDS_TRIGGERS
>
>thanks.
>-- 
>~Randy

Thanks, will do in v2.


Cheers,

Enzo

Re: [RFC PATCH 2/2] leds: trigger: implement block trigger

[Marek Behun]

On Fri, 30 Apr 2021 15:32:11 -0300
Enzo Matsumiya <ematsumiya@suse.de> wrote:

> Activity is then represented in an accumulated manner (part_read_stat_accum()),
> with a fixed blinking interval of 50ms.

part_stat_read_accum, not part_read_stat_accum

Why only accum? With the netdev trigger, you can choose whether rx, tx,
or both are blinking the LED.

Also I think the trigger should be called "blockdev" instead of
"block". This is consistent with "netdev", and avoids misinterpretation
with the verb "to block".

Marek

Re: [RFC PATCH 2/2] leds: trigger: implement block trigger

[Enzo Matsumiya]

On 04/30, Marek Behun wrote:
>On Fri, 30 Apr 2021 15:32:11 -0300
>Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
>> Activity is then represented in an accumulated manner (part_read_stat_accum()),
>> with a fixed blinking interval of 50ms.
>
>part_stat_read_accum, not part_read_stat_accum

Good catch, will fix in v2. Thanks.

>Why only accum? With the netdev trigger, you can choose whether rx, tx,
>or both are blinking the LED.

The original patch from Akinobu Mita could distinct between READ,
WRITE, and DISCARD. My reasoning to not follow that was I've seen
NICs with a TX and RX LED (i.e. netdev follows that), but I've never
seen any disk activity indicator with separated LEDs for read and write,
for example. So accum made sense to me.

If this is really desired, I can come up with this, but I'd like to hear
from others.

>Also I think the trigger should be called "blockdev" instead of
>"block". This is consistent with "netdev", and avoids misinterpretation
>with the verb "to block".

Thanks. I'll change this in v2.

>
>Marek

Cheers,

Enzo

Re: [RFC PATCH 2/2] leds: trigger: implement block trigger

[Hannes Reinecke]

On 4/30/21 8:32 PM, Enzo Matsumiya wrote:
> Usage (using capslock LED as example/test):
> 
>      openSUSE-tw:/sys/class/leds/input0::capslock # echo block > trigger
>      openSUSE-tw:/sys/class/leds/input0::capslock # cat trigger
>      none usb-gadget ... ... [block]
> 
> A single "block" trigger is created, with each non-empty block device
> being available to have its stats polled.
> 
>      openSUSE-tw:/sys/class/leds/input0::capslock # ls
>      block_devices  brightness  device  max_brightness  power  subsystem  trigger  uevent
>      openSUSE-tw:/sys/class/leds/input0::capslock # ls block_devices/
>      sda  sr0
>      openSUSE-tw:/sys/class/leds/input0::capslock # cat block_devices/sda
>      1
>      openSUSE-tw:/sys/class/leds/input0::capslock # echo 0 > block_devices/sr0
> 
> Activity is then represented in an accumulated manner (part_read_stat_accum()),
> with a fixed blinking interval of 50ms.
> 
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
>   drivers/leds/trigger/Kconfig         |  10 +
>   drivers/leds/trigger/Makefile        |   1 +
>   drivers/leds/trigger/ledtrig-block.c | 293 +++++++++++++++++++++++++++
>   3 files changed, 304 insertions(+)
>   create mode 100644 drivers/leds/trigger/ledtrig-block.c
> 
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index b77a01bd27f4..bead31a19148 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -153,4 +153,14 @@ config LEDS_TRIGGER_TTY
>   
>   	  When build as a module this driver will be called ledtrig-tty.
>   
> +config LEDS_TRIGGER_BLOCK
> +	tristate "LED Block Device Trigger"
> +	depends on BLOCK
> +	default m
> +	help
> +	  This allows LEDs to be controlled by block device activity.
> +	  This trigger doesn't require the lower level drivers to have any
> +	  instrumentation. The activity is collected by polling the disk stats.
> +	  If unsure, say Y.
> +
>   endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 25c4db97cdd4..cadc77d95802 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
>   obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
>   obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
>   obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
> +obj-$(CONFIG_LEDS_TRIGGER_BLOCK)	+= ledtrig-block.o
> diff --git a/drivers/leds/trigger/ledtrig-block.c b/drivers/leds/trigger/ledtrig-block.c
> new file mode 100644
> index 000000000000..b00dbf916876
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-block.c
> @@ -0,0 +1,293 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LED block trigger
> + *
> + * Copyright (C) 2021 Enzo Matsumiya <ematsumiya@suse.de>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/genhd.h>
> +#include <linux/leds.h>
> +#include <linux/workqueue.h>
> +#include <linux/part_stat.h>
> +
> +#include "../leds.h"
> +
> +extern struct class block_class;
> +extern const struct device_type disk_type;
> +
> +struct ledtrig_blk_data {
> +	struct led_classdev *led_cdev;
> +	struct list_head block_devices;
> +};
> +
> +struct ledtrig_blk_device {
> +	struct list_head list;
> +
> +	struct ledtrig_blk_data *data;
> +
> +	struct gendisk *disk;
> +	struct device_attribute attr;
> +	struct delayed_work work;
> +	struct mutex lock;
> +	u64 last_activity;
> +	bool observed;
> +};
> +
> +/*
> + * Blink interval in msecs
> + */
> +#define BLINK_INTERVAL 50
> +
> +
> +/*
> + * Helpers
> + */
> +
> +static int _for_each_blk(void *data,
> +			  int (*fn)(void *, struct gendisk *))
> +{
> +	struct class_dev_iter iter;
> +	struct device *dev;
> +	int err;
> +
> +	/* iterate through all block devices on the system */
> +	class_dev_iter_init(&iter, &block_class, NULL, &disk_type);
> +	while ((dev = class_dev_iter_next(&iter))) {
> +	        struct gendisk *disk = dev_to_disk(dev);
> +
> +	        err = fn(data, disk);
> +
> +	        if (err) {
> +	                pr_err("error running fn() on disk %s\n", disk->disk_name);
> +	                return err;
> +	        }
> +	}
> +	class_dev_iter_exit(&iter);
> +
> +	return 0;
> +}
> +
> +
> +/*
> + * Device attr
> + */
> +
> +static ssize_t ledtrig_blk_device_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct ledtrig_blk_device *device = container_of(attr,
> +							 struct ledtrig_blk_device,
> +							 attr);
> +	bool observed;
> +
> +	mutex_lock(&device->lock);
> +	observed = device->observed;
> +	mutex_unlock(&device->lock);
> +
> +	return sprintf(buf, "%d\n", observed) + 1;
> +}
> +
> +static ssize_t ledtrig_blk_device_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t size)
> +{
> +	struct ledtrig_blk_device *device = container_of(attr,
> +							 struct ledtrig_blk_device,
> +							 attr);
> +	int err = -EINVAL;
> +
> +	mutex_lock(&device->lock);
> +	if (!strcmp(buf, "0") || !strcmp(buf, "0\n"))
> +		device->observed = 0;
> +	else if (!strcmp(buf, "1") || !strcmp(buf, "1\n"))
> +		device->observed = 1;
> +	else
> +		goto out_unlock;
> +
> +	err = size;
> +
> +out_unlock:
> +	mutex_unlock(&device->lock);
> +
> +	return err;
> +}
> +
> +static struct attribute *devices_attrs[] = {
> +	NULL,
> +};
> +
> +static const struct attribute_group devices_group = {
> +	.name = "block_devices",
> +	.attrs = devices_attrs,
> +};
> +
> +
> +/*
> + * Work
> + */
> +
> +static void ledtrig_blk_work(struct work_struct *work)
> +{
> +	struct ledtrig_blk_device *device = container_of(work, struct ledtrig_blk_device, work.work);
> +	struct gendisk *disk;
> +	unsigned long interval = BLINK_INTERVAL;
> +	u64 activity;
> +
> +	if (!device->observed)
> +		goto out;
> +
> +	disk = device->disk;
> +	activity = part_stat_read_accum(disk->part0, ios);
> +
> +	if (device->last_activity != activity) {
> +		led_stop_software_blink(device->data->led_cdev);
> +		led_blink_set_oneshot(device->data->led_cdev, &interval, &interval, 0);
> +
> +		device->last_activity = activity;
> +	}
> +
> +out:
> +	schedule_delayed_work(&device->work, interval * 2);
> +}
> +
> +
> +/*
> + * Adding & removing block devices
> + */
> +
> +static int ledtrig_blk_add_device(void *data,
> +				  struct gendisk *disk)
> +{
> +	struct ledtrig_blk_data *led_blk_data = (struct ledtrig_blk_data *) data;
> +	struct led_classdev *led_cdev = led_blk_data->led_cdev;
> +	struct ledtrig_blk_device *device;
> +	int err;
> +
> +	device = kzalloc(sizeof(*device), GFP_KERNEL);
> +	if (!device) {
> +		err = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	device->data = led_blk_data;
> +	device->observed = true;
> +
> +	sysfs_attr_init(&device->attr.attr);
> +	device->attr.attr.name = disk->disk_name;
> +	device->attr.attr.mode = S_IRUSR | S_IWUSR;
> +	device->attr.show = ledtrig_blk_device_show;
> +	device->attr.store = ledtrig_blk_device_store;
> +	device->disk = disk;
> +	device->last_activity = 0;
> +
> +	INIT_DELAYED_WORK(&device->work, ledtrig_blk_work);
> +	mutex_init(&device->lock);
> +
> +	list_add_tail(&device->list, &led_blk_data->block_devices);
> +
> +	err = sysfs_add_file_to_group(&led_cdev->dev->kobj, &device->attr.attr,
> +				      devices_group.name);
> +	if (err)
> +		goto err_free;
> +
> +	schedule_delayed_work(&device->work, BLINK_INTERVAL * 2);
> +
> +	return 0;
> +
> +err_free:
> +	kfree(device);
> +err_out:
> +	return err;
> +}
> +
> +static int ledtrig_blk_add_all_devices(struct ledtrig_blk_data *led_blk_data)
> +{
> +	(void)_for_each_blk(led_blk_data, ledtrig_blk_add_device);
> +
> +	return 0;
> +}
> +
> +static void ledtrig_blk_remove_device(struct ledtrig_blk_data *led_blk_data,
> +				      struct ledtrig_blk_device *device)
> +{
> +	struct led_classdev *led_cdev = led_blk_data->led_cdev;
> +
> +	list_del(&device->list);
> +	sysfs_remove_file_from_group(&led_cdev->dev->kobj, &device->attr.attr,
> +				     devices_group.name);
> +	kfree(device);
> +}
> +
> +
> +/*
> + * Init, exit, etc
> + */
> +
> +static int ledtrig_blk_activate(struct led_classdev *led_cdev)
> +{
> +	struct ledtrig_blk_data *led_blk_data;
> +	int err;
> +
> +	led_blk_data = kzalloc(sizeof(*led_blk_data), GFP_KERNEL);
> +	if (!led_blk_data)
> +		return -ENOMEM;
> +
> +	led_blk_data->led_cdev = led_cdev;
> +
> +	/* List of devices */
> +	INIT_LIST_HEAD(&led_blk_data->block_devices);
> +	err = sysfs_create_group(&led_cdev->dev->kobj, &devices_group);
> +	if (err)
> +		goto err_free;
> +
> +	ledtrig_blk_add_all_devices(led_blk_data);
> +	led_set_trigger_data(led_cdev, led_blk_data);
> +
> +	return 0;
> +
> +err_free:
> +	kfree(led_blk_data);
> +	return err;
> +}
> +
> +static void ledtrig_blk_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct ledtrig_blk_data *led_blk_data = led_get_trigger_data(led_cdev);
> +	struct ledtrig_blk_device *device, *tmp;
> +
> +	list_for_each_entry_safe(device, tmp, &led_blk_data->block_devices, list) {
> +		cancel_delayed_work_sync(&device->work);
> +		ledtrig_blk_remove_device(led_blk_data, device);
> +	}
> +
> +	sysfs_remove_group(&led_cdev->dev->kobj, &devices_group);
> +
> +	kfree(led_blk_data);
> +}
> +
> +static struct led_trigger ledtrig_blk_trigger = {
> +	.name = "block",
> +	.activate = ledtrig_blk_activate,
> +	.deactivate = ledtrig_blk_deactivate,
> +};
> +
> +static int __init ledtrig_blk_init(void)
> +{
> +	return led_trigger_register(&ledtrig_blk_trigger);
> +}
> +
> +static void __exit ledtrig_blk_exit(void)
> +{
> +	led_trigger_unregister(&ledtrig_blk_trigger);
> +}
> +
> +module_init(ledtrig_blk_init);
> +module_exit(ledtrig_blk_exit);
> +
> +MODULE_AUTHOR("Enzo Matsumiya <ematsumiya@suse.de>");
> +MODULE_DESCRIPTION("LED block trigger");
> +MODULE_LICENSE("GPL v2");
> +
> 
As already commented on, this for_each_blk() construct is not a good 
idea. Infact, I guess it would be better if you could invert the logic:
Not having the block trigger enumerating all devices, but rather let the 
devices register with the block trigger.
That would have the benefit that one could choose which block device 
should be handled by the LED trigger subsystem, _and_ you would avoid 
the need for a for_each_blk() construct.
Thing is, I don't think that all block devices should be handled by the 
LED trigger; eg for things like 'loop' or 'ramdisk' it is very questionable.

Downside is that you would need to modify the drivers, but realistically 
there are only very few drivers which should be modified; I would go for 
nvme-pci and the sd driver for starters. Maybe floppy, but arguably that 
can omitted as one has a very good audio indicator for floppy accesses :-)

But apart from that: Yay! Good job!

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [RFC PATCH 2/2] leds: trigger: implement block trigger

[Pavel Machek]

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

Hi!

Please trim the emails you are responding to.

> >+MODULE_AUTHOR("Enzo Matsumiya <ematsumiya@suse.de>");
> >+MODULE_DESCRIPTION("LED block trigger");
> >+MODULE_LICENSE("GPL v2");
> >+
> >
> As already commented on, this for_each_blk() construct is not a good idea.
> Infact, I guess it would be better if you could invert the logic:
> Not having the block trigger enumerating all devices, but rather let the
> devices register with the block trigger.
> That would have the benefit that one could choose which block device should
> be handled by the LED trigger subsystem, _and_ you would avoid the need for
> a for_each_blk() construct.
> Thing is, I don't think that all block devices should be handled by the LED
> trigger; eg for things like 'loop' or 'ramdisk' it is very
> >questionable.

> Downside is that you would need to modify the drivers, but realistically
> there are only very few drivers which should be modified; I would go for
> nvme-pci and the sd driver for starters. Maybe floppy, but arguably that can
> omitted as one has a very good audio indicator for floppy accesses
> :-)

And we already have disk activity trigger. Maybe NVMe and SD needs to
be modified to use it?

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC PATCH 2/2] leds: trigger: implement block trigger

[Enzo Matsumiya]

On 05/03, Pavel Machek wrote:
>> As already commented on, this for_each_blk() construct is not a good idea.
>> Infact, I guess it would be better if you could invert the logic:
>> Not having the block trigger enumerating all devices, but rather let the
>> devices register with the block trigger.
>> That would have the benefit that one could choose which block device should
>> be handled by the LED trigger subsystem, _and_ you would avoid the need for
>> a for_each_blk() construct.
>> Thing is, I don't think that all block devices should be handled by the LED
>> trigger; eg for things like 'loop' or 'ramdisk' it is very
>> >questionable.
>
>> Downside is that you would need to modify the drivers, but realistically
>> there are only very few drivers which should be modified; I would go for
>> nvme-pci and the sd driver for starters. Maybe floppy, but arguably that can
>> omitted as one has a very good audio indicator for floppy accesses
>> :-)
>
>And we already have disk activity trigger. Maybe NVMe and SD needs to
>be modified to use it?
>
>Best regards,
>								Pavel

TBH I haven't thought of that. My initial idea was to actually offer
maximum flexibility to the user, so exposing all block devices on the
system [*], being able to set any LED available as an indicator for each
of those.

But, indeed, just using ledtrig-disk in NVMe and SD might just be
simpler.


[*] - again, I see now this was a bad idea and will be changed in a
possible next version

Re: [RFC PATCH 2/2] leds: trigger: implement block trigger

[Pavel Machek]

On Mon 2021-05-03 13:56:15, Enzo Matsumiya wrote:
> On 05/03, Pavel Machek wrote:
> > > As already commented on, this for_each_blk() construct is not a good idea.
> > > Infact, I guess it would be better if you could invert the logic:
> > > Not having the block trigger enumerating all devices, but rather let the
> > > devices register with the block trigger.
> > > That would have the benefit that one could choose which block device should
> > > be handled by the LED trigger subsystem, _and_ you would avoid the need for
> > > a for_each_blk() construct.
> > > Thing is, I don't think that all block devices should be handled by the LED
> > > trigger; eg for things like 'loop' or 'ramdisk' it is very
> > > >questionable.
> > 
> > > Downside is that you would need to modify the drivers, but realistically
> > > there are only very few drivers which should be modified; I would go for
> > > nvme-pci and the sd driver for starters. Maybe floppy, but arguably that can
> > > omitted as one has a very good audio indicator for floppy accesses
> > > :-)
> > 
> > And we already have disk activity trigger. Maybe NVMe and SD needs to
> > be modified to use it?
> 
> TBH I haven't thought of that. My initial idea was to actually offer
> maximum flexibility to the user, so exposing all block devices on the
> system [*], being able to set any LED available as an indicator for each
> of those.
> 
> But, indeed, just using ledtrig-disk in NVMe and SD might just be
> simpler.
> 
> 
> [*] - again, I see now this was a bad idea and will be changed in a
> possible next version

Sounds like there should be no new version. Modify NVMe/SD if
required, instead.

Oh and disk-activity LED trigger blinks when disk is fully loaded. I
believe that is a bug and I'd not mind if it was fixed. I probably
have local patch that needs cleaning up somewhere.

Best regards,
								Pavel

--