[RFC PATCH v2 00/10] Add configurable block device LED triggers

This is a significant rewrite of the patchset that I originally posted
back on 28 July.  It incorporates almost all of the suggestions that I
received as feedback to my original patchset (largely because I was
finally able to wrap my head around how complex LED triggers work).

One thing that has not changed is that associations between block
devices and LEDs are still set via an attribute on the device, rather
than the LED.  This is much simpler, as the device attribute only has
to handle a single value (the name of the associated LED), rather than
potentially handling multiple device names.

More importantly, it avoids the need to iterate through all of the
block devices on the system, searching by name.  This was proposed
fairly recently, and the reaction was not positive.[1][2]

I have modeled the interface for the /sys/block/<DEVICE>/led
attribute on the sysfs interface used for selecting a trigger.  All
available LEDs (all LEDs associated with the blkdev trigger) are
shown when the attribute is read, with the currently selected LED
enclosed in square brackets ([]).

As before, this is all very new to me (particularly the RCU stuff), so
I welcome feedback.

Thanks!

Changes from the original patchset:

* Use a single complex LED trigger ("blkdev"), rather than multiple
  user-defined triggers.

* Configurable blink time and interval for each LED:

    /sys/class/leds/<LED>/blink_{on,off}

* Associated block devices linked from LED subdirectory:

    /sys/class/leds/<LED>/block_devices

  (Avoids violating the "one value per entry" sysfs rule.)

* Device-LED associations set via /sys/block/<DEVICE>/led

* Document all sysfs attributes in Documentation/ABI/testing (but also kept
  the overview doc)

* Removed unnecessary [un]likely macros

* Reduced number of "user error" log messages and changed level to INFO

* More detailed commit messages

* Add Kconfig option in final commit

* Use RCU-protected pointer (rather than mutex)

* No in-kernel APIs for now

[1] https://www.spinics.net/lists/linux-leds/msg18256.html
[2] https://www.spinics.net/lists/linux-leds/msg18261.html

Ian Pilcher (10):
  docs: Add block device LED trigger documentation
  block: Add file (blk-ledtrig.c) for block device LED trigger
    implementation
  block: Add block device LED trigger fields to gendisk structure
  block: Add functions to set & clear block device LEDs
  block: Add block device sysfs attribute to set/clear/show LED
  block: Add activate and deactivate functions for block device LED
    trigger
  block: Add sysfs attributes to LEDs associated with blkdev trigger
  block: Add init function for block device LED trigger
  block: Blink device LED (if any) when request is sent to its driver
  block: Add config option to enable block device LED triggers

 Documentation/ABI/testing/sysfs-block         |  16 +
 .../testing/sysfs-class-led-trigger-blkdev    |  28 ++
 Documentation/block/blk-ledtrig.rst           |  79 +++
 Documentation/block/index.rst                 |   1 +
 block/Kconfig                                 |   8 +
 block/Makefile                                |   1 +
 block/blk-ledtrig.c                           | 469 ++++++++++++++++++
 block/blk-ledtrig.h                           |  45 ++
 block/blk-mq.c                                |   2 +
 block/genhd.c                                 |  11 +
 include/linux/genhd.h                         |   4 +
 11 files changed, 664 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
 create mode 100644 Documentation/block/blk-ledtrig.rst
 create mode 100644 block/blk-ledtrig.c
 create mode 100644 block/blk-ledtrig.h

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

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

  * /sys/class/leds/<led>/blink_on
  * /sys/class/leds/<led>/blink_off
  * /sys/class/leds/<led>/block_devices

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

Add usage overview in Documentation/block/blk-ledtrig.rst

Signed-off-by: Ian Pilcher <redacted>
---
 Documentation/ABI/testing/sysfs-block         | 16 ++++
 .../testing/sysfs-class-led-trigger-blkdev    | 28 +++++++
 Documentation/block/blk-ledtrig.rst           | 79 +++++++++++++++++++
 Documentation/block/index.rst                 |  1 +
 4 files changed, 124 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
 create mode 100644 Documentation/block/blk-ledtrig.rst

diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
index e34cdeeeb9d4..36253fff25b2 100644
--- a/Documentation/ABI/testing/sysfs-block
+++ b/Documentation/ABI/testing/sysfs-block

@@ -316,3 +316,19 @@ 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>/led
+Date:		August 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Set the LED associated with this block device (or show available
+		LEDs and the currently selected LED, if any).
+
+		Reading the attribute will display the available LEDs (LEDs that
+		are associated with the blkdev LED trigger).  The currently
+		selected LED is enclosed in square brackets.  To clear the
+		device's LED association write 'none' (without the quotes) or
+		an empty string/line to the attribute.
+
+		See Documentation/ABI/testing/sysfs-class-led-trigger-blkdev and
+		Documentation/block/blk-ledtrig.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..1139a099a3cc
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-blkdev

@@ -0,0 +1,28 @@
+What:		/sys/class/leds/<led>/blink_on
+Date:		August 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>/blink_off
+Date:		August 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Time (in milliseconds) that the LED will be off during a single
+		"blink".  Effectively the amount of time that the LED will be
+		off before the next blink, when the associated block device(s)
+		are continuously active.
+
+What:		/sys/class/leds/<led>/block_devices
+Date:		August 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Directory containing links to all block devices that are
+		associated with this LED.
+
+		Once an LED has been associated with the blkdev trigger, a block
+		device can be associated with that LED by writing the LED name
+		to the device's /sys/[class/]block/<disk>/led attribute.  (See
+		Documentation/ABI/testing/sysfs-block and
+		Documentation/block/blk-ledtrig.rst.)

diff --git a/Documentation/block/blk-ledtrig.rst b/Documentation/block/blk-ledtrig.rst
new file mode 100644
index 000000000000..194fc6a51019
--- /dev/null
+++ b/Documentation/block/blk-ledtrig.rst

@@ -0,0 +1,79 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=================================
+Block Device (blkdev) LED Trigger
+=================================
+
+Available when ``CONFIG_BLK_LED_TRIGGERS=y``.
+
+See also:
+
+* ``Documentation/ABI/testing/sysfs-class-led-trigger-blkdev``
+* ``Documentation/ABI/testing/sysfs-block`` (``/sys/block/<disk>/led``)
+
+.. 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 (``Documentation/leds/uleds.rst``).
+
+Associate the LED with the ``blkdev`` LED trigger::
+
+	# echo blkdev > /sys/class/leds/<LED>/trigger
+
+	# cat /sys/class/leds/<LED>/trigger
+	... kbd-ctrlrlock [blkdev] disk-activity ...
+
+Note that the ``blink_on`` and ``blink_off`` attributes have been added to the
+LED, along with the ``block_devices`` subdirectory.
+
+The LED is now available for association with block devices::
+
+	# cat /sys/block/sda/led
+	[none] <LED>
+
+Associate the LED with the block device::
+
+	# echo <LED> > /sys/block/sda/led
+
+	# cat /sys/block/sda/led
+	none [<LED>]
+
+Reads 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_on``, and the minimum delay between blinks can
+be set via ``/sys/class/leds/<LED>/blink_off``.
+
+Associate a second device with the LED::
+
+	# echo <LED> > /sys/block/sdb/led
+
+	# cat /sys/block/sdb/led
+	none [<LED>]
+
+Note that both block devices are linked from the LED's ``block_devices``
+subdirectory::
+
+	# ls /sys/class/leds/<LED>/block_devices
+	sda sdb
+
+Other notes:
+
+* Many 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
+  * SD cards
+  * loopback block devices (``/dev/loop*``)
+
+* NVMe SSDs and most virtual block devices can be associated with LEDs, but
+  they will produce little or no LED activity.
+
+* Multiple LEDs can be associated with the ``blkdev`` trigger; different block
+  devices can be associated with different LEDs.
+
+* This trigger causes associated LED(s) to blink (per the LED's ``blink_on``
+  and ``blink_off`` attributes) when a request is sent to an associated
+  block device's low-level driver.  It does not track the duration (or
+  result) of requests further.  Thus, it provides an approximate visual
+  indication of device activity, not an exact measurement.

diff --git a/Documentation/block/index.rst b/Documentation/block/index.rst
index 86dcf7159f99..cbb11a42f825 100644
--- a/Documentation/block/index.rst
+++ b/Documentation/block/index.rst

@@ -25,3 +25,4 @@ Block
    stat
    switching-sched
    writeback_cache_control
+   blk-ledtrig

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Define data structure for all associated LEDs

Add list of associated LEDs and list search helper

Add trigger mutex - must be held when accessing trigger/LED or device/LED
associations

Signed-off-by: Ian Pilcher <redacted>
---
 block/blk-ledtrig.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 block/blk-ledtrig.c

diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
new file mode 100644
index 000000000000..c5ad57ed9c3b
--- /dev/null
+++ b/block/blk-ledtrig.c

@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ *	Block device LED triggers
+ *
+ *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
+ */
+
+#include <linux/leds.h>
+#include <linux/mutex.h>
+
+/*
+ *
+ *	Trigger mutex and LED list
+ *
+ */
+
+// Must hold when doing anything with LED/trigger/block device
+// associations
+static DEFINE_MUTEX(blk_ledtrig_mutex);
+
+static LIST_HEAD(blk_ledtrig_leds);
+
+// Every LED associated with the blkdev trigger gets one of these
+struct blk_ledtrig_led {
+	struct kobject		*dir;		// block_devices subdirectory
+	struct led_classdev	*led;
+	unsigned int		blink_on;
+	unsigned int		blink_off;
+	struct list_head	leds_list_node;
+	struct list_head	dev_list;
+};
+
+// Caller must hold blk_ledtrig_mutex
+static struct blk_ledtrig_led *blk_ledtrig_find(const char *const led_name,
+						const size_t name_len)
+{
+	struct blk_ledtrig_led *bd_led;
+
+	list_for_each_entry(bd_led, &blk_ledtrig_leds, leds_list_node) {
+		if (strlen(bd_led->led->name) != name_len)
+			continue;
+		if (memcmp(bd_led->led->name, led_name, name_len) == 0)
+			return bd_led;
+	}
+
+	return NULL;
+}

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Add header (blk-ledtrig.h) for LED trigger-related declarations

Add (inline) function to ensure trigger pointer is initialized
(to NULL) and call it from __device_add_disk()

Signed-off-by: Ian Pilcher <redacted>
---
 block/blk-ledtrig.h   | 25 +++++++++++++++++++++++++
 block/genhd.c         |  2 ++
 include/linux/genhd.h |  4 ++++
 3 files changed, 31 insertions(+)
 create mode 100644 block/blk-ledtrig.h

diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h
new file mode 100644
index 000000000000..95a79d2fe447
--- /dev/null
+++ b/block/blk-ledtrig.h

@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ *	Block device LED triggers
+ *
+ *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
+ */
+
+#ifndef _BLOCK_BLK_LEDTRIG_H
+#define _BLOCK_BLK_LEDTRIG_H
+
+#ifdef CONFIG_BLK_LED_TRIGGERS
+
+static inline void blk_ledtrig_disk_init(struct gendisk *const disk)
+{
+	RCU_INIT_POINTER(disk->led, NULL);
+}
+
+#else	// CONFIG_BLK_LED_TRIGGERS
+
+static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {}
+
+#endif	// CONFIG_BLK_LED_TRIGGERS
+
+#endif	// _BLOCK_BLK_LEDTRIG_H

diff --git a/block/genhd.c b/block/genhd.c
index 298ee78c1bda..b168172e664b 100644
--- a/block/genhd.c
+++ b/block/genhd.c

@@ -26,6 +26,7 @@
 #include <linux/badblocks.h>
 
 #include "blk.h"
+#include "blk-ledtrig.h"
 
 static struct kobject *block_depr;
 

@@ -539,6 +540,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 
 	disk_add_events(disk);
 	blk_integrity_add(disk);
+	blk_ledtrig_disk_init(disk);
 }
 
 void device_add_disk(struct device *parent, struct gendisk *disk,

diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 13b34177cc85..efcb1ca62f17 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h

@@ -168,6 +168,10 @@ struct gendisk {
 #endif	/* CONFIG_BLK_DEV_INTEGRITY */
 #if IS_ENABLED(CONFIG_CDROM)
 	struct cdrom_device_info *cdi;
+#endif
+#ifdef CONFIG_BLK_LED_TRIGGERS
+	struct blk_ledtrig_led __rcu *led;
+	struct list_head led_dev_list_node;
 #endif
 	int node_id;
 	struct badblocks *bb;

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Set up activated LED:

  * Allocate per-LED data structure
  * Add LED to list of LEDs associated with blkdev trigger
  * Create block_devices subdirectory for links to associated devices

Clean up deactivated LED:

  * Clear LED of any associated devices
  * Remove from LED from blkdev trigger list
  * Remove block_devices subdirectory
  * Wait for any blinks using LED to complete (synchronize_rcu())
  * Free per-LED structure

Signed-off-by: Ian Pilcher <redacted>
---
 block/blk-ledtrig.c | 95 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
index 1af94dc7ea51..f8cb6de203f8 100644
--- a/block/blk-ledtrig.c
+++ b/block/blk-ledtrig.c

@@ -13,6 +13,11 @@
 
 #include "blk-ledtrig.h"
 
+// Default blink_on & blink_off values
+#define BLK_LEDTRIG_BLINK_ON	75
+#define BLK_LEDTRIG_BLINK_OFF	25
+#define BLK_LEDTRIG_BLINK_MAX	10000		// 10 seconds
+
 
 /*
  *

@@ -248,3 +253,93 @@ ssize_t blk_ledtrig_dev_led_show(struct device *const dev,
 led_show_exit_return:
 	return ret;
 }
+
+
+/*
+ *
+ *	Associate an LED with the blkdev trigger
+ *
+ */
+
+// Helper function to create <LED>/blkdevs subdirectory - doesn't
+// swallow error code like kobject_create_and_add()
+static int blk_ledtrig_subdir_create(struct blk_ledtrig_led *const bd_led,
+				     struct led_classdev *const led)
+{
+	int ret;
+
+	bd_led->dir = kobject_create();
+	if (bd_led->dir == NULL)
+		return -ENOMEM;
+
+	ret = kobject_add(bd_led->dir, &led->dev->kobj, "block_devices");
+	if (ret != 0)
+		kobject_put(bd_led->dir);
+
+	return ret;
+}
+
+static int blk_ledtrig_activate(struct led_classdev *const led)
+{
+	struct blk_ledtrig_led *bd_led;
+	int ret;
+
+	bd_led = kmalloc(sizeof(*bd_led), GFP_KERNEL);
+	if (bd_led == NULL) {
+		ret = -ENOMEM;
+		goto activate_exit_return;
+	}
+
+	bd_led->led = led;
+	bd_led->blink_on = BLK_LEDTRIG_BLINK_ON;
+	bd_led->blink_off = BLK_LEDTRIG_BLINK_OFF;
+	INIT_LIST_HEAD(&bd_led->dev_list);
+
+	ret = mutex_lock_interruptible(&blk_ledtrig_mutex);
+	if (ret != 0)
+		goto activate_exit_free;
+
+	ret = blk_ledtrig_subdir_create(bd_led, led);
+	if (ret != 0)
+		goto activate_exit_unlock;
+
+	list_add(&bd_led->leds_list_node, &blk_ledtrig_leds);
+	led_set_trigger_data(led, bd_led);
+	ret = 0;
+
+activate_exit_unlock:
+	mutex_unlock(&blk_ledtrig_mutex);
+activate_exit_free:
+	if (ret != 0)
+		kfree(bd_led);
+activate_exit_return:
+	return ret;
+}
+
+
+/*
+ *
+ *	Disassociate an LED from the blkdev trigger
+ *
+ */
+
+static void blk_ledtrig_deactivate(struct led_classdev *const led)
+{
+	struct blk_ledtrig_led *const bd_led = led_get_trigger_data(led);
+	struct gendisk *disk, *next;
+
+	mutex_lock(&blk_ledtrig_mutex);
+
+	list_for_each_entry_safe(disk, next,
+				 &bd_led->dev_list, led_dev_list_node) {
+
+		blk_ledtrig_dev_clear_locked(disk, bd_led);
+	}
+
+	list_del(&bd_led->leds_list_node);
+	kobject_put(bd_led->dir);
+
+	mutex_unlock(&blk_ledtrig_mutex);
+	synchronize_rcu();
+	kfree(bd_led);
+}

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Create a symlink from /sys/class/leds/<LED>/block_devices to each block
device that is associated with that LED

Ensure device LED is cleared when device is removed

Signed-off-by: Ian Pilcher <redacted>
---
 block/blk-ledtrig.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
 block/blk-ledtrig.h |  3 ++
 block/genhd.c       |  1 +
 3 files changed, 97 insertions(+)

diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
index c5ad57ed9c3b..280fa9edc2dd 100644
--- a/block/blk-ledtrig.c
+++ b/block/blk-ledtrig.c

@@ -6,9 +6,13 @@
  *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
  */
 
+#include <linux/genhd.h>
 #include <linux/leds.h>
 #include <linux/mutex.h>
 
+#include "blk-ledtrig.h"
+
+
 /*
  *
  *	Trigger mutex and LED list

@@ -46,3 +50,92 @@ static struct blk_ledtrig_led *blk_ledtrig_find(const char *const led_name,
 
 	return NULL;
 }
+
+
+/*
+ *
+ *	Clear a block device's LED
+ *
+ */
+
+// Also called from blk_ledtrig_dev_set()
+static void blk_ledtrig_dev_cleanup(struct gendisk *const disk,
+				    struct blk_ledtrig_led *const old_led)
+{
+	sysfs_remove_link(old_led->dir, disk->disk_name);
+	list_del(&disk->led_dev_list_node);
+}
+
+// Also called from blk_ledtrig_deactivate()
+static void blk_ledtrig_dev_clear_locked(struct gendisk *const disk,
+					 struct blk_ledtrig_led *const old_led)
+{
+	RCU_INIT_POINTER(disk->led, NULL);
+	if (old_led != NULL)
+		blk_ledtrig_dev_cleanup(disk, old_led);
+}
+
+// Also called from genhd.c:del_gendisk()
+void blk_ledtrig_dev_clear(struct gendisk *const disk)
+{
+	struct blk_ledtrig_led *old_led;
+
+	mutex_lock(&blk_ledtrig_mutex);
+	old_led = rcu_dereference_protected(disk->led,
+					lockdep_is_held(&blk_ledtrig_mutex));
+	blk_ledtrig_dev_clear_locked(disk, old_led);
+	mutex_unlock(&blk_ledtrig_mutex);
+}
+
+
+/*
+ *
+ *	Set a block device's LED
+ *
+ */
+
+static int blk_ledtrig_dev_set(struct gendisk *const disk,
+			       const char *const led_name,
+			       const size_t name_len)
+{
+	struct blk_ledtrig_led *new_led, *old_led;
+	int ret;
+
+	ret = mutex_lock_interruptible(&blk_ledtrig_mutex);
+	if (ret != 0)
+		goto led_set_exit_return;
+
+	new_led = blk_ledtrig_find(led_name, name_len);
+	if (new_led == NULL) {
+		pr_info("no LED named %.*s associated with blkdev trigger\n",
+			(int)name_len, led_name);
+		ret = -ENODEV;
+		goto led_set_exit_unlock;
+	}
+
+	old_led = rcu_dereference_protected(disk->led,
+					lockdep_is_held(&blk_ledtrig_mutex));
+
+	if (old_led == new_led) {
+		ret = 0;
+		goto led_set_exit_unlock;
+	}
+
+	ret = sysfs_create_link(new_led->dir, &disk_to_dev(disk)->kobj,
+				disk->disk_name);
+	if (ret != 0)
+		goto led_set_exit_unlock;
+
+	if (old_led != NULL)
+		blk_ledtrig_dev_cleanup(disk, old_led);
+
+	rcu_assign_pointer(disk->led, new_led);
+	list_add(&disk->led_dev_list_node, &new_led->dev_list);
+
+	ret = 0;
+
+led_set_exit_unlock:
+	mutex_unlock(&blk_ledtrig_mutex);
+led_set_exit_return:
+	return ret;
+}

diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h
index 95a79d2fe447..66a1302a4174 100644
--- a/block/blk-ledtrig.h
+++ b/block/blk-ledtrig.h

@@ -16,9 +16,12 @@ static inline void blk_ledtrig_disk_init(struct gendisk *const disk)
 	RCU_INIT_POINTER(disk->led, NULL);
 }
 
+void blk_ledtrig_dev_clear(struct gendisk *const disk);
+
 #else	// CONFIG_BLK_LED_TRIGGERS
 
 static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {}
+static inline void blk_ledtrig_dev_clear(const struct gendisk *disk) {}
 
 #endif	// CONFIG_BLK_LED_TRIGGERS
 

diff --git a/block/genhd.c b/block/genhd.c
index b168172e664b..9fa734aeab0f 100644
--- a/block/genhd.c
+++ b/block/genhd.c

@@ -583,6 +583,7 @@ void del_gendisk(struct gendisk *disk)
 	if (WARN_ON_ONCE(!disk->queue))
 		return;
 
+	blk_ledtrig_dev_clear(disk);
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Add show & store functions in blk-ledtrig.c (attributes defined in genhd.c)

Show function shows all available LEDs (LEDs associated with blkdev trigger);
currently associated LED is shown in square brackets ([])

Store function accepts either all whitespace or "none" to clear LED

Signed-off-by: Ian Pilcher <redacted>
---
 block/blk-ledtrig.c | 109 ++++++++++++++++++++++++++++++++++++++++++++
 block/blk-ledtrig.h |   8 ++++
 block/genhd.c       |   8 ++++
 3 files changed, 125 insertions(+)

diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
index 280fa9edc2dd..1af94dc7ea51 100644
--- a/block/blk-ledtrig.c
+++ b/block/blk-ledtrig.c

@@ -6,6 +6,7 @@
  *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
  */
 
+#include <linux/ctype.h>
 #include <linux/genhd.h>
 #include <linux/leds.h>
 #include <linux/mutex.h>

@@ -139,3 +140,111 @@ static int blk_ledtrig_dev_set(struct gendisk *const disk,
 led_set_exit_return:
 	return ret;
 }
+
+
+/*
+ *
+ *	sysfs attribute store function to set or clear device LED
+ *
+ */
+
+// Returns a pointer to the first non-whitespace character in s (or a pointer
+// to the terminating null).
+static const char *blk_ledtrig_skip_whitespace(const char *s)
+{
+	while (*s != 0 && isspace(*s))
+		++s;
+
+	return s;
+}
+
+// Returns a pointer to the first whitespace character in s (or a pointer to
+// the terminating null), which is effectively a pointer to the position *after*
+// the last character in the non-whitespace token at the beginning of s.  (s is
+// expected to be the result of a previous call to blk_ledtrig_skip_whitespace.)
+static const char *blk_ledtrig_find_whitespace(const char *s)
+{
+	while (*s != 0 && !isspace(*s))
+		++s;
+
+	return s;
+}
+
+static bool blk_ledtrig_name_is_none(const char *const name, const size_t len)
+{
+	static const char none[4] = "none";	// no terminating null
+
+	return len == sizeof(none) && memcmp(name, none, sizeof(none)) == 0;
+}
+
+ssize_t blk_ledtrig_dev_led_store(struct device *const dev,
+				  struct device_attribute *const attr,
+				  const char *const buf, const size_t count)
+{
+	struct gendisk *const disk = dev_to_disk(dev);
+	const char *const led_name = blk_ledtrig_skip_whitespace(buf);
+	const char *const endp = blk_ledtrig_find_whitespace(led_name);
+	const ptrdiff_t name_len = endp - led_name;	// always >= 0
+	int ret;
+
+	if (name_len == 0 || blk_ledtrig_name_is_none(led_name, name_len)) {
+		blk_ledtrig_dev_clear(disk);
+		ret = 0;
+	} else {
+		ret = blk_ledtrig_dev_set(disk, led_name, name_len);
+	}
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+
+/*
+ *
+ *	sysfs attribute show function for device LED
+ *
+ */
+
+ssize_t blk_ledtrig_dev_led_show(struct device *const dev,
+				 struct device_attribute *const attr,
+				 char *const buf)
+{
+	struct gendisk *const disk = dev_to_disk(dev);
+	struct blk_ledtrig_led *bd_led, *disk_led;
+	int ret, c = 0;
+
+	ret = mutex_lock_interruptible(&blk_ledtrig_mutex);
+	if (ret != 0)
+		goto led_show_exit_return;
+
+	disk_led = rcu_dereference_protected(disk->led,
+					lockdep_is_held(&blk_ledtrig_mutex));
+
+	if (disk_led == NULL)
+		c += sprintf(buf, "[none]");
+	else
+		c += sprintf(buf, "none");
+
+	list_for_each_entry(bd_led, &blk_ledtrig_leds, leds_list_node) {
+
+		ret = snprintf(buf + c, PAGE_SIZE - c - 1,
+			       bd_led == disk_led ? " [%s]" : " %s",
+			       bd_led->led->name);
+		if (ret >= PAGE_SIZE - c - 1) {
+			ret = -EOVERFLOW;
+			goto led_show_exit_unlock;
+		}
+
+		c += ret;
+	}
+
+	buf[c] = '\n';
+	ret = c + 1;
+
+led_show_exit_unlock:
+	mutex_unlock(&blk_ledtrig_mutex);
+led_show_exit_return:
+	return ret;
+}

diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h
index 66a1302a4174..771000d43647 100644
--- a/block/blk-ledtrig.h
+++ b/block/blk-ledtrig.h

@@ -18,6 +18,14 @@ static inline void blk_ledtrig_disk_init(struct gendisk *const disk)
 
 void blk_ledtrig_dev_clear(struct gendisk *const disk);
 
+ssize_t blk_ledtrig_dev_led_store(struct device *const dev,
+				  struct device_attribute *const attr,
+				  const char *const buf, const size_t count);
+
+ssize_t blk_ledtrig_dev_led_show(struct device *const dev,
+				 struct device_attribute *const attr,
+				 char *const buf);
+
 #else	// CONFIG_BLK_LED_TRIGGERS
 
 static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {}

diff --git a/block/genhd.c b/block/genhd.c
index 9fa734aeab0f..d5413a633410 100644
--- a/block/genhd.c
+++ b/block/genhd.c

@@ -1012,6 +1012,11 @@ static struct device_attribute dev_attr_fail_timeout =
 	__ATTR(io-timeout-fail, 0644, part_timeout_show, part_timeout_store);
 #endif
 
+#ifdef CONFIG_BLK_LED_TRIGGERS
+static struct device_attribute dev_attr_led =
+	__ATTR(led, 0644, blk_ledtrig_dev_led_show, blk_ledtrig_dev_led_store);
+#endif
+
 static struct attribute *disk_attrs[] = {
 	&dev_attr_range.attr,
 	&dev_attr_ext_range.attr,

@@ -1033,6 +1038,9 @@ static struct attribute *disk_attrs[] = {
 #endif
 #ifdef CONFIG_FAIL_IO_TIMEOUT
 	&dev_attr_fail_timeout.attr,
+#endif
+#ifdef CONFIG_BLK_LED_TRIGGERS
+	&dev_attr_led.attr,
 #endif
 	NULL
 };

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Add blink_on & blink_off attributes to control the duration of each LED blink
(blink_on) and the minimum time between blinks (blink_off) in milliseconds

Signed-off-by: Ian Pilcher <redacted>
---
 block/blk-ledtrig.c | 63 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
index f8cb6de203f8..d02f32205985 100644
--- a/block/blk-ledtrig.c
+++ b/block/blk-ledtrig.c

@@ -343,3 +343,66 @@ static void blk_ledtrig_deactivate(struct led_classdev *const led)
 	synchronize_rcu();
 	kfree(bd_led);
 }
+
+
+/*
+ *
+ *	Per-LED blink_on & blink_off device attributes
+ *
+ */
+
+static ssize_t blk_ledtrig_blink_show(struct device *const dev,
+				      struct device_attribute *const attr,
+				      char *const buf);
+
+static ssize_t blk_ledtrig_blink_store(struct device *const dev,
+				       struct device_attribute *const attr,
+				       const char *const buf,
+				       const size_t count);
+
+static struct device_attribute blk_ledtrig_attr_blink_on =
+	__ATTR(blink_on, 0644,
+	       blk_ledtrig_blink_show, blk_ledtrig_blink_store);
+
+static struct device_attribute blk_ledtrig_attr_blink_off =
+	__ATTR(blink_off, 0644,
+	       blk_ledtrig_blink_show, blk_ledtrig_blink_store);
+
+static ssize_t blk_ledtrig_blink_show(struct device *const dev,
+				      struct device_attribute *const attr,
+				      char *const buf)
+{
+	struct blk_ledtrig_led *const bd_led = led_trigger_get_drvdata(dev);
+	unsigned int value;
+
+	if (attr == &blk_ledtrig_attr_blink_on)
+		value = READ_ONCE(bd_led->blink_on);
+	else	// attr == &blk_ledtrig_attr_blink_off
+		value = READ_ONCE(bd_led->blink_off);
+
+	return sprintf(buf, "%u\n", value);
+}
+
+static ssize_t blk_ledtrig_blink_store(struct device *const dev,
+				       struct device_attribute *const attr,
+				       const char *const buf,
+				       const size_t count)
+{
+	struct blk_ledtrig_led *const bd_led = led_trigger_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &value);
+	if (ret != 0)
+		return ret;
+
+	if (value > BLK_LEDTRIG_BLINK_MAX)
+		return -ERANGE;
+
+	if (attr == &blk_ledtrig_attr_blink_on)
+		WRITE_ONCE(bd_led->blink_on, value);
+	else	// attr == &blk_ledtrig_attr_blink_off
+		WRITE_ONCE(bd_led->blink_off, value);
+
+	return count;
+}

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Register the blkdev LED trigger

Signed-off-by: Ian Pilcher <redacted>
---
 block/blk-ledtrig.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
index d02f32205985..14b1d33a2953 100644
--- a/block/blk-ledtrig.c
+++ b/block/blk-ledtrig.c

@@ -406,3 +406,38 @@ static ssize_t blk_ledtrig_blink_store(struct device *const dev,
 
 	return count;
 }
+
+
+/*
+ *
+ *	Initialization - register the trigger
+ *
+ */
+
+static struct attribute *blk_ledtrig_attrs[] = {
+	&blk_ledtrig_attr_blink_on.attr,
+	&blk_ledtrig_attr_blink_off.attr,
+	NULL
+};
+
+static const struct attribute_group blk_ledtrig_attr_group = {
+	.attrs	= blk_ledtrig_attrs,
+};
+
+static const struct attribute_group *blk_ledtrig_attr_groups[] = {
+	&blk_ledtrig_attr_group,
+	NULL
+};
+
+static struct led_trigger blk_ledtrig_trigger = {
+	.name		= "blkdev",
+	.activate	= blk_ledtrig_activate,
+	.deactivate	= blk_ledtrig_deactivate,
+	.groups		= blk_ledtrig_attr_groups,
+};
+
+static int __init blk_ledtrig_init(void)
+{
+	return led_trigger_register(&blk_ledtrig_trigger);
+}
+device_initcall(blk_ledtrig_init);

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Call led_blink_set_oneshot() in RCU critical section to ensure that LED
doesn't disappear.  (See synchronize_rcu() call in blk_ledtrig_deactivate()).

Signed-off-by: Ian Pilcher <redacted>
---
 block/blk-ledtrig.c | 26 ++++++++++++++++++++++++++
 block/blk-ledtrig.h |  9 +++++++++
 block/blk-mq.c      |  2 ++
 3 files changed, 37 insertions(+)

diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
index 14b1d33a2953..10588f3dea15 100644
--- a/block/blk-ledtrig.c
+++ b/block/blk-ledtrig.c

@@ -6,6 +6,7 @@
  *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
  */
 
+#include <linux/blkdev.h>
 #include <linux/ctype.h>
 #include <linux/genhd.h>
 #include <linux/leds.h>

@@ -441,3 +442,28 @@ static int __init blk_ledtrig_init(void)
 	return led_trigger_register(&blk_ledtrig_trigger);
 }
 device_initcall(blk_ledtrig_init);
+
+
+/*
+ *
+ *	Blink the LED associated with a (non-NULL) disk (if set)
+ *
+ */
+
+void __blk_ledtrig_try_blink(struct request *const rq)
+{
+	struct blk_ledtrig_led *bd_led;
+	unsigned long delay_on, delay_off;
+
+	rcu_read_lock();
+
+	bd_led = rcu_dereference(rq->rq_disk->led);
+
+	if (bd_led != NULL) {
+		delay_on = READ_ONCE(bd_led->blink_on);
+		delay_off = READ_ONCE(bd_led->blink_off);
+		led_blink_set_oneshot(bd_led->led, &delay_on, &delay_off, 0);
+	}
+
+	rcu_read_unlock();
+}

diff --git a/block/blk-ledtrig.h b/block/blk-ledtrig.h
index 771000d43647..7eb1d88e2b3c 100644
--- a/block/blk-ledtrig.h
+++ b/block/blk-ledtrig.h

@@ -26,10 +26,19 @@ ssize_t blk_ledtrig_dev_led_show(struct device *const dev,
 				 struct device_attribute *const attr,
 				 char *const buf);
 
+void __blk_ledtrig_try_blink(struct request *const rq);
+
+static inline void blk_ledtrig_try_blink(struct request *const rq)
+{
+	if (rq->rq_disk != NULL)
+		__blk_ledtrig_try_blink(rq);
+}
+
 #else	// CONFIG_BLK_LED_TRIGGERS
 
 static inline void blk_ledtrig_disk_init(const struct gendisk *disk) {}
 static inline void blk_ledtrig_dev_clear(const struct gendisk *disk) {}
+static inline void blk_ledtrig_try_blink(const struct request *rq) {}
 
 #endif	// CONFIG_BLK_LED_TRIGGERS
 

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2c4ac51e54eb..7732e31f3ca8 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c

@@ -40,6 +40,7 @@
 #include "blk-stat.h"
 #include "blk-mq-sched.h"
 #include "blk-rq-qos.h"
+#include "blk-ledtrig.h"
 
 static DEFINE_PER_CPU(struct llist_head, blk_cpu_done);
 

@@ -1381,6 +1382,7 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
 		switch (ret) {
 		case BLK_STS_OK:
 			queued++;
+			blk_ledtrig_try_blink(rq);
 			break;
 		case BLK_STS_RESOURCE:
 		case BLK_STS_DEV_RESOURCE:

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Signed-off-by: Ian Pilcher <redacted>
---
 block/Kconfig  | 8 ++++++++
 block/Makefile | 1 +
 2 files changed, 9 insertions(+)

diff --git a/block/Kconfig b/block/Kconfig
index fd732aede922..ef05b36b5251 100644
--- a/block/Kconfig
+++ b/block/Kconfig

@@ -220,6 +220,14 @@ config BLK_INLINE_ENCRYPTION_FALLBACK
 	  by falling back to the kernel crypto API when inline
 	  encryption hardware is not present.
 
+config BLK_LED_TRIGGERS
+	bool "Enable the blkdev LED trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  The blkdev LED trigger allows LEDs to be controlled by
+	  block device activity (reads and writes).  See
+	  Documentation/block/blk-ledtrig.rst.
+
 menu "Partition Types"
 
 source "block/partitions/Kconfig"

diff --git a/block/Makefile b/block/Makefile
index bfbe4e13ca1e..bcd97ee26462 100644
--- a/block/Makefile
+++ b/block/Makefile

@@ -42,3 +42,4 @@ obj-$(CONFIG_BLK_SED_OPAL)	+= sed-opal.o
 obj-$(CONFIG_BLK_PM)		+= blk-pm.o
 obj-$(CONFIG_BLK_INLINE_ENCRYPTION)	+= keyslot-manager.o blk-crypto.o
 obj-$(CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK)	+= blk-crypto-fallback.o
+obj-$(CONFIG_BLK_LED_TRIGGERS)	+= blk-ledtrig.o

-- 
2.31.1


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

�� 2021/8/9 ����11:32, Ian Pilcher ����:

These are not linux kernel style comments.

On 8/8/21 11:21 PM, Jackie Liu wrote:

quoted

+// Returns a pointer to the first whitespace character in s (or a pointer to
+// the terminating null), which is effectively a pointer to the position *after*
+// the last character in the non-whitespace token at the beginning of s.  (s is
+// expected to be the result of a previous call to blk_ledtrig_skip_whitespace.)

These are not linux kernel style comments.

Do you mean kernel-doc comments?  If so, they're not supposed to be, as
the functions are not exported.

quoted

+static const char *blk_ledtrig_find_whitespace(const char *s)     ^^^^^^

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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Hello Ian,

thank you for your proposal. Some comments below:

On Sun,  8 Aug 2021 22:32:07 -0500
Ian Pilcher [off-list ref] wrote:

One thing that has not changed is that associations between block
devices and LEDs are still set via an attribute on the device, rather
than the LED.  This is much simpler, as the device attribute only has
to handle a single value (the name of the associated LED), rather than
potentially handling multiple device names.

It may be simpler, but it is in contrast to how the netdev trigger
works, which already is in upstream for many years. I really think we
should try to have similar sysfs ABIs here. (I understand that the
netdev trigger is currently unable to handle multiple network
interfaces - but it is possible to extend it so.)

I have modeled the interface for the /sys/block/<DEVICE>/led
attribute on the sysfs interface used for selecting a trigger.  All
available LEDs (all LEDs associated with the blkdev trigger) are
shown when the attribute is read, with the currently selected LED
enclosed in square brackets ([]).

I think it is reasonable to be able to set something like this:
  led0 : blink on activity on any of [sda, sdb, sdc]
  led1 : blink on activity on sda
  led2 : blink on activity on sdb
  led3 : blink on activity on sdc

If I am reading your code correctly, it looks that only one LED can be
configured for a block device. Is this true? If so, then the above
configuration cannot be set.

Also you are blinking the LED on any request to the block device. I
would rather expect to be able to set the LED to blink on read and on
write. (And possibly on other functions, like discard, or critical
temperature, or error, ...) I would like to know what other people
think about this.

Marek

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On Monday 09 August 2021 20:56:33 Marek Behún wrote:

Hello Ian,

thank you for your proposal. Some comments below:

On Sun,  8 Aug 2021 22:32:07 -0500
Ian Pilcher [off-list ref] wrote:

quoted

One thing that has not changed is that associations between block
devices and LEDs are still set via an attribute on the device, rather
than the LED.  This is much simpler, as the device attribute only has
to handle a single value (the name of the associated LED), rather than
potentially handling multiple device names.

It may be simpler, but it is in contrast to how the netdev trigger
works, which already is in upstream for many years. I really think we
should try to have similar sysfs ABIs here. (I understand that the
netdev trigger is currently unable to handle multiple network
interfaces - but it is possible to extend it so.)

quoted

I have modeled the interface for the /sys/block/<DEVICE>/led
attribute on the sysfs interface used for selecting a trigger.  All
available LEDs (all LEDs associated with the blkdev trigger) are
shown when the attribute is read, with the currently selected LED
enclosed in square brackets ([]).

I think it is reasonable to be able to set something like this:
  led0 : blink on activity on any of [sda, sdb, sdc]
  led1 : blink on activity on sda
  led2 : blink on activity on sdb
  led3 : blink on activity on sdc

If I am reading your code correctly, it looks that only one LED can be
configured for a block device. Is this true? If so, then the above
configuration cannot be set.

Also you are blinking the LED on any request to the block device. I
would rather expect to be able to set the LED to blink on read and on
write. (And possibly on other functions, like discard, or critical
temperature, or error, ...) I would like to know what other people
think about this.

Hello!

HP EliteBook laptops had dedicated LED for some kind of error and
encryption indication. And there is kernel acpi/wmi driver which can
control this LED. I do not know if recent HP laptops still have these
LEDs, but I would suggest to design API in a way that would allow to use
these dedicated LEDs for their original "vendor" purpose.

I'm mentioning it just because this functionality and design is already
on existing production mainstream laptops, and not something imaginary.

If Linux distributions are still cooperating with laptop vendors and
doing "official" Linux preloads then they may be interested in having
"native" LED functionality support in kernel.

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Marek -

Thanks for taking the time to look at this.

On 8/9/21 1:56 PM, Marek Behún wrote:

It may be simpler, but it is in contrast to how the netdev trigger
works, which already is in upstream for many years. I really think we
should try to have similar sysfs ABIs here. (I understand that the
netdev trigger is currently unable to handle multiple network
interfaces - but it is possible to extend it so.)

I'm not unalterably opposed to the idea, but I don't currently see a way
to do that without resolving block devices (struct gendisk) by name, and
that seems to be a no-no.

If you (or anyone else) has a suggestion on how to get around this
obstacle, I'd be willing to give it a shot.

I think it is reasonable to be able to set something like this:
   led0 : blink on activity on any of [sda, sdb, sdc]
   led1 : blink on activity on sda
   led2 : blink on activity on sdb
   led3 : blink on activity on sdc

If I am reading your code correctly, it looks that only one LED can be
configured for a block device. Is this true? If so, then the above
configuration cannot be set.

You're correct that it's not possible with the current code.  Multiple
devices can be associated to with a single LED, but there's not
currently a way to drive more than 1 LED from a single device.  This
is something that could be changed.

Also you are blinking the LED on any request to the block device. I
would rather expect to be able to set the LED to blink on read and on
write. (And possibly on other functions, like discard, or critical
temperature, or error, ...) I would like to know what other people
think about this.

I wanted to keep things as simple as possible for now.  I don't think
that there's any particular reason that separated LEDs couldn't be
configured for read and write requests.  (It looks like it should be
pretty easy to distinguish reads vs writes in a struct request.)

My feeling is that things like temperature, errors, etc. are better
monitored from user space, as they tend to require actively querying
the drive.

Like you, I'm interested in knowing if there is actually hardware out
there that has separate read/write LEDs.

All in all, I feel like I should be able to implement almost everything
that you've suggested, *if* I can figure out the block device lookup
issue, but I really don't have any ideas on that.

Thanks for your patience and feedback!

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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On Mon, 9 Aug 2021 14:54:26 -0500
Ian Pilcher [off-list ref] wrote:

I'm not unalterably opposed to the idea, but I don't currently see a way
to do that without resolving block devices (struct gendisk) by name, and
that seems to be a no-no.

So a name like "sda1" is not viable? Why? What about "MAJOR:MINOR"?

I confess that I am not very familiar with internal blkdev API.

Quick look reveals that there is a struct block_device, containing a
member bd_disk, which is a pointer to struct gendisk.

What is the relationship between these two? Can there be a block device
without a gendisk, for example?

Marek

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On 8/9/21 5:43 PM, Marek Behún wrote:

I confess that I am not very familiar with internal blkdev API.

It's mainly a matter of symbol visibility.  See this thread from a few
months ago:

   https://www.spinics.net/lists/linux-leds/msg18244.html

Now ... my code currently lives in block/, so there isn't actually
anything technically preventing it from iterating through the block
devices.

The reactions to Enzo's patch (which you can see in that thread) make me
think that anything that iterates through all block devices is likely to
be rejected, but maybe I'm reading too much into it.


Greg / Christoph -

(As you were the people who expressed disapproval of Enzo's patch to
export block_class and disk_type ...)

Can you weigh in on the acceptability of iterating through the block
devices (searching by name) from LED trigger code within the block
subsystem (i.e. no new symbols would need to be exported)?

This would allow the trigger to implement the sysfs API that Marek and
Pavel want.

Thanks!

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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:

On 8/9/21 5:43 PM, Marek Behún wrote:

quoted

I confess that I am not very familiar with internal blkdev API.

It's mainly a matter of symbol visibility.  See this thread from a few
months ago:

  https://www.spinics.net/lists/linux-leds/msg18244.html

Now ... my code currently lives in block/, so there isn't actually
anything technically preventing it from iterating through the block
devices.

The reactions to Enzo's patch (which you can see in that thread) make me
think that anything that iterates through all block devices is likely to
be rejected, but maybe I'm reading too much into it.


Greg / Christoph -

(As you were the people who expressed disapproval of Enzo's patch to
export block_class and disk_type ...)

Can you weigh in on the acceptability of iterating through the block
devices (searching by name) from LED trigger code within the block
subsystem (i.e. no new symbols would need to be exported)?

This would allow the trigger to implement the sysfs API that Marek and
Pavel want.

No idea, let's see the change first, we can never promise anything :)

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On Tue, 10 Aug 2021 08:35:08 +0200
Greg KH [off-list ref] wrote:

On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:

quoted

On 8/9/21 5:43 PM, Marek Behún wrote:  

quoted

I confess that I am not very familiar with internal blkdev API.  

It's mainly a matter of symbol visibility.  See this thread from a few
months ago:

  https://www.spinics.net/lists/linux-leds/msg18244.html

Now ... my code currently lives in block/, so there isn't actually
anything technically preventing it from iterating through the block
devices.

The reactions to Enzo's patch (which you can see in that thread) make me
think that anything that iterates through all block devices is likely to
be rejected, but maybe I'm reading too much into it.


Greg / Christoph -

(As you were the people who expressed disapproval of Enzo's patch to
export block_class and disk_type ...)

Can you weigh in on the acceptability of iterating through the block
devices (searching by name) from LED trigger code within the block
subsystem (i.e. no new symbols would need to be exported)?

This would allow the trigger to implement the sysfs API that Marek and
Pavel want.  

No idea, let's see the change first, we can never promise anything :)

Hi Greg,

Can't we use blkdev_get_by_path() (or blk_lookup_devt() with
blkdev_get_by_dev())?
This would open the block device and return a struct block_device *.
When the LED trigger is disabled, it would also have to release the
device.

Marek

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Hi!

I have to agree with Marek / Pali -- this is very strange interface.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

On Tue, Aug 10, 2021 at 03:38:40PM +0200, Marek Behún wrote:

On Tue, 10 Aug 2021 08:35:08 +0200
Greg KH [off-list ref] wrote:

quoted

On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:

quoted

On 8/9/21 5:43 PM, Marek Behún wrote:  

quoted

I confess that I am not very familiar with internal blkdev API.  

It's mainly a matter of symbol visibility.  See this thread from a few
months ago:

  https://www.spinics.net/lists/linux-leds/msg18244.html

Now ... my code currently lives in block/, so there isn't actually
anything technically preventing it from iterating through the block
devices.

The reactions to Enzo's patch (which you can see in that thread) make me
think that anything that iterates through all block devices is likely to
be rejected, but maybe I'm reading too much into it.


Greg / Christoph -

(As you were the people who expressed disapproval of Enzo's patch to
export block_class and disk_type ...)

Can you weigh in on the acceptability of iterating through the block
devices (searching by name) from LED trigger code within the block
subsystem (i.e. no new symbols would need to be exported)?

This would allow the trigger to implement the sysfs API that Marek and
Pavel want.  

No idea, let's see the change first, we can never promise anything :)

Hi Greg,

Can't we use blkdev_get_by_path() (or blk_lookup_devt() with
blkdev_get_by_dev())?
This would open the block device and return a struct block_device *.
When the LED trigger is disabled, it would also have to release the
device.

But what about when the device is removed from the system first?  Be
careful about that...

Anyway, sure, try those functions, I really do not know, all I
originally complained about was those exports which did not need to be
exported.

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On 8/10/21 9:48 AM, Greg KH wrote:

But what about when the device is removed from the system first?  Be
careful about that...

Anyway, sure, try those functions, I really do not know, all I
originally complained about was those exports which did not need to be
exported.

Sounds good.  I'll work something up.  (I'm actually thinking that
class_find_device() may be the best way to go, as it grabs a reference
to the device.)

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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On Tue, Aug 10, 2021 at 10:55:33AM -0500, Ian Pilcher wrote:

On 8/10/21 9:48 AM, Greg KH wrote:

quoted

But what about when the device is removed from the system first?  Be
careful about that...

Anyway, sure, try those functions, I really do not know, all I
originally complained about was those exports which did not need to be
exported.

Sounds good.  I'll work something up.  (I'm actually thinking that
class_find_device() may be the best way to go, as it grabs a reference
to the device.)

There should not be anything "odd" about block devices here, just do
whatever all other LED drivers do when referencing a device.

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On Tue, 10 Aug 2021 18:24:12 +0200
Greg KH [off-list ref] wrote:

quoted

Sounds good.  I'll work something up.  (I'm actually thinking that
class_find_device() may be the best way to go, as it grabs a reference
to the device.)  

There should not be anything "odd" about block devices here, just do
whatever all other LED drivers do when referencing a device.

Ian, look at ledtrig-tty and maybe get inspiration from there :)

Marek

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On 8/10/21 11:24 AM, Greg KH wrote:

There should not be anything "odd" about block devices here, just do
whatever all other LED drivers do when referencing a device.

AFAIK, the only LED trigger that does anything similar is the netdev
trigger.  It uses dev_get_by_name(), which is specific to network
devices.

The block subsystem doesn't appear to have any similar API, which is
why Enzo submitted his patch to export block_class and disk_type back
in April[1], when he wanted to do something similar.

I'm basically bypassing the need to export the symbols, because my
trigger code is actually in the block subsystem, rather than the LEDs
subsystem.

[1] https://www.spinics.net/lists/linux-leds/msg18244.html

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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:

On 8/9/21 5:43 PM, Marek Behún wrote:

quoted

I confess that I am not very familiar with internal blkdev API.

It's mainly a matter of symbol visibility.  See this thread from a few
months ago:

  https://www.spinics.net/lists/linux-leds/msg18244.html

Now ... my code currently lives in block/, so there isn't actually
anything technically preventing it from iterating through the block
devices.

The reactions to Enzo's patch (which you can see in that thread) make me
think that anything that iterates through all block devices is likely to
be rejected, but maybe I'm reading too much into it.

I think the main issue with this series is that it adds a shitload of
code and a hook in the absolute I/O fastpath for fricking blinkenlights.
I don't think it is even worth wasting time on something this ridiculous.

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

On Wed, 11 Aug 2021 08:26:42 +0200
Christoph Hellwig [off-list ref] wrote:

On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:

quoted

On 8/9/21 5:43 PM, Marek Behún wrote:  

quoted

I confess that I am not very familiar with internal blkdev API.  

It's mainly a matter of symbol visibility.  See this thread from a few
months ago:

  https://www.spinics.net/lists/linux-leds/msg18244.html

Now ... my code currently lives in block/, so there isn't actually
anything technically preventing it from iterating through the block
devices.

The reactions to Enzo's patch (which you can see in that thread) make me
think that anything that iterates through all block devices is likely to
be rejected, but maybe I'm reading too much into it.  

I think the main issue with this series is that it adds a shitload of
code and a hook in the absolute I/O fastpath for fricking blinkenlights.
I don't think it is even worth wasting time on something this ridiculous.

That's why I think we should do this the way the netdev trigger does.
Periodically reading block_device's stats, and if they are greater,
blink the LED.

Marek

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies