[RFC PATCH 0/8] Add configurable block device LED triggers
This patch series adds configurable (i.e. user-defined) block device LED triggers. * Triggers can be created, listed, and deleted via sysfs block class attributes (led_trigger_{new,list,del}). * Once created, block device LED triggers are associated with LEDs just like any other LED trigger (via /sys/class/leds/${LED}/trigger). * Each block device gains a new device attribute (led_trigger) that can be used to associate the device with a trigger or clear its association. * My expectation is that most configuration will be done via sysfs (driven by udev), but there also in-kernel APIs for creating, deleting, and (dis)associating triggers. * Multiple devices can be associated with one trigger, so this supports a single LED driven by multiple devices, multiple device-specific LEDs, or arbitrary combinations. Along with support for more than just ATA devices, this is the main difference between this function and the current disk activity trigger. It makes it suitable for use on systems like the Thecus N5550 NAS, which has a software-driven activity LEDs for each disk. * In addition to physical block devices, many types of virtual block devices can drive LEDs; device mapper, MD RAID, and loop devices work (but zram swap devices do not). * The led trigger is "blinked" (75 msec on, 25 msec off) when a request is successfully sent to the low-level driver. The intent is to provide a visual indication of device activity, not any sort of exact measurement. * Related to the previous bullet, if the blink function is unable to immediately acquire a lock on the device's LED trigger information it simply returns, so that I/O processing can continue. It's probably obvious that I'm basically a complete newbie at kernel development, so I welcome feedback. Thanks! Ian Pilcher (8): docs: Add block device LED trigger documentation block: Add block device LED trigger list block: Add kernel APIs to create & delete block device LED triggers block: Add block class attributes to manage LED trigger list block: Add block device LED trigger info to struct genhd block: Add kernel APIs to set & clear per-block device LED triggers block: Add block device attributes to set & clear LED triggers block: Blink device LED when request is sent to low-level driver Documentation/block/index.rst | 1 + Documentation/block/led-triggers.rst | 124 ++++++ block/Kconfig | 10 + block/Makefile | 1 + block/blk-ledtrig.c | 570 +++++++++++++++++++++++++++ block/blk-ledtrig.h | 51 +++ block/blk-mq.c | 2 + block/genhd.c | 14 + include/linux/blk-ledtrig.h | 24 ++ include/linux/genhd.h | 4 + 10 files changed, 801 insertions(+) create mode 100644 Documentation/block/led-triggers.rst create mode 100644 block/blk-ledtrig.c create mode 100644 block/blk-ledtrig.h create mode 100644 include/linux/blk-ledtrig.h -- 2.31.1
* Document the sysfs attributes (/sys/class/block/led_trigger_* and /sys/class/block/${DEV}/led_trigger) that can be used to create, list, and delete block device LED triggers and to set and clear device/trigger associations. * Pull API documentation from block/blk-ledtrig.c (once it exists). Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- Documentation/block/index.rst | 1 + Documentation/block/led-triggers.rst | 124 +++++++++++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 Documentation/block/led-triggers.rst diff --git a/Documentation/block/index.rst b/Documentation/block/index.rst index 86dcf7159f99..a125ecdb4c7b 100644 --- a/Documentation/block/index.rst +++ b/Documentation/block/index.rst @@ -25,3 +25,4 @@ Block stat switching-sched writeback_cache_control + led-triggers diff --git a/Documentation/block/led-triggers.rst b/Documentation/block/led-triggers.rst new file mode 100644 index 000000000000..a67e06c68073 --- /dev/null +++ b/Documentation/block/led-triggers.rst @@ -0,0 +1,124 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============ +LED Triggers +============ + +Available when ``CONFIG_BLK_LED_TRIGGERS=y``. + +sysfs interface +=============== + +Create a new block device LED trigger:: + + # echo foo > /sys/class/block/led_trigger_new + +The name must be unique among all LED triggers (not just block device LED +triggers). + +Create two more:: + + # echo bar baz > /sys/class/block/led_trigger_new + +List the triggers:: + + # cat /sys/class/block/led_trigger_list + baz: 0 + bar: 0 + foo: 0 + +(The number after each trigger is its reference count.) + +Associate a trigger with a block device:: + + # cat /sys/class/block/sda/led_trigger + (none) + + # echo foo > /sys/class/block/sda/led_trigger + # cat /sys/class/block/sda/led_trigger + foo + +Note that ``foo``'s reference count has increased, and it cannot be deleted:: + + # cat /sys/class/block/led_trigger_list + baz: 0 + bar: 0 + foo: 1 + + # echo foo > /sys/class/block/led_trigger_del + -bash: echo: write error: Device or resource busy + + # dmesg | tail -n 1 + [23176.475424] blockdev LED trigger foo still in use + +Associate the ``foo`` trigger with an LED:: + + # cat /sys/class/leds/input1::scrolllock/trigger + none usb-gadget usb-host rc-feedback [kbd-scrolllock] kbd-numlock + kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock + kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock + disk-activity disk-read disk-write ide-disk mtd nand-disk panic + audio-mute audio-micmute rfkill-any rfkill-none foo bar baz + + # echo foo > /sys/class/leds/input1::scrolllock/trigger + + # cat /sys/class/leds/input1::scrolllock/trigger + none usb-gadget usb-host rc-feedback [kbd-scrolllock] kbd-numlock + kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock + kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock + disk-activity disk-read disk-write ide-disk mtd nand-disk panic + audio-mute audio-micmute rfkill-any rfkill-none [foo] bar baz + +Reads and writes to ``sda`` should now cause the scroll lock LED on your +keyboard to blink (assuming that it has one). + +Multiple devices can be associated with a trigger:: + + # echo foo > /sys/class/block/sdb/led_trigger + + # cat /sys/class/block/led_trigger_list + baz: 0 + bar: 0 + foo: 2 + +Activity on either ``sda`` or ``sdb`` should now be shown by your scroll lock +LED. + +Clear ``sda``'s LED trigger:: + + # echo > /sys/class/block/sda/led_trigger + + # cat /sys/class/block/sda/led_trigger + (none) + + # cat /sys/class/block/led_trigger_list + baz: 0 + bar: 0 + foo: 1 + +And ``sdb``'s trigger:: + + # echo > /sys/class/block/sdb/led_trigger + +Delete the triggers:: + + # echo foo bar baz > /sys/class/block/led_trigger_del + + # cat /sys/class/block/led_trigger_list + + # cat /sys/class/leds/input1::scrolllock/trigger + none usb-gadget usb-host rc-feedback [kbd-scrolllock] kbd-numlock + kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock + kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock + disk-activity disk-read disk-write ide-disk mtd nand-disk panic + audio-mute audio-micmute rfkill-any rfkill-none + +Also see **Userspace LEDs** (``Documentation/leds/uleds.rst``). + +Kernel API +========== + +``#include <linux/blk-ledtrig.h>`` + +.. kernel-doc:: block/blk-ledtrig.c + :export: -- 2.31.1
On Wed, 28 Jul 2021 20:53:37 -0500, Ian Pilcher said:
+Create a new block device LED trigger:: + + # echo foo > /sys/class/block/led_trigger_new + +The name must be unique among all LED triggers (not just block device LED +triggers). + +Create two more:: + + # echo bar baz > /sys/class/block/led_trigger_new
+ # cat /sys/class/block/led_trigger_list + baz: 0 + bar: 0 + foo: 0
This looks like an abuse of the "one entry one value" rule for sysfs. Perhaps this should be a directory /sys/class/block/defined_triggers/ and separate files under that for foo, bar, and baz? That would probably make reference counting a lot easier as well....
On 7/28/21 10:09 PM, Valdis Klētnieks wrote:
+ # cat /sys/class/block/led_trigger_list + baz: 0 + bar: 0 + foo: 0
This looks like an abuse of the "one entry one value" rule for sysfs. Perhaps this should be a directory /sys/class/block/defined_triggers/ and separate files under that for foo, bar, and baz? That would probably make reference counting a lot easier as well....
Indeed it is. Funny that you should mention using a subdirectory. I originally wanted to put all of the trigger-related stuff into /sys/class/block/led_triggers/, but I couldn't find any API to create a subdirectory for *class* attributes (only for device attributes), nor do I see any such subdirectories on my system. # find /sys/class -type d | egrep '^/sys/class/[^/]+/' (no output) Is is possible to create subdirectories for class attributes? -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Thu, Jul 29, 2021 at 10:52:06AM -0500, Ian Pilcher wrote:
On 7/28/21 10:09 PM, Valdis Klētnieks wrote:
+ # cat /sys/class/block/led_trigger_list + baz: 0 + bar: 0 + foo: 0
This looks like an abuse of the "one entry one value" rule for sysfs. Perhaps this should be a directory /sys/class/block/defined_triggers/ and separate files under that for foo, bar, and baz? That would probably make reference counting a lot easier as well....
Indeed it is.
Funny that you should mention using a subdirectory. I originally wanted to put all of the trigger-related stuff into /sys/class/block/led_triggers/, but I couldn't find any API to create a subdirectory for *class* attributes (only for device attributes), nor do I see any such subdirectories on my system.
Add a name to your attribute group and sysfs creates the subdirectory automagically for you. thanks, greg k-h
On Wed, Jul 28, 2021 at 08:53:37PM -0500, Ian Pilcher wrote:
* Document the sysfs attributes (/sys/class/block/led_trigger_* and /sys/class/block/${DEV}/led_trigger) that can be used to create, list, and delete block device LED triggers and to set and clear device/trigger associations.
sysfs apis belong in Documentation/ABI/ like all other entries.
* Pull API documentation from block/blk-ledtrig.c (once it exists).
Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- Documentation/block/index.rst | 1 + Documentation/block/led-triggers.rst | 124 +++++++++++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 Documentation/block/led-triggers.rst
diff --git a/Documentation/block/index.rst b/Documentation/block/index.rst index 86dcf7159f99..a125ecdb4c7b 100644 --- a/Documentation/block/index.rst +++ b/Documentation/block/index.rst @@ -25,3 +25,4 @@ Block stat switching-sched writeback_cache_control + led-triggers diff --git a/Documentation/block/led-triggers.rst b/Documentation/block/led-triggers.rst new file mode 100644 index 000000000000..a67e06c68073 --- /dev/null +++ b/Documentation/block/led-triggers.rst @@ -0,0 +1,124 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============ +LED Triggers +============ + +Available when ``CONFIG_BLK_LED_TRIGGERS=y``. + +sysfs interface +=============== + +Create a new block device LED trigger:: + + # echo foo > /sys/class/block/led_trigger_new + +The name must be unique among all LED triggers (not just block device LED +triggers). + +Create two more:: + + # echo bar baz > /sys/class/block/led_trigger_new + +List the triggers:: + + # cat /sys/class/block/led_trigger_list + baz: 0 + bar: 0 + foo: 0
As was pointed out, this is not ok for a sysfs file, sorry. thanks, greg k-h
Dear Ian, On Wed, 28 Jul 2021 20:53:37 -0500 Ian Pilcher <arequipeno@gmail.com> wrote:
* Document the sysfs attributes (/sys/class/block/led_trigger_* and /sys/class/block/${DEV}/led_trigger) that can be used to create, list, and delete block device LED triggers and to set and clear device/trigger associations.
* Pull API documentation from block/blk-ledtrig.c (once it exists).
Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
thank you for this proposal. I don't really see the purpose for having multiple different block device LED triggers. Moreover we really do not want userspace to be able to add LED triggers with arbitrary names, and as many as the userspace wants. There is no sense in making userspace be able to create 10000 triggers. Also if userspace can create triggers with arbitrary names, it could "steal" a name for a real trigger. For example if netdev trigger is compiled as a module, and before loading someone creates blockdev trigger with name "netdev", the loading of netdev trigger will fail. I would like the blkdev trigger to work in a similar way the netdev trigger works: - only one trigger, with name "blkdev" - when activated on a LED, new sysfs files will be created: * device_name, where user can write sda1, vdb, ... * read (binary value, 1 means blink on read) * write (binary value, 1 means blink on write) * interval (blink interval) Note that device_name could allow multiple names, in theory... Also some other disk states may be included, like error, or something - also the blinking itself can be done as is done netdev trigger: every 50ms the work function would look at blkdev stats, and if current stat (number of bytes read/written) is different from previous, then blink the LED Marek
* New config option (CONFIG_BLK_LED_TRIGGERS) to enable/disable block device LED triggers * New file - block/blk-ledtrig.c * Use a linked list of dynamically allocated triggers. There aren't likely to be that many of them, and the list is only searched when creating/deleting a trigger or setting/clearing a device/trigger association - none of which should occur very often. Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/Kconfig | 10 +++++++++ block/Makefile | 1 + block/blk-ledtrig.c | 51 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+) create mode 100644 block/blk-ledtrig.c diff --git a/block/Kconfig b/block/Kconfig index fd732aede922..051488413d6e 100644 --- a/block/Kconfig +++ b/block/Kconfig @@ -220,6 +220,16 @@ 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 block device LED triggers" + depends on LEDS_TRIGGERS + help + Enabling this allows LED triggers to be created and + associated with block devices via sysfs/udev (or an + in-kernel API). These trigers can be used to drive + physical or user-space activity indicators. See + Documentation/block/led-triggers.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 diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c new file mode 100644 index 000000000000..345a3b6bdbc6 --- /dev/null +++ b/block/blk-ledtrig.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/* + * Block device LED triggers + * + * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> + */ + +#include <linux/leds.h> +#include <linux/list.h> +#include <linux/mutex.h> + + +/* + * + * The list of block device LED triggers + * + */ + +struct blk_ledtrig { + struct led_trigger trigger; + struct list_head list_node; + struct mutex refcount_mutex; + int refcount; + char name[]; +}; + +LIST_HEAD(blk_ledtrig_list); +DEFINE_MUTEX(blk_ledtrig_list_mutex); + +static inline +struct blk_ledtrig *blk_ledtrig_from_node(struct list_head *const node) +{ + return container_of(node, struct blk_ledtrig, list_node); +} + +// Caller must hold blk_ledtrig_list_mutex +static struct blk_ledtrig *blk_ledtrig_find(const char *const name, + const size_t len) +{ + struct blk_ledtrig *t; + struct list_head *n; + + list_for_each(n, &blk_ledtrig_list) { + t = blk_ledtrig_from_node(n); + if (strlen(t->name) == len && memcmp(name, t->name, len) == 0) + return t; + } + + return NULL; +} -- 2.31.1
On Wed, 28 Jul 2021 20:53:38 -0500, Ian Pilcher said:
* New config option (CONFIG_BLK_LED_TRIGGERS) to enable/disable block device LED triggers
* New file - block/blk-ledtrig.c
Is this bisect-clean (as in "will it build properly with that config option set after each of the succeeding patches")? Usually, the config option is added in the *last* patch, so that even if you have a bisect issue it won't manifest because it's wrapped in a '#ifdef CONFIG_WHATEVER' that can't possibly be compiled in because there's no way for Kconfig to set that variable.
On 7/28/21 10:14 PM, Valdis Klētnieks wrote:
Is this bisect-clean (as in "will it build properly with that config option set after each of the succeeding patches")? Usually, the config option is added in the *last* patch, so that even if you have a bisect issue it won't manifest because it's wrapped in a '#ifdef CONFIG_WHATEVER' that can't possibly be compiled in because there's no way for Kconfig to set that variable.
Yes it is. I tested compiling each patch with the CONFIG option both enabled and disabled. (You will get an unused function warning for blk_ledtrig_find() until patch #3 is applied.) I'll switch to adding the option in the last patch of the series in the future. Thanks! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
* New file - include/linux/blk-ledtrig.h Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 152 ++++++++++++++++++++++++++++++++++++ include/linux/blk-ledtrig.h | 19 +++++ 2 files changed, 171 insertions(+) create mode 100644 include/linux/blk-ledtrig.h diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 345a3b6bdbc6..c69ea1539336 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -6,9 +6,11 @@ * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> */ +#include <linux/blk-ledtrig.h> #include <linux/leds.h> #include <linux/list.h> #include <linux/mutex.h> +#include <linux/slab.h> /* @@ -49,3 +51,153 @@ static struct blk_ledtrig *blk_ledtrig_find(const char *const name, return NULL; } + + +/* + * + * Create a new trigger + * + */ + +static int __blk_ledtrig_create(const char *const name, const size_t len) +{ + struct blk_ledtrig *t; + int ret; + + if (len == 0) { + pr_warn("empty name specified for blockdev LED trigger\n"); + ret = -EINVAL; + goto create_exit_return; + } + + ret = mutex_lock_interruptible(&blk_ledtrig_list_mutex); + if (unlikely(ret != 0)) + goto create_exit_return; + + if (blk_ledtrig_find(name, len) != NULL) { + pr_warn("blockdev LED trigger named %.*s already exists\n", + (int)len, name); + ret = -EEXIST; + goto create_exit_unlock_list; + } + + t = kzalloc(sizeof(*t) + len + 1, GFP_KERNEL); + if (unlikely(t == NULL)) { + ret = -ENOMEM; + goto create_exit_unlock_list; + } + + memcpy(t->name, name, len); + t->trigger.name = t->name; + mutex_init(&t->refcount_mutex); + + ret = led_trigger_register(&t->trigger); + if (ret != 0) { + if (likely(ret == -EEXIST)) { + pr_warn("LED trigger named %.*s already exists\n", + (int)len, name); + } + goto create_exit_free; + } + + list_add(&t->list_node, &blk_ledtrig_list); + ret = 0; + +create_exit_free: + if (ret != 0) + kfree(t); +create_exit_unlock_list: + mutex_unlock(&blk_ledtrig_list_mutex); +create_exit_return: + return ret; +} + +/** + * blk_ledtrig_create() - creates a new block device LED trigger + * @name: the name of the new trigger + * + * Context: Process context (can sleep). Takes and releases + * @blk_ledtrig_list_mutex. + * + * Return: 0 on success; -@errno on error + */ +int blk_ledtrig_create(const char *const name) +{ + return __blk_ledtrig_create(name, strlen(name)); +} +EXPORT_SYMBOL_GPL(blk_ledtrig_create); + + +/* + * + * Delete a trigger + * + */ + +static int __blk_ledtrig_delete(const char *const name, const size_t len) +{ + struct blk_ledtrig *t; + int ret; + + if (len == 0) { + pr_warn("empty name specified for blockdev LED trigger\n"); + ret = -EINVAL; + goto delete_exit_return; + } + + ret = mutex_lock_interruptible(&blk_ledtrig_list_mutex); + if (unlikely(ret != 0)) + goto delete_exit_return; + + t = blk_ledtrig_find(name, len); + if (t == NULL) { + pr_warn("blockdev LED trigger named %.*s doesn't exist\n", + (int)len, name); + ret = -ENODEV; + goto delete_exit_unlock_list; + } + + ret = mutex_lock_interruptible(&t->refcount_mutex); + if (unlikely(ret != 0)) + goto delete_exit_unlock_list; + + if (WARN_ON(t->refcount < 0)) { + ret = -EBADFD; + goto delete_exit_unlock_refcount; + } + + if (t->refcount > 0) { + pr_warn("blockdev LED trigger %s still in use\n", t->name); + ret = -EBUSY; + goto delete_exit_unlock_refcount; + } + + led_trigger_unregister(&t->trigger); + list_del(&t->list_node); + + ret = 0; + +delete_exit_unlock_refcount: + mutex_unlock(&t->refcount_mutex); + if (ret == 0) + kfree(t); +delete_exit_unlock_list: + mutex_unlock(&blk_ledtrig_list_mutex); +delete_exit_return: + return ret; +} + +/** + * blk_ledtrig_delete() - deletes a block device LED trigger + * @name: the name of the trigger to be deleted + * + * Context: Process context (can sleep). Takes and releases + * @blk_ledtrig_list_mutex and trigger's @refcount_mutex. + * + * Return: 0 on success; -@errno on error + */ +int blk_ledtrig_delete(const char *const name) +{ + return __blk_ledtrig_delete(name, strlen(name)); +} +EXPORT_SYMBOL_GPL(blk_ledtrig_delete); diff --git a/include/linux/blk-ledtrig.h b/include/linux/blk-ledtrig.h new file mode 100644 index 000000000000..6f73635f65ec --- /dev/null +++ b/include/linux/blk-ledtrig.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* + * Block device LED triggers + * + * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> + */ + +#ifndef _LINUX_BLK_LEDTRIG_H +#define _LINUX_BLK_LEDTRIG_H + +#ifdef CONFIG_BLK_LED_TRIGGERS + +int blk_ledtrig_create(const char *name); +int blk_ledtrig_delete(const char *name); + +#endif // CONFIG_BLK_LED_TRIGGERS + +#endif // _LINUX_BLK_LEDTRIG_H -- 2.31.1
On Wed, 28 Jul 2021 20:53:39 -0500, Ian Pilcher said:
* New file - include/linux/blk-ledtrig.h
Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 152 ++++++++++++++++++++++++++++++++++++ include/linux/blk-ledtrig.h | 19 +++++ 2 files changed, 171 insertions(+) create mode 100644 include/linux/blk-ledtrig.h
diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 345a3b6bdbc6..c69ea1539336 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c
+ +static int __blk_ledtrig_create(const char *const name, const size_t len)
(...) + if (blk_ledtrig_find(name, len) != NULL) { + pr_warn("blockdev LED trigger named %.*s already exists\n", + (int)len, name); Is pr_warn() the right level for this stuff? I'd think this sort of pilot error should be pr_info() or even pr_debug(), if mentioned at all. pr_warn() would be for something like an unexpected situation like trying to blink an LED but failing. Simple syntax errors should probably just toss a -EINVAL and return. (Among other things, this allows a userspace script to spam the log by simply repeatedly trying to create the same entry)
On 7/28/21 10:45 PM, Valdis Klētnieks wrote:
Is pr_warn() the right level for this stuff? I'd think this sort of pilot error should be pr_info() or even pr_debug(), if mentioned at all. pr_warn() would be for something like an unexpected situation like trying to blink an LED but failing. Simple syntax errors should probably just toss a -EINVAL and return.
Fair point. I'll change it to pr_info(). I'm reluctant to completely "swallow" the error message, as I've been on the other side as a system administrator trying to guess at the reason for an error code.
(Among other things, this allows a userspace script to spam the log by simply repeatedly trying to create the same entry)
Only root, and they've got plenty of ways to do that. Thanks! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Wed, Jul 28, 2021 at 08:53:39PM -0500, Ian Pilcher wrote:
* New file - include/linux/blk-ledtrig.h
Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 152 ++++++++++++++++++++++++++++++++++++ include/linux/blk-ledtrig.h | 19 +++++ 2 files changed, 171 insertions(+) create mode 100644 include/linux/blk-ledtrig.h
diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 345a3b6bdbc6..c69ea1539336 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -6,9 +6,11 @@ * Copyright 2021 Ian Pilcher <arequipeno@gmail.com> */
+#include <linux/blk-ledtrig.h> #include <linux/leds.h> #include <linux/list.h> #include <linux/mutex.h> +#include <linux/slab.h>
/* @@ -49,3 +51,153 @@ static struct blk_ledtrig *blk_ledtrig_find(const char *const name,
return NULL; } + + +/* + * + * Create a new trigger + * + */ + +static int __blk_ledtrig_create(const char *const name, const size_t len) +{ + struct blk_ledtrig *t; + int ret; + + if (len == 0) { + pr_warn("empty name specified for blockdev LED trigger\n"); + ret = -EINVAL; + goto create_exit_return; + } + + ret = mutex_lock_interruptible(&blk_ledtrig_list_mutex); + if (unlikely(ret != 0))
Only ever use likely/unlikely if you can measure the difference without it. Otherwise the CPU and compiler will almost always get it right and you should not clutter up the code with them at all. For something like this function, where there is no speed difference at all, there is no need for these types of markings, so I would recommend just removing them all from your patchset. thanks, greg k-h
* New class attributes - /sys/class/block/led_trigger_{new,list,del} * Add init function - blk_ledtrig_init() - to create the attributes in sysfs. Call blk_ledtrig_init() from genhd_device_init() (in block/genhd.c). * New file - block/blk-ledtrig.h Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 147 ++++++++++++++++++++++++++++++++++++++++++++ block/blk-ledtrig.h | 22 +++++++ block/genhd.c | 2 + 3 files changed, 171 insertions(+) create mode 100644 block/blk-ledtrig.h diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index c69ea1539336..6392ab4169f9 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -7,11 +7,15 @@ */ #include <linux/blk-ledtrig.h> +#include <linux/ctype.h> +#include <linux/genhd.h> #include <linux/leds.h> #include <linux/list.h> #include <linux/mutex.h> #include <linux/slab.h> +#include "blk-ledtrig.h" + /* * @@ -201,3 +205,146 @@ int blk_ledtrig_delete(const char *const name) return __blk_ledtrig_delete(name, strlen(name)); } EXPORT_SYMBOL_GPL(blk_ledtrig_delete); + + +/* + * + * Class attributes to manage the trigger list + * + */ + +static ssize_t blk_ledtrig_attr_store(struct class *, struct class_attribute *, + const char *, const size_t); +static ssize_t blk_ledtrig_list_show(struct class *, struct class_attribute *, + char *); + +static struct class_attribute blk_ledtrig_attr_new = + __ATTR(led_trigger_new, 0200, 0, blk_ledtrig_attr_store); + +static struct class_attribute blk_ledtrig_attr_del = + __ATTR(led_trigger_del, 0200, 0, blk_ledtrig_attr_store); + +static struct class_attribute blk_ledtrig_attr_list = + __ATTR(led_trigger_list, 0444, blk_ledtrig_list_show, 0); + +// Returns a pointer to the first non-whitespace character in s (or a pointer +// to the terminating nul). +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 nul), 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 ssize_t blk_ledtrig_attr_store(struct class *const class, + struct class_attribute *const attr, + const char *const buf, + const size_t count) +{ + const char *const name = blk_ledtrig_skip_whitespace(buf); + const char *const endp = blk_ledtrig_find_whitespace(name); + const ptrdiff_t name_len = endp - name; // always >= 0 + int ret; + + if (attr == &blk_ledtrig_attr_new) + ret = __blk_ledtrig_create(name, name_len); + else // attr == &blk_ledtrig_attr_del + ret = __blk_ledtrig_delete(name, name_len); + + if (ret < 0) + return ret; + + // Avoid potential "empty name" error by skipping whitespace + // to next token or terminating nul + return blk_ledtrig_skip_whitespace(endp) - buf; +} + +static ssize_t blk_ledtrig_list_show(struct class *const class, + struct class_attribute *const attr, + char *const buf) +{ + struct list_head *n; + int ret, c = 0; + + ret = mutex_lock_interruptible(&blk_ledtrig_list_mutex); + if (unlikely(ret != 0)) + goto list_exit_return; + + list_for_each(n, &blk_ledtrig_list) { + + struct blk_ledtrig *const t = blk_ledtrig_from_node(n); + int refcount; + + ret = mutex_lock_interruptible(&t->refcount_mutex); + if (unlikely(ret != 0)) + goto list_exit_unlock_list; + + refcount = t->refcount; + + mutex_unlock(&t->refcount_mutex); + + ret = snprintf(buf + c, PAGE_SIZE - c, "%s: %d\n", + t->name, refcount); + if (unlikely(ret < 0)) + goto list_exit_unlock_list; + + c += ret; + if (unlikely(c >= PAGE_SIZE)) { + ret = -EOVERFLOW; + goto list_exit_unlock_list; + } + } + + ret = c; + +list_exit_unlock_list: + mutex_unlock(&blk_ledtrig_list_mutex); +list_exit_return: + return ret; +} + + +/* + * + * Initialization - create the class attributes + * + */ + +void __init blk_ledtrig_init(void) +{ + int ret; + + ret = class_create_file(&block_class, &blk_ledtrig_attr_new); + if (unlikely(ret != 0)) + goto init_error_new; + + ret = class_create_file(&block_class, &blk_ledtrig_attr_del); + if (unlikely(ret != 0)) + goto init_error_del; + + ret = class_create_file(&block_class, &blk_ledtrig_attr_list); + if (unlikely(ret != 0)) + goto init_error_list; + + return; + +init_error_list: + class_remove_file(&block_class, &blk_ledtrig_attr_del); +init_error_del: + class_remove_file(&block_class, &blk_ledtrig_attr_new); +init_error_new: + pr_err("failed to initialize blkdev LED triggers (%d)\n", ret); +} diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h new file mode 100644 index 000000000000..894843249deb --- /dev/null +++ b/block/blk-ledtrig.h @@ -0,0 +1,22 @@ +/* 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 + +void blk_ledtrig_init(void); + +#else // CONFIG_BLK_LED_TRIGGERS + +static inline void blk_ledtrig_init(void) {} + +#endif // CONFIG_BLK_LED_TRIGGERS + +#endif // _BLOCK_BLK_LEDTRIG_H diff --git a/block/genhd.c b/block/genhd.c index af4d2ab4a633..d0b1d8f743ae 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; @@ -824,6 +825,7 @@ static int __init genhd_device_init(void) if (unlikely(error)) return error; blk_dev_init(); + blk_ledtrig_init(); register_blkdev(BLOCK_EXT_MAJOR, "blkext"); -- 2.31.1
On Wed, Jul 28, 2021 at 08:53:40PM -0500, Ian Pilcher wrote:
* New class attributes - /sys/class/block/led_trigger_{new,list,del}
* Add init function - blk_ledtrig_init() - to create the attributes in sysfs. Call blk_ledtrig_init() from genhd_device_init() (in block/genhd.c).
* New file - block/blk-ledtrig.h
That is an odd way to write a changelog, please read the documentation file about how to write a good changelog. thanks, greg k-h
* Initialize trigger info when device is added. Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.h | 7 +++++++ block/genhd.c | 1 + include/linux/genhd.h | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h index 894843249deb..5854b21a210c 100644 --- a/block/blk-ledtrig.h +++ b/block/blk-ledtrig.h @@ -13,9 +13,16 @@ void blk_ledtrig_init(void); +static inline void blk_ledtrig_disk_init(struct gendisk *const gd) +{ + gd->ledtrig = NULL; + mutex_init(&gd->ledtrig_mutex); +} + #else // CONFIG_BLK_LED_TRIGGERS static inline void blk_ledtrig_init(void) {} +static inline void blk_ledtrig_disk_init(const struct gendisk *gd) {} #endif // CONFIG_BLK_LED_TRIGGERS diff --git a/block/genhd.c b/block/genhd.c index d0b1d8f743ae..420325447c5d 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -540,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..3409334c9b4c 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -169,6 +169,10 @@ struct gendisk { #if IS_ENABLED(CONFIG_CDROM) struct cdrom_device_info *cdi; #endif +#ifdef CONFIG_BLK_LED_TRIGGERS + struct blk_ledtrig *ledtrig; + struct mutex ledtrig_mutex; +#endif /* CONFIG_BLK_LED_TRIGGERS */ int node_id; struct badblocks *bb; struct lockdep_map lockdep_map; -- 2.31.1
* Clear LED trigger (decrement trigger reference count) when device is deleted, e.g. when a USB disk is unplugged. Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 131 ++++++++++++++++++++++++++++++++++++ block/blk-ledtrig.h | 5 ++ block/genhd.c | 2 + include/linux/blk-ledtrig.h | 5 ++ 4 files changed, 143 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 6392ab4169f9..7c8fdff88683 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -348,3 +348,134 @@ void __init blk_ledtrig_init(void) init_error_new: pr_err("failed to initialize blkdev LED triggers (%d)\n", ret); } + + +/* + * + * Set a device trigger + * + */ + +static int __blk_ledtrig_set(struct gendisk *const gd, const char *const name, + const size_t name_len) +{ + struct blk_ledtrig *t; + bool already_set; + int ret; + + ret = mutex_lock_interruptible(&blk_ledtrig_list_mutex); + if (unlikely(ret != 0)) + goto set_exit_return; + + t = blk_ledtrig_find(name, name_len); + if (t == NULL) { + pr_warn("blockdev LED trigger named %.*s doesn't exist\n", + (int)name_len, name); + ret = -ENODEV; + goto set_exit_unlock_list; + } + + ret = mutex_lock_interruptible(&t->refcount_mutex); + if (unlikely(ret != 0)) + goto set_exit_unlock_list; + + // Holding the refcount mutex blocks __blk_ledtrig_delete, so we don't + // actually need to hold the list mutex anymore, but it makes the flow + // much simpler to do so + + if (WARN_ON_ONCE(t->refcount == INT_MAX)) { + ret = -ERANGE; + goto set_exit_unlock_refcount; + } + + ret = mutex_lock_interruptible(&gd->ledtrig_mutex); + if (unlikely(ret != 0)) + goto set_exit_unlock_refcount; + + if (gd->ledtrig == NULL) { + already_set = false; + gd->ledtrig = t; + } else { + already_set = true; + } + + mutex_unlock(&gd->ledtrig_mutex); + + if (already_set) { + pr_warn("blockdev trigger for %s already set\n", + gd->disk_name); + ret = -EBUSY; + goto set_exit_unlock_refcount; + } + + ++(t->refcount); + ret = 0; + +set_exit_unlock_refcount: + mutex_unlock(&t->refcount_mutex); +set_exit_unlock_list: + mutex_unlock(&blk_ledtrig_list_mutex); +set_exit_return: + return ret; +} + +/** + * blk_ledtrig_set() - set the LED trigger for a block device + * @gd: the block device + * @name: the name of the LED trigger + * + * Context: Process context (can sleep). Takes and releases + * @blk_ledtrig_list_mutex, trigger's @refcount_mutex, + * and @gd->ledtrig_mutex. + * + * Return: 0 on success; -@errno on error + */ +int blk_ledtrig_set(struct gendisk *const gd, const char *const name) +{ + return __blk_ledtrig_set(gd, name, strlen(name)); +} +EXPORT_SYMBOL_GPL(blk_ledtrig_set); + + +/* + * + * Clear a device trigger + * + */ + +/** + * blk_ledtrig_clear() - clear the LED trigger of a block device + * @gd: the block device + * + * Context: Process context (can sleep). Takes and releases + * @gd->ledtrig_mutex and @gd->ledtrig->refcount_mutex. + * + * Return: @true if the trigger was actually cleared; @false if it wasn't set + */ +bool blk_ledtrig_clear(struct gendisk *const gd) +{ + struct blk_ledtrig *t; + bool changed; + int new_refcount; + + mutex_lock(&gd->ledtrig_mutex); + + t = gd->ledtrig; + if (t == NULL) { + changed = false; + goto clear_exit_unlock_ledtrig; + } + + mutex_lock(&t->refcount_mutex); + new_refcount = --(t->refcount); + mutex_unlock(&t->refcount_mutex); + + gd->ledtrig = NULL; + changed = true; + +clear_exit_unlock_ledtrig: + mutex_unlock(&gd->ledtrig_mutex); + WARN_ON(changed && (new_refcount < 0)); + return changed; +} +EXPORT_SYMBOL_GPL(blk_ledtrig_clear); diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h index 5854b21a210c..9b718d45783f 100644 --- a/block/blk-ledtrig.h +++ b/block/blk-ledtrig.h @@ -24,6 +24,11 @@ static inline void blk_ledtrig_disk_init(struct gendisk *const gd) static inline void blk_ledtrig_init(void) {} static inline void blk_ledtrig_disk_init(const struct gendisk *gd) {} +// Real function (declared in include/linux/blk-ledtrig.h) returns a bool. +// This is only here for del_gendisk() (in genhd.c), which doesn't check +// the return value. +static inline void blk_ledtrig_clear(const struct gendisk *gd) {} + #endif // CONFIG_BLK_LED_TRIGGERS #endif // _BLOCK_BLK_LEDTRIG_H diff --git a/block/genhd.c b/block/genhd.c index 420325447c5d..fb1617f21d79 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -24,6 +24,7 @@ #include <linux/log2.h> #include <linux/pm_runtime.h> #include <linux/badblocks.h> +#include <linux/blk-ledtrig.h> #include "blk.h" #include "blk-ledtrig.h" @@ -583,6 +584,7 @@ void del_gendisk(struct gendisk *disk) if (WARN_ON_ONCE(!disk->queue)) return; + blk_ledtrig_clear(disk); blk_integrity_del(disk); disk_del_events(disk); diff --git a/include/linux/blk-ledtrig.h b/include/linux/blk-ledtrig.h index 6f73635f65ec..4ab4658df280 100644 --- a/include/linux/blk-ledtrig.h +++ b/include/linux/blk-ledtrig.h @@ -11,8 +11,13 @@ #ifdef CONFIG_BLK_LED_TRIGGERS +#include <linux/genhd.h> +#include <linux/types.h> + int blk_ledtrig_create(const char *name); int blk_ledtrig_delete(const char *name); +int blk_ledtrig_set(struct gendisk *const gd, const char *const name); +bool blk_ledtrig_clear(struct gendisk *const gd); #endif // CONFIG_BLK_LED_TRIGGERS -- 2.31.1
* Set/store functions in block/blk-ledtrig.c * Add device attributes to disk_attrs in block/genhd.c Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ block/blk-ledtrig.h | 8 ++++++ block/genhd.c | 9 +++++++ 3 files changed, 82 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 7c8fdff88683..2d324df45149 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -479,3 +479,68 @@ bool blk_ledtrig_clear(struct gendisk *const gd) return changed; } EXPORT_SYMBOL_GPL(blk_ledtrig_clear); + + +/* + * + * Set, clear & show LED triggers via sysfs device attributes + * + * (See dev_attr_led_trigger and disk_attrs in genhd.c) + * + */ + +ssize_t blk_ledtrig_devattr_store(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, const size_t count) +{ + struct gendisk *const gd = dev_to_disk(dev); + const char *const name = blk_ledtrig_skip_whitespace(buf); + const char *const endp = blk_ledtrig_find_whitespace(name); + const ptrdiff_t name_len = endp - name; // always >= 0 + int ret; + + if (name_len == 0) + ret = blk_ledtrig_clear(gd); + else + ret = __blk_ledtrig_set(gd, name, name_len); + + if (ret < 0) + return ret; + + return blk_ledtrig_skip_whitespace(endp) - buf; +} + +ssize_t blk_ledtrig_devattr_show(struct device *const dev, + struct device_attribute *const attr, + char *const buf) +{ + struct gendisk *const gd = dev_to_disk(dev); + const struct blk_ledtrig *t; + size_t name_len; + int ret; + + ret = mutex_lock_interruptible(&gd->ledtrig_mutex); + if (unlikely(ret != 0)) + return ret; + + t = gd->ledtrig; + + if (t != NULL) { + name_len = strlen(t->name); + if (likely(name_len < PAGE_SIZE - 1)) + memcpy(buf, t->name, name_len); + } + + mutex_unlock(&gd->ledtrig_mutex); + + if (t == NULL) + return sprintf(buf, "(none)\n"); + + if (unlikely(name_len >= PAGE_SIZE - 1)) + return -EOVERFLOW; + + buf[name_len] = '\n'; + buf[name_len + 1] = 0; + + return (ssize_t)(name_len + 1); +} diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h index 9b718d45783f..5d228905edbf 100644 --- a/block/blk-ledtrig.h +++ b/block/blk-ledtrig.h @@ -19,6 +19,14 @@ static inline void blk_ledtrig_disk_init(struct gendisk *const gd) mutex_init(&gd->ledtrig_mutex); } +ssize_t blk_ledtrig_devattr_store(struct device *const dev, + struct device_attribute *const attr, + const char *const buf, const size_t count); + +ssize_t blk_ledtrig_devattr_show(struct device *const dev, + struct device_attribute *const attr, + char *const buf); + #else // CONFIG_BLK_LED_TRIGGERS static inline void blk_ledtrig_init(void) {} diff --git a/block/genhd.c b/block/genhd.c index fb1617f21d79..fd37efe74d48 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1014,6 +1014,12 @@ 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_trigger = + __ATTR(led_trigger, 0644, + blk_ledtrig_devattr_show, blk_ledtrig_devattr_store); +#endif + static struct attribute *disk_attrs[] = { &dev_attr_range.attr, &dev_attr_ext_range.attr, @@ -1035,6 +1041,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_trigger.attr, #endif NULL }; -- 2.31.1
* Don't wait to lock the device's LED trigger mutex; just don't blink the LED this time if it can't be locked right away, i.e. if mutex_trylock() fails. Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/blk-ledtrig.c | 24 ++++++++++++++++++++++++ block/blk-ledtrig.h | 9 +++++++++ block/blk-mq.c | 2 ++ 3 files changed, 35 insertions(+) diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c index 2d324df45149..1b475530ce6c 100644 --- a/block/blk-ledtrig.c +++ b/block/blk-ledtrig.c @@ -544,3 +544,27 @@ ssize_t blk_ledtrig_devattr_show(struct device *const dev, return (ssize_t)(name_len + 1); } + + +/* + * + * Try to blink an LED + * + */ + +void __blk_ledtrig_try_blink(struct gendisk *const gd) +{ + if (mutex_trylock(&gd->ledtrig_mutex)) { + + if (gd->ledtrig != NULL) { + + unsigned long delay_on = 75; + unsigned long delay_off = 25; + + led_trigger_blink_oneshot(&gd->ledtrig->trigger, + &delay_on, &delay_off, 0); + } + + mutex_unlock(&gd->ledtrig_mutex); + } +} diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h index 5d228905edbf..146deda92a8e 100644 --- a/block/blk-ledtrig.h +++ b/block/blk-ledtrig.h @@ -27,10 +27,19 @@ ssize_t blk_ledtrig_devattr_show(struct device *const dev, struct device_attribute *const attr, char *const buf); +void __blk_ledtrig_try_blink(struct gendisk *gd); + +static inline void blk_ledtrig_try_blink(struct gendisk *const gd) +{ + if (gd != NULL) + __blk_ledtrig_try_blink(gd); +} + #else // CONFIG_BLK_LED_TRIGGERS static inline void blk_ledtrig_init(void) {} static inline void blk_ledtrig_disk_init(const struct gendisk *gd) {} +static inline void blk_ledtrig_try_blink(const struct gendisk *gd) {} // Real function (declared in include/linux/blk-ledtrig.h) returns a bool. // This is only here for del_gendisk() (in genhd.c), which doesn't check diff --git a/block/blk-mq.c b/block/blk-mq.c index 2c4ac51e54eb..5593ece7b676 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->rq_disk); break; case BLK_STS_RESOURCE: case BLK_STS_DEV_RESOURCE: -- 2.31.1
Hi!
This patch series adds configurable (i.e. user-defined) block device LED triggers.
* Triggers can be created, listed, and deleted via sysfs block class attributes (led_trigger_{new,list,del}).
* Once created, block device LED triggers are associated with LEDs just like any other LED trigger (via /sys/class/leds/${LED}/trigger).
* Each block device gains a new device attribute (led_trigger) that can be used to associate the device with a trigger or clear its association.
That's not how this is normally done. We normally have a trigger ("block device activity") which can then expose parameters ("I watch for read" and "I monitor sda1"). Is there a reason normal solution can not be used? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
On 7/29/21 3:54 AM, Pavel Machek wrote:
We normally have a trigger ("block device activity") which can then expose parameters ("I watch for read" and "I monitor sda1").
Is there a reason normal solution can not be used?
This big difference is that this allows different devices to drive different LEDs. For example, my NAS has 5 drive bays, each of which has its own activity LED. With these patches, I can create a separate trigger for each of those LEDs and associate the drive in each bay with the correct LED: sdb --> trigger1 --> LED1 ⋮ ⋮ ⋮ sdf --> trigger5 --> LED5 (sda is the SATA DOM boot drive.) Note that this also supports associating multiple devices with a single trigger, so it can be used for more complicated schemes. For example, if my NAS had an additional LED and an optical drive, I could do this: sr0 --+ | +--> trigger0 --> LED0 | sda --+ sdb -----> trigger1 --> LED1 ⋮ ⋮ ⋮ sdf -----> trigger5 --> LED5 As far as I know, the current triggers (disk-activity, disk-read, disk-write, and ide-disk) don't support this sort of arbitrary device-trigger association. This patch set also support triggering LEDs from pretty much any block device (virtual as well as physical), not just ATA devices, although that's just a matter of the place from which the trigger is "fired". I hope this explains things. Thanks! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Thu 2021-07-29 12:03:04, Ian Pilcher wrote:
On 7/29/21 3:54 AM, Pavel Machek wrote:
We normally have a trigger ("block device activity") which can then expose parameters ("I watch for read" and "I monitor sda1").
Is there a reason normal solution can not be used?
This big difference is that this allows different devices to drive different LEDs. For example, my NAS has 5 drive bays, each of which has its own activity LED. With these patches, I can create a separate trigger for each of those LEDs and associate the drive in each bay with the correct LED:
Yes, and I'd like to have that functionality, but I believe userland API should be similar to what we do elsewhere. Marek described it in more details. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
On 7/29/21 1:35 PM, Pavel Machek wrote:
Yes, and I'd like to have that functionality, but I believe userland API should be similar to what we do elsewhere. Marek described it in more details.
On 7/29/21 6:59 AM, Marek Behún wrote: ...
- only one trigger, with name "blkdev"
I guess I'm missing something, because I just don't understand how this can work for multiple, per-device LEDs. -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
participants (6)
-
Greg KH -
Greg KH -
Ian Pilcher -
Marek Behún -
Pavel Machek -
Valdis Klētnieks