Message ID | 1563807552-23498-2-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | introduce LED block device activity trigger | expand |
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.
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.
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 [...]
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.
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.
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 */
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