diff mbox series

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

Message ID 20210430183216.27458-3-ematsumiya@suse.de (mailing list archive)
State New, archived
Headers show
Series leds: trigger: introduce block trigger | expand

Commit Message

Enzo Matsumiya April 30, 2021, 6:32 p.m. UTC 
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

Comments

Randy Dunlap April 30, 2021, 6:52 p.m. UTC | #1 
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.
Marek Behún April 30, 2021, 8:11 p.m. UTC | #2 
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
Enzo Matsumiya May 3, 2021, 2:38 a.m. UTC | #3 
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
Enzo Matsumiya May 3, 2021, 2:46 a.m. UTC | #4 
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
Hannes Reinecke May 3, 2021, 5:53 a.m. UTC | #5 
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
Pavel Machek May 3, 2021, 10:11 a.m. UTC | #6 
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
Enzo Matsumiya May 3, 2021, 4:56 p.m. UTC | #7 
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
Pavel Machek May 4, 2021, 3:43 p.m. UTC | #8 
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

--
diff mbox series

Patch

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");
+