LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RESEND PATCH v5 0/2] Introduce block device LED trigger
@ 2021-10-04 15:55 Ian Pilcher
  2021-10-04 15:55 ` [RESEND PATCH v5 1/2] docs: Add block device (blkdev) LED trigger documentation Ian Pilcher
  2021-10-04 15:55 ` [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger Ian Pilcher
  0 siblings, 2 replies; 7+ messages in thread
From: Ian Pilcher @ 2021-10-04 15:55 UTC (permalink / raw)
  To: pavel; +Cc: linux-leds, linux-kernel, gregkh, kabel, hch

Changes from v4:
================

* Use xarrays, rather than lists, to model "links" between LEDs and block
  devices.  This allows many-to-many relationships without the need for a
  separate link object.

* When resolving (getting) a block device by path, don't retry with
  "/dev/" prepended to the path in the ENOENT case.

* Use an enum, rather than a boolean, to tell led_bdev_unlink() whether
  the block device is being released or not.

* Use preprocessor constant, rather than magic number, for the mode passed
  to blkdev_get_by_path() and blkdev_put().

* Split the data structure used by mode attribute show & store functions
  into 2 separate arrays and move them into the functions that use them.

Changes from v3:
================

* Use blkdev_get_by_path() to "resolve" block devices
  (struct block_device).  With this change, there are now no changes
  required to the block subsystem, so there are only 2 patches in this
  series.

* link_device and unlink_device attributes now take paths to block device
  special files (e.g. /dev/sda), rather than kernel names.  Symbolic
  links also work.

  If the path written to the attribute doesn't exist (-ENOENT), we re-try
  with /dev/ prepended, so "simple" names like sda will still work as long
  as the corresponding special file exists in /dev.

* Fixed a bug that could cause "phantom" blinks because of old device
  activity that was not recognized at the correct time.

* (Slightly) more detailed commit message for the patch that adds the
  trigger code.  As with v3, the real details are found in the comments
  in the source file.

Changes from v2:
================

* Allow LEDs to be "linked" to partitions, as well as whole devices.
  Internally, the trigger now works with block_device structs, rather
  than gendisk structs.

  (Investigating the lifecycle of block_device structs led me to
  discover the device resource API, so ...)

* Use the device resource API to manage the trigger's per-block device
  data structure (struct led_bdev_bdi).  The trigger now uses a release
  function to remove references to block devices that have been removed.

  Because the release function is automatically called by the driver core,
  there is no longer any need for the block layer to explictly call the
  trigger's cleanup function.

* Since there is no need to provide a built-in "stub" cleanup function
  when the trigger is built as a module, I have removed the always
  built-in "core" portion of the trigger.

* Without a built-in component, the module does need access to the
  block_class symbol.  The second patch in this series exports the symbol
  to the LEDTRIG_BLKDEV namespace and explains the reason for doing so.

* Changed the interval sysfs attribute from a device attribute to a class
  attribute.  It's single value that applies to all LEDs, so it didn't
  make sense as a device atribute.

* As requested, I am posting the trigger code (ledtrig-blkdev.c) as a
  single patch.  This eliminates the commit messages that would otherwise
  describe sections of the code, so I have added fairly extensive comments
  to each function.

Changes from v1:
================

* Use correct address for LKML.

* Renamed the sysfs attributes used to manage and view the set of block
  devices associated ("linked") with an LED.

  - /sys/class/leds/<LED>/link_device to create associations

  - /sys/class/leds/<LED>/unlink_device to remove associations

  - /sys/class/leds/<LED>/linked_devices/ contains symlinks to all block
    devices associated with the LED

  - /sys/block/<DEVICE>/linked_leds (which only exists when the device is
    associated with at least one LED) contains symlinks to all LEDs with
    which the device is associated

  link_device and unlink_device are write-only attributes, each of which
  represents a single action, rather than any state.  (The current state
  is shown by the symbolic links in the <LED>/linked_devices/ and
  <DEVICE>/linked_leds/ directories.)

* Simplified sysfs attribute store functions.  link_device and
  unlink_device no longer accept multiple devices at once, but this was
  really just an artifact of the way that sysfs repeatedly calls the
  store function when it doesn't "consume" all of its input, and it
  seemed to be confusing and unpopular anyway.

* Use DEVICE_ATTR_* macros (rather than __ATTR) for the sysfs attributes.

* Removed all pr_info() "system administrator error" messages.

* Different minimum values for LED blink time (10 ms) and activity check
  interval (25 ms).

v1 summary:
===========

This patch series adds a new "blkdev" LED trigger for disk (or other block
device) activity LEDs.

It has the following functionality.

* Supports all types of block devices, including virtual devices
  (unlike the existing disk trigger which only works with ATA devices).

* LEDs can be configured to show read activity, write activity, or both.

* Supports multiple devices and multiple LEDs in arbitrary many-to-many
  configurations.  For example, it is possible to configure multiple
  devices with device-specific read activity LEDs and a shared write
  activity LED.  (See Documentation/leds/ledtrig-blkdev.rst in the first
  patch.)

* Doesn't add any overhead in the I/O path.  Like the netdev LED trigger,
  it periodically checks the configured devices for activity and blinks
  its LEDs as appropriate.

* Blink duration (per LED) and interval between activity checks (global)
  are configurable.

* Requires minimal changes to the block subsystem.

  - Adds 1 pointer to struct gendisk,

  - Adds (inline function) call in device_add_disk() to ensure that the
    pointer is initialized to NULL (as protection against any drivers
    that allocate a gendisk themselves and don't use kzalloc()), and

  - Adds call in del_gendisk() to remove a device from the trigger when
    that device is being removed.

  These changes are all in patch #4, "block: Add block device LED trigger
  integrations."

* The trigger can be mostly built as a module.

  When the trigger is modular, a small portion is built in to provide a
  "stub" function which can be called from del_gendisk().  The stub calls
  into the modular code via a function pointer when needed.  The trigger
  also needs the ability to find gendisk's by name, which requires access
  to the un-exported block_class and disk_type symbols.

Ian Pilcher (2):
  docs: Add block device (blkdev) LED trigger documentation
  leds: trigger: Add block device LED trigger

 Documentation/ABI/testing/sysfs-block         |   9 +
 .../testing/sysfs-class-led-trigger-blkdev    |  48 +
 Documentation/leds/index.rst                  |   1 +
 Documentation/leds/ledtrig-blkdev.rst         | 148 +++
 drivers/leds/trigger/Kconfig                  |   9 +
 drivers/leds/trigger/Makefile                 |   1 +
 drivers/leds/trigger/ledtrig-blkdev.c         | 965 ++++++++++++++++++
 7 files changed, 1181 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
 create mode 100644 Documentation/leds/ledtrig-blkdev.rst
 create mode 100644 drivers/leds/trigger/ledtrig-blkdev.c


base-commit: 9e1ff307c779ce1f0f810c7ecce3d95bbae40896
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RESEND PATCH v5 1/2] docs: Add block device (blkdev) LED trigger documentation
  2021-10-04 15:55 [RESEND PATCH v5 0/2] Introduce block device LED trigger Ian Pilcher
@ 2021-10-04 15:55 ` Ian Pilcher
  2021-10-04 15:55 ` [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger Ian Pilcher
  1 sibling, 0 replies; 7+ messages in thread
From: Ian Pilcher @ 2021-10-04 15:55 UTC (permalink / raw)
  To: pavel; +Cc: linux-leds, linux-kernel, gregkh, kabel, hch

Add Documentation/ABI/testing/sysfs-class-led-trigger-blkdev to
document:

  * /sys/class/leds/<led>/blink_time
  * /sys/class/leds/<led>/mode
  * /sys/class/leds/<led>/link_device
  * /sys/class/leds/<led>/unlink_device
  * /sys/class/leds/<led>/linked_devices
  * /sys/class/ledtrig_blkdev/interval

Add /sys/block/<disk>/linked_leds to
Documentation/ABI/testing/sysfs-block.

Add overview in Documentation/leds/ledtrig-blkdev.rst.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 Documentation/ABI/testing/sysfs-block         |   9 ++
 .../testing/sysfs-class-led-trigger-blkdev    |  48 ++++++
 Documentation/leds/index.rst                  |   1 +
 Documentation/leds/ledtrig-blkdev.rst         | 148 ++++++++++++++++++
 4 files changed, 206 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
 create mode 100644 Documentation/leds/ledtrig-blkdev.rst

diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
index a0ed87386639..26931894084b 100644
--- a/Documentation/ABI/testing/sysfs-block
+++ b/Documentation/ABI/testing/sysfs-block
@@ -328,3 +328,12 @@ Description:
 		does not complete in this time then the block driver timeout
 		handler is invoked. That timeout handler can decide to retry
 		the request, to fail it or to start a device recovery strategy.
+
+What:		/sys/block/<disk>/linked_leds
+Date:		October 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Directory containing links to all LEDs that are associated
+		with this block device through the blkdev LED trigger.  Only
+		present when at least one LED is associated.  (See
+		Documentation/leds/ledtrig-blkdev.rst.)
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-blkdev b/Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
new file mode 100644
index 000000000000..292c581e2cda
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
@@ -0,0 +1,48 @@
+What:		/sys/class/leds/<led>/blink_time
+Date:		October 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Time (in milliseconds) that the LED will be on during a single
+		"blink".
+
+What:		/sys/class/leds/<led>/mode
+Date:		October 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Type of events for which LED will blink - read, write,
+		or rw (both).  Note that any activity that changes the state of
+		the device's non-volatile storage (including discards and cache
+		flushes) is considered to be a write.
+
+What:		/sys/class/leds/<led>/link_device
+Date:		October 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Associate a block device with this LED by writing the path to
+		the device special file (e.g. /dev/sda) to this attribute.
+		Symbolic links are followed.
+
+What:		/sys/class/leds/<led>/unlink_device
+Date:		October 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Remove the association between this LED and a block device by
+		writing the path to the device special file (e.g. /dev/sda) to
+		this attribute.  Symbolic links are followed.
+
+What:		/sys/class/leds/<led>/linked_devices
+Date:		October 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Directory containing links to all block devices that are
+		associated with this LED.  (Note that the names of the
+		symbolic links in this directory are *kernel* names, which
+		may not match the device special file paths written to
+		link_device and unlink_device.)
+
+What:		/sys/class/ledtrig_blkdev/interval
+Date:		October 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Frequency (in milliseconds) with which block devices associated
+		with the blkdev LED trigger will be checked for activity.
diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
index e5d63b940045..e3c24e468cbc 100644
--- a/Documentation/leds/index.rst
+++ b/Documentation/leds/index.rst
@@ -10,6 +10,7 @@ LEDs
    leds-class
    leds-class-flash
    leds-class-multicolor
+   ledtrig-blkdev
    ledtrig-oneshot
    ledtrig-transient
    ledtrig-usbport
diff --git a/Documentation/leds/ledtrig-blkdev.rst b/Documentation/leds/ledtrig-blkdev.rst
new file mode 100644
index 000000000000..5be141add33f
--- /dev/null
+++ b/Documentation/leds/ledtrig-blkdev.rst
@@ -0,0 +1,148 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=================================
+Block Device (blkdev) LED Trigger
+=================================
+
+Available when ``CONFIG_LEDS_TRIGGER_BLKDEV=y`` or
+``CONFIG_LEDS_TRIGGER_BLKDEV=m``.
+
+See also:
+
+* ``Documentation/ABI/testing/sysfs-class-led-trigger-blkdev``
+* ``Documentation/ABI/testing/sysfs-block`` (``/sys/block/<disk>/linked_leds``)
+
+Overview
+========
+
+.. note::
+	The examples below use ``<LED>`` to refer to the name of a
+	system-specific LED.  If no suitable LED is available on a test
+	system (in a virtual machine, for example), it is possible to
+	use a userspace LED.  (See ``Documentation/leds/uleds.rst``.)
+
+Verify that the ``blkdev`` LED trigger is available::
+
+	# grep blkdev /sys/class/leds/<LED>/trigger
+	... rfkill-none blkdev
+
+(If the previous command produces no output, you may need to load the trigger
+module - ``modprobe ledtrig_blkdev``.  If the module is not available, check
+the value of ``CONFIG_LEDS_TRIGGER_BLKDEV`` in your kernel configuration.)
+
+Associate the LED with the ``blkdev`` LED trigger::
+
+	# echo blkdev > /sys/class/leds/<LED>/trigger
+
+	# cat /sys/class/leds/<LED>/trigger
+	... rfkill-none [blkdev]
+
+Note that several new device attributes are available in the
+``/sys/class/leds/<LED>`` directory.
+
+* ``link_device`` and ``unlink_device`` are used to manage the set of block
+  devices associated with this LED.  The LED will blink in response to read or
+  write activity on its linked devices.
+
+* ``mode`` is used to control the type of device activity that will cause this
+  LED to blink - read activity, write activity, or both.  (Note that any
+  activity that changes the state of a device's non-volatile storage is
+  considered to be a write.  This includes discard and cache flush requests.)
+
+* ``blink_time`` is the duration (in milliseconds) of each blink of this LED.
+  (The minimum value is 10 milliseconds.)
+
+* The ``linked_devices`` directory will contain a symbolic link to every device
+  that is associated with this LED.
+
+Link a block device to the LED::
+
+	# echo /dev/sda > /sys/class/leds/<LED>/link_device
+
+	# ls /sys/class/leds/<LED>/linked_devices
+	sda
+
+(The value written to ``link_device`` must be the path of the device special
+file, such as ``/dev/sda``, that represents the block device - or the path of a
+symbolic link to such a device special file.)
+
+Read and write activity on the device should cause the LED to blink.  The
+duration of each blink (in milliseconds) can be adjusted by setting
+``/sys/class/leds/<LED>/blink_time``.  (But see **interval and blink_time**
+below.)
+
+Associate a second device with the LED::
+
+	# echo /dev/sdb > /sys/class/leds/<LED>/link_device
+
+	# ls /sys/class/leds/<LED>/linked_devices
+	sda  sdb
+
+When a block device is linked to one or more LEDs, the LEDs are linked from
+the device's ``linked_leds`` directory::
+
+	# ls /sys/class/block/sd{a,b}/linked_leds
+	/sys/class/block/sda/linked_leds:
+	<LED>
+
+	/sys/class/block/sdb/linked_leds:
+	<LED>
+
+(The ``linked_leds`` directory only exists when the block device is linked to
+at least one LED.)
+
+``interval`` and ``blink_time``
+===============================
+
+* By default, linked block devices are checked for activity every 100
+  milliseconds.  This frequency can be changed via the
+  ``/sys/class/ledtrig_blkdev/interval`` attribute.  (The minimum value is 25
+  milliseconds.)
+
+* All associated devices are checked for activity every ``interval``
+  milliseconds, and a blink is triggered on appropriate LEDs.  The duration
+  of an LED's blink is determined by its ``blink_time`` attribute.  Thus
+  (assuming that activity of the relevant type has occurred on one of an LED's
+  linked devices), the LED will be on for ``blink_time`` milliseconds and off
+  for ``interval - blink_time`` milliseconds.
+
+* The LED subsystem ignores new blink requests for an LED that is already in
+  in the process of blinking, so setting a ``blink_time`` greater than or equal
+  to ``interval`` will cause some blinks to be missed.
+
+* Because of processing times, scheduling latencies, etc., avoiding missed
+  blinks actually requires a difference of at least a few milliseconds between
+  the ``blink_time`` and ``interval``.  The required difference is likely to
+  vary from system to system.  As a  reference, a Thecus N5550 NAS requires a
+  difference of 7 milliseconds (``interval == 100``, ``blink_time == 93``).
+
+* The default values (``interval == 100``, ``blink_time == 75``) cause the LED
+  associated with a continuously active device to blink rapidly.  For a more
+  "always on" effect, increase the ``blink_time`` (but not too much; see the
+  previous bullet).
+
+Other Notes
+===========
+
+* Many (possibly all) types of block devices work with this trigger, including:
+
+  * SCSI (including SATA and USB) hard disk drives and SSDs
+  * SCSI (including SATA and USB) optical drives
+  * NVMe SSDs
+  * SD cards
+  * loopback block devices (``/dev/loop*``)
+  * device mapper devices, such as LVM logical volumes
+  * MD RAID devices
+  * zRAM compressed RAM-disks
+  * partitions on block devics that support them
+
+* The names of the symbolic links in ``/sys/class/leds/<LED>/linked_devices``
+  are **kernel** names, which may not match the paths used for
+  ``link_device`` and ``unlink_device``.  This is most likely when a symbolic
+  link is used to refer to the device (as is common with logical volumes), but
+  it can be true for any device, because nothing prevents the creation of
+  device special files with arbitrary names (e.g. ``sudo mknod /foo b 8 0``).
+
+* The ``blkdev`` LED trigger supports many-to-many device/LED associations.
+  A device can be associated with multiple LEDs, and an LED can be associated
+  with multiple devices.
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger
  2021-10-04 15:55 [RESEND PATCH v5 0/2] Introduce block device LED trigger Ian Pilcher
  2021-10-04 15:55 ` [RESEND PATCH v5 1/2] docs: Add block device (blkdev) LED trigger documentation Ian Pilcher
@ 2021-10-04 15:55 ` Ian Pilcher
  2021-10-05 21:27   ` Marek Behún
  1 sibling, 1 reply; 7+ messages in thread
From: Ian Pilcher @ 2021-10-04 15:55 UTC (permalink / raw)
  To: pavel; +Cc: linux-leds, linux-kernel, gregkh, kabel, hch

Add "blkdev" LED trigger to blink LEDs in response to block device
activity.

Add LEDS_TRIGGER_BLKDEV (tristate) config option to control building of
the trigger.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/Kconfig          |   9 +
 drivers/leds/trigger/Makefile         |   1 +
 drivers/leds/trigger/ledtrig-blkdev.c | 965 ++++++++++++++++++++++++++
 3 files changed, 975 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-blkdev.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 1f1d57288085..219d15c046d7 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -153,4 +153,13 @@ config LEDS_TRIGGER_TTY
 
 	  When build as a module this driver will be called ledtrig-tty.
 
+config LEDS_TRIGGER_BLKDEV
+	tristate "LED Trigger for block devices"
+	depends on BLOCK
+	help
+	  The blkdev LED trigger allows LEDs to be controlled by block device
+	  activity (reads and writes).
+
+	  See Documentation/leds/ledtrig-blkdev.rst.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 25c4db97cdd4..d53bab5d93f1 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
 obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
+obj-$(CONFIG_LEDS_TRIGGER_BLKDEV)	+= ledtrig-blkdev.o
diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
new file mode 100644
index 000000000000..feead76c2e9d
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -0,0 +1,965 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ *	Block device LED trigger
+ *
+ *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
+ */
+
+#include <linux/blkdev.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/part_stat.h>
+#include <linux/xarray.h>
+
+/* Default blink time & check interval (milliseconds) */
+#define LED_BDEV_BLINK_MSEC	75
+#define LED_BDEV_INTERVAL	100
+
+/* Minimum blink time & check interval (milliseconds) */
+#define LED_BDEV_MIN_BLINK	10
+#define LED_BDEV_MIN_INT	25
+
+/* Mode for blkdev_get_by_path() & blkdev_put() */
+#define LED_BDEV_FMODE		0
+
+/* Device activity type that will make LED blink */
+enum led_bdev_led_mode {
+	LED_BDEV_MODE_RO	= 0,
+	LED_BDEV_MODE_WO	= 1,
+	LED_BDEV_MODE_RW	= 2
+};
+
+/* When unlinking a block device from an LED, is the blkdev being released? */
+enum led_bdev_unlink_mode {
+	LED_BDEV_RELEASING,
+	LED_BDEV_NOT_RELEASING
+};
+
+/* An "LED block device" (LBD) - 1 per blkdev linked to at least 1 LED */
+struct led_bdev_lbd {
+	unsigned long		index;
+	struct block_device	*bdev;
+	struct xarray		linked_leds;
+	unsigned long		read_ios;
+	unsigned long		write_ios;
+	unsigned int		generation;
+	bool			read_act;
+	bool			write_act;
+};
+
+/* Every LED associated with the blkdev trigger gets one of these */
+struct led_bdev_led {
+	unsigned long		index;
+	struct led_classdev	*led_cdev;
+	unsigned int		blink_msec;
+	struct xarray		linked_lbds;
+	struct hlist_node	all_leds_node;
+	enum led_bdev_led_mode	mode;
+};
+
+/* Forward declarations to make this file compile in a more readable order */
+static void led_bdev_process(struct work_struct *work);
+static struct led_bdev_lbd *led_bdev_get_lbd(const char *buf, size_t size);
+static struct block_device *led_bdev_get_bdev(const char *buf, size_t size,
+					      fmode_t mode);
+static int led_bdev_link(struct led_bdev_led *led, struct led_bdev_lbd *lbd);
+static void led_bdev_put_lbd(struct led_bdev_lbd *lbd);
+static void led_bdev_lbd_release(struct device *dev, void *res);
+static void led_bdev_unlink(struct led_bdev_led *led,
+			    struct led_bdev_lbd *lbd,
+			    enum led_bdev_unlink_mode unlink_mode);
+static void led_bdev_update_lbd(struct led_bdev_lbd *lbd);
+static bool led_bdev_blink(const struct led_bdev_led *led,
+			   const struct led_bdev_lbd *lbd);
+
+/* Index for next LBD or LED */
+static unsigned long led_bdev_next_index;
+
+/* Protects everything except atomic sysfs attributes */
+static DEFINE_MUTEX(led_bdev_mutex);
+
+/* All LEDs associated with the trigger */
+static HLIST_HEAD(led_bdev_all_leds);
+
+/* sysfs class for "global" trigger attributes (interval) */
+static struct class *led_bdev_class;
+
+/* Delayed work to periodically check for activity & blink LEDs */
+static DECLARE_DELAYED_WORK(led_bdev_work, led_bdev_process);
+
+/* How often to run the delayed work - in jiffies */
+static unsigned int led_bdev_interval;
+
+/* Incremented every time the delayed work runs */
+static unsigned int led_bdev_generation;
+
+/* Total number of device-to-LED associations (links) */
+static unsigned int led_bdev_link_count;
+
+/* Empty attribute list for the linked_leds & linked_devices "groups" */
+static struct attribute *led_bdev_attrs_empty[] = { NULL };
+
+/* linked_leds sysfs directory for block devs linked to 1 or more LEDs */
+static const struct attribute_group led_bdev_linked_leds = {
+	.name	= "linked_leds",
+	.attrs	= led_bdev_attrs_empty,
+};
+
+/* linked_devices sysfs directory for each LED associated with the trigger */
+static const struct attribute_group led_bdev_linked_devs = {
+	.name	= "linked_devices",
+	.attrs	= led_bdev_attrs_empty,
+};
+
+/**
+ * led_bdev_activate() - Called when an LED is associated with the trigger.
+ * @led_cdev:	The LED
+ *
+ * Allocates & initializes the @led_bdev_led structure, adds it to the
+ * @led_bdev_all_leds list, and sets the LED's trigger data.
+ *
+ * Context:	Process context.  Takes and releases @led_bdev_mutex.
+ * Return:	``0`` on success, ``-errno`` on error.
+ */
+static int led_bdev_activate(struct led_classdev *const led_cdev)
+{
+	struct led_bdev_led *led;
+	int ret;
+
+	if (led_bdev_next_index == ULONG_MAX) {
+		ret = -EOVERFLOW;
+		goto exit_return;
+	}
+
+	led = kmalloc(sizeof(*led), GFP_KERNEL);
+	if (led == NULL) {
+		ret = -ENOMEM;
+		goto exit_return;
+	}
+
+	led->index = led_bdev_next_index++;
+	led->led_cdev = led_cdev;
+	led->blink_msec = LED_BDEV_BLINK_MSEC;
+	led->mode = LED_BDEV_MODE_RW;
+	xa_init(&led->linked_lbds);
+
+	ret = mutex_lock_interruptible(&led_bdev_mutex);
+	if (ret != 0)
+		goto exit_free;
+
+	hlist_add_head(&led->all_leds_node, &led_bdev_all_leds);
+	led_set_trigger_data(led_cdev, led);
+
+	mutex_unlock(&led_bdev_mutex);
+
+exit_free:
+	if (ret != 0)
+		kfree(led);
+exit_return:
+	return ret;
+}
+
+/**
+ * link_device_store() - ``link_device`` device attribute store function.
+ * @dev:	The LED device
+ * @attr:	The ``link_device`` attribute (@dev_attr_link_device)
+ * @buf:	The value written to the attribute, which should be the path to
+ *		the special file that represents the block device to be linked
+ *		to the LED (e.g. /dev/sda)
+ * @count:	The number of characters in @buf
+ *
+ * Calls led_bdev_get_lbd() to find or create the LBD for the block device,
+ * checks that the device isn't already linked to this LED, and calls
+ * led_bdev_link() to create the link.
+ *
+ * Context:	Process context.  Takes and releases @led_bdev_mutex.
+ * Return:	@count on success, ``-errno`` on error.
+ */
+static ssize_t link_device_store(struct device *const dev,
+				 struct device_attribute *const attr,
+				 const char *const buf, const size_t count)
+{
+	struct led_bdev_led *const led = led_trigger_get_drvdata(dev);
+	struct led_bdev_lbd *lbd;
+	int ret;
+
+	ret = mutex_lock_interruptible(&led_bdev_mutex);
+	if (ret != 0)
+		goto exit_return;
+
+	lbd = led_bdev_get_lbd(buf, count);
+	if (IS_ERR(lbd)) {
+		ret = PTR_ERR(lbd);
+		goto exit_unlock;
+	}
+
+	if (xa_load(&lbd->linked_leds, led->index) != NULL) {
+		ret = -EEXIST;
+		goto exit_put_lbd;
+	}
+
+	ret = led_bdev_link(led, lbd);
+
+exit_put_lbd:
+	if (ret != 0)
+		led_bdev_put_lbd(lbd);
+exit_unlock:
+	mutex_unlock(&led_bdev_mutex);
+exit_return:
+	return (ret == 0) ? count : ret;
+}
+
+/**
+ * led_bdev_get_lbd() - Find or create the LBD for a block device.
+ * @buf:	The value written to the ``link_device`` attribute, which should
+ *		be the path to a special file that represents a block device
+ * @count:	The number of characters in @buf
+ *
+ * Calls led_bdev_get() to get the block device represented by the path in @buf.
+ * If the device already has an LBD (because it is already linked to an LED),
+ * simply returns the existing LBD.
+ *
+ * Otherwise, allocates a new LBD (as a device resource), creates the block
+ * device's ``linked_leds`` directory (attribute group), calls
+ * led_bdev_update_lbd() to set the LBD's activity counters, and adds the LBD
+ * resource to the block device.
+ *
+ * Context:	Process context.  Caller must hold @led_bdev_mutex.
+ * Return:	Pointer to the LBD, error pointer on error.
+ */
+static struct led_bdev_lbd *led_bdev_get_lbd(const char *const buf,
+					     const size_t count)
+{
+	struct block_device *bdev;
+	struct led_bdev_lbd *lbd;
+	int ret;
+
+	bdev = led_bdev_get_bdev(buf, count, LED_BDEV_FMODE);
+	if (IS_ERR(bdev)) {
+		ret = PTR_ERR(bdev);
+		goto exit_return;
+	}
+
+	lbd = devres_find(&bdev->bd_device, led_bdev_lbd_release, NULL, NULL);
+	if (lbd != NULL) {
+		ret = 0;
+		goto exit_put_bdev;
+	}
+
+	if (led_bdev_next_index == ULONG_MAX) {
+		ret = -EOVERFLOW;
+		goto exit_put_bdev;
+	}
+
+	lbd = devres_alloc(led_bdev_lbd_release, sizeof(*lbd), GFP_KERNEL);
+	if (lbd == NULL) {
+		ret = -ENOMEM;
+		goto exit_put_bdev;
+	}
+
+	ret = sysfs_create_group(bdev_kobj(bdev), &led_bdev_linked_leds);
+	if (ret != 0)
+		goto exit_free_lbd;
+
+	lbd->index = led_bdev_next_index++;
+	lbd->bdev = bdev;
+	xa_init(&lbd->linked_leds);
+
+	/* Ensure that led_bdev_update_lbd() updates the LBD */
+	lbd->generation = led_bdev_generation - 1;
+	led_bdev_update_lbd(lbd);
+	/* Don't blink for activity that may have happened long ago */
+	lbd->read_act = false;
+	lbd->write_act = false;
+
+	devres_add(&bdev->bd_device, lbd);
+
+exit_free_lbd:
+	if (ret != 0)
+		devres_free(lbd);
+exit_put_bdev:
+	blkdev_put(bdev, LED_BDEV_FMODE);  /* Allow the device to be released */
+exit_return:
+	return (ret == 0) ? lbd : ERR_PTR(ret);
+}
+
+/**
+ * led_bdev_get_bdev() - Get a block device by path.
+ * @buf:	The value written to the ``link_device`` or ``unlink_device``
+ *		attribute, which should be the path to a special file that
+ *		represents a block device
+ * @count:	The number of characters in @buf (not including its terminating
+ *		null)
+ *
+ * Copies @buf to a writable buffer, trims the trailing newline (if any), and
+ * calls blkdev_get_by_path() to resolve the block device.
+ *
+ * The caller must call blkdev_put() when finished with the device.
+ *
+ * Context:	Process context.
+ * Return:	The block device, or an error pointer.
+ */
+static struct block_device *led_bdev_get_bdev(const char *const buf,
+					      const size_t count,
+					      const fmode_t mode)
+{
+	struct block_device *bdev;
+	char *path;
+
+	path = kmemdup(buf, count + 1, GFP_KERNEL);  /* include null */
+	if (path == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	if (path[count - 1] == '\n')
+		path[count - 1] = 0;
+
+	bdev = blkdev_get_by_path(path, mode, THIS_MODULE);
+	kfree(path);
+	return bdev;
+}
+
+/**
+ * led_bdev_update_lbd() - Update an LBD's activity counters for the current
+ *			   generation
+ * @lbd:	The LBD
+ *
+ * Does nothing if the LBD has already been updated during this generation.
+ *
+ * Otherwise, updates the LBD's activity counters (@read_ios & @write_ios) and
+ * @generation, and sets @read_act and/or @write_act if the corresponding
+ * counters have changed.
+ *
+ * Context:	Process context.  Caller must hold @led_bdev_mutex.
+ */
+static void led_bdev_update_lbd(struct led_bdev_lbd *const lbd)
+{
+	unsigned long read_ios, write_ios;
+
+	if (lbd->generation != led_bdev_generation) {
+
+		read_ios = part_stat_read(lbd->bdev, ios[STAT_READ]);
+
+		write_ios = part_stat_read(lbd->bdev, ios[STAT_WRITE])
+				+ part_stat_read(lbd->bdev, ios[STAT_DISCARD])
+				+ part_stat_read(lbd->bdev, ios[STAT_FLUSH]);
+
+		if (lbd->read_ios != read_ios) {
+			lbd->read_act = true;
+			lbd->read_ios = read_ios;
+		} else {
+			lbd->read_act = false;
+		}
+
+		if (lbd->write_ios != write_ios) {
+			lbd->write_act = true;
+			lbd->write_ios = write_ios;
+		} else {
+			lbd->write_act = false;
+		}
+
+		lbd->generation = led_bdev_generation;
+	}
+}
+
+/**
+ * led_bdev_link() - "Link" a block device to an LED.
+ * @led:	The LED
+ * @lbd:	The block device
+ *
+ * Called from link_device_store() to create the link between an LED and a
+ * block device.
+ *
+ *   * Adds block device symlink to LED's ``linked_devices`` directory.
+ *   * Adds LED symlink to block devices's ``linked_leds`` directory.
+ *   * Adds the LBD to the LED's @linked_lbds and adds the LED to the LBD's
+ *     @linked_leds.
+ *
+ * If the new link is the only one (i.e. no other block device/LED links
+ * already exist), schedule delayed work to periodically check for block
+ * device activity and blink LEDs.
+ *
+ * Context:	Process context.  Caller must hold @led_bdev_mutex.
+ * Return:	0 on success, ``-errno`` on error.
+ */
+static int led_bdev_link(struct led_bdev_led *const led,
+			 struct led_bdev_lbd *const lbd)
+{
+	unsigned long delay;
+	int ret;
+
+	ret = xa_insert(&lbd->linked_leds, led->index, led, GFP_KERNEL);
+	if (ret != 0)
+		goto error_return;
+
+	ret = xa_insert(&led->linked_lbds, lbd->index, lbd, GFP_KERNEL);
+	if (ret != 0)
+		goto error_erase_led;
+
+	/* /sys/class/block/<bdev>/linked_leds/<led> */
+	ret = sysfs_add_link_to_group(bdev_kobj(lbd->bdev),
+				      led_bdev_linked_leds.name,
+				      &led->led_cdev->dev->kobj,
+				      led->led_cdev->name);
+	if (ret != 0)
+		goto error_erase_lbd;
+
+	/* /sys/class/leds/<led>/linked_devices/<bdev> */
+	ret = sysfs_add_link_to_group(&led->led_cdev->dev->kobj,
+				      led_bdev_linked_devs.name,
+				      bdev_kobj(lbd->bdev),
+				      dev_name(&lbd->bdev->bd_device));
+	if (ret != 0)
+		goto error_remove_symlink;
+
+	if (led_bdev_link_count == 0) {
+		delay = READ_ONCE(led_bdev_interval);
+		WARN_ON(!schedule_delayed_work(&led_bdev_work, delay));
+	}
+
+	++led_bdev_link_count;
+
+	return 0;
+
+error_remove_symlink:
+	sysfs_remove_link_from_group(bdev_kobj(lbd->bdev),
+				     led_bdev_linked_leds.name,
+				     led->led_cdev->name);
+error_erase_lbd:
+	xa_erase(&led->linked_lbds, lbd->index);
+error_erase_led:
+	xa_erase(&lbd->linked_leds, led->index);
+error_return:
+	return ret;
+}
+
+/**
+ * unlink_device_store() - ``unlink_device`` device attribute store function.
+ * @dev:	The LED device
+ * @attr:	The ``unlink_device`` attribute (@dev_attr_unlink_device)
+ * @buf:	The value written to the attribute, which should be the path to
+ *		the special file that represents the block device to be unlinked
+ *		from the LED (e.g. /dev/sda)
+ * @count:	The number of characters in @buf
+ *
+ * Block device name is written to the attribute to "unlink" the block device
+ * from the LED.  I.e. the LED will no longer blink to show activity on that
+ * block device.
+ *
+ * Calls led_bdev_get() to get the block device represented by the path in @buf.
+ * If the device has an LBD, searches the LBD's list of LEDs for a link to this
+ * LED and (if found) calls led_bdev_unlink() to destroy the link.
+ *
+ * Context:	Process context.  Takes and releases @led_bdev_mutex.
+ * Return:	@count on success, ``-errno`` on error.
+ */
+static ssize_t unlink_device_store(struct device *const dev,
+				   struct device_attribute *const attr,
+				   const char *const buf, const size_t count)
+{
+	struct led_bdev_led *const led = led_trigger_get_drvdata(dev);
+	struct block_device *bdev;
+	struct led_bdev_lbd *lbd;
+	int ret;
+
+	bdev = led_bdev_get_bdev(buf, count, LED_BDEV_FMODE);
+	if (IS_ERR(bdev)) {
+		ret = PTR_ERR(bdev);
+		goto exit_return;
+	}
+
+	ret = mutex_lock_interruptible(&led_bdev_mutex);
+	if (ret != 0)
+		goto exit_put_bdev;
+
+	lbd = devres_find(&bdev->bd_device, led_bdev_lbd_release, NULL, NULL);
+	if (lbd == NULL) {
+		ret = -EUNATCH;  /* bdev isn't linked to any LED */
+		goto exit_unlock;
+	}
+
+	if (xa_load(&lbd->linked_leds, led->index) == NULL) {
+		ret = -EUNATCH;  /* bdev isn't linked to this LED */
+		goto exit_unlock;
+	}
+
+	led_bdev_unlink(led, lbd, LED_BDEV_NOT_RELEASING);
+
+exit_unlock:
+	mutex_unlock(&led_bdev_mutex);
+exit_put_bdev:
+	blkdev_put(bdev, LED_BDEV_FMODE);
+exit_return:
+	return (ret == 0) ? count : ret;
+}
+
+/**
+ * led_bdev_unlink() - "Unlink" a block device from an LED.
+ * @led:		The LED
+ * @lbd:		The block device
+ * @unlink_mode:	Indicates whether the LBD is being released (because the
+ *			block device has been removed)
+ *
+ * Removes the link between an LED and a block device.
+ *
+ *   * Removes the LBD from the LED's @linked_lbds and removes the LED from the
+ *     LBD's @linked_leds.
+ *   * Removes the block device symlink from the LED's ``linked_devices``
+ *     directory.
+ *
+ * If the block device is **not** being released:
+ *
+ *   * Removes the LED symlink from the block device's ``linked_leds``
+ *     directory.
+ *   * Calls led_bdev_put_lbd() to clean up the LBD, if required.
+ *
+ * If the removed link was the only one (i.e. there are no existing block
+ * device/LED links after its removal), cancels the periodic delayed work
+ * which checks for device activity.
+ *
+ * This function is called from multiple locations.
+ *
+ *   * unlink_device_store() calls this function when a block device is unlinked
+ *     from an LED via the ``unlink_device`` sysfs attribute.   (@unlink_mode ==
+ *     ``LED_BDEV_NOT_RELEASING``)
+ *   * led_bdev_deactivate() calls this function for each block device linked to
+ *     an LED that is being deactivated (disassociated from the trigger).
+ *     (@unlink_mode == ``LED_BDEV_NOT_RELEASING``).
+ *   * led_bdev_lbd_release() calls this function for each LED linked to a block
+ *     device that has been removed from the system.  (@unlink_mode ==
+ *     ``LED_BDEV_RELEASING).
+ *
+ * Context:	Process context.  Caller must hold @led_bdev_mutex.
+ */
+static void led_bdev_unlink(struct led_bdev_led *const led,
+			    struct led_bdev_lbd *const lbd,
+			    const enum led_bdev_unlink_mode unlink_mode)
+{
+	--led_bdev_link_count;
+
+	if (led_bdev_link_count == 0)
+		WARN_ON(!cancel_delayed_work_sync(&led_bdev_work));
+
+	xa_erase(&lbd->linked_leds, led->index);
+	xa_erase(&led->linked_lbds, lbd->index);
+
+	/* /sys/class/leds/<led>/linked_devices/<bdev> */
+	sysfs_remove_link_from_group(&led->led_cdev->dev->kobj,
+				     led_bdev_linked_devs.name,
+				     dev_name(&lbd->bdev->bd_device));
+
+	/*
+	 * If the LBD is being released, the device's attribute groups have
+	 * already been removed, and the LBD will be freed automatically.
+	 */
+	if (unlink_mode == LED_BDEV_NOT_RELEASING) {
+
+		/* /sys/class/block/<bdev>/linked_leds/<led> */
+		sysfs_remove_link_from_group(bdev_kobj(lbd->bdev),
+					     led_bdev_linked_leds.name,
+					     led->led_cdev->name);
+		led_bdev_put_lbd(lbd);
+	}
+}
+
+/**
+ * led_bdev_put_lbd() - Remove and free an LBD, if it is no longer needed.
+ * @lbd:	The LBD
+ *
+ * Does nothing if the LBD (block device) is still linked to at least one LED.
+ *
+ * If the LBD is no longer linked to any LEDs, removes the block device's
+ * ``linked_leds`` directory (attribute group), removes the LBD from the
+ * block device's resource list, and frees the LBD.
+ *
+ * Called from led_bdev_unlink() (and in the link_device_store() error path).
+ *
+ * Context:	Process context.  Caller must hold @led_bdev_mutex.
+ */
+static void led_bdev_put_lbd(struct led_bdev_lbd *const lbd)
+{
+	struct block_device *const bdev = lbd->bdev;
+	int ret;
+
+	if (xa_empty(&lbd->linked_leds)) {
+
+		sysfs_remove_group(bdev_kobj(bdev), &led_bdev_linked_leds);
+		ret = devres_destroy(&bdev->bd_device, led_bdev_lbd_release,
+				     NULL, NULL);
+		WARN_ON(ret != 0);
+	}
+}
+
+/**
+ * led_bdev_deactivate() - Called when an LED is disassociated from the
+ *			   trigger.
+ * @led_cdev:	The LED
+ *
+ * Calls led_bdev_unlink() for each block device linked to the LED, removes
+ * the LED from the @led_bdev_all_leds list, and frees the @led_bdev_led.
+ *
+ * Context:	Process context.  Takes and releases @led_bdev_mutex.
+ */
+static void led_bdev_deactivate(struct led_classdev *const led_cdev)
+{
+	struct led_bdev_led *const led = led_get_trigger_data(led_cdev);
+	struct led_bdev_lbd *lbd;
+	unsigned long index;
+
+	mutex_lock(&led_bdev_mutex);
+
+	xa_for_each (&led->linked_lbds, index, lbd)
+		led_bdev_unlink(led, lbd, LED_BDEV_NOT_RELEASING);
+
+	hlist_del(&led->all_leds_node);
+	kfree(led);
+
+	mutex_unlock(&led_bdev_mutex);
+}
+
+/**
+ * led_bdev_lbd_release() - LBD device resource release function.
+ * @dev:	The block device
+ * @res:	The LBD
+ *
+ * Called by the driver core when a block device with an LBD is removed from the
+ * system.  Calls led_bdev_unlink() for each LED linked to the block device.
+ *
+ * Context:	Process context.  Takes and releases @led_bdev_mutex.
+ */
+static void led_bdev_lbd_release(struct device *const dev, void *const res)
+{
+	struct led_bdev_lbd *const lbd = res;
+	struct led_bdev_led *led;
+	unsigned long index;
+
+	mutex_lock(&led_bdev_mutex);
+
+	xa_for_each (&lbd->linked_leds, index, led)
+		led_bdev_unlink(led, lbd, LED_BDEV_RELEASING);
+
+	mutex_unlock(&led_bdev_mutex);
+}
+
+/**
+ * led_bdev_process() - Check linked devices for activity and blink LEDs.
+ * @work:	Delayed work (@led_bdev_work)
+ *
+ * Iterates through block devices linked to LEDs and calls led_bdev_update_lbd()
+ * to update the LBD's activity counters.  If a particular LED hasn't already
+ * been blinked, calls led_bdev_blink() to blink the LED if appropriate for that
+ * device's activity.
+ *
+ * When finished, schedules itself to run again after @led_bdev_interval
+ * jiffies.
+ *
+ * Context:	Process context.  Takes and releases @led_bdev_mutex.
+ */
+static void led_bdev_process(struct work_struct *const work)
+{
+	struct led_bdev_led *led;
+	unsigned long delay;
+
+	if (!mutex_trylock(&led_bdev_mutex))
+		goto exit_reschedule;
+
+	hlist_for_each_entry (led, &led_bdev_all_leds, all_leds_node) {
+
+		struct led_bdev_lbd *lbd;
+		unsigned long index;
+		bool blinked = false;
+
+		xa_for_each (&led->linked_lbds, index, lbd) {
+
+			led_bdev_update_lbd(lbd);
+			if (!blinked)
+				blinked = led_bdev_blink(led, lbd);
+		}
+	}
+
+	++led_bdev_generation;
+
+	mutex_unlock(&led_bdev_mutex);
+
+exit_reschedule:
+	delay = READ_ONCE(led_bdev_interval);
+	WARN_ON_ONCE(!schedule_delayed_work(&led_bdev_work, delay));
+}
+
+/**
+ * led_bdev_blink() - Blink an LED, if the correct type of activity has occurred
+ *		      on the block device.
+ * @led:	The LED
+ * @lbd:	The block device
+ *
+ * Context:	Process context.  Caller must hold @led_bdev_mutex.
+ * Return:	``true`` if the LED is blinked, ``false`` if not.
+ */
+static bool led_bdev_blink(const struct led_bdev_led *const led,
+			   const struct led_bdev_lbd *const lbd)
+{
+	unsigned long delay_on, delay_off;
+
+	if ((lbd->read_act && led->mode != LED_BDEV_MODE_WO)
+			|| (lbd->write_act && led->mode != LED_BDEV_MODE_RO)) {
+
+		delay_on = READ_ONCE(led->blink_msec);
+		delay_off = 1;	/* 0 leaves LED turned on */
+
+		led_blink_set_oneshot(led->led_cdev, &delay_on, &delay_off, 0);
+		return true;
+	}
+
+	return false;
+}
+
+/**
+ * blink_time_show() - ``blink_time`` device attribute show function.
+ * @dev:	The LED device
+ * @attr:	The ``blink_time`` attribute (@dev_attr_blink_time)
+ * @buf:	Output buffer
+ *
+ * Writes the current value of the LED's @blink_msec to @buf.
+ *
+ * Context:	Process context.
+ * Return:	The number of characters written to @buf.
+ */
+static ssize_t blink_time_show(struct device *const dev,
+			       struct device_attribute *const attr,
+			       char *const buf)
+{
+	const struct led_bdev_led *const led = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", READ_ONCE(led->blink_msec));
+}
+
+/**
+ * blink_time_store() - ``blink_time`` device attribute store function.
+ * @dev:	The LED device
+ * @attr:	The ``blink_time`` attribute (@dev_attr_blink_time)
+ * @buf:	The new value (as written to the sysfs attribute)
+ * @count:	The number of characters in @buf
+ *
+ * Sets the LED's @blink_msec (the duration in milliseconds of one blink).
+ *
+ * Context:	Process context.
+ * Return:	@count on success, ``-errno`` on error.
+ */
+static ssize_t blink_time_store(struct device *const dev,
+				struct device_attribute *const attr,
+				const char *const buf, const size_t count)
+{
+	struct led_bdev_led *const led = led_trigger_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &value);
+	if (ret != 0)
+		return ret;
+
+	if (value < LED_BDEV_MIN_BLINK)
+		return -ERANGE;
+
+	WRITE_ONCE(led->blink_msec, value);
+	return count;
+}
+
+/**
+ * mode_show() - ``mode`` device attribute show function.
+ * @dev:	The LED device
+ * @attr:	The ``mode`` attribute (@dev_attr_mode)
+ * @buf:	Output buffer
+ *
+ * Writes the current value of the LED's @mode to @buf.
+ *
+ * Context:	Process context.
+ * Return:	The number of characters written to @buf.
+ */
+static ssize_t mode_show(struct device *const dev,
+			 struct device_attribute *const attr, char *const buf)
+{
+	static const char modes[][sizeof("[read] write rw\n")] = {
+		[LED_BDEV_MODE_RO] = "[read] write rw\n",
+		[LED_BDEV_MODE_WO] = "read [write] rw\n",
+		[LED_BDEV_MODE_RW] = "read write [rw]\n"
+	};
+
+	const struct led_bdev_led *const led = led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, modes[READ_ONCE(led->mode)]);
+}
+
+/**
+ * mode_store() - ``mode`` device attribute store function.
+ * @dev:	The LED device
+ * @attr:	The ``mode`` attribute (@dev_attr_mode)
+ * @buf:	The new value (as written to the sysfs attribute)
+ * @count:	The number of characters in @buf
+ *
+ * Sets the LED's @mode (``read``, ``write``, or ``rw``).
+ *
+ * Context:	Process context.
+ * Return:	@count on success, ``-errno`` on error.
+ */
+static ssize_t mode_store(struct device *const dev,
+			  struct device_attribute *const attr,
+			  const char *const buf, const size_t count)
+{
+	static const char modes[][sizeof("write")] = {
+		[LED_BDEV_MODE_RO] = "read",
+		[LED_BDEV_MODE_WO] = "write",  /* longest */
+		[LED_BDEV_MODE_RW] = "rw"
+	};
+
+	struct led_bdev_led *const led = led_trigger_get_drvdata(dev);
+	enum led_bdev_led_mode mode;
+
+	for (mode = LED_BDEV_MODE_RO; mode <= LED_BDEV_MODE_RW; ++mode) {
+
+		if (sysfs_streq(modes[mode], buf)) {
+			WRITE_ONCE(led->mode, mode);
+			return count;
+		}
+	}
+
+	return -EINVAL;
+}
+
+/**
+ * interval_show() - ``interval`` class attribute show function.
+ * @class:	The ``ledtrig_blkdev`` class (@led_bdev_class)
+ * @attr:	The ``interval`` attribute (@class_attr_interval)
+ * @buf:	Output buffer
+ *
+ * Writes the current value of @led_bdev_interval to @buf.
+ *
+ * Context:	Process context.
+ * Return:	The number of characters written to @buf.
+ */
+static ssize_t interval_show(struct class *const class,
+			     struct class_attribute *const attr,
+			     char *const buf)
+{
+	return sprintf(buf, "%u\n",
+		       jiffies_to_msecs(READ_ONCE(led_bdev_interval)));
+}
+
+/**
+ * interval_store() - ``interval`` class attribute store function
+ * @class:	The ``ledtrig_blkdev`` class (@led_bdev_class)
+ * @attr:	The ``interval`` attribute (@class_attr_interval)
+ * @buf:	The new value (as written to the sysfs attribute)
+ * @count:	The number of characters in @buf
+ *
+ * Sets @led_bdev_interval (after converting to jiffies).
+ *
+ * Context:	Process context.
+ * Return:	@count on success, ``-errno`` on error.
+ */
+static ssize_t interval_store(struct class *const class,
+			      struct class_attribute *const attr,
+			      const char *const buf, const size_t count)
+{
+	unsigned int value;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &value);
+	if (ret != 0)
+		return ret;
+
+	if (value < LED_BDEV_MIN_INT)
+		return -ERANGE;
+
+	WRITE_ONCE(led_bdev_interval, msecs_to_jiffies(value));
+
+	return count;
+}
+
+/* Device and class attributes */
+static DEVICE_ATTR_WO(link_device);
+static DEVICE_ATTR_WO(unlink_device);
+static DEVICE_ATTR_RW(blink_time);
+static DEVICE_ATTR_RW(mode);
+static CLASS_ATTR_RW(interval);
+
+/* Device attributes in LED directory (/sys/class/leds/<led>/...) */
+static struct attribute *led_bdev_attrs[] = {
+	&dev_attr_link_device.attr,
+	&dev_attr_unlink_device.attr,
+	&dev_attr_blink_time.attr,
+	&dev_attr_mode.attr,
+	NULL
+};
+
+/* Unnamed attribute group == no subdirectory */
+static const struct attribute_group led_bdev_attr_group = {
+	.attrs	= led_bdev_attrs,
+};
+
+/* Attribute groups for the trigger */
+static const struct attribute_group *led_bdev_attr_groups[] = {
+	&led_bdev_attr_group,   /* /sys/class/leds/<led>/... */
+	&led_bdev_linked_devs,  /* /sys/class/leds/<led>/linked_devices/ */
+	NULL
+};
+
+/* Trigger registration data */
+static struct led_trigger led_bdev_trigger = {
+	.name		= "blkdev",
+	.activate	= led_bdev_activate,
+	.deactivate	= led_bdev_deactivate,
+	.groups		= led_bdev_attr_groups,
+};
+
+/**
+ * led_bdev_init() - Block device LED trigger initialization.
+ *
+ * Converts default @led_bdev_interval from milliseconds to jiffies, creates
+ * the ``/sys/class/ledtrig_blkdev/interval`` attribute, and registers the LED
+ * trigger.
+ *
+ * Return:	0 on success, ``-errno`` on failure.
+ */
+static int __init led_bdev_init(void)
+{
+	int ret;
+
+	WRITE_ONCE(led_bdev_interval,
+		   msecs_to_jiffies(LED_BDEV_INTERVAL));
+
+	led_bdev_class = class_create(THIS_MODULE, "ledtrig_blkdev");
+	if (IS_ERR(led_bdev_class)) {
+		ret = PTR_ERR(led_bdev_class);
+		goto exit_return;
+	}
+
+	ret = class_create_file(led_bdev_class, &class_attr_interval);
+	if (ret != 0)
+		goto exit_destroy_class;
+
+	ret = led_trigger_register(&led_bdev_trigger);
+
+exit_destroy_class:
+	if (ret != 0)
+		class_destroy(led_bdev_class);  /* removes the attribute file */
+exit_return:
+	return ret;
+}
+module_init(led_bdev_init);
+
+/**
+ * led_bdev_exit() - Block device LED trigger module exit.
+ *
+ * Unregisters the LED trigger and removes the
+ * ``/sys/class/ledtrig_blkdev/interval`` attribute.
+ */
+static void __exit led_bdev_exit(void)
+{
+	led_trigger_unregister(&led_bdev_trigger);
+	class_destroy(led_bdev_class);
+}
+module_exit(led_bdev_exit);
+
+MODULE_DESCRIPTION("Block device LED trigger");
+MODULE_AUTHOR("Ian Pilcher <arequipeno@gmail.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger
  2021-10-04 15:55 ` [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger Ian Pilcher
@ 2021-10-05 21:27   ` Marek Behún
  2021-10-06 16:07     ` Ian Pilcher
  0 siblings, 1 reply; 7+ messages in thread
From: Marek Behún @ 2021-10-05 21:27 UTC (permalink / raw)
  To: Ian Pilcher; +Cc: pavel, linux-leds, linux-kernel, gregkh, hch

Hello Ian,

first, as I replied in one of the previous versions: it is not customary
to use the const keyword in Linux the way you do use it, i.e.
  int f(const int var)
Your argument was that this makes the code more safe when changes are
made in the future. Maybe this is true (although in my opinion it is
not), but since this isn't done anywhere in kernel AFAIK, please drop it
in this proposal. If you want to introduce it, you can try sending
proposals to other kernel subsystems. I am almost sure they will be
rejected. Also, I won't give my Reviewed-by for this patch unless you
drop it (or if Pavel or some other long-term maintainer thinks it
should be used).

Second, I really would like to solve the problem looking up the block
device, given name. Since block API does not provide such function, I
think we should try to add it. It does not make sense to not be able to
set, for example "sda1" to trigger a LED, when /sys/class/block/sda1
exists, but someone deleted the /dev/sda1 device file.

Anyway, see other comments below:

On Mon,  4 Oct 2021 10:55:46 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> +/* Device activity type that will make LED blink */
> +enum led_bdev_led_mode {
> +	LED_BDEV_MODE_RO	= 0,
> +	LED_BDEV_MODE_WO	= 1,
> +	LED_BDEV_MODE_RW	= 2
> +};

Please use a bitmap as the netdev trigger does.

> +struct led_bdev_lbd {

Could this be renamed to trig_bdev?

> +struct led_bdev_led {

And this to trig_led?

Also, the led_bdev prefixes everywhere make the code a little confusing,
IMO.
Could this prefix be changed to blkdev_trig? This would be similar to
netdev trigger.

> +/* Delayed work to periodically check for activity & blink LEDs */
> +static DECLARE_DELAYED_WORK(led_bdev_work, led_bdev_process);
>
> +/* How often to run the delayed work - in jiffies */
> +static unsigned int led_bdev_interval;

This is wrong. The interval can't be same for all triggers, this does
not make sense. I want to be able to set different intervals for
different LEDs, as in netdev trigger. Sure maybe not many people will
use it, but the possibility should be there.

The work will then also be per LED and your structures should become
more simple. You won't need to have NxM mapping between LEDs and block
devices.

> +static int led_bdev_activate(struct led_classdev *const led_cdev)
> +{
> +	struct led_bdev_led *led;
> +	int ret;
> +
> +	if (led_bdev_next_index == ULONG_MAX) {

I don't understand why this must be done.

> +		ret = -EOVERFLOW;
> +		goto exit_return;

exit_return label only returns. The whole idea behind using goto in such
a way is to avoid code duplication. Instead you do the opposite. You
can simply do
  return -EOVERFLOW;

> +	}
> +
> +	led = kmalloc(sizeof(*led), GFP_KERNEL);

kzalloc

> +	if (led == NULL) {
> +		ret = -ENOMEM;
> +		goto exit_return;

return -ENOMEM.

> +	}
> +
> +	led->index = led_bdev_next_index++;

This variable led_bdev_next_index never gets subtracted from, it only
increases. So if I enable and disable blkdev trigger ULONG_MAX times,
I won't be able to enable it anymore since you return -EOVERFLOW ?
This doesn't make any sense.

> +	led->led_cdev = led_cdev;
> +	led->blink_msec = LED_BDEV_BLINK_MSEC;
> +	led->mode = LED_BDEV_MODE_RW;
> +	xa_init(&led->linked_lbds);
> +
> +	ret = mutex_lock_interruptible(&led_bdev_mutex);
> +	if (ret != 0)

	if (ret)

we don't do if(ret == 0) and if(ret != 0) normally, instead we do
if(!ret) and if(ret). Explicitly mentioning 0 makes sense when you have
something like
  if (byte == 0x03 || byte == 0x00)

> +		goto exit_free;

> +static ssize_t link_device_store(struct device *const dev,
> +				 struct device_attribute *const attr,
> +				 const char *const buf, const size_t count)
> +{
> +	struct led_bdev_led *const led = led_trigger_get_drvdata(dev);
> +	struct led_bdev_lbd *lbd;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&led_bdev_mutex);
> +	if (ret != 0)
> +		goto exit_return;
> +
> +	lbd = led_bdev_get_lbd(buf, count);
> +	if (IS_ERR(lbd)) {
> +		ret = PTR_ERR(lbd);
> +		goto exit_unlock;
> +	}
> +
> +	if (xa_load(&lbd->linked_leds, led->index) != NULL) {
> +		ret = -EEXIST;
> +		goto exit_put_lbd;
> +	}
> +
> +	ret = led_bdev_link(led, lbd);
> +
> +exit_put_lbd:
> +	if (ret != 0)
> +		led_bdev_put_lbd(lbd);
> +exit_unlock:
> +	mutex_unlock(&led_bdev_mutex);
> +exit_return:
> +	return (ret == 0) ? count : ret;

	return ret ? : count;

> +static struct block_device *led_bdev_get_bdev(const char *const buf,
> +					      const size_t count,
> +					      const fmode_t mode)
> +{
> +	struct block_device *bdev;
> +	char *path;
> +
> +	path = kmemdup(buf, count + 1, GFP_KERNEL);  /* include null */
> +	if (path == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (path[count - 1] == '\n')
> +		path[count - 1] = 0;
> +
> +	bdev = blkdev_get_by_path(path, mode, THIS_MODULE);

And this is what we should discuss with blk API people. I would really
like to find the block device by name as seen in /sys/class/block.

> +static void led_bdev_update_lbd(struct led_bdev_lbd *const lbd)
> +{
> +	unsigned long read_ios, write_ios;
> +
> +	if (lbd->generation != led_bdev_generation) {
> +
> +		read_ios = part_stat_read(lbd->bdev, ios[STAT_READ]);
> +
> +		write_ios = part_stat_read(lbd->bdev, ios[STAT_WRITE])
> +				+ part_stat_read(lbd->bdev, ios[STAT_DISCARD])
> +				+ part_stat_read(lbd->bdev, ios[STAT_FLUSH]);

Wrong indentation.

Also discards and flushed should be separate (I want to be able to set
"blink on dicard" and "blink on flush").

> +
> +		if (lbd->read_ios != read_ios) {
> +			lbd->read_act = true;
> +			lbd->read_ios = read_ios;
> +		} else {
> +			lbd->read_act = false;
> +		}

	lbd->read_act = lbd->read_ios != read_ios;
	lbd->read_ios = read_ios;

> +		lbd->generation = led_bdev_generation;

I don't like this led_bdev_generation. Can't we somehow do this without
a static variable??

Marek

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger
  2021-10-05 21:27   ` Marek Behún
@ 2021-10-06 16:07     ` Ian Pilcher
  2021-10-08 10:01       ` Marek Behún
  0 siblings, 1 reply; 7+ messages in thread
From: Ian Pilcher @ 2021-10-06 16:07 UTC (permalink / raw)
  To: Marek Behún; +Cc: pavel, linux-leds, linux-kernel, gregkh, hch

Marek -

Thanks for taking the time to review this.

On 10/5/21 16:27, Marek Behún wrote:
> first, as I replied in one of the previous versions: it is not customary
> to use the const keyword in Linux the way you do use it, i.e.
>    int f(const int var)
> Your argument was that this makes the code more safe when changes are
> made in the future. Maybe this is true (although in my opinion it is
> not), but since this isn't done anywhere in kernel AFAIK, please drop it
> in this proposal.

I can do this.

> Second, I really would like to solve the problem looking up the block
> device, given name. Since block API does not provide such function, I
> think we should try to add it. It does not make sense to not be able to
> set, for example "sda1" to trigger a LED, when /sys/class/block/sda1
> exists, but someone deleted the /dev/sda1 device file.

I agree with you (and Greg) that this would be preferable, and if
someone were to implement such an API I would happily use it.

However, having looked at fs/block_dev.c and fs/inode.c, I can say with
confidence that I don't have the knowledge of how the inode cache works
that would be needed to implement such an API properly .  More
importantly, I have the definite impression that the block subsystem
maintainers would not be receptive to the idea.

> Anyway, see other comments below:

A few explanatory responses below.  Any points that aren't mentioned
below are changes that I will go ahead and make.

>> +/* Delayed work to periodically check for activity & blink LEDs */
>> +static DECLARE_DELAYED_WORK(led_bdev_work, led_bdev_process);
>>
>> +/* How often to run the delayed work - in jiffies */
>> +static unsigned int led_bdev_interval;
> 
> This is wrong. The interval can't be same for all triggers, this does
> not make sense. I want to be able to set different intervals for
> different LEDs, as in netdev trigger. Sure maybe not many people will
> use it, but the possibility should be there.

I think that it's different, rather than wrong, but I believe that I
can change the interval to a per-LED setting, rather than global.

> The work will then also be per LED and your structures should become
> more simple. You won't need to have NxM mapping between LEDs and block
> devices.

I have feeling that per-LED work items are likely to cause contention
for the mutex, since they will probably all have the same (default)
interval and they will usually be set up at about the same time (i.e.
at system boot).  Instead, I would propose to have a single work item
that is simply scheduled for the next time work is "needed" and then
checks all LEDs that are due at that time.

I also don't see that this would eliminate the many-to-many mapping.  It
seems like a useful feature.  For example, one could have a bunch of
device-specific read activity LEDs and a shared write (or discard) LED.

>> +	led->index = led_bdev_next_index++;
> 
> This variable led_bdev_next_index never gets subtracted from, it only
> increases. So if I enable and disable blkdev trigger ULONG_MAX times,
> I won't be able to enable it anymore since you return -EOVERFLOW ?
> This doesn't make any sense.

Each trig_bdev and trig_led needs a unique index so that it can be added
to one or more xarrays.  The increment-only pattern (capped at
ULONG_MAX - 1) is an easy way to generate unique indices.

And yes, this does mean that you can only associate an LED or a block
device with the trigger about 4 billion times on a 32-bit platform.  I
really can't think of a scenario in which that limitation would be an
issue.

 >> +	bdev = blkdev_get_by_path(path, mode, THIS_MODULE);
 >
> And this is what we should discuss with blk API people. I would really
> like to find the block device by name as seen in /sys/class/block.

Right ... but how?

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger
  2021-10-06 16:07     ` Ian Pilcher
@ 2021-10-08 10:01       ` Marek Behún
  2021-10-08 14:04         ` Ian Pilcher
  0 siblings, 1 reply; 7+ messages in thread
From: Marek Behún @ 2021-10-08 10:01 UTC (permalink / raw)
  To: Ian Pilcher; +Cc: pavel, linux-leds, linux-kernel, gregkh, hch

On Wed, 6 Oct 2021 11:07:06 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> I have feeling that per-LED work items are likely to cause contention
> for the mutex, since they will probably all have the same (default)
> interval and they will usually be set up at about the same time (i.e.
> at system boot).  Instead, I would propose to have a single work item
> that is simply scheduled for the next time work is "needed" and then
> checks all LEDs that are due at that time.

What about creating one work struct for all different interval values?

That way if the user never changes the interval, there will be only one
work struct.

I wonder if this can be done in a sensible (i.e. not overcomplicated
code) way.

Marek

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger
  2021-10-08 10:01       ` Marek Behún
@ 2021-10-08 14:04         ` Ian Pilcher
  0 siblings, 0 replies; 7+ messages in thread
From: Ian Pilcher @ 2021-10-08 14:04 UTC (permalink / raw)
  To: Marek Behún; +Cc: pavel, linux-leds, linux-kernel, gregkh, hch

On 10/8/21 05:01, Marek Behún wrote:
> On Wed, 6 Oct 2021 11:07:06 -0500
> Ian Pilcher <arequipeno@gmail.com> wrote:
> 
>> I have feeling that per-LED work items are likely to cause contention
>> for the mutex, since they will probably all have the same (default)
>> interval and they will usually be set up at about the same time (i.e.
>> at system boot).  Instead, I would propose to have a single work item
>> that is simply scheduled for the next time work is "needed" and then
>> checks all LEDs that are due at that time.
> 
> What about creating one work struct for all different interval values?
> 
> That way if the user never changes the interval, there will be only one
> work struct.
> 
> I wonder if this can be done in a sensible (i.e. not overcomplicated
> code) way.

I've been working on this (along with the other changes), and it's about
ready to go.  I'll send it out later today, once I've had a chance to
write up the changelog.

Thanks!

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-08 14:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-04 15:55 [RESEND PATCH v5 0/2] Introduce block device LED trigger Ian Pilcher
2021-10-04 15:55 ` [RESEND PATCH v5 1/2] docs: Add block device (blkdev) LED trigger documentation Ian Pilcher
2021-10-04 15:55 ` [RESEND PATCH v5 2/2] leds: trigger: Add block device LED trigger Ian Pilcher
2021-10-05 21:27   ` Marek Behún
2021-10-06 16:07     ` Ian Pilcher
2021-10-08 10:01       ` Marek Behún
2021-10-08 14:04         ` Ian Pilcher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).