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
* 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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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
* 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 <[email protected]>
---
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
* New file - include/linux/blk-ledtrig.h
Signed-off-by: Ian Pilcher <[email protected]>
---
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 <[email protected]>
*/
+#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 <[email protected]>
+ */
+
+#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
* 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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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
* Initialize trigger info when device is added.
Signed-off-by: Ian Pilcher <[email protected]>
---
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 <[email protected]>
---
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
* 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 <[email protected]>
---
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
* Set/store functions in block/blk-ledtrig.c
* Add device attributes to disk_attrs in block/genhd.c
Signed-off-by: Ian Pilcher <[email protected]>
---
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
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 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 Wed, 28 Jul 2021 20:53:39 -0500, Ian Pilcher said:
> * New file - include/linux/blk-ledtrig.h
>
> Signed-off-by: Ian Pilcher <[email protected]>
> ---
> 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 Wed, Jul 28, 2021 at 08:53:39PM -0500, Ian Pilcher wrote:
> * New file - include/linux/blk-ledtrig.h
>
> Signed-off-by: Ian Pilcher <[email protected]>
> ---
> 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 <[email protected]>
> */
>
> +#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
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 <[email protected]>
> ---
> 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
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
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
Dear Ian,
On Wed, 28 Jul 2021 20:53:37 -0500
Ian Pilcher <[email protected]> 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 <[email protected]>
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
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 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!
========================================================================
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 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 7/29/21 6:59 AM, Marek Behún wrote:
> I don't really see the purpose for having multiple different block
> device LED triggers.
Is there a different/better way to control per-device LEDs? (I'm
thinking of something like my NAS, which has 5 drive bays, each with
its own activity LED.)
> Moreover we really do not want userspace to be
> able to add LED triggers with arbitrary names, and as many as the
> userspace wants.
To be slightly flippant, why not? "Userspace" in this case is the
system/device administrator. They presumably know what LEDs they have
and what they want to use them for, something which the kernel cannot
know (assuming a "generic" disto kernel).
> There is no sense in making userspace be able to
> create 10000 triggers.
It would certainly be possible to impose a limit on the number of
triggers that could be created. But then someone has to decide what
that limit should be. Personally, I lean very much toward giving the
system administrator the freedom to configure their system as they see
fit, even if that means that they can break it. (Where "break"
basically means that they need to reboot.)
> 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.
Would adding a prefix to the LED trigger name address your concern
about arbitrary names and potential conflicts? I.e. the system
administrator creates a block device LED trigger named "foo", and it
shows up as an LED trigger named "blkdev:foo" (or something like that).
> 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
How would you support multiple, per-device LEDs (the NAS use case above)
in this scheme?
> - 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
Is there a reason that you prefer this approach to simply having the
block layer "fire" the trigger?
Thanks for the feedback!
--
========================================================================
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!
========================================================================
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