[RFC PATCH v2 00/10] Add configurable block device LED triggers
This is a significant rewrite of the patchset that I originally posted back on 28 July. It incorporates almost all of the suggestions that I received as feedback to my original patchset (largely because I was finally able to wrap my head around how complex LED triggers work). One thing that has not changed is that associations between block devices and LEDs are still set via an attribute on the device, rather than the LED. This is much simpler, as the device attribute only has to handle a single value (the name of the associated LED), rather than potentially handling multiple device names. More importantly, it avoids the need to iterate through all of the block devices on the system, searching by name. This was proposed fairly recently, and the reaction was not positive.[1][2] I have modeled the interface for the /sys/block/<DEVICE>/led attribute on the sysfs interface used for selecting a trigger. All available LEDs (all LEDs associated with the blkdev trigger) are shown when the attribute is read, with the currently selected LED enclosed in square brackets ([]). As before, this is all very new to me (particularly the RCU stuff), so I welcome feedback. Thanks! Changes from the original patchset: * Use a single complex LED trigger ("blkdev"), rather than multiple user-defined triggers. * Configurable blink time and interval for each LED: /sys/class/leds/<LED>/blink_{on,off} * Associated block devices linked from LED subdirectory: /sys/class/leds/<LED>/block_devices (Avoids violating the "one value per entry" sysfs rule.) * Device-LED associations set via /sys/block/<DEVICE>/led * Document all sysfs attributes in Documentation/ABI/testing (but also kept the overview doc) * Removed unnecessary [un]likely macros * Reduced number of "user error" log messages and changed level to INFO * More detailed commit messages * Add Kconfig option in final commit * Use RCU-protected pointer (rather than mutex) * No in-kernel APIs for now [1] https://www.spinics.net/lists/linux-leds/msg18256.html [2] https://www.spinics.net/lists/linux-leds/msg18261.html Ian Pilcher (10): docs: Add block device LED trigger documentation block: Add file (blk-ledtrig.c) for block device LED trigger implementation block: Add block device LED trigger fields to gendisk structure block: Add functions to set & clear block device LEDs block: Add block device sysfs attribute to set/clear/show LED block: Add activate and deactivate functions for block device LED trigger block: Add sysfs attributes to LEDs associated with blkdev trigger block: Add init function for block device LED trigger block: Blink device LED (if any) when request is sent to its driver block: Add config option to enable block device LED triggers Documentation/ABI/testing/sysfs-block | 16 + .../testing/sysfs-class-led-trigger-blkdev | 28 ++ Documentation/block/blk-ledtrig.rst | 79 +++ Documentation/block/index.rst | 1 + block/Kconfig | 8 + block/Makefile | 1 + block/blk-ledtrig.c | 469 ++++++++++++++++++ block/blk-ledtrig.h | 45 ++ block/blk-mq.c | 2 + block/genhd.c | 11 + include/linux/genhd.h | 4 + 11 files changed, 664 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev create mode 100644 Documentation/block/blk-ledtrig.rst create mode 100644 block/blk-ledtrig.c create mode 100644 block/blk-ledtrig.h -- 2.31.1
Add header (blk-ledtrig.h) for LED trigger-related declarations Add (inline) function to ensure trigger pointer is initialized (to NULL) and call it from __device_add_disk() Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.h | 25 +++++++++++++++++++++++++ block/genhd.c | 2 ++ include/linux/genhd.h | 4 ++++ 3 files changed, 31 insertions(+) create mode 100644 block/blk-ledtrig.h diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h new file mode 100644 index 000000000000..95a79d2fe447 --- /dev/null +++ b/block/blk-ledtrig.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* + * Block device LED triggers + * + * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> + */ + +#ifndef _BLOCK_BLK_LEDTRIG_H +#define _BLOCK_BLK_LEDTRIG_H + +#ifdef CONFIG_BLK_LED_TRIGGERS + +static inline void blk_ledtrig_disk_init(struct gendisk *const disk) +{ + RCU_INIT_POINTER(disk->led, NULL); +} + +#else // CONFIG_BLK_LED_TRIGGERS + +static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {} + +#endif // CONFIG_BLK_LED_TRIGGERS + +#endif // _BLOCK_BLK_LEDTRIG_H diff --git a/block/genhd.c b/block/genhd.c index 298ee78c1bda..b168172e664b 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -26,6 +26,7 @@ #include <linux/badblocks.h> #include "blk.h" +#include "blk-ledtrig.h" static struct kobject *block_depr; @@ -539,6 +540,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, disk_add_events(disk); blk_integrity_add(disk); + blk_ledtrig_disk_init(disk); } void device_add_disk(struct device *parent, struct gendisk *disk, diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 13b34177cc85..efcb1ca62f17 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -168,6 +168,10 @@ struct gendisk { #endif /* CONFIG_BLK_DEV_INTEGRITY */ #if IS_ENABLED(CONFIG_CDROM) struct cdrom_device_info *cdi; +#endif +#ifdef CONFIG_BLK_LED_TRIGGERS + struct blk_ledtrig_led __rcu *led; + struct list_head led_dev_list_node; #endif int node_id; struct badblocks *bb; -- 2.31.1
Create a symlink from /sys/class/leds/<LED>/block_devices to each block device that is associated with that LED Ensure device LED is cleared when device is removed Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ block/blk-ledtrig.h | 3 ++ block/genhd.c | 1 + 3 files changed, 97 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index c5ad57ed9c3b..280fa9edc2dd 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -6,9 +6,13 @@ * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> */ +#include <linux/genhd.h> #include <linux/leds.h> #include <linux/mutex.h> +#include "blk-ledtrig.h" + + /* * * Trigger mutex and LED list @@ -46,3 +50,92 @@ static struct blk_ledtrig_led *blk_ledtrig_find(const char *const led_name, return NULL; } + + +/* + * + * Clear a block device's LED + * + */ + +// Also called from blk_ledtrig_dev_set() +static void blk_ledtrig_dev_cleanup(struct gendisk *const disk, + struct blk_ledtrig_led *const old_led) +{ + sysfs_remove_link(old_led->dir, disk->disk_name); + list_del(&disk->led_dev_list_node); +} + +// Also called from blk_ledtrig_deactivate() +static void blk_ledtrig_dev_clear_locked(struct gendisk *const disk, + struct blk_ledtrig_led *const old_led) +{ + RCU_INIT_POINTER(disk->led, NULL); + if (old_led != NULL) + blk_ledtrig_dev_cleanup(disk, old_led); +} + +// Also called from genhd.c:del_gendisk() +void blk_ledtrig_dev_clear(struct gendisk *const disk) +{ + struct blk_ledtrig_led *old_led; + + mutex_lock(&blk_ledtrig_mutex); + old_led = rcu_dereference_protected(disk->led, + lockdep_is_held(&blk_ledtrig_mutex)); + blk_ledtrig_dev_clear_locked(disk, old_led); + mutex_unlock(&blk_ledtrig_mutex); +} + + +/* + * + * Set a block device's LED + * + */ + +static int blk_ledtrig_dev_set(struct gendisk *const disk, + const char *const led_name, + const size_t name_len) +{ + struct blk_ledtrig_led *new_led, *old_led; + int ret; + + ret = mutex_lock_interruptible(&blk_ledtrig_mutex); + if (ret != 0) + goto led_set_exit_return; + + new_led = blk_ledtrig_find(led_name, name_len); + if (new_led == NULL) { + pr_info("no LED named %.*s associated with blkdev trigger\n", + (int)name_len, led_name); + ret = -ENODEV; + goto led_set_exit_unlock; + } + + old_led = rcu_dereference_protected(disk->led, + lockdep_is_held(&blk_ledtrig_mutex)); + + if (old_led == new_led) { + ret = 0; + goto led_set_exit_unlock; + } + + ret = sysfs_create_link(new_led->dir, &disk_to_dev(disk)->kobj, + disk->disk_name); + if (ret != 0) + goto led_set_exit_unlock; + + if (old_led != NULL) + blk_ledtrig_dev_cleanup(disk, old_led); + + rcu_assign_pointer(disk->led, new_led); + list_add(&disk->led_dev_list_node, &new_led->dev_list); + + ret = 0; + +led_set_exit_unlock: + mutex_unlock(&blk_ledtrig_mutex); +led_set_exit_return: + return ret; +} diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h index 95a79d2fe447..66a1302a4174 100644 --- a/block/blk-ledtrig.h +++ b/block/blk-ledtrig.h @@ -16,9 +16,12 @@ static inline void blk_ledtrig_disk_init(struct gendisk *const disk) RCU_INIT_POINTER(disk->led, NULL); } +void blk_ledtrig_dev_clear(struct gendisk *const disk); + #else // CONFIG_BLK_LED_TRIGGERS static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {} +static inline void blk_ledtrig_dev_clear(const struct gendisk *disk) {} #endif // CONFIG_BLK_LED_TRIGGERS diff --git a/block/genhd.c b/block/genhd.c index b168172e664b..9fa734aeab0f 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -583,6 +583,7 @@ void del_gendisk(struct gendisk *disk) if (WARN_ON_ONCE(!disk->queue)) return; + blk_ledtrig_dev_clear(disk); blk_integrity_del(disk); disk_del_events(disk); -- 2.31.1
Add show & store functions in blk-ledtrig.c (attributes defined in genhd.c) Show function shows all available LEDs (LEDs associated with blkdev trigger); currently associated LED is shown in square brackets ([]) Store function accepts either all whitespace or "none" to clear LED Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 109 ++++++++++++++++++++++++++++++++++++++++++++ block/blk-ledtrig.h | 8 ++++ block/genhd.c | 8 ++++ 3 files changed, 125 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 280fa9edc2dd..1af94dc7ea51 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -6,6 +6,7 @@ * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> */ +#include <linux/ctype.h> #include <linux/genhd.h> #include <linux/leds.h> #include <linux/mutex.h> @@ -139,3 +140,111 @@ static int blk_ledtrig_dev_set(struct gendisk *const disk, led_set_exit_return: return ret; } + + +/* + * + * sysfs attribute store function to set or clear device LED + * + */ + +// Returns a pointer to the first non-whitespace character in s (or a pointer +// to the terminating null). +static const char *blk_ledtrig_skip_whitespace(const char *s) +{ + while (*s != 0 && isspace(*s)) + ++s; + + return s; +} + +// Returns a pointer to the first whitespace character in s (or a pointer to +// the terminating null), which is effectively a pointer to the position *after* +// the last character in the non-whitespace token at the beginning of s. (s is +// expected to be the result of a previous call to blk_ledtrig_skip_whitespace.) +static const char *blk_ledtrig_find_whitespace(const char *s) +{ + while (*s != 0 && !isspace(*s)) + ++s; + + return s; +} + +static bool blk_ledtrig_name_is_none(const char *const name, const size_t len) +{ + static const char none[4] = "none"; // no terminating null + + return len == sizeof(none) && memcmp(name, none, sizeof(none)) == 0; +} + +ssize_t blk_ledtrig_dev_led_store(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, const size_t count) +{ + struct gendisk *const disk = dev_to_disk(dev); + const char *const led_name = blk_ledtrig_skip_whitespace(buf); + const char *const endp = blk_ledtrig_find_whitespace(led_name); + const ptrdiff_t name_len = endp - led_name; // always >= 0 + int ret; + + if (name_len == 0 || blk_ledtrig_name_is_none(led_name, name_len)) { + blk_ledtrig_dev_clear(disk); + ret = 0; + } else { + ret = blk_ledtrig_dev_set(disk, led_name, name_len); + } + + if (ret < 0) + return ret; + + return count; +} + + +/* + * + * sysfs attribute show function for device LED + * + */ + +ssize_t blk_ledtrig_dev_led_show(struct device *const dev, + struct device_attribute *const attr, + char *const buf) +{ + struct gendisk *const disk = dev_to_disk(dev); + struct blk_ledtrig_led *bd_led, *disk_led; + int ret, c = 0; + + ret = mutex_lock_interruptible(&blk_ledtrig_mutex); + if (ret != 0) + goto led_show_exit_return; + + disk_led = rcu_dereference_protected(disk->led, + lockdep_is_held(&blk_ledtrig_mutex)); + + if (disk_led == NULL) + c += sprintf(buf, "[none]"); + else + c += sprintf(buf, "none"); + + list_for_each_entry(bd_led, &blk_ledtrig_leds, leds_list_node) { + + ret = snprintf(buf + c, PAGE_SIZE - c - 1, + bd_led == disk_led ? " [%s]" : " %s", + bd_led->led->name); + if (ret >= PAGE_SIZE - c - 1) { + ret = -EOVERFLOW; + goto led_show_exit_unlock; + } + + c += ret; + } + + buf[c] = '\n'; + ret = c + 1; + +led_show_exit_unlock: + mutex_unlock(&blk_ledtrig_mutex); +led_show_exit_return: + return ret; +} diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h index 66a1302a4174..771000d43647 100644 --- a/block/blk-ledtrig.h +++ b/block/blk-ledtrig.h @@ -18,6 +18,14 @@ static inline void blk_ledtrig_disk_init(struct gendisk *const disk) void blk_ledtrig_dev_clear(struct gendisk *const disk); +ssize_t blk_ledtrig_dev_led_store(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, const size_t count); + +ssize_t blk_ledtrig_dev_led_show(struct device *const dev, + struct device_attribute *const attr, + char *const buf); + #else // CONFIG_BLK_LED_TRIGGERS static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {} diff --git a/block/genhd.c b/block/genhd.c index 9fa734aeab0f..d5413a633410 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1012,6 +1012,11 @@ static struct device_attribute dev_attr_fail_timeout = __ATTR(io-timeout-fail, 0644, part_timeout_show, part_timeout_store); #endif +#ifdef CONFIG_BLK_LED_TRIGGERS +static struct device_attribute dev_attr_led = + __ATTR(led, 0644, blk_ledtrig_dev_led_show, blk_ledtrig_dev_led_store); +#endif + static struct attribute *disk_attrs[] = { &dev_attr_range.attr, &dev_attr_ext_range.attr, @@ -1033,6 +1038,9 @@ static struct attribute *disk_attrs[] = { #endif #ifdef CONFIG_FAIL_IO_TIMEOUT &dev_attr_fail_timeout.attr, +#endif +#ifdef CONFIG_BLK_LED_TRIGGERS + &dev_attr_led.attr, #endif NULL }; -- 2.31.1
ÔÚ 2021/8/9 ÉÏÎç11:32, Ian Pilcher ÐŽµÀ:
Add show & store functions in blk-ledtrig.c (attributes defined in genhd.c)
Show function shows all available LEDs (LEDs associated with blkdev trigger); currently associated LED is shown in square brackets ([])
Store function accepts either all whitespace or "none" to clear LED
Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 109 ++++++++++++++++++++++++++++++++++++++++++++ block/blk-ledtrig.h | 8 ++++ block/genhd.c | 8 ++++ 3 files changed, 125 insertions(+)
diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 280fa9edc2dd..1af94dc7ea51 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -6,6 +6,7 @@ * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> */
+#include <linux/ctype.h> #include <linux/genhd.h> #include <linux/leds.h> #include <linux/mutex.h> @@ -139,3 +140,111 @@ static int blk_ledtrig_dev_set(struct gendisk *const disk, led_set_exit_return: return ret; } + + +/* + * + * sysfs attribute store function to set or clear device LED + * + */ + +// Returns a pointer to the first non-whitespace character in s (or a pointer +// to the terminating null). +static const char *blk_ledtrig_skip_whitespace(const char *s) +{ + while (*s != 0 && isspace(*s)) + ++s; + + return s; +} + +// Returns a pointer to the first whitespace character in s (or a pointer to +// the terminating null), which is effectively a pointer to the position *after* +// the last character in the non-whitespace token at the beginning of s. (s is +// expected to be the result of a previous call to blk_ledtrig_skip_whitespace.)
These are not linux kernel style comments.
+static const char *blk_ledtrig_find_whitespace(const char *s) +{ + while (*s != 0 && !isspace(*s)) + ++s; + + return s; +} + +static bool blk_ledtrig_name_is_none(const char *const name, const size_t len) +{ + static const char none[4] = "none"; // no terminating null + + return len == sizeof(none) && memcmp(name, none, sizeof(none)) == 0; +} + +ssize_t blk_ledtrig_dev_led_store(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, const size_t count) +{ + struct gendisk *const disk = dev_to_disk(dev); + const char *const led_name = blk_ledtrig_skip_whitespace(buf); + const char *const endp = blk_ledtrig_find_whitespace(led_name); + const ptrdiff_t name_len = endp - led_name; // always >= 0 + int ret; + + if (name_len == 0 || blk_ledtrig_name_is_none(led_name, name_len)) { + blk_ledtrig_dev_clear(disk); + ret = 0; + } else { + ret = blk_ledtrig_dev_set(disk, led_name, name_len); + } + + if (ret < 0) + return ret; + + return count; +} + + +/* + * + * sysfs attribute show function for device LED + * + */ + +ssize_t blk_ledtrig_dev_led_show(struct device *const dev, + struct device_attribute *const attr, + char *const buf) +{ + struct gendisk *const disk = dev_to_disk(dev); + struct blk_ledtrig_led *bd_led, *disk_led; + int ret, c = 0; + + ret = mutex_lock_interruptible(&blk_ledtrig_mutex); + if (ret != 0) + goto led_show_exit_return; + + disk_led = rcu_dereference_protected(disk->led, + lockdep_is_held(&blk_ledtrig_mutex)); + + if (disk_led == NULL) + c += sprintf(buf, "[none]"); + else + c += sprintf(buf, "none"); + + list_for_each_entry(bd_led, &blk_ledtrig_leds, leds_list_node) { + + ret = snprintf(buf + c, PAGE_SIZE - c - 1, + bd_led == disk_led ? " [%s]" : " %s", + bd_led->led->name); + if (ret >= PAGE_SIZE - c - 1) { + ret = -EOVERFLOW; + goto led_show_exit_unlock; + } + + c += ret; + } + + buf[c] = '\n'; + ret = c + 1; + +led_show_exit_unlock: + mutex_unlock(&blk_ledtrig_mutex); +led_show_exit_return: + return ret; +} diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h index 66a1302a4174..771000d43647 100644 --- a/block/blk-ledtrig.h +++ b/block/blk-ledtrig.h @@ -18,6 +18,14 @@ static inline void blk_ledtrig_disk_init(struct gendisk *const disk)
void blk_ledtrig_dev_clear(struct gendisk *const disk);
+ssize_t blk_ledtrig_dev_led_store(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, const size_t count); + +ssize_t blk_ledtrig_dev_led_show(struct device *const dev, + struct device_attribute *const attr, + char *const buf); + #else // CONFIG_BLK_LED_TRIGGERS
static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {} diff --git a/block/genhd.c b/block/genhd.c index 9fa734aeab0f..d5413a633410 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1012,6 +1012,11 @@ static struct device_attribute dev_attr_fail_timeout = __ATTR(io-timeout-fail, 0644, part_timeout_show, part_timeout_store); #endif
+#ifdef CONFIG_BLK_LED_TRIGGERS +static struct device_attribute dev_attr_led = + __ATTR(led, 0644, blk_ledtrig_dev_led_show, blk_ledtrig_dev_led_store); +#endif + static struct attribute *disk_attrs[] = { &dev_attr_range.attr, &dev_attr_ext_range.attr, @@ -1033,6 +1038,9 @@ static struct attribute *disk_attrs[] = { #endif #ifdef CONFIG_FAIL_IO_TIMEOUT &dev_attr_fail_timeout.attr, +#endif +#ifdef CONFIG_BLK_LED_TRIGGERS + &dev_attr_led.attr, #endif NULL };
Content-type: Text/plain No virus found Checked by Hillstone Network AntiVirus
Set up activated LED: * Allocate per-LED data structure * Add LED to list of LEDs associated with blkdev trigger * Create block_devices subdirectory for links to associated devices Clean up deactivated LED: * Clear LED of any associated devices * Remove from LED from blkdev trigger list * Remove block_devices subdirectory * Wait for any blinks using LED to complete (synchronize_rcu()) * Free per-LED structure Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 95 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 1af94dc7ea51..f8cb6de203f8 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -13,6 +13,11 @@ #include "blk-ledtrig.h" +// Default blink_on & blink_off values +#define BLK_LEDTRIG_BLINK_ON 75 +#define BLK_LEDTRIG_BLINK_OFF 25 +#define BLK_LEDTRIG_BLINK_MAX 10000 // 10 seconds + /* * @@ -248,3 +253,93 @@ ssize_t blk_ledtrig_dev_led_show(struct device *const dev, led_show_exit_return: return ret; } + + +/* + * + * Associate an LED with the blkdev trigger + * + */ + +// Helper function to create <LED>/blkdevs subdirectory - doesn't +// swallow error code like kobject_create_and_add() +static int blk_ledtrig_subdir_create(struct blk_ledtrig_led *const bd_led, + struct led_classdev *const led) +{ + int ret; + + bd_led->dir = kobject_create(); + if (bd_led->dir == NULL) + return -ENOMEM; + + ret = kobject_add(bd_led->dir, &led->dev->kobj, "block_devices"); + if (ret != 0) + kobject_put(bd_led->dir); + + return ret; +} + +static int blk_ledtrig_activate(struct led_classdev *const led) +{ + struct blk_ledtrig_led *bd_led; + int ret; + + bd_led = kmalloc(sizeof(*bd_led), GFP_KERNEL); + if (bd_led == NULL) { + ret = -ENOMEM; + goto activate_exit_return; + } + + bd_led->led = led; + bd_led->blink_on = BLK_LEDTRIG_BLINK_ON; + bd_led->blink_off = BLK_LEDTRIG_BLINK_OFF; + INIT_LIST_HEAD(&bd_led->dev_list); + + ret = mutex_lock_interruptible(&blk_ledtrig_mutex); + if (ret != 0) + goto activate_exit_free; + + ret = blk_ledtrig_subdir_create(bd_led, led); + if (ret != 0) + goto activate_exit_unlock; + + list_add(&bd_led->leds_list_node, &blk_ledtrig_leds); + led_set_trigger_data(led, bd_led); + ret = 0; + +activate_exit_unlock: + mutex_unlock(&blk_ledtrig_mutex); +activate_exit_free: + if (ret != 0) + kfree(bd_led); +activate_exit_return: + return ret; +} + + +/* + * + * Disassociate an LED from the blkdev trigger + * + */ + +static void blk_ledtrig_deactivate(struct led_classdev *const led) +{ + struct blk_ledtrig_led *const bd_led = led_get_trigger_data(led); + struct gendisk *disk, *next; + + mutex_lock(&blk_ledtrig_mutex); + + list_for_each_entry_safe(disk, next, + &bd_led->dev_list, led_dev_list_node) { + + blk_ledtrig_dev_clear_locked(disk, bd_led); + } + + list_del(&bd_led->leds_list_node); + kobject_put(bd_led->dir); + + mutex_unlock(&blk_ledtrig_mutex); + synchronize_rcu(); + kfree(bd_led); +} -- 2.31.1
Add blink_on & blink_off attributes to control the duration of each LED blink (blink_on) and the minimum time between blinks (blink_off) in milliseconds Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index f8cb6de203f8..d02f32205985 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -343,3 +343,66 @@ static void blk_ledtrig_deactivate(struct led_classdev *const led) synchronize_rcu(); kfree(bd_led); } + + +/* + * + * Per-LED blink_on & blink_off device attributes + * + */ + +static ssize_t blk_ledtrig_blink_show(struct device *const dev, + struct device_attribute *const attr, + char *const buf); + +static ssize_t blk_ledtrig_blink_store(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, + const size_t count); + +static struct device_attribute blk_ledtrig_attr_blink_on = + __ATTR(blink_on, 0644, + blk_ledtrig_blink_show, blk_ledtrig_blink_store); + +static struct device_attribute blk_ledtrig_attr_blink_off = + __ATTR(blink_off, 0644, + blk_ledtrig_blink_show, blk_ledtrig_blink_store); + +static ssize_t blk_ledtrig_blink_show(struct device *const dev, + struct device_attribute *const attr, + char *const buf) +{ + struct blk_ledtrig_led *const bd_led = led_trigger_get_drvdata(dev); + unsigned int value; + + if (attr == &blk_ledtrig_attr_blink_on) + value = READ_ONCE(bd_led->blink_on); + else // attr == &blk_ledtrig_attr_blink_off + value = READ_ONCE(bd_led->blink_off); + + return sprintf(buf, "%u\n", value); +} + +static ssize_t blk_ledtrig_blink_store(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, + const size_t count) +{ + struct blk_ledtrig_led *const bd_led = led_trigger_get_drvdata(dev); + unsigned int value; + int ret; + + ret = kstrtouint(buf, 0, &value); + if (ret != 0) + return ret; + + if (value > BLK_LEDTRIG_BLINK_MAX) + return -ERANGE; + + if (attr == &blk_ledtrig_attr_blink_on) + WRITE_ONCE(bd_led->blink_on, value); + else // attr == &blk_ledtrig_attr_blink_off + WRITE_ONCE(bd_led->blink_off, value); + + return count; +} -- 2.31.1
Register the blkdev LED trigger Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index d02f32205985..14b1d33a2953 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -406,3 +406,38 @@ static ssize_t blk_ledtrig_blink_store(struct device *const dev, return count; } + + +/* + * + * Initialization - register the trigger + * + */ + +static struct attribute *blk_ledtrig_attrs[] = { + &blk_ledtrig_attr_blink_on.attr, + &blk_ledtrig_attr_blink_off.attr, + NULL +}; + +static const struct attribute_group blk_ledtrig_attr_group = { + .attrs = blk_ledtrig_attrs, +}; + +static const struct attribute_group *blk_ledtrig_attr_groups[] = { + &blk_ledtrig_attr_group, + NULL +}; + +static struct led_trigger blk_ledtrig_trigger = { + .name = "blkdev", + .activate = blk_ledtrig_activate, + .deactivate = blk_ledtrig_deactivate, + .groups = blk_ledtrig_attr_groups, +}; + +static int __init blk_ledtrig_init(void) +{ + return led_trigger_register(&blk_ledtrig_trigger); +} +device_initcall(blk_ledtrig_init); -- 2.31.1
Call led_blink_set_oneshot() in RCU critical section to ensure that LED doesn't disappear. (See synchronize_rcu() call in blk_ledtrig_deactivate()). Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 26 ++++++++++++++++++++++++++ block/blk-ledtrig.h | 9 +++++++++ block/blk-mq.c | 2 ++ 3 files changed, 37 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 14b1d33a2953..10588f3dea15 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -6,6 +6,7 @@ * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> */ +#include <linux/blkdev.h> #include <linux/ctype.h> #include <linux/genhd.h> #include <linux/leds.h> @@ -441,3 +442,28 @@ static int __init blk_ledtrig_init(void) return led_trigger_register(&blk_ledtrig_trigger); } device_initcall(blk_ledtrig_init); + + +/* + * + * Blink the LED associated with a (non-NULL) disk (if set) + * + */ + +void __blk_ledtrig_try_blink(struct request *const rq) +{ + struct blk_ledtrig_led *bd_led; + unsigned long delay_on, delay_off; + + rcu_read_lock(); + + bd_led = rcu_dereference(rq->rq_disk->led); + + if (bd_led != NULL) { + delay_on = READ_ONCE(bd_led->blink_on); + delay_off = READ_ONCE(bd_led->blink_off); + led_blink_set_oneshot(bd_led->led, &delay_on, &delay_off, 0); + } + + rcu_read_unlock(); +} diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h index 771000d43647..7eb1d88e2b3c 100644 --- a/block/blk-ledtrig.h +++ b/block/blk-ledtrig.h @@ -26,10 +26,19 @@ ssize_t blk_ledtrig_dev_led_show(struct device *const dev, struct device_attribute *const attr, char *const buf); +void __blk_ledtrig_try_blink(struct request *const rq); + +static inline void blk_ledtrig_try_blink(struct request *const rq) +{ + if (rq->rq_disk != NULL) + __blk_ledtrig_try_blink(rq); +} + #else // CONFIG_BLK_LED_TRIGGERS static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {} static inline void blk_ledtrig_dev_clear(const struct gendisk *disk) {} +static inline void blk_ledtrig_try_blink(const struct request *rq) {} #endif // CONFIG_BLK_LED_TRIGGERS diff --git a/block/blk-mq.c b/block/blk-mq.c index 2c4ac51e54eb..7732e31f3ca8 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -40,6 +40,7 @@ #include "blk-stat.h" #include "blk-mq-sched.h" #include "blk-rq-qos.h" +#include "blk-ledtrig.h" static DEFINE_PER_CPU(struct llist_head, blk_cpu_done); @@ -1381,6 +1382,7 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list, switch (ret) { case BLK_STS_OK: queued++; + blk_ledtrig_try_blink(rq); break; case BLK_STS_RESOURCE: case BLK_STS_DEV_RESOURCE: -- 2.31.1
Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/Kconfig | 8 ++++++++ block/Makefile | 1 + 2 files changed, 9 insertions(+) diff --git a/block/Kconfig b/block/Kconfig index fd732aede922..ef05b36b5251 100644 --- a/block/Kconfig +++ b/block/Kconfig @@ -220,6 +220,14 @@ config BLK_INLINE_ENCRYPTION_FALLBACK by falling back to the kernel crypto API when inline encryption hardware is not present. +config BLK_LED_TRIGGERS + bool "Enable the blkdev LED trigger" + depends on LEDS_TRIGGERS + help + The blkdev LED trigger allows LEDs to be controlled by + block device activity (reads and writes). See + Documentation/block/blk-ledtrig.rst. + menu "Partition Types" source "block/partitions/Kconfig" diff --git a/block/Makefile b/block/Makefile index bfbe4e13ca1e..bcd97ee26462 100644 --- a/block/Makefile +++ b/block/Makefile @@ -42,3 +42,4 @@ obj-$(CONFIG_BLK_SED_OPAL) += sed-opal.o obj-$(CONFIG_BLK_PM) += blk-pm.o obj-$(CONFIG_BLK_INLINE_ENCRYPTION) += keyslot-manager.o blk-crypto.o obj-$(CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK) += blk-crypto-fallback.o +obj-$(CONFIG_BLK_LED_TRIGGERS) += blk-ledtrig.o -- 2.31.1
Hello Ian, thank you for your proposal. Some comments below: On Sun, 8 Aug 2021 22:32:07 -0500 Ian Pilcher <arequipeno@gmail.com> wrote:
One thing that has not changed is that associations between block devices and LEDs are still set via an attribute on the device, rather than the LED. This is much simpler, as the device attribute only has to handle a single value (the name of the associated LED), rather than potentially handling multiple device names.
It may be simpler, but it is in contrast to how the netdev trigger works, which already is in upstream for many years. I really think we should try to have similar sysfs ABIs here. (I understand that the netdev trigger is currently unable to handle multiple network interfaces - but it is possible to extend it so.)
I have modeled the interface for the /sys/block/<DEVICE>/led attribute on the sysfs interface used for selecting a trigger. All available LEDs (all LEDs associated with the blkdev trigger) are shown when the attribute is read, with the currently selected LED enclosed in square brackets ([]).
I think it is reasonable to be able to set something like this: led0 : blink on activity on any of [sda, sdb, sdc] led1 : blink on activity on sda led2 : blink on activity on sdb led3 : blink on activity on sdc If I am reading your code correctly, it looks that only one LED can be configured for a block device. Is this true? If so, then the above configuration cannot be set. Also you are blinking the LED on any request to the block device. I would rather expect to be able to set the LED to blink on read and on write. (And possibly on other functions, like discard, or critical temperature, or error, ...) I would like to know what other people think about this. Marek
On Monday 09 August 2021 20:56:33 Marek Behún wrote:
Hello Ian,
thank you for your proposal. Some comments below:
On Sun, 8 Aug 2021 22:32:07 -0500 Ian Pilcher <arequipeno@gmail.com> wrote:
One thing that has not changed is that associations between block devices and LEDs are still set via an attribute on the device, rather than the LED. This is much simpler, as the device attribute only has to handle a single value (the name of the associated LED), rather than potentially handling multiple device names.
It may be simpler, but it is in contrast to how the netdev trigger works, which already is in upstream for many years. I really think we should try to have similar sysfs ABIs here. (I understand that the netdev trigger is currently unable to handle multiple network interfaces - but it is possible to extend it so.)
I have modeled the interface for the /sys/block/<DEVICE>/led attribute on the sysfs interface used for selecting a trigger. All available LEDs (all LEDs associated with the blkdev trigger) are shown when the attribute is read, with the currently selected LED enclosed in square brackets ([]).
I think it is reasonable to be able to set something like this: led0 : blink on activity on any of [sda, sdb, sdc] led1 : blink on activity on sda led2 : blink on activity on sdb led3 : blink on activity on sdc
If I am reading your code correctly, it looks that only one LED can be configured for a block device. Is this true? If so, then the above configuration cannot be set.
Also you are blinking the LED on any request to the block device. I would rather expect to be able to set the LED to blink on read and on write. (And possibly on other functions, like discard, or critical temperature, or error, ...) I would like to know what other people think about this.
Hello! HP EliteBook laptops had dedicated LED for some kind of error and encryption indication. And there is kernel acpi/wmi driver which can control this LED. I do not know if recent HP laptops still have these LEDs, but I would suggest to design API in a way that would allow to use these dedicated LEDs for their original "vendor" purpose. I'm mentioning it just because this functionality and design is already on existing production mainstream laptops, and not something imaginary. If Linux distributions are still cooperating with laptop vendors and doing "official" Linux preloads then they may be interested in having "native" LED functionality support in kernel.
Marek - Thanks for taking the time to look at this. On 8/9/21 1:56 PM, Marek Behún wrote:
It may be simpler, but it is in contrast to how the netdev trigger works, which already is in upstream for many years. I really think we should try to have similar sysfs ABIs here. (I understand that the netdev trigger is currently unable to handle multiple network interfaces - but it is possible to extend it so.)
I'm not unalterably opposed to the idea, but I don't currently see a way to do that without resolving block devices (struct gendisk) by name, and that seems to be a no-no. If you (or anyone else) has a suggestion on how to get around this obstacle, I'd be willing to give it a shot.
I think it is reasonable to be able to set something like this: led0 : blink on activity on any of [sda, sdb, sdc] led1 : blink on activity on sda led2 : blink on activity on sdb led3 : blink on activity on sdc
If I am reading your code correctly, it looks that only one LED can be configured for a block device. Is this true? If so, then the above configuration cannot be set.
You're correct that it's not possible with the current code. Multiple devices can be associated to with a single LED, but there's not currently a way to drive more than 1 LED from a single device. This is something that could be changed.
Also you are blinking the LED on any request to the block device. I would rather expect to be able to set the LED to blink on read and on write. (And possibly on other functions, like discard, or critical temperature, or error, ...) I would like to know what other people think about this.
I wanted to keep things as simple as possible for now. I don't think that there's any particular reason that separated LEDs couldn't be configured for read and write requests. (It looks like it should be pretty easy to distinguish reads vs writes in a struct request.) My feeling is that things like temperature, errors, etc. are better monitored from user space, as they tend to require actively querying the drive. Like you, I'm interested in knowing if there is actually hardware out there that has separate read/write LEDs. All in all, I feel like I should be able to implement almost everything that you've suggested, *if* I can figure out the block device lookup issue, but I really don't have any ideas on that. Thanks for your patience and feedback! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Mon, 9 Aug 2021 14:54:26 -0500 Ian Pilcher <arequipeno@gmail.com> wrote:
I'm not unalterably opposed to the idea, but I don't currently see a way to do that without resolving block devices (struct gendisk) by name, and that seems to be a no-no.
So a name like "sda1" is not viable? Why? What about "MAJOR:MINOR"? I confess that I am not very familiar with internal blkdev API. Quick look reveals that there is a struct block_device, containing a member bd_disk, which is a pointer to struct gendisk. What is the relationship between these two? Can there be a block device without a gendisk, for example? Marek
On 8/9/21 5:43 PM, Marek Behún wrote:
I confess that I am not very familiar with internal blkdev API.
It's mainly a matter of symbol visibility. See this thread from a few months ago: https://www.spinics.net/lists/linux-leds/msg18244.html Now ... my code currently lives in block/, so there isn't actually anything technically preventing it from iterating through the block devices. The reactions to Enzo's patch (which you can see in that thread) make me think that anything that iterates through all block devices is likely to be rejected, but maybe I'm reading too much into it. Greg / Christoph - (As you were the people who expressed disapproval of Enzo's patch to export block_class and disk_type ...) Can you weigh in on the acceptability of iterating through the block devices (searching by name) from LED trigger code within the block subsystem (i.e. no new symbols would need to be exported)? This would allow the trigger to implement the sysfs API that Marek and Pavel want. Thanks! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
On 8/9/21 5:43 PM, Marek Behún wrote:
I confess that I am not very familiar with internal blkdev API.
It's mainly a matter of symbol visibility. See this thread from a few months ago:
https://www.spinics.net/lists/linux-leds/msg18244.html
Now ... my code currently lives in block/, so there isn't actually anything technically preventing it from iterating through the block devices.
The reactions to Enzo's patch (which you can see in that thread) make me think that anything that iterates through all block devices is likely to be rejected, but maybe I'm reading too much into it.
Greg / Christoph -
(As you were the people who expressed disapproval of Enzo's patch to export block_class and disk_type ...)
Can you weigh in on the acceptability of iterating through the block devices (searching by name) from LED trigger code within the block subsystem (i.e. no new symbols would need to be exported)?
This would allow the trigger to implement the sysfs API that Marek and Pavel want.
No idea, let's see the change first, we can never promise anything :) thanks, greg k-h
On Tue, 10 Aug 2021 08:35:08 +0200 Greg KH <gregkh@linuxfoundation.org> wrote:
On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
On 8/9/21 5:43 PM, Marek Behún wrote:
I confess that I am not very familiar with internal blkdev API.
It's mainly a matter of symbol visibility. See this thread from a few months ago:
https://www.spinics.net/lists/linux-leds/msg18244.html
Now ... my code currently lives in block/, so there isn't actually anything technically preventing it from iterating through the block devices.
The reactions to Enzo's patch (which you can see in that thread) make me think that anything that iterates through all block devices is likely to be rejected, but maybe I'm reading too much into it.
Greg / Christoph -
(As you were the people who expressed disapproval of Enzo's patch to export block_class and disk_type ...)
Can you weigh in on the acceptability of iterating through the block devices (searching by name) from LED trigger code within the block subsystem (i.e. no new symbols would need to be exported)?
This would allow the trigger to implement the sysfs API that Marek and Pavel want.
No idea, let's see the change first, we can never promise anything :)
Hi Greg, Can't we use blkdev_get_by_path() (or blk_lookup_devt() with blkdev_get_by_dev())? This would open the block device and return a struct block_device *. When the LED trigger is disabled, it would also have to release the device. Marek
On Tue, Aug 10, 2021 at 03:38:40PM +0200, Marek Behún wrote:
On Tue, 10 Aug 2021 08:35:08 +0200 Greg KH <gregkh@linuxfoundation.org> wrote:
On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
On 8/9/21 5:43 PM, Marek Behún wrote:
I confess that I am not very familiar with internal blkdev API.
It's mainly a matter of symbol visibility. See this thread from a few months ago:
https://www.spinics.net/lists/linux-leds/msg18244.html
Now ... my code currently lives in block/, so there isn't actually anything technically preventing it from iterating through the block devices.
The reactions to Enzo's patch (which you can see in that thread) make me think that anything that iterates through all block devices is likely to be rejected, but maybe I'm reading too much into it.
Greg / Christoph -
(As you were the people who expressed disapproval of Enzo's patch to export block_class and disk_type ...)
Can you weigh in on the acceptability of iterating through the block devices (searching by name) from LED trigger code within the block subsystem (i.e. no new symbols would need to be exported)?
This would allow the trigger to implement the sysfs API that Marek and Pavel want.
No idea, let's see the change first, we can never promise anything :)
Hi Greg,
Can't we use blkdev_get_by_path() (or blk_lookup_devt() with blkdev_get_by_dev())? This would open the block device and return a struct block_device *. When the LED trigger is disabled, it would also have to release the device.
But what about when the device is removed from the system first? Be careful about that... Anyway, sure, try those functions, I really do not know, all I originally complained about was those exports which did not need to be exported. thanks, greg k-h
On 8/10/21 9:48 AM, Greg KH wrote:
But what about when the device is removed from the system first? Be careful about that...
Anyway, sure, try those functions, I really do not know, all I originally complained about was those exports which did not need to be exported.
Sounds good. I'll work something up. (I'm actually thinking that class_find_device() may be the best way to go, as it grabs a reference to the device.) -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Tue, Aug 10, 2021 at 10:55:33AM -0500, Ian Pilcher wrote:
On 8/10/21 9:48 AM, Greg KH wrote:
But what about when the device is removed from the system first? Be careful about that...
Anyway, sure, try those functions, I really do not know, all I originally complained about was those exports which did not need to be exported.
Sounds good. I'll work something up. (I'm actually thinking that class_find_device() may be the best way to go, as it grabs a reference to the device.)
There should not be anything "odd" about block devices here, just do whatever all other LED drivers do when referencing a device. thanks, greg k-h
On Tue, 10 Aug 2021 18:24:12 +0200 Greg KH <gregkh@linuxfoundation.org> wrote:
Sounds good. I'll work something up. (I'm actually thinking that class_find_device() may be the best way to go, as it grabs a reference to the device.)
There should not be anything "odd" about block devices here, just do whatever all other LED drivers do when referencing a device.
Ian, look at ledtrig-tty and maybe get inspiration from there :) Marek
On 8/10/21 11:24 AM, Greg KH wrote:
There should not be anything "odd" about block devices here, just do whatever all other LED drivers do when referencing a device.
AFAIK, the only LED trigger that does anything similar is the netdev trigger. It uses dev_get_by_name(), which is specific to network devices. The block subsystem doesn't appear to have any similar API, which is why Enzo submitted his patch to export block_class and disk_type back in April[1], when he wanted to do something similar. I'm basically bypassing the need to export the symbols, because my trigger code is actually in the block subsystem, rather than the LEDs subsystem. [1] https://www.spinics.net/lists/linux-leds/msg18244.html -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
On 8/9/21 5:43 PM, Marek Behún wrote:
I confess that I am not very familiar with internal blkdev API.
It's mainly a matter of symbol visibility. See this thread from a few months ago:
https://www.spinics.net/lists/linux-leds/msg18244.html
Now ... my code currently lives in block/, so there isn't actually anything technically preventing it from iterating through the block devices.
The reactions to Enzo's patch (which you can see in that thread) make me think that anything that iterates through all block devices is likely to be rejected, but maybe I'm reading too much into it.
I think the main issue with this series is that it adds a shitload of code and a hook in the absolute I/O fastpath for fricking blinkenlights. I don't think it is even worth wasting time on something this ridiculous.
On Wed, 11 Aug 2021 08:26:42 +0200 Christoph Hellwig <hch@lst.de> wrote:
On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
On 8/9/21 5:43 PM, Marek Behún wrote:
I confess that I am not very familiar with internal blkdev API.
It's mainly a matter of symbol visibility. See this thread from a few months ago:
https://www.spinics.net/lists/linux-leds/msg18244.html
Now ... my code currently lives in block/, so there isn't actually anything technically preventing it from iterating through the block devices.
The reactions to Enzo's patch (which you can see in that thread) make me think that anything that iterates through all block devices is likely to be rejected, but maybe I'm reading too much into it.
I think the main issue with this series is that it adds a shitload of code and a hook in the absolute I/O fastpath for fricking blinkenlights. I don't think it is even worth wasting time on something this ridiculous.
That's why I think we should do this the way the netdev trigger does. Periodically reading block_device's stats, and if they are greater, blink the LED. Marek
participants (6)
-
Christoph Hellwig -
Greg KH -
Ian Pilcher -
Jackie Liu -
Marek Behún -
Pali Rohár