[PATCH 0/2] introduce LED block device activity trigger

[PATCH 0/2] introduce LED block device activity trigger

[Akinobu Mita]

This work is inspired by the recent report on linux-nvme mailing list.

disk-activity trigger not working for nvme disk:
http://lists.infradead.org/pipermail/linux-nvme/2019-July/025253.html

This LED block device activity trigger works with any block devices.

Akinobu Mita (2):
  leds: move declaration of led_stop_software_blink() to linux/leds.h
  block: introduce LED block device activity trigger

 block/Makefile        |   1 +
 block/blk-ledtrig.c   | 219 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/blk.h           |  13 +++
 block/genhd.c         |   2 +
 drivers/leds/leds.h   |   1 -
 include/linux/genhd.h |   4 +
 include/linux/leds.h  |   2 +
 7 files changed, 241 insertions(+), 1 deletion(-)
 create mode 100644 block/blk-ledtrig.c

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
-- 
2.7.4

[PATCH 1/2] leds: move declaration of led_stop_software_blink() to linux/leds.h

[Akinobu Mita]

In preparation for adding LED block device activity trigger, expose
led_stop_software_blink() to outside the leds subsystem.

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/leds/leds.h  | 1 -
 include/linux/leds.h | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 47b2294..64fe774 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -18,7 +18,6 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
 }
 
 void led_init_core(struct led_classdev *led_cdev);
-void led_stop_software_blink(struct led_classdev *led_cdev);
 void led_set_brightness_nopm(struct led_classdev *led_cdev,
 				enum led_brightness value);
 void led_set_brightness_nosleep(struct led_classdev *led_cdev,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf57..6aeda4f 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -189,6 +189,8 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
 extern void led_set_brightness(struct led_classdev *led_cdev,
 			       enum led_brightness brightness);
 
+extern void led_stop_software_blink(struct led_classdev *led_cdev);
+
 /**
  * led_set_brightness_sync - set LED brightness synchronously
  * @led_cdev: the LED to set
-- 
2.7.4

[PATCH 2/2] block: introduce LED block device activity trigger

[Akinobu Mita]

This allows LEDs to be controlled by block device activity.

We already have ledtrig-disk (LED disk activity trigger), but the lower
level disk drivers need to utilize ledtrig_disk_activity() to make the
LED blink.

The LED block device trigger doesn't require the lower level drivers to
have any instrumentation. The activity is collected by polling the disk
stats.

Example:

echo block-nvme0n1 > /sys/class/leds/diy/trigger

Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 block/Makefile        |   1 +
 block/blk-ledtrig.c   | 219 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/blk.h           |  13 +++
 block/genhd.c         |   2 +
 include/linux/genhd.h |   4 +
 5 files changed, 239 insertions(+)
 create mode 100644 block/blk-ledtrig.c

diff --git a/block/Makefile b/block/Makefile
index eee1b4c..c74d84e6 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -35,3 +35,4 @@ obj-$(CONFIG_BLK_DEBUG_FS)	+= blk-mq-debugfs.o
 obj-$(CONFIG_BLK_DEBUG_FS_ZONED)+= blk-mq-debugfs-zoned.o
 obj-$(CONFIG_BLK_SED_OPAL)	+= sed-opal.o
 obj-$(CONFIG_BLK_PM)		+= blk-pm.o
+obj-$(CONFIG_LEDS_TRIGGERS)	+= blk-ledtrig.o
diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
new file mode 100644
index 0000000..da93b06
--- /dev/null
+++ b/block/blk-ledtrig.c
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-2.0
+// LED Kernel Blockdev Trigger
+// Derived from ledtrig-netdev.c
+
+#include <linux/atomic.h>
+#include <linux/genhd.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+
+struct blk_ledtrig_data {
+	struct delayed_work work;
+	struct led_classdev *led_cdev;
+
+	atomic_t interval;
+	u64 last_activity;
+
+	unsigned long mode;
+#define BLK_LEDTRIG_READ BIT(0)
+#define BLK_LEDTRIG_WRITE BIT(1)
+#define BLK_LEDTRIG_DISCARD BIT(2)
+};
+
+static ssize_t blk_ledtrig_attr_show(struct device *dev, char *buf,
+				     unsigned long blk_ledtrig)
+{
+	struct blk_ledtrig_data *trig_data = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", test_bit(blk_ledtrig, &trig_data->mode));
+}
+
+static ssize_t blk_ledtrig_attr_store(struct device *dev, const char *buf,
+				      size_t size, unsigned long blk_ledtrig)
+{
+	struct blk_ledtrig_data *trig_data = led_trigger_get_drvdata(dev);
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	if (state)
+		set_bit(blk_ledtrig, &trig_data->mode);
+	else
+		clear_bit(blk_ledtrig, &trig_data->mode);
+
+	return size;
+}
+
+static ssize_t read_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	return blk_ledtrig_attr_show(dev, buf, BLK_LEDTRIG_READ);
+}
+static ssize_t read_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	return blk_ledtrig_attr_store(dev, buf, size, BLK_LEDTRIG_READ);
+}
+static DEVICE_ATTR_RW(read);
+
+static ssize_t write_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return blk_ledtrig_attr_show(dev, buf, BLK_LEDTRIG_WRITE);
+}
+static ssize_t write_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t size)
+{
+	return blk_ledtrig_attr_store(dev, buf, size, BLK_LEDTRIG_WRITE);
+}
+static DEVICE_ATTR_RW(write);
+
+static ssize_t discard_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	return blk_ledtrig_attr_show(dev, buf, BLK_LEDTRIG_DISCARD);
+}
+static ssize_t discard_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	return blk_ledtrig_attr_store(dev, buf, size, BLK_LEDTRIG_DISCARD);
+}
+static DEVICE_ATTR_RW(discard);
+
+static ssize_t interval_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct blk_ledtrig_data *trig_data = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n",
+		       jiffies_to_msecs(atomic_read(&trig_data->interval)));
+}
+static ssize_t interval_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct blk_ledtrig_data *trig_data = led_trigger_get_drvdata(dev);
+	unsigned long value;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &value);
+	if (ret)
+		return ret;
+
+	/* impose some basic bounds on the timer interval */
+	if (value >= 5 && value <= 10000) {
+		cancel_delayed_work_sync(&trig_data->work);
+		atomic_set(&trig_data->interval, msecs_to_jiffies(value));
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(interval);
+
+static struct attribute *blk_ledtrig_attrs[] = {
+	&dev_attr_read.attr,
+	&dev_attr_write.attr,
+	&dev_attr_discard.attr,
+	&dev_attr_interval.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(blk_ledtrig);
+
+static void blk_ledtrig_work(struct work_struct *work)
+{
+	struct blk_ledtrig_data *trig_data =
+		container_of(work, struct blk_ledtrig_data, work.work);
+	struct gendisk *disk = container_of(trig_data->led_cdev->trigger,
+					    struct gendisk, led_trig);
+	u64 activity = 0;
+
+	if (test_bit(BLK_LEDTRIG_READ, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_READ]);
+	if (test_bit(BLK_LEDTRIG_WRITE, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_WRITE]);
+	if (test_bit(BLK_LEDTRIG_DISCARD, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_DISCARD]);
+
+	if (trig_data->last_activity != activity) {
+		unsigned long interval;
+
+		led_stop_software_blink(trig_data->led_cdev);
+		interval = jiffies_to_msecs(atomic_read(&trig_data->interval));
+		led_blink_set_oneshot(trig_data->led_cdev, &interval, &interval,
+				      0);
+
+		trig_data->last_activity = activity;
+	}
+
+	schedule_delayed_work(&trig_data->work,
+			      atomic_read(&trig_data->interval) * 2);
+}
+
+static int blk_ledtrig_activate(struct led_classdev *led_cdev)
+{
+	struct blk_ledtrig_data *trig_data;
+
+	trig_data = kzalloc(sizeof(*trig_data), GFP_KERNEL);
+	if (!trig_data)
+		return -ENOMEM;
+
+	trig_data->mode = BLK_LEDTRIG_READ | BLK_LEDTRIG_WRITE |
+			  BLK_LEDTRIG_DISCARD;
+
+	atomic_set(&trig_data->interval, msecs_to_jiffies(50));
+	trig_data->last_activity = 0;
+	trig_data->led_cdev = led_cdev;
+
+	INIT_DELAYED_WORK(&trig_data->work, blk_ledtrig_work);
+
+	led_set_trigger_data(led_cdev, trig_data);
+
+	schedule_delayed_work(&trig_data->work,
+			      atomic_read(&trig_data->interval) * 2);
+
+	return 0;
+}
+
+static void blk_ledtrig_deactivate(struct led_classdev *led_cdev)
+{
+	struct blk_ledtrig_data *trig_data = led_get_trigger_data(led_cdev);
+
+	cancel_delayed_work_sync(&trig_data->work);
+	kfree(trig_data);
+}
+
+int blk_ledtrig_register(struct gendisk *disk)
+{
+	int ret;
+
+	disk->led_trig.name = kasprintf(GFP_KERNEL, "block-%s",
+					disk->disk_name);
+	if (!disk->led_trig.name)
+		return -ENOMEM;
+
+	disk->led_trig.activate = blk_ledtrig_activate;
+	disk->led_trig.deactivate = blk_ledtrig_deactivate;
+	disk->led_trig.groups = blk_ledtrig_groups;
+
+	ret = led_trigger_register(&disk->led_trig);
+	if (ret) {
+		kfree(disk->led_trig.name);
+		disk->led_trig.name = NULL;
+	}
+
+	return ret;
+}
+
+void blk_ledtrig_unregister(struct gendisk *disk)
+{
+	if (!disk->led_trig.name)
+		return;
+
+	led_trigger_unregister(&disk->led_trig);
+	kfree(disk->led_trig.name);
+	disk->led_trig.name = NULL;
+}
diff --git a/block/blk.h b/block/blk.h
index 7814aa2..dd4c230a 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -331,4 +331,17 @@ void blk_queue_free_zone_bitmaps(struct request_queue *q);
 static inline void blk_queue_free_zone_bitmaps(struct request_queue *q) {}
 #endif
 
+#ifdef CONFIG_LEDS_TRIGGERS
+int blk_ledtrig_register(struct gendisk *disk);
+void blk_ledtrig_unregister(struct gendisk *disk);
+#else
+static inline int blk_ledtrig_register(struct gendisk *disk)
+{
+	return 0;
+}
+static inline void blk_ledtrig_unregister(struct gendisk *disk)
+{
+}
+#endif /* CONFIG_LEDS_TRIGGERS */
+
 #endif /* BLK_INTERNAL_H */
diff --git a/block/genhd.c b/block/genhd.c
index 24654e1..dfd210c 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -745,6 +745,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 
 	disk_add_events(disk);
 	blk_integrity_add(disk);
+	blk_ledtrig_register(disk);
 }
 
 void device_add_disk(struct device *parent, struct gendisk *disk,
@@ -766,6 +767,7 @@ void del_gendisk(struct gendisk *disk)
 	struct disk_part_iter piter;
 	struct hd_struct *part;
 
+	blk_ledtrig_unregister(disk);
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 8b5330d..9250e9c 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -17,6 +17,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/uuid.h>
 #include <linux/blk_types.h>
+#include <linux/leds.h>
 #include <asm/local.h>
 
 #ifdef CONFIG_BLOCK
@@ -219,6 +220,9 @@ struct gendisk {
 	int node_id;
 	struct badblocks *bb;
 	struct lockdep_map lockdep_map;
+#ifdef CONFIG_LEDS_TRIGGERS
+	struct led_trigger led_trig;
+#endif
 };
 
 static inline struct gendisk *part_to_disk(struct hd_struct *part)
-- 
2.7.4

Re: [PATCH 2/2] block: introduce LED block device activity trigger

[Jacek Anaszewski]

Hi Akinobu,

Thank you for the patch set. It looks nice in general, but I'd like
to maintain it under LED subsystem. See my below comments.

On 7/6/19 7:58 PM, Akinobu Mita wrote:
> This allows LEDs to be controlled by block device activity.
> 
> We already have ledtrig-disk (LED disk activity trigger), but the lower
> level disk drivers need to utilize ledtrig_disk_activity() to make the
> LED blink.
> 
> The LED block device trigger doesn't require the lower level drivers to
> have any instrumentation. The activity is collected by polling the disk
> stats.
> 
> Example:
> 
> echo block-nvme0n1 > /sys/class/leds/diy/trigger
> 
> Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  block/Makefile        |   1 +
>  block/blk-ledtrig.c   | 219 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/blk.h           |  13 +++
>  block/genhd.c         |   2 +
>  include/linux/genhd.h |   4 +
>  5 files changed, 239 insertions(+)
>  create mode 100644 block/blk-ledtrig.c
> 
> diff --git a/block/Makefile b/block/Makefile
> index eee1b4c..c74d84e6 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -35,3 +35,4 @@ obj-$(CONFIG_BLK_DEBUG_FS)	+= blk-mq-debugfs.o
>  obj-$(CONFIG_BLK_DEBUG_FS_ZONED)+= blk-mq-debugfs-zoned.o
>  obj-$(CONFIG_BLK_SED_OPAL)	+= sed-opal.o
>  obj-$(CONFIG_BLK_PM)		+= blk-pm.o
> +obj-$(CONFIG_LEDS_TRIGGERS)	+= blk-ledtrig.o
> diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c

Please move the whole trigger implementation to
drivers/leds/trigger and rename the file to ledtrig-blk.c

> new file mode 100644
> index 0000000..da93b06
> --- /dev/null
> +++ b/block/blk-ledtrig.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// LED Kernel Blockdev Trigger
> +// Derived from ledtrig-netdev.c
> +
> +#include <linux/atomic.h>
> +#include <linux/genhd.h>
> +#include <linux/leds.h>
> +#include <linux/workqueue.h>
> +
> +struct blk_ledtrig_data {
> +	struct delayed_work work;
> +	struct led_classdev *led_cdev;
> +
> +	atomic_t interval;
> +	u64 last_activity;
> +
> +	unsigned long mode;
> +#define BLK_LEDTRIG_READ BIT(0)
> +#define BLK_LEDTRIG_WRITE BIT(1)
> +#define BLK_LEDTRIG_DISCARD BIT(2)

s/BLK_LEDTRIG/LEDTRIG_BLK/

> +};
> +
> +static ssize_t blk_ledtrig_attr_show(struct device *dev, char *buf,
> +				     unsigned long blk_ledtrig)
> +{
> +	struct blk_ledtrig_data *trig_data = led_trigger_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", test_bit(blk_ledtrig, &trig_data->mode));
> +}
> +
> +static ssize_t blk_ledtrig_attr_store(struct device *dev, const char *buf,
> +				      size_t size, unsigned long blk_ledtrig)
> +{
> +	struct blk_ledtrig_data *trig_data = led_trigger_get_drvdata(dev);
> +	unsigned long state;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &state);
> +	if (ret)
> +		return ret;
> +
> +	if (state)
> +		set_bit(blk_ledtrig, &trig_data->mode);
> +	else
> +		clear_bit(blk_ledtrig, &trig_data->mode);
> +
> +	return size;
> +}
> +
> +static ssize_t read_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	return blk_ledtrig_attr_show(dev, buf, BLK_LEDTRIG_READ);
> +}
> +static ssize_t read_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t size)
> +{
> +	return blk_ledtrig_attr_store(dev, buf, size, BLK_LEDTRIG_READ);
> +}
> +static DEVICE_ATTR_RW(read);
> +
> +static ssize_t write_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	return blk_ledtrig_attr_show(dev, buf, BLK_LEDTRIG_WRITE);
> +}
> +static ssize_t write_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t size)
> +{
> +	return blk_ledtrig_attr_store(dev, buf, size, BLK_LEDTRIG_WRITE);
> +}
> +static DEVICE_ATTR_RW(write);
> +
> +static ssize_t discard_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	return blk_ledtrig_attr_show(dev, buf, BLK_LEDTRIG_DISCARD);
> +}
> +static ssize_t discard_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t size)
> +{
> +	return blk_ledtrig_attr_store(dev, buf, size, BLK_LEDTRIG_DISCARD);
> +}
> +static DEVICE_ATTR_RW(discard);
> +
> +static ssize_t interval_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct blk_ledtrig_data *trig_data = led_trigger_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n",
> +		       jiffies_to_msecs(atomic_read(&trig_data->interval)));
> +}
> +static ssize_t interval_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct blk_ledtrig_data *trig_data = led_trigger_get_drvdata(dev);
> +	unsigned long value;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &value);
> +	if (ret)
> +		return ret;
> +
> +	/* impose some basic bounds on the timer interval */
> +	if (value >= 5 && value <= 10000) {
> +		cancel_delayed_work_sync(&trig_data->work);
> +		atomic_set(&trig_data->interval, msecs_to_jiffies(value));
> +		schedule_delayed_work(&trig_data->work,
> +				      atomic_read(&trig_data->interval) * 2);
> +	}
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(interval);
> +
> +static struct attribute *blk_ledtrig_attrs[] = {
> +	&dev_attr_read.attr,
> +	&dev_attr_write.attr,
> +	&dev_attr_discard.attr,
> +	&dev_attr_interval.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(blk_ledtrig);
> +
> +static void blk_ledtrig_work(struct work_struct *work)
> +{
> +	struct blk_ledtrig_data *trig_data =
> +		container_of(work, struct blk_ledtrig_data, work.work);
> +	struct gendisk *disk = container_of(trig_data->led_cdev->trigger,
> +					    struct gendisk, led_trig);
> +	u64 activity = 0;
> +
> +	if (test_bit(BLK_LEDTRIG_READ, &trig_data->mode))
> +		activity += part_stat_read(&disk->part0, ios[STAT_READ]);
> +	if (test_bit(BLK_LEDTRIG_WRITE, &trig_data->mode))
> +		activity += part_stat_read(&disk->part0, ios[STAT_WRITE]);
> +	if (test_bit(BLK_LEDTRIG_DISCARD, &trig_data->mode))
> +		activity += part_stat_read(&disk->part0, ios[STAT_DISCARD]);
> +
> +	if (trig_data->last_activity != activity) {
> +		unsigned long interval;
> +
> +		led_stop_software_blink(trig_data->led_cdev);
> +		interval = jiffies_to_msecs(atomic_read(&trig_data->interval));
> +		led_blink_set_oneshot(trig_data->led_cdev, &interval, &interval,
> +				      0);
> +
> +		trig_data->last_activity = activity;
> +	}
> +
> +	schedule_delayed_work(&trig_data->work,
> +			      atomic_read(&trig_data->interval) * 2);
> +}
> +
> +static int blk_ledtrig_activate(struct led_classdev *led_cdev)
> +{
> +	struct blk_ledtrig_data *trig_data;
> +
> +	trig_data = kzalloc(sizeof(*trig_data), GFP_KERNEL);
> +	if (!trig_data)
> +		return -ENOMEM;
> +
> +	trig_data->mode = BLK_LEDTRIG_READ | BLK_LEDTRIG_WRITE |
> +			  BLK_LEDTRIG_DISCARD;
> +
> +	atomic_set(&trig_data->interval, msecs_to_jiffies(50));
> +	trig_data->last_activity = 0;
> +	trig_data->led_cdev = led_cdev;
> +
> +	INIT_DELAYED_WORK(&trig_data->work, blk_ledtrig_work);
> +
> +	led_set_trigger_data(led_cdev, trig_data);
> +
> +	schedule_delayed_work(&trig_data->work,
> +			      atomic_read(&trig_data->interval) * 2);
> +
> +	return 0;
> +}
> +
> +static void blk_ledtrig_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct blk_ledtrig_data *trig_data = led_get_trigger_data(led_cdev);
> +
> +	cancel_delayed_work_sync(&trig_data->work);
> +	kfree(trig_data);
> +}
> +
> +int blk_ledtrig_register(struct gendisk *disk)
> +{
> +	int ret;
> +
> +	disk->led_trig.name = kasprintf(GFP_KERNEL, "block-%s",
> +					disk->disk_name);
> +	if (!disk->led_trig.name)
> +		return -ENOMEM;
> +
> +	disk->led_trig.activate = blk_ledtrig_activate;
> +	disk->led_trig.deactivate = blk_ledtrig_deactivate;
> +	disk->led_trig.groups = blk_ledtrig_groups;
> +
> +	ret = led_trigger_register(&disk->led_trig);
> +	if (ret) {
> +		kfree(disk->led_trig.name);
> +		disk->led_trig.name = NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +void blk_ledtrig_unregister(struct gendisk *disk)
> +{
> +	if (!disk->led_trig.name)
> +		return;
> +
> +	led_trigger_unregister(&disk->led_trig);
> +	kfree(disk->led_trig.name);
> +	disk->led_trig.name = NULL;
> +}
> diff --git a/block/blk.h b/block/blk.h
> index 7814aa2..dd4c230a 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -331,4 +331,17 @@ void blk_queue_free_zone_bitmaps(struct request_queue *q);
>  static inline void blk_queue_free_zone_bitmaps(struct request_queue *q) {}
>  #endif
>  
> +#ifdef CONFIG_LEDS_TRIGGERS
> +int blk_ledtrig_register(struct gendisk *disk);
> +void blk_ledtrig_unregister(struct gendisk *disk);
> +#else
> +static inline int blk_ledtrig_register(struct gendisk *disk)
> +{
> +	return 0;
> +}
> +static inline void blk_ledtrig_unregister(struct gendisk *disk)
> +{
> +}
> +#endif /* CONFIG_LEDS_TRIGGERS */

Please move this part to include/linux/leds.h, next to the other
triggers' facilities.

>  #endif /* BLK_INTERNAL_H */
> diff --git a/block/genhd.c b/block/genhd.c
> index 24654e1..dfd210c 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -745,6 +745,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
>  
>  	disk_add_events(disk);
>  	blk_integrity_add(disk);
> +	blk_ledtrig_register(disk);
>  }
>  
>  void device_add_disk(struct device *parent, struct gendisk *disk,
> @@ -766,6 +767,7 @@ void del_gendisk(struct gendisk *disk)
>  	struct disk_part_iter piter;
>  	struct hd_struct *part;
>  
> +	blk_ledtrig_unregister(disk);
>  	blk_integrity_del(disk);
>  	disk_del_events(disk);
>  
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 8b5330d..9250e9c 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -17,6 +17,7 @@
>  #include <linux/percpu-refcount.h>
>  #include <linux/uuid.h>
>  #include <linux/blk_types.h>
> +#include <linux/leds.h>
>  #include <asm/local.h>
>  
>  #ifdef CONFIG_BLOCK
> @@ -219,6 +220,9 @@ struct gendisk {
>  	int node_id;
>  	struct badblocks *bb;
>  	struct lockdep_map lockdep_map;
> +#ifdef CONFIG_LEDS_TRIGGERS
> +	struct led_trigger led_trig;
> +#endif
>  };
>  
>  static inline struct gendisk *part_to_disk(struct hd_struct *part)
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 2/2] block: introduce LED block device activity trigger

[Akinobu Mita]

2019年7月17日(水) 5:57 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> Hi Akinobu,
>
> Thank you for the patch set. It looks nice in general, but I'd like
> to maintain it under LED subsystem. See my below comments.

Thanks for reviewing. I'll apply your feedback.

> On 7/6/19 7:58 PM, Akinobu Mita wrote:
> > This allows LEDs to be controlled by block device activity.
> >
> > We already have ledtrig-disk (LED disk activity trigger), but the lower
> > level disk drivers need to utilize ledtrig_disk_activity() to make the
> > LED blink.
> >
> > The LED block device trigger doesn't require the lower level drivers to
> > have any instrumentation. The activity is collected by polling the disk
> > stats.
> >
> > Example:
> >
> > echo block-nvme0n1 > /sys/class/leds/diy/trigger
> >
> > Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
> > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Dan Murphy <dmurphy@ti.com>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  block/Makefile        |   1 +
> >  block/blk-ledtrig.c   | 219 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  block/blk.h           |  13 +++
> >  block/genhd.c         |   2 +
> >  include/linux/genhd.h |   4 +
> >  5 files changed, 239 insertions(+)
> >  create mode 100644 block/blk-ledtrig.c
> >
> > diff --git a/block/Makefile b/block/Makefile
> > index eee1b4c..c74d84e6 100644
> > --- a/block/Makefile
> > +++ b/block/Makefile
> > @@ -35,3 +35,4 @@ obj-$(CONFIG_BLK_DEBUG_FS)  += blk-mq-debugfs.o
> >  obj-$(CONFIG_BLK_DEBUG_FS_ZONED)+= blk-mq-debugfs-zoned.o
> >  obj-$(CONFIG_BLK_SED_OPAL)   += sed-opal.o
> >  obj-$(CONFIG_BLK_PM)         += blk-pm.o
> > +obj-$(CONFIG_LEDS_TRIGGERS)  += blk-ledtrig.o
> > diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
>
> Please move the whole trigger implementation to
> drivers/leds/trigger and rename the file to ledtrig-blk.c

OK. Then we don't need to patch 1/2 ("leds: move declaration of
led_stop_software_blink() to linux/leds.h") anymore.

> > new file mode 100644
> > index 0000000..da93b06
> > --- /dev/null
> > +++ b/block/blk-ledtrig.c
> > @@ -0,0 +1,219 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// LED Kernel Blockdev Trigger
> > +// Derived from ledtrig-netdev.c
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/genhd.h>
> > +#include <linux/leds.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct blk_ledtrig_data {
> > +     struct delayed_work work;
> > +     struct led_classdev *led_cdev;
> > +
> > +     atomic_t interval;
> > +     u64 last_activity;
> > +
> > +     unsigned long mode;
> > +#define BLK_LEDTRIG_READ BIT(0)
> > +#define BLK_LEDTRIG_WRITE BIT(1)
> > +#define BLK_LEDTRIG_DISCARD BIT(2)
>
> s/BLK_LEDTRIG/LEDTRIG_BLK/

OK.

> > diff --git a/block/blk.h b/block/blk.h
> > index 7814aa2..dd4c230a 100644
> > --- a/block/blk.h
> > +++ b/block/blk.h
> > @@ -331,4 +331,17 @@ void blk_queue_free_zone_bitmaps(struct request_queue *q);
> >  static inline void blk_queue_free_zone_bitmaps(struct request_queue *q) {}
> >  #endif
> >
> > +#ifdef CONFIG_LEDS_TRIGGERS
> > +int blk_ledtrig_register(struct gendisk *disk);
> > +void blk_ledtrig_unregister(struct gendisk *disk);
> > +#else
> > +static inline int blk_ledtrig_register(struct gendisk *disk)
> > +{
> > +     return 0;
> > +}
> > +static inline void blk_ledtrig_unregister(struct gendisk *disk)
> > +{
> > +}
> > +#endif /* CONFIG_LEDS_TRIGGERS */
>
> Please move this part to include/linux/leds.h, next to the other
> triggers' facilities.

OK.