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@xxxxxxxxxxxxxx>
> Cc: Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>
> Cc: Pavel Machek <pavel@xxxxxx>
> Cc: Dan Murphy <dmurphy@xxxxxx>
> Cc: Jens Axboe <axboe@xxxxxxxxx>
> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> ---
> 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
