[PATCH v2 0/3] introduce LED block device activity trigger

[PATCH v2 0/3] introduce LED block device activity trigger

[Akinobu Mita]

This work is inspired by the 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.

* v2
- Remove "move declaration of led_stop_software_blink() to linux/leds.h" patch
- Move the trigger implementation to drivers/leds/trigger
- s/blk_ledtrig/ledtrig_blk/
- Add CONFIG_LEDS_TRIGGER_BLOCK
- Fix wrong bitops usages
- Add interface to stop and restart polling disk stats
- Stop polling disk stats for scsi disk during runtime suspend

Akinobu Mita (3):
  block: introduce LED block device activity trigger
  ledtrig-blk: add interface to stop and restart polling disk stats
  scsi: sd: stop polling disk stats by ledtrig-blk during runtime
    suspend

 block/genhd.c                      |   2 +
 drivers/leds/trigger/Kconfig       |   7 +
 drivers/leds/trigger/Makefile      |   1 +
 drivers/leds/trigger/ledtrig-blk.c | 258 +++++++++++++++++++++++++++++++++++++
 drivers/scsi/sd.c                  |  40 +++---
 include/linux/genhd.h              |   3 +
 include/linux/leds.h               |  38 ++++++
 7 files changed, 332 insertions(+), 17 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-blk.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>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
-- 
2.7.4

[PATCH v2 1/3] 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>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 block/genhd.c                      |   2 +
 drivers/leds/trigger/Kconfig       |   7 ++
 drivers/leds/trigger/Makefile      |   1 +
 drivers/leds/trigger/ledtrig-blk.c | 225 +++++++++++++++++++++++++++++++++++++
 include/linux/genhd.h              |   3 +
 include/linux/leds.h               |  27 +++++
 6 files changed, 265 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-blk.c

diff --git a/block/genhd.c b/block/genhd.c
index 54f1f0d3..1c68861 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);
+	ledtrig_blk_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;
 
+	ledtrig_blk_unregister(disk);
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index ce9429c..74ce25d 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -144,4 +144,11 @@ config LEDS_TRIGGER_AUDIO
 	  the audio mute and mic-mute changes.
 	  If unsure, say N
 
+config LEDS_TRIGGER_BLOCK
+	bool "LED Block device Trigger"
+	depends on BLOCK
+	help
+	  This allows LEDs to be controlled by block device activity.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 733a83e..60200eb 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 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_BLOCK)	+= ledtrig-blk.o
diff --git a/drivers/leds/trigger/ledtrig-blk.c b/drivers/leds/trigger/ledtrig-blk.c
new file mode 100644
index 0000000..d5808c9
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blk.c
@@ -0,0 +1,225 @@
+// 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>
+#include "../leds.h"
+
+enum ledtrig_blk_attr {
+	LEDTRIG_BLK_READ,
+	LEDTRIG_BLK_WRITE,
+	LEDTRIG_BLK_DISCARD
+};
+
+struct ledtrig_blk_data {
+	struct delayed_work work;
+	struct led_classdev *led_cdev;
+
+	atomic_t interval;
+	u64 last_activity;
+
+	unsigned long mode;
+};
+
+static ssize_t ledtrig_blk_attr_show(struct device *dev, char *buf,
+				     enum ledtrig_blk_attr attr)
+{
+	struct ledtrig_blk_data *trig_data = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", test_bit(attr, &trig_data->mode));
+}
+
+static ssize_t ledtrig_blk_attr_store(struct device *dev, const char *buf,
+				      size_t size, enum ledtrig_blk_attr attr)
+{
+	struct ledtrig_blk_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(attr, &trig_data->mode);
+	else
+		clear_bit(attr, &trig_data->mode);
+
+	return size;
+}
+
+static ssize_t read_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_READ);
+}
+static ssize_t read_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_READ);
+}
+static DEVICE_ATTR_RW(read);
+
+static ssize_t write_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_WRITE);
+}
+static ssize_t write_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_WRITE);
+}
+static DEVICE_ATTR_RW(write);
+
+static ssize_t discard_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	return ledtrig_blk_attr_show(dev, buf, LEDTRIG_BLK_DISCARD);
+}
+static ssize_t discard_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	return ledtrig_blk_attr_store(dev, buf, size, LEDTRIG_BLK_DISCARD);
+}
+static DEVICE_ATTR_RW(discard);
+
+static ssize_t interval_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct ledtrig_blk_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 ledtrig_blk_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 *ledtrig_blk_attrs[] = {
+	&dev_attr_read.attr,
+	&dev_attr_write.attr,
+	&dev_attr_discard.attr,
+	&dev_attr_interval.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ledtrig_blk);
+
+static void ledtrig_blk_work(struct work_struct *work)
+{
+	struct ledtrig_blk_data *trig_data =
+		container_of(work, struct ledtrig_blk_data, work.work);
+	struct gendisk *disk = container_of(trig_data->led_cdev->trigger,
+					    struct gendisk, led.trig);
+	u64 activity = 0;
+
+	if (test_bit(LEDTRIG_BLK_READ, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_READ]);
+	if (test_bit(LEDTRIG_BLK_WRITE, &trig_data->mode))
+		activity += part_stat_read(&disk->part0, ios[STAT_WRITE]);
+	if (test_bit(LEDTRIG_BLK_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 ledtrig_blk_activate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_blk_data *trig_data;
+
+	trig_data = kzalloc(sizeof(*trig_data), GFP_KERNEL);
+	if (!trig_data)
+		return -ENOMEM;
+
+	trig_data->mode = BIT(LEDTRIG_BLK_READ) | BIT(LEDTRIG_BLK_WRITE) |
+			  BIT(LEDTRIG_BLK_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, ledtrig_blk_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 ledtrig_blk_deactivate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_blk_data *trig_data = led_get_trigger_data(led_cdev);
+
+	cancel_delayed_work_sync(&trig_data->work);
+	kfree(trig_data);
+}
+
+int ledtrig_blk_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 = ledtrig_blk_activate;
+	disk->led.trig.deactivate = ledtrig_blk_deactivate;
+	disk->led.trig.groups = ledtrig_blk_groups;
+
+	ret = led_trigger_register(&disk->led.trig);
+	if (ret) {
+		kfree(disk->led.trig.name);
+		disk->led.trig.name = NULL;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_register);
+
+void ledtrig_blk_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;
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_unregister);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 8b5330d..b2c934e 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,8 @@ struct gendisk {
 	int node_id;
 	struct badblocks *bb;
 	struct lockdep_map lockdep_map;
+
+	struct ledtrig_blk led;
 };
 
 static inline struct gendisk *part_to_disk(struct hd_struct *part)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf57..395fa61 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -517,4 +517,31 @@ static inline void ledtrig_audio_set(enum led_audio type,
 }
 #endif
 
+struct gendisk;
+
+#ifdef CONFIG_LEDS_TRIGGER_BLOCK
+
+struct ledtrig_blk {
+	struct led_trigger trig;
+};
+
+int ledtrig_blk_register(struct gendisk *disk);
+void ledtrig_blk_unregister(struct gendisk *disk);
+
+#else
+
+struct ledtrig_blk {
+};
+
+static inline int ledtrig_blk_register(struct gendisk *disk)
+{
+	return 0;
+}
+
+static inline void ledtrig_blk_unregister(struct gendisk *disk)
+{
+}
+
+#endif /* CONFIG_LEDS_TRIGGER_BLOCK */
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
2.7.4

[PATCH v2 2/3] ledtrig-blk: add interface to stop and restart polling disk stats

[Akinobu Mita]

The LED block device activity trigger periodically polls the disk stats
to collect the activity.  However, it is pointless to poll while the
block device is in quiescent state.

This provides an optional interface to stop and restart polling disk stats
for the lower-level block drivers.

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>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/leds/trigger/ledtrig-blk.c | 37 +++++++++++++++++++++++++++++++++++--
 include/linux/leds.h               | 11 +++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-blk.c b/drivers/leds/trigger/ledtrig-blk.c
index d5808c9..6b826ed 100644
--- a/drivers/leds/trigger/ledtrig-blk.c
+++ b/drivers/leds/trigger/ledtrig-blk.c
@@ -153,8 +153,9 @@ static void ledtrig_blk_work(struct work_struct *work)
 		trig_data->last_activity = activity;
 	}
 
-	schedule_delayed_work(&trig_data->work,
-			      atomic_read(&trig_data->interval) * 2);
+	if (atomic_read(&disk->led.enable_count))
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
 }
 
 static int ledtrig_blk_activate(struct led_classdev *led_cdev)
@@ -190,6 +191,36 @@ static void ledtrig_blk_deactivate(struct led_classdev *led_cdev)
 	kfree(trig_data);
 }
 
+void ledtrig_blk_disable(struct gendisk *disk)
+{
+	if (disk)
+		atomic_dec(&disk->led.enable_count);
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_disable);
+
+void ledtrig_blk_enable(struct gendisk *disk)
+{
+	struct led_classdev *led_cdev;
+
+	if (!disk)
+		return;
+
+	atomic_inc(&disk->led.enable_count);
+
+	read_lock(&disk->led.trig.leddev_list_lock);
+
+	list_for_each_entry(led_cdev, &disk->led.trig.led_cdevs, trig_list) {
+		struct ledtrig_blk_data *trig_data =
+			led_get_trigger_data(led_cdev);
+
+		schedule_delayed_work(&trig_data->work,
+				      atomic_read(&trig_data->interval) * 2);
+	}
+
+	read_unlock(&disk->led.trig.leddev_list_lock);
+}
+EXPORT_SYMBOL_GPL(ledtrig_blk_enable);
+
 int ledtrig_blk_register(struct gendisk *disk)
 {
 	int ret;
@@ -203,6 +234,8 @@ int ledtrig_blk_register(struct gendisk *disk)
 	disk->led.trig.deactivate = ledtrig_blk_deactivate;
 	disk->led.trig.groups = ledtrig_blk_groups;
 
+	atomic_set(&disk->led.enable_count, 1);
+
 	ret = led_trigger_register(&disk->led.trig);
 	if (ret) {
 		kfree(disk->led.trig.name);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 395fa61..fd2eb7c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -523,8 +523,11 @@ struct gendisk;
 
 struct ledtrig_blk {
 	struct led_trigger trig;
+	atomic_t enable_count;
 };
 
+void ledtrig_blk_enable(struct gendisk *disk);
+void ledtrig_blk_disable(struct gendisk *disk);
 int ledtrig_blk_register(struct gendisk *disk);
 void ledtrig_blk_unregister(struct gendisk *disk);
 
@@ -533,6 +536,14 @@ void ledtrig_blk_unregister(struct gendisk *disk);
 struct ledtrig_blk {
 };
 
+static inline void ledtrig_blk_enable(struct gendisk *disk)
+{
+}
+
+static inline void ledtrig_blk_disable(struct gendisk *disk)
+{
+}
+
 static inline int ledtrig_blk_register(struct gendisk *disk)
 {
 	return 0;
-- 
2.7.4

[PATCH v2 3/3] scsi: sd: stop polling disk stats by ledtrig-blk during runtime suspend

[Akinobu Mita]

The LED block device activity trigger periodically polls the disk stats
to collect the activity.  However, it is pointless to poll while the
scsi device is in runtime suspend.

This stops polling disk stats when the device is successfully runtime
suspended, and restarts polling when the device is successfully runtime
resumed.

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>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/scsi/sd.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 149d406..5f73142 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3538,7 +3538,7 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 	struct scsi_sense_hdr sshdr;
-	int ret = 0;
+	int ret;
 
 	if (!sdkp)	/* E.g.: runtime suspend following sd_remove() */
 		return 0;
@@ -3550,18 +3550,16 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 		if (ret) {
 			/* ignore OFFLINE device */
 			if (ret == -ENODEV)
-				return 0;
-
-			if (!scsi_sense_valid(&sshdr) ||
-			    sshdr.sense_key != ILLEGAL_REQUEST)
-				return ret;
+				goto success;
 
 			/*
 			 * sshdr.sense_key == ILLEGAL_REQUEST means this drive
 			 * doesn't support sync. There's not much to do and
 			 * suspend shouldn't fail.
 			 */
-			ret = 0;
+			if (!scsi_sense_valid(&sshdr) ||
+			    sshdr.sense_key != ILLEGAL_REQUEST)
+				return ret;
 		}
 	}
 
@@ -3569,11 +3567,14 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		/* an error is not worth aborting a system sleep */
 		ret = sd_start_stop_device(sdkp, 0);
-		if (ignore_stop_errors)
-			ret = 0;
+		if (ret && !ignore_stop_errors)
+			return ret;
 	}
 
-	return ret;
+success:
+	ledtrig_blk_disable(sdkp->disk);
+
+	return 0;
 }
 
 static int sd_suspend_system(struct device *dev)
@@ -3589,19 +3590,24 @@ static int sd_suspend_runtime(struct device *dev)
 static int sd_resume(struct device *dev)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
-	int ret;
 
 	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
 		return 0;
 
-	if (!sdkp->device->manage_start_stop)
-		return 0;
+	if (sdkp->device->manage_start_stop) {
+		int ret;
+
+		sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
+		ret = sd_start_stop_device(sdkp, 1);
+		if (ret)
+			return ret;
 
-	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
-	ret = sd_start_stop_device(sdkp, 1);
-	if (!ret)
 		opal_unlock_from_suspend(sdkp->opal_dev);
-	return ret;
+	}
+
+	ledtrig_blk_enable(sdkp->disk);
+
+	return 0;
 }
 
 /**
-- 
2.7.4

Re: [PATCH v2 1/3] block: introduce LED block device activity trigger

[kbuild test robot]

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

Hi Akinobu,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.3-rc1 next-20190722]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Akinobu-Mita/block-introduce-LED-block-device-activity-trigger/20190723-074956
config: x86_64-fedora-25 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from drivers/block/umem.c:52:0:
>> drivers/block/umem.h:39:19: error: expected identifier before numeric constant
    #define  LED_OFF  0x00
                      ^
>> include/linux/leds.h:27:2: note: in expansion of macro 'LED_OFF'
     LED_OFF  = 0,
     ^~~~~~~

vim +/LED_OFF +27 include/linux/leds.h

af410fc1 Johannes Berg  2006-09-29  19  
c72a1d60 Richard Purdie 2006-03-31  20  struct device;
5fd752b6 Baolin Wang    2018-10-11  21  struct led_pattern;
c72a1d60 Richard Purdie 2006-03-31  22  /*
c72a1d60 Richard Purdie 2006-03-31  23   * LED Core
c72a1d60 Richard Purdie 2006-03-31  24   */
c72a1d60 Richard Purdie 2006-03-31  25  
c72a1d60 Richard Purdie 2006-03-31  26  enum led_brightness {
c72a1d60 Richard Purdie 2006-03-31 @27  	LED_OFF		= 0,
4e552c8c Andi Shyti     2017-01-05  28  	LED_ON		= 1,
c72a1d60 Richard Purdie 2006-03-31  29  	LED_HALF	= 127,
c72a1d60 Richard Purdie 2006-03-31  30  	LED_FULL	= 255,
c72a1d60 Richard Purdie 2006-03-31  31  };
c72a1d60 Richard Purdie 2006-03-31  32  

:::::: The code at line 27 was first introduced by commit
:::::: c72a1d608dd0eb3d553a08bfdf1c0041bebaa8a0 [PATCH] LED: add LED class

:::::: TO: Richard Purdie <rpurdie@rpsys.net>
:::::: CC: Linus Torvalds <torvalds@g5.osdl.org>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50846 bytes --]

Re: [PATCH v2 1/3] block: introduce LED block device activity trigger

[kbuild test robot]

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

Hi Akinobu,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc1 next-20190722]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Akinobu-Mita/block-introduce-LED-block-device-activity-trigger/20190723-074956
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/scsi/mvsas/mv_94xx.c:11:0:
>> drivers/scsi/mvsas/mv_94xx.h:278:2: error: redeclaration of enumerator 'LED_OFF'
     LED_OFF = 0,
     ^~~~~~~
   In file included from include/linux/genhd.h:20:0,
                    from include/linux/blkdev.h:11,
                    from include/linux/blk-mq.h:5,
                    from include/scsi/scsi_host.h:11,
                    from include/linux/libata.h:21,
                    from include/scsi/libsas.h:16,
                    from drivers/scsi/mvsas/mv_sas.h:27,
                    from drivers/scsi/mvsas/mv_94xx.c:10:
   include/linux/leds.h:27:2: note: previous definition of 'LED_OFF' was here
     LED_OFF  = 0,
     ^~~~~~~
   In file included from drivers/scsi/mvsas/mv_94xx.c:11:0:
>> drivers/scsi/mvsas/mv_94xx.h:279:2: error: redeclaration of enumerator 'LED_ON'
     LED_ON = 1,
     ^~~~~~
   In file included from include/linux/genhd.h:20:0,
                    from include/linux/blkdev.h:11,
                    from include/linux/blk-mq.h:5,
                    from include/scsi/scsi_host.h:11,
                    from include/linux/libata.h:21,
                    from include/scsi/libsas.h:16,
                    from drivers/scsi/mvsas/mv_sas.h:27,
                    from drivers/scsi/mvsas/mv_94xx.c:10:
   include/linux/leds.h:28:2: note: previous definition of 'LED_ON' was here
     LED_ON  = 1,
     ^~~~~~

vim +/LED_OFF +278 drivers/scsi/mvsas/mv_94xx.h

c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  276  
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  277  enum sgpio_led_status {
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27 @278  	LED_OFF	= 0,
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27 @279  	LED_ON	= 1,
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  280  	LED_BLINKA	= 2,
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  281  	LED_BLINKA_INV	= 3,
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  282  	LED_BLINKA_SOF	= 4,
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  283  	LED_BLINKA_EOF	= 5,
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  284  	LED_BLINKB	= 6,
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  285  	LED_BLINKB_INV	= 7,
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  286  };
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  287  
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  288  #define DEFAULT_SGPIO_BITS ((LED_BLINKA_SOF << \
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  289  				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 3) | \
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  290  			(LED_BLINKA_SOF << \
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  291  				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 2) | \
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  292  			(LED_BLINKA_SOF << \
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  293  				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 1) | \
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  294  			(LED_BLINKA_SOF << \
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  295  				MVS_SGPIO_DCTRL_ACT_SHIFT) << (8 * 0))
c56f5f1de3a6ab8 Wilfried Weissmann 2015-12-27  296  
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  297  /*
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  298   * these registers are accessed through port vendor
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  299   * specific address/data registers
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  300   */
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  301  enum sas_sata_phy_regs {
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  302  	GENERATION_1_SETTING		= 0x118,
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  303  	GENERATION_1_2_SETTING		= 0x11C,
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  304  	GENERATION_2_3_SETTING		= 0x120,
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  305  	GENERATION_3_4_SETTING		= 0x124,
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  306  };
f1f82a919d7fff2 Xiangliang Yu      2011-05-24  307  
20b09c2992fefbe Andy Yan           2009-05-08  308  #define SPI_CTRL_REG_94XX           	0xc800
20b09c2992fefbe Andy Yan           2009-05-08  309  #define SPI_ADDR_REG_94XX            	0xc804
20b09c2992fefbe Andy Yan           2009-05-08  310  #define SPI_WR_DATA_REG_94XX         0xc808
20b09c2992fefbe Andy Yan           2009-05-08  311  #define SPI_RD_DATA_REG_94XX         	0xc80c
20b09c2992fefbe Andy Yan           2009-05-08  312  #define SPI_CTRL_READ_94XX         	(1U << 2)
20b09c2992fefbe Andy Yan           2009-05-08  313  #define SPI_ADDR_VLD_94XX         	(1U << 1)
20b09c2992fefbe Andy Yan           2009-05-08  314  #define SPI_CTRL_SpiStart_94XX     	(1U << 0)
20b09c2992fefbe Andy Yan           2009-05-08  315  
20b09c2992fefbe Andy Yan           2009-05-08  316  static inline int
20b09c2992fefbe Andy Yan           2009-05-08  317  mv_ffc64(u64 v)
20b09c2992fefbe Andy Yan           2009-05-08  318  {
beecadea1b8d67f Xi Wang            2012-11-16  319  	u64 x = ~v;
beecadea1b8d67f Xi Wang            2012-11-16  320  	return x ? __ffs64(x) : -1;
20b09c2992fefbe Andy Yan           2009-05-08  321  }
20b09c2992fefbe Andy Yan           2009-05-08  322  
20b09c2992fefbe Andy Yan           2009-05-08  323  #define r_reg_set_enable(i) \
20b09c2992fefbe Andy Yan           2009-05-08  324  	(((i) > 31) ? mr32(MVS_STP_REG_SET_1) : \
20b09c2992fefbe Andy Yan           2009-05-08  325  	mr32(MVS_STP_REG_SET_0))
20b09c2992fefbe Andy Yan           2009-05-08  326  
20b09c2992fefbe Andy Yan           2009-05-08  327  #define w_reg_set_enable(i, tmp) \
20b09c2992fefbe Andy Yan           2009-05-08  328  	(((i) > 31) ? mw32(MVS_STP_REG_SET_1, tmp) : \
20b09c2992fefbe Andy Yan           2009-05-08  329  	mw32(MVS_STP_REG_SET_0, tmp))
20b09c2992fefbe Andy Yan           2009-05-08  330  
20b09c2992fefbe Andy Yan           2009-05-08  331  extern const struct mvs_dispatch mvs_94xx_dispatch;
20b09c2992fefbe Andy Yan           2009-05-08  332  #endif
20b09c2992fefbe Andy Yan           2009-05-08  333  

:::::: The code at line 278 was first introduced by commit
:::::: c56f5f1de3a6ab8ec985edbc358e1fd8d4e36a65 mvsas: Add SGPIO support to Marvell 94xx

:::::: TO: Wilfried Weissmann <Wilfried.Weissmann@gmx.at>
:::::: CC: Martin K. Petersen <martin.petersen@oracle.com>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 43527 bytes --]

Re: [PATCH v2 1/3] block: introduce LED block device activity trigger

[Akinobu Mita]

2019年7月23日(火) 11:05 kbuild test robot <lkp@intel.com>:
>
> Hi Akinobu,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v5.3-rc1 next-20190722]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Akinobu-Mita/block-introduce-LED-block-device-activity-trigger/20190723-074956
> config: x86_64-fedora-25 (attached as .config)
> compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All error/warnings (new ones prefixed by >>):
>
>    In file included from drivers/block/umem.c:52:0:
> >> drivers/block/umem.h:39:19: error: expected identifier before numeric constant
>     #define  LED_OFF  0x00
>                       ^
> >> include/linux/leds.h:27:2: note: in expansion of macro 'LED_OFF'
>      LED_OFF  = 0,
>      ^~~~~~~

The umem driver defines LED_* macros for MEMCTRLCMD_LEDCTRL register
values. I'm going to prepare a patch that renames these macros with
s/LED_/LEDCTRL_/ in umem.

Re: [PATCH v2 1/3] block: introduce LED block device activity trigger

[Akinobu Mita]

2019年7月23日(火) 11:22 kbuild test robot <lkp@intel.com>:
>
> Hi Akinobu,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [cannot apply to v5.3-rc1 next-20190722]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Akinobu-Mita/block-introduce-LED-block-device-activity-trigger/20190723-074956
> config: x86_64-rhel (attached as .config)
> compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    In file included from drivers/scsi/mvsas/mv_94xx.c:11:0:
> >> drivers/scsi/mvsas/mv_94xx.h:278:2: error: redeclaration of enumerator 'LED_OFF'
>      LED_OFF = 0,
>      ^~~~~~~
>    In file included from include/linux/genhd.h:20:0,
>                     from include/linux/blkdev.h:11,
>                     from include/linux/blk-mq.h:5,
>                     from include/scsi/scsi_host.h:11,
>                     from include/linux/libata.h:21,
>                     from include/scsi/libsas.h:16,
>                     from drivers/scsi/mvsas/mv_sas.h:27,
>                     from drivers/scsi/mvsas/mv_94xx.c:10:
>    include/linux/leds.h:27:2: note: previous definition of 'LED_OFF' was here
>      LED_OFF  = 0,
>      ^~~~~~~
>    In file included from drivers/scsi/mvsas/mv_94xx.c:11:0:
> >> drivers/scsi/mvsas/mv_94xx.h:279:2: error: redeclaration of enumerator 'LED_ON'
>      LED_ON = 1,
>      ^~~~~~
>    In file included from include/linux/genhd.h:20:0,
>                     from include/linux/blkdev.h:11,
>                     from include/linux/blk-mq.h:5,
>                     from include/scsi/scsi_host.h:11,
>                     from include/linux/libata.h:21,
>                     from include/scsi/libsas.h:16,
>                     from drivers/scsi/mvsas/mv_sas.h:27,
>                     from drivers/scsi/mvsas/mv_94xx.c:10:
>    include/linux/leds.h:28:2: note: previous definition of 'LED_ON' was here
>      LED_ON  = 1,
>      ^~~~~~

The mvsas driver declares LED_* enums for enum sgpio_led_status.
I'm going to prepare a patch that adds 'SGPIO_' prefix to these enums.

Re: [PATCH v2 1/3] block: introduce LED block device activity trigger

[Jacek Anaszewski]

Hi Akinobu,

Thank you for the v2. I've checked and it works as expected.

One thing is missing though - ABI documentation.

Please add Documentation/ABI/testing/sysfs-class-led-trigger-blk
and document read, write and discard files.

Best regards,
Jacek Anaszewski

On 7/22/19 4:59 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>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  block/genhd.c                      |   2 +
>  drivers/leds/trigger/Kconfig       |   7 ++
>  drivers/leds/trigger/Makefile      |   1 +
>  drivers/leds/trigger/ledtrig-blk.c | 225 +++++++++++++++++++++++++++++++++++++
>  include/linux/genhd.h              |   3 +
>  include/linux/leds.h               |  27 +++++
>  6 files changed, 265 insertions(+)
>  create mode 100644 drivers/leds/trigger/ledtrig-blk.c
[...]

Re: [PATCH v2 1/3] block: introduce LED block device activity trigger

[Akinobu Mita]

2019年7月27日(土) 6:22 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> Hi Akinobu,
>
> Thank you for the v2. I've checked and it works as expected.
>
> One thing is missing though - ABI documentation.
>
> Please add Documentation/ABI/testing/sysfs-class-led-trigger-blk
> and document read, write and discard files.

OK. I'll add document like below.

What:           /sys/class/leds/<led>/interval
Date:           Aug 2019
KernelVersion:  5.4
Contact:        linux-leds@vger.kernel.org
Description:
                Specifies the duration of the LED blink in milliseconds.
                Defaults to 50 ms.

What:           /sys/class/leds/<led>/read
Date:           Aug 2019
KernelVersion:  5.4
Contact:        linux-leds@vger.kernel.org
Description:
                Signal data read on the block device.
                If set to 0, the LED will not blink on data read.
                If set to 1 (default), the LED will blink for the milliseconds
                specified in interval to signal data read.

What:           /sys/class/leds/<led>/write
Date:           Aug 2019
KernelVersion:  5.4
Contact:        linux-leds@vger.kernel.org
Description:
                Signal data written on the block device.
                If set to 0, the LED will not blink on data written.
                If set to 1 (default), the LED will blink for the milliseconds
                specified in interval to signal data written.

What:           /sys/class/leds/<led>/discard
Date:           Aug 2019
KernelVersion:  5.4
Contact:        linux-leds@vger.kernel.org
Description:
                Signal data discarded on the block device.
                If set to 0, the LED will not blink on data discarded.
                If set to 1 (default), the LED will blink for the milliseconds
                specified in interval to signal data discarded.

Re: [PATCH v2 1/3] block: introduce LED block device activity trigger

[Jacek Anaszewski]

Hi Akinobu,

On 7/28/19 3:51 PM, Akinobu Mita wrote:
> 2019年7月27日(土) 6:22 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>> Hi Akinobu,
>>
>> Thank you for the v2. I've checked and it works as expected.
>>
>> One thing is missing though - ABI documentation.
>>
>> Please add Documentation/ABI/testing/sysfs-class-led-trigger-blk
>> and document read, write and discard files.
> 
> OK. I'll add document like below.
> 
> What:           /sys/class/leds/<led>/interval
> Date:           Aug 2019
> KernelVersion:  5.4
> Contact:        linux-leds@vger.kernel.org
> Description:
>                 Specifies the duration of the LED blink in milliseconds.
>                 Defaults to 50 ms.
> 
> What:           /sys/class/leds/<led>/read
> Date:           Aug 2019
> KernelVersion:  5.4
> Contact:        linux-leds@vger.kernel.org
> Description:
>                 Signal data read on the block device.
>                 If set to 0, the LED will not blink on data read.
>                 If set to 1 (default), the LED will blink for the milliseconds
>                 specified in interval to signal data read.
> 
> What:           /sys/class/leds/<led>/write
> Date:           Aug 2019
> KernelVersion:  5.4
> Contact:        linux-leds@vger.kernel.org
> Description:
>                 Signal data written on the block device.
>                 If set to 0, the LED will not blink on data written.
>                 If set to 1 (default), the LED will blink for the milliseconds
>                 specified in interval to signal data written.
> 
> What:           /sys/class/leds/<led>/discard
> Date:           Aug 2019
> KernelVersion:  5.4
> Contact:        linux-leds@vger.kernel.org
> Description:
>                 Signal data discarded on the block device.
>                 If set to 0, the LED will not blink on data discarded.
>                 If set to 1 (default), the LED will blink for the milliseconds
>                 specified in interval to signal data discarded.
> 

Looks good, please submit it officially.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 0/3] introduce LED block device activity trigger

[Frank Steiner]

Akinobu Mita wrote:

> This work is inspired by the 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.

I've backported/hacked this (together with the "rename LED_OFF and LED_ON"
and some additional patches) to the current SLES 15 kernel (4.12.14)
and can confirm that it works great for my Thinkpad T580 with e.g. the
FnLock LED, as well as for all our office desktops, using block-nvme0n1
as trigger.

cu,
Frank


-- 
Dipl.-Inform. Frank Steiner   Web:  http://www.bio.ifi.lmu.de/~steiner/
Lehrstuhl f. Bioinformatik    Mail: http://www.bio.ifi.lmu.de/~steiner/m/
LMU, Amalienstr. 17           Phone: +49 89 2180-4049
80333 Muenchen, Germany       Fax:   +49 89 2180-99-4049
* Rekursion kann man erst verstehen, wenn man Rekursion verstanden hat. *