[PATCH 00/18] Introduce block device LED trigger

[PATCH 00/18] Introduce block device LED trigger

[Ian Pilcher]

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.

NOTES:

* This patch series applies cleanly to the linux-block and linux-next
  (20210903) trees.  All patches other than the block subsystem patch
  (patch #4) apply cleanly to the linux-leds tree.

* All patches compile (modulo warnings) with the trigger disabled,
  modular, or built-in.

Ian Pilcher (18):
  docs: Add block device (blkdev) LED trigger documentation
  ledtrig-blkdev: Add build infra for block device LED trigger
  ledtrig-blkdev: Add function placeholders needed by block changes
  block: Add block device LED trigger integrations
  ledtrig-blkdev: Implement functions called from block subsystem
  ledtrig-blkdev: Add function to get gendisk by name
  ledtrig-blkdev: Add constants, data types, and global variables
  ledtrig-blkdev: Add miscellaneous helper functions
  ledtrig-blkdev: Periodically check devices for activity & blink LEDs
  ledtrig-blkdev: Add function to associate the trigger with an LED
  ledtrig-blkdev: Add function to associate a device with an LED
  ledtrig-blkdev: Add function to remove LED/device association
  ledtrig-blkdev: Add function to disassociate a device from all LEDs
  ledtrig-blkdev: Add function to disassociate an LED from the trigger
  ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices
  ledtrig-blkdev: Add blink_time & interval sysfs attributes
  ledtrig-blkdev: Add mode (read/write/rw) sysfs attributue
  ledtrig-blkdev: Add initialization & exit functions

 Documentation/ABI/testing/sysfs-block         |   9 +
 .../testing/sysfs-class-led-trigger-blkdev    |  48 ++
 Documentation/leds/index.rst                  |   1 +
 Documentation/leds/ledtrig-blkdev.rst         | 144 ++++
 block/genhd.c                                 |   4 +
 drivers/leds/trigger/Kconfig                  |  18 +
 drivers/leds/trigger/Makefile                 |   2 +
 drivers/leds/trigger/ledtrig-blkdev-core.c    |  78 ++
 drivers/leds/trigger/ledtrig-blkdev.c         | 767 ++++++++++++++++++
 drivers/leds/trigger/ledtrig-blkdev.h         |  27 +
 include/linux/genhd.h                         |   3 +
 include/linux/leds.h                          |  20 +
 12 files changed, 1121 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-core.c
 create mode 100644 drivers/leds/trigger/ledtrig-blkdev.c
 create mode 100644 drivers/leds/trigger/ledtrig-blkdev.h

-- 
2.31.1

[PATCH 01/18] docs: Add block device (blkdev) LED trigger documentation

[Ian Pilcher]

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

  * /sys/class/leds/<led>/blink_time
  * /sys/class/leds/<led>/interval
  * /sys/class/leds/<led>/mode
  * /sys/class/leds/<led>/add_blkdev
  * /sys/class/leds/<led>/delete_blkdev
  * /sys/class/leds/<led>/block_devices

Add /sys/block/<disk>/blkdev_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         | 144 ++++++++++++++++++
 4 files changed, 202 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..4d27e2d98091 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>/blkdev_leds
+Date:		September 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..514f23f9a72d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
@@ -0,0 +1,48 @@
+What:		/sys/class/leds/<led>/blink_time
+Date:		September 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>/interval
+Date:		September 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.
+
+		NOTE that this attribute is a global setting.  All changes
+		apply to all LEDs associated with the blkdev LED trigger.
+
+What:		/sys/class/leds/<led>/mode
+Date:		September 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>/add_blkdev
+Date:		September 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Associate a block device with this LED by writing its kernel
+		name (as shown in /sys/block) to this attribute.  Multiple
+		device names may be written at once, separated by whitespace.
+
+What:		/sys/class/leds/<led>/delete_blkdev
+Date:		September 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Remove the association between this LED and a block device by
+		writing the device's kernel name to this attribute.  Multiple
+		device names may be written at once, separated by whitespace.
+
+What:		/sys/class/leds/<led>/block_devices
+Date:		September 2021
+Contact:	Ian Pilcher <arequipeno@gmail.com>
+Description:
+		Directory containing links to all block devices that are
+		associated with this LED.
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..1a2b6c406258
--- /dev/null
+++ b/Documentation/leds/ledtrig-blkdev.rst
@@ -0,0 +1,144 @@
+.. 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>/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.
+
+* ``add_blkdev`` and ``delete_blkdev`` are used to associate block devices with
+  this LED, and to remove associations.
+
+* ``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.
+
+* ``interval`` is the frequency (in milliseconds) with which devices are checked
+  for activity.  (Note that this is a global setting.  Any change affects all
+  LEDs associated with the ``blkdev`` trigger.)
+
+* The ``block_devices`` directory will contain a symbolic link to every device
+  that is associated with this LED.
+
+Associate the LED with the block device::
+
+	# echo sda > /sys/class/leds/<LED>/add_blkdev
+
+	# ls /sys/class/leds/<LED>/block_devices
+	sda
+
+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 sdb > /sys/class/leds/<LED>/add_blkdev
+
+	# ls /sys/class/leds/<LED>/block_devices
+	sda  sdb
+
+When a block device is associated with one or more LEDs, the LEDs are linked
+from the device's ``blkdev_leds`` directory::
+
+	# ls /sys/block/sd{a,b}/blkdev_leds
+	/sys/block/sda/blkdev_leds:
+	<LED>
+
+	/sys/block/sdb/blkdev_leds:
+	<LED>
+
+(The ``blkdev_leds`` directory only exists when the block device is associated
+with at least one LED.)
+
+The ``add_blkdev`` and ``delete_blkdev`` attributes both accept multiple,
+whitespace separated, devices.  For example::
+
+	# echo sda sdb > /sys/class/leds/<LED>/delete_blkdev
+
+	# ls /sys/class/leds/<LED>/block_devices
+
+``interval`` and ``blink_time``
+===============================
+
+* The ``interval`` attribute is a global setting.  Changing the value via
+  ``/sys/class/leds/<LED>/interval`` will affect all LEDs associated with
+  the ``blkdev`` LED trigger.
+
+* 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 (also in
+  milliseconds).  Thus (assuming that activity of the relevant type has occurred
+  on one of an LED's associated 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 currently in
+  in the process of blinking, so setting a ``blink_time`` greater than or equal
+  to ``interval`` will cause some blinks to be dropped.
+
+* 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
+  "constantly 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
+
+* 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

[PATCH 02/18] ledtrig-blkdev: Add build infra for block device LED trigger

[Ian Pilcher]

Add files:
 * ledtrig-blkdev-core.c - trigger components that are always built in
 * ledtrig-blkdev.c - trigger components that can be built as a module
 * ledtrig-blkdev.h - common declatarions

Add Kconfig options - CONFIG_LEDTRIG_BLKDEV (visible, tristate) and
CONFIG_LEDTRIG_BLKDEV_CORE (hidden, boolean)

Make CONFIG_LEDTRIG_BLKDEV select CONFIG_LEDTRIG_BLKDEV_CORE, so built-in
portion of the trigger is enabled when trigger is built-in or modular

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/Kconfig               | 18 ++++++++++++++++++
 drivers/leds/trigger/Makefile              |  2 ++
 drivers/leds/trigger/ledtrig-blkdev-core.c |  9 +++++++++
 drivers/leds/trigger/ledtrig-blkdev.c      | 16 ++++++++++++++++
 drivers/leds/trigger/ledtrig-blkdev.h      | 12 ++++++++++++
 5 files changed, 57 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-blkdev-core.c
 create mode 100644 drivers/leds/trigger/ledtrig-blkdev.c
 create mode 100644 drivers/leds/trigger/ledtrig-blkdev.h

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 1f1d57288085..7c31be355349 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -153,4 +153,22 @@ 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
+	select LEDS_TRIGGER_BLKDEV_CORE
+	help
+	  The blkdev LED trigger allows LEDs to be controlled by block device
+	  activity (reads and writes).
+
+	  See Documentation/leds/ledtrig-blkdev.rst.
+
+config LEDS_TRIGGER_BLKDEV_CORE
+	bool
+	depends on LEDS_TRIGGER_BLKDEV
+	help
+	  This enables the portion of the block device LED trigger that must
+	  be built in to the kernel, even when the trigger is built as a
+	  module.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 25c4db97cdd4..3732eeb86775 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -16,3 +16,5 @@ 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
+obj-$(CONFIG_LEDS_TRIGGER_BLKDEV_CORE)	+= ledtrig-blkdev-core.o
diff --git a/drivers/leds/trigger/ledtrig-blkdev-core.c b/drivers/leds/trigger/ledtrig-blkdev-core.c
new file mode 100644
index 000000000000..a126c7fc0f9f
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blkdev-core.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ *	Block device LED triggers - built-in components
+ *
+ *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
+ */
+
+#include "ledtrig-blkdev.h"
diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
new file mode 100644
index 000000000000..8cbeb80697ae
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ *	Block device LED triggers - modular components
+ *
+ *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
+ */
+
+#include <linux/module.h>
+
+#include "ledtrig-blkdev.h"
+
+MODULE_DESCRIPTION("Block device LED trigger");
+MODULE_AUTHOR("Ian Pilcher <arequipeno@gmail.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(LEDTRIG_BLKDEV);
diff --git a/drivers/leds/trigger/ledtrig-blkdev.h b/drivers/leds/trigger/ledtrig-blkdev.h
new file mode 100644
index 000000000000..50b44461d4ab
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blkdev.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ *	Block device LED triggers
+ *
+ *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
+ */
+
+#ifndef __LEDTRIG_BLKDEV_H
+#define __LEDTRIG_BLKDEV_H
+
+#endif	/* __LEDTRIG_BLKDEV_H */
-- 
2.31.1

[PATCH 03/18] ledtrig-blkdev: Add function placeholders needed by block changes

[Ian Pilcher]

Add ledtrig_blkdev_disk_init() and ledtrig_blkdev_disk_cleanup()
placeholders.  (Implementations depend on block subsystem changes in
next commit.)

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev-core.c | 11 +++++++++++
 include/linux/leds.h                       | 19 +++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev-core.c b/drivers/leds/trigger/ledtrig-blkdev-core.c
index a126c7fc0f9f..f5b92d37f0df 100644
--- a/drivers/leds/trigger/ledtrig-blkdev-core.c
+++ b/drivers/leds/trigger/ledtrig-blkdev-core.c
@@ -6,4 +6,15 @@
  *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
  */
 
+#include <linux/leds.h>
+
 #include "ledtrig-blkdev.h"
+
+/**
+ * ledtrig_blkdev_disk_cleanup - remove a block device from the blkdev LED
+ * trigger
+ * @gd:	the disk to be removed
+ */
+void ledtrig_blkdev_disk_cleanup(struct gendisk *const gd)
+{
+}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index a0b730be40ad..5d53921e4229 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -10,6 +10,7 @@
 
 #include <dt-bindings/leds/common.h>
 #include <linux/device.h>
+#include <linux/genhd.h>
 #include <linux/kernfs.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
@@ -605,4 +606,22 @@ static inline void ledtrig_audio_set(enum led_audio type,
 }
 #endif
 
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_BLKDEV)
+/**
+ * ledtrig_blkdev_disk_init - initialize the ledtrig field of a new gendisk
+ * @gd:	the gendisk to be initialized
+ */
+static inline void ledtrig_blkdev_disk_init(struct gendisk *const gd)
+{
+}
+void ledtrig_blkdev_disk_cleanup(struct gendisk *const gd);
+#else	/* CONFIG_LEDS_TRIGGER_BLKDEV */
+static inline void ledtrig_blkdev_disk_init(const struct gendisk *gd)
+{
+}
+static inline void ledtrig_blkdev_disk_cleanup(const struct gendisk *gd)
+{
+}
+#endif	/* CONFIG_LEDS_TRIGGER_BLKDEV */
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
2.31.1

[PATCH 04/18] block: Add block device LED trigger integrations

[Ian Pilcher]

Add LED trigger disk info pointer to gendisk structure

Call ledtrig_blkdev_disk_init() from device_add_disk() to ensure that
ledtrig is initialized to NULL, in case a driver allocates the structure
itself and doesn't use kzalloc()

Call ledtrig_blkdev_disk_cleanup() from del_gendisk() to ensure that the
LED trigger stops trying to check the disk for activity

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 block/genhd.c         | 4 ++++
 include/linux/genhd.h | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index 567549a011d1..6f340a717099 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -24,6 +24,7 @@
 #include <linux/log2.h>
 #include <linux/pm_runtime.h>
 #include <linux/badblocks.h>
+#include <linux/leds.h>
 
 #include "blk.h"
 
@@ -390,6 +391,8 @@ int device_add_disk(struct device *parent, struct gendisk *disk,
 	struct device *ddev = disk_to_dev(disk);
 	int ret;
 
+	ledtrig_blkdev_disk_init(disk);
+
 	/*
 	 * The disk queue should now be all set with enough information about
 	 * the device for the elevator code to pick an adequate default
@@ -559,6 +562,7 @@ void del_gendisk(struct gendisk *disk)
 	if (WARN_ON_ONCE(!disk_live(disk) && !(disk->flags & GENHD_FL_HIDDEN)))
 		return;
 
+	ledtrig_blkdev_disk_cleanup(disk);
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index c68d83c87f83..29039367ccad 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -166,6 +166,9 @@ struct gendisk {
 #endif	/* CONFIG_BLK_DEV_INTEGRITY */
 #if IS_ENABLED(CONFIG_CDROM)
 	struct cdrom_device_info *cdi;
+#endif
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_BLKDEV)
+	struct ledtrig_blkdev_disk *ledtrig;
 #endif
 	int node_id;
 	struct badblocks *bb;
-- 
2.31.1

[PATCH 05/18] ledtrig-blkdev: Implement functions called from block subsystem

[Ian Pilcher]

Now that the ledtrig member has been added to struct gendisk, add the
function bodies to ledtrig_blkdev_disk_init() and
ledtrig_blkdev_disk_cleanup()

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev-core.c | 15 +++++++++++++++
 drivers/leds/trigger/ledtrig-blkdev.h      |  3 +++
 include/linux/leds.h                       |  1 +
 3 files changed, 19 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev-core.c b/drivers/leds/trigger/ledtrig-blkdev-core.c
index f5b92d37f0df..102cdbe26d66 100644
--- a/drivers/leds/trigger/ledtrig-blkdev-core.c
+++ b/drivers/leds/trigger/ledtrig-blkdev-core.c
@@ -10,6 +10,13 @@
 
 #include "ledtrig-blkdev.h"
 
+DEFINE_MUTEX(ledtrig_blkdev_mutex);
+EXPORT_SYMBOL_NS_GPL(ledtrig_blkdev_mutex, LEDTRIG_BLKDEV);
+
+/* Set when module is loaded (or trigger is initialized) */
+void (*__ledtrig_blkdev_disk_cleanup)(struct gendisk *gd) = NULL;
+EXPORT_SYMBOL_NS_GPL(__ledtrig_blkdev_disk_cleanup, LEDTRIG_BLKDEV);
+
 /**
  * ledtrig_blkdev_disk_cleanup - remove a block device from the blkdev LED
  * trigger
@@ -17,4 +24,12 @@
  */
 void ledtrig_blkdev_disk_cleanup(struct gendisk *const gd)
 {
+	mutex_lock(&ledtrig_blkdev_mutex);
+
+	if (gd->ledtrig != NULL) {
+		if (!WARN_ON(__ledtrig_blkdev_disk_cleanup == NULL))
+			__ledtrig_blkdev_disk_cleanup(gd);
+	}
+
+	mutex_unlock(&ledtrig_blkdev_mutex);
 }
diff --git a/drivers/leds/trigger/ledtrig-blkdev.h b/drivers/leds/trigger/ledtrig-blkdev.h
index 50b44461d4ab..914fb1523a2f 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.h
+++ b/drivers/leds/trigger/ledtrig-blkdev.h
@@ -9,4 +9,7 @@
 #ifndef __LEDTRIG_BLKDEV_H
 #define __LEDTRIG_BLKDEV_H
 
+extern struct mutex ledtrig_blkdev_mutex;
+extern void (*__ledtrig_blkdev_disk_cleanup)(struct gendisk *gd);
+
 #endif	/* __LEDTRIG_BLKDEV_H */
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 5d53921e4229..6cf3c5de5c93 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -613,6 +613,7 @@ static inline void ledtrig_audio_set(enum led_audio type,
  */
 static inline void ledtrig_blkdev_disk_init(struct gendisk *const gd)
 {
+	gd->ledtrig = NULL;
 }
 void ledtrig_blkdev_disk_cleanup(struct gendisk *const gd);
 #else	/* CONFIG_LEDS_TRIGGER_BLKDEV */
-- 
2.31.1

[PATCH 06/18] ledtrig-blkdev: Add function to get gendisk by name

[Ian Pilcher]

Add ledtrig_blkdev_get_disk() to find block device by name and increment
its reference count.  (Caller must call put_disk().)  Must be built in to
access block_class and disk_type symbols.

Add (inline) helper function to compare C string with non-terminated
character sequence (within the buffer passed to a sysfs attribute store
function).

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev-core.c | 43 ++++++++++++++++++++++
 drivers/leds/trigger/ledtrig-blkdev.h      | 12 ++++++
 2 files changed, 55 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev-core.c b/drivers/leds/trigger/ledtrig-blkdev-core.c
index 102cdbe26d66..87d439f425ad 100644
--- a/drivers/leds/trigger/ledtrig-blkdev-core.c
+++ b/drivers/leds/trigger/ledtrig-blkdev-core.c
@@ -33,3 +33,46 @@ void ledtrig_blkdev_disk_cleanup(struct gendisk *const gd)
 
 	mutex_unlock(&ledtrig_blkdev_mutex);
 }
+
+
+/*
+ *
+ *	ledtrig_blkdev_get_disk() - get a gendisk by name
+ *
+ *	Must be built in for access to block_class and disk_type
+ *	Caller must call put_disk()
+ *
+ */
+
+/* Non-null-terminated character sequence of known length */
+struct ledtrig_blkdev_gdname {
+	const char	*buf;
+	size_t		len;
+};
+
+/* Match function for ledtrig_blkdev_get_disk() */
+static int blkdev_match_gdname(struct device *const dev, const void *const data)
+{
+	const struct ledtrig_blkdev_gdname *const gdname = data;
+
+	if (dev->type != &disk_type)
+		return 0;
+
+	return ledtrig_blkdev_streq(dev_to_disk(dev)->disk_name,
+				    gdname->buf, gdname->len);
+}
+
+struct gendisk *ledtrig_blkdev_get_disk(const char *const name,
+					const size_t len)
+{
+	const struct ledtrig_blkdev_gdname gdname = { .buf = name, .len = len };
+	struct device *dev;
+
+	dev = class_find_device(&block_class, NULL,
+				&gdname, blkdev_match_gdname);
+	if (dev == NULL)
+		return NULL;
+
+	return dev_to_disk(dev);
+}
+EXPORT_SYMBOL_NS_GPL(ledtrig_blkdev_get_disk, LEDTRIG_BLKDEV);
diff --git a/drivers/leds/trigger/ledtrig-blkdev.h b/drivers/leds/trigger/ledtrig-blkdev.h
index 914fb1523a2f..9a3528fd183a 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.h
+++ b/drivers/leds/trigger/ledtrig-blkdev.h
@@ -11,5 +11,17 @@
 
 extern struct mutex ledtrig_blkdev_mutex;
 extern void (*__ledtrig_blkdev_disk_cleanup)(struct gendisk *gd);
+extern struct gendisk *ledtrig_blkdev_get_disk(const char *name, size_t len);
+
+/*
+ * Compare a null-terminated C string with a non-null-terminated character
+ * sequence of a known length.  Returns true (1) if equal, false (0) if not.
+ */
+static inline bool ledtrig_blkdev_streq(const char *const cstr,
+					const char *const cbuf,
+					const size_t buf_len)
+{
+	return strlen(cstr) == buf_len && memcmp(cstr, cbuf, buf_len) == 0;
+}
 
 #endif	/* __LEDTRIG_BLKDEV_H */
-- 
2.31.1

[PATCH 07/18] ledtrig-blkdev: Add constants, data types, and global variables

[Ian Pilcher]

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 52 +++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 8cbeb80697ae..db8326874400 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -14,3 +14,55 @@ MODULE_DESCRIPTION("Block device LED trigger");
 MODULE_AUTHOR("Ian Pilcher <arequipeno@gmail.com>");
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(LEDTRIG_BLKDEV);
+
+/* Default blink time & polling interval (milliseconds) */
+#define LEDTRIG_BLKDEV_BLINK_MSEC	75
+#define LEDTRIG_BLKDEV_INTERVAL		100
+
+/* Minimum VALUE for interval or blink_time */
+#define LEDTRIG_BLKDEV_MIN_TIME		25
+
+enum ledtrig_blkdev_mode {
+	LEDTRIG_BLKDEV_MODE_RO	= 0,	/* blink for reads */
+	LEDTRIG_BLKDEV_MODE_WO	= 1,	/* blink for writes */
+	LEDTRIG_BLKDEV_MODE_RW	= 2	/* blink for reads and writes */
+};
+
+/* Trigger-specific info about a block device */
+struct ledtrig_blkdev_disk {
+	struct gendisk		*gd;
+	struct kobject		*dir;		/* blkdev_leds dir */
+	struct hlist_head	leds;
+	unsigned long		read_ios;
+	unsigned long		write_ios;
+	unsigned int		generation;
+	bool			read_act;
+	bool			write_act;
+};
+
+/* For many-to-many relationships between "disks" (block devices) and LEDs */
+struct ledtrig_blkdev_link {
+	struct hlist_node		disk_leds_node;
+	struct hlist_node		led_disks_node;
+	struct ledtrig_blkdev_disk	*disk;
+	struct ledtrig_blkdev_led	*led;
+};
+
+/* Every LED associated with the blkdev trigger gets one of these */
+struct ledtrig_blkdev_led {
+	struct kobject			*dir;		/* block_devices dir */
+	struct led_classdev		*led_dev;
+	unsigned int			blink_msec;
+	struct hlist_head		disks;		/* linked block devs */
+	struct hlist_node		leds_node;
+	enum ledtrig_blkdev_mode	mode;
+};
+
+/* All LEDs associated with the trigger */
+static HLIST_HEAD(ledtrig_blkdev_leds);
+
+/* Total number of device-to-LED associations */
+static unsigned int ledtrig_blkdev_count;
+
+/* How often to check for drive activity - in jiffies */
+static unsigned int ledtrig_blkdev_interval;
-- 
2.31.1

[PATCH 08/18] ledtrig-blkdev: Add miscellaneous helper functions

[Ian Pilcher]

Add blkdev_skip_space() and blkdev_find_space() for parsing writes to
sysfs attributes

Add blkdev_read_mode() and blkdev_write_mode() LED comparison helpers

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 44 +++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index db8326874400..1f319529c3be 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -6,6 +6,7 @@
  *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
  */
 
+#include <linux/ctype.h>
 #include <linux/module.h>
 
 #include "ledtrig-blkdev.h"
@@ -66,3 +67,46 @@ static unsigned int ledtrig_blkdev_count;
 
 /* How often to check for drive activity - in jiffies */
 static unsigned int ledtrig_blkdev_interval;
+
+
+/*
+ *
+ *	Miscellaneous helper functions
+ *
+ */
+
+/*
+ * Returns a pointer to the first non-whitespace character in s
+ * (or a pointer to the terminating null).
+ */
+static const char *blkdev_skip_space(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 blkdev_skip_space()).
+ */
+static const char *blkdev_find_space(const char *s)
+{
+	while (*s != 0 && !isspace(*s))
+		++s;
+
+	return s;
+}
+
+static bool blkdev_read_mode(const enum ledtrig_blkdev_mode mode)
+{
+	return mode != LEDTRIG_BLKDEV_MODE_WO;
+}
+
+static bool blkdev_write_mode(const enum ledtrig_blkdev_mode mode)
+{
+	return mode != LEDTRIG_BLKDEV_MODE_RO;
+}
-- 
2.31.1

[PATCH 09/18] ledtrig-blkdev: Periodically check devices for activity & blink LEDs

[Ian Pilcher]

Use a delayed workqueue to periodically check configured block devices for
activity since the last check.  Blink LEDs associated with devices on which
the configured type of activity (read/write) has occurred.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 88 +++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 1f319529c3be..37ba9bb3542e 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -7,7 +7,9 @@
  */
 
 #include <linux/ctype.h>
+#include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/part_stat.h>
 
 #include "ledtrig-blkdev.h"
 
@@ -68,6 +70,10 @@ static unsigned int ledtrig_blkdev_count;
 /* How often to check for drive activity - in jiffies */
 static unsigned int ledtrig_blkdev_interval;
 
+/* Delayed work used to periodically check for activity & blink LEDs */
+static void blkdev_process(struct work_struct *const work);
+static DECLARE_DELAYED_WORK(ledtrig_blkdev_work, blkdev_process);
+
 
 /*
  *
@@ -110,3 +116,85 @@ static bool blkdev_write_mode(const enum ledtrig_blkdev_mode mode)
 {
 	return mode != LEDTRIG_BLKDEV_MODE_RO;
 }
+
+
+/*
+ *
+ *	Periodically check for device acitivity and blink LEDs
+ *
+ */
+
+static void blkdev_blink(const struct ledtrig_blkdev_led *const led)
+{
+	unsigned long delay_on = READ_ONCE(led->blink_msec);
+	unsigned long delay_off = 1;	/* 0 leaves LED turned on */
+
+	led_blink_set_oneshot(led->led_dev, &delay_on, &delay_off, 0);
+}
+
+static void blkdev_update_disk(struct ledtrig_blkdev_disk *const disk,
+			       const unsigned int generation)
+{
+	const struct block_device *const part0 = disk->gd->part0;
+	const unsigned long read_ios = part_stat_read(part0, ios[STAT_READ]);
+	const unsigned long write_ios = part_stat_read(part0, ios[STAT_WRITE])
+				+ part_stat_read(part0, ios[STAT_DISCARD])
+				+ part_stat_read(part0, ios[STAT_FLUSH]);
+
+	if (disk->read_ios != read_ios) {
+		disk->read_act = true;
+		disk->read_ios = read_ios;
+	} else {
+		disk->read_act = false;
+	}
+
+	if (disk->write_ios != write_ios) {
+		disk->write_act = true;
+		disk->write_ios = write_ios;
+	} else {
+		disk->write_act = false;
+	}
+
+	disk->generation = generation;
+}
+
+static void blkdev_process(struct work_struct *const work)
+{
+	static unsigned int generation;
+
+	struct ledtrig_blkdev_led *led;
+	struct ledtrig_blkdev_link *link;
+	unsigned long delay;
+
+	if (!mutex_trylock(&ledtrig_blkdev_mutex))
+		goto exit_reschedule;
+
+	hlist_for_each_entry(led, &ledtrig_blkdev_leds, leds_node) {
+
+		hlist_for_each_entry(link, &led->disks, led_disks_node) {
+
+			struct ledtrig_blkdev_disk *const disk = link->disk;
+
+			if (disk->generation != generation)
+				blkdev_update_disk(disk, generation);
+
+			if (disk->read_act && blkdev_read_mode(led->mode)) {
+				blkdev_blink(led);
+				break;
+			}
+
+			if (disk->write_act && blkdev_write_mode(led->mode)) {
+				blkdev_blink(led);
+				break;
+			}
+		}
+	}
+
+	++generation;
+
+	mutex_unlock(&ledtrig_blkdev_mutex);
+
+exit_reschedule:
+	delay = READ_ONCE(ledtrig_blkdev_interval);
+	WARN_ON_ONCE(!schedule_delayed_work(&ledtrig_blkdev_work, delay));
+}
-- 
2.31.1

[PATCH 10/18] ledtrig-blkdev: Add function to associate the trigger with an LED

[Ian Pilcher]

Allocate per-LED data structure and initialize with default values

Create /sys/class/leds/<led>/block_devices directory

Increment module reference count.  Module can only be removed when no LEDs
are associated with the trigger.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 54 +++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 37ba9bb3542e..bbf71ff18057 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -198,3 +198,57 @@ static void blkdev_process(struct work_struct *const work)
 	delay = READ_ONCE(ledtrig_blkdev_interval);
 	WARN_ON_ONCE(!schedule_delayed_work(&ledtrig_blkdev_work, delay));
 }
+
+
+/*
+ *
+ *	Associate an LED with the blkdev trigger
+ *
+ */
+
+static int blkdev_activate(struct led_classdev *const led_dev)
+{
+	struct ledtrig_blkdev_led *led;
+	int ret;
+
+	if (WARN_ON(!try_module_get(THIS_MODULE))) {
+		ret = -ENODEV;		/* -ESHOULDNEVERHAPPEN */
+		goto exit_return;
+	}
+
+	led = kmalloc(sizeof(*led), GFP_KERNEL);
+	if (led == NULL) {
+		ret = -ENOMEM;
+		goto exit_put_module;
+	}
+
+	led->led_dev = led_dev;
+	led->blink_msec = LEDTRIG_BLKDEV_BLINK_MSEC;
+	led->mode = LEDTRIG_BLKDEV_MODE_RW;
+	INIT_HLIST_HEAD(&led->disks);
+
+	ret = mutex_lock_interruptible(&ledtrig_blkdev_mutex);
+	if (ret != 0)
+		goto exit_free;
+
+	led->dir = kobject_create_and_add("block_devices", &led_dev->dev->kobj);
+	if (led->dir == NULL) {
+		ret = -ENOMEM;
+		goto exit_unlock;
+	}
+
+	hlist_add_head(&led->leds_node, &ledtrig_blkdev_leds);
+	led_set_trigger_data(led_dev, led);
+	ret = 0;
+
+exit_unlock:
+	mutex_unlock(&ledtrig_blkdev_mutex);
+exit_free:
+	if (ret != 0)
+		kfree(led);
+exit_put_module:
+	if (ret != 0)
+		module_put(THIS_MODULE);
+exit_return:
+	return ret;
+}
-- 
2.31.1

[PATCH 11/18] ledtrig-blkdev: Add function to associate a device with an LED

[Ian Pilcher]

If this is the first LED associated with the device, create the
/sys/block/<disk>/blkdev_leds directory.  Otherwise, increment its
reference count.

Create symlinks in /sys/class/leds/<led>/block_devices and
/sys/block/<disk>/blkdev_leds

If this the first device associated with any LED, schedule delayed work
to periodically check associated devices and blink LEDs

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 157 ++++++++++++++++++++++++++
 1 file changed, 157 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index bbf71ff18057..e3169d1f0e38 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -252,3 +252,160 @@ static int blkdev_activate(struct led_classdev *const led_dev)
 exit_return:
 	return ret;
 }
+
+
+/*
+ *
+ *	Associate a block device with an LED
+ *
+ */
+
+/* Gets or allocs & initializes the blkdev disk for a gendisk */
+static int blkdev_get_disk(struct gendisk *const gd)
+{
+	struct ledtrig_blkdev_disk *disk;
+	struct kobject *dir;
+
+	if (gd->ledtrig != NULL) {
+		kobject_get(gd->ledtrig->dir);
+		return 0;
+	}
+
+	disk = kmalloc(sizeof(*disk), GFP_KERNEL);
+	if (disk == NULL)
+		return -ENOMEM;
+
+	dir = kobject_create_and_add("blkdev_leds", &disk_to_dev(gd)->kobj);
+	if (dir == NULL) {
+		kfree(disk);
+		return -ENOMEM;
+	}
+
+	INIT_HLIST_HEAD(&disk->leds);
+	disk->gd = gd;
+	disk->dir = dir;
+	disk->read_ios = 0;
+	disk->write_ios = 0;
+
+	gd->ledtrig = disk;
+
+	return 0;
+}
+
+static void blkdev_put_disk(struct ledtrig_blkdev_disk *const disk)
+{
+	kobject_put(disk->dir);
+
+	if (hlist_empty(&disk->leds)) {
+		disk->gd->ledtrig = NULL;
+		kfree(disk);
+	}
+}
+
+static int blkdev_disk_add_locked(struct ledtrig_blkdev_led *const led,
+				  struct gendisk *const gd)
+{
+	struct ledtrig_blkdev_link *link;
+	struct ledtrig_blkdev_disk *disk;
+	unsigned long delay;
+	int ret;
+
+	link = kmalloc(sizeof(*link), GFP_KERNEL);
+	if (link == NULL) {
+		ret = -ENOMEM;
+		goto error_return;
+	}
+
+	ret = blkdev_get_disk(gd);
+	if (ret != 0)
+		goto error_free_link;
+
+	disk = gd->ledtrig;
+
+	ret = sysfs_create_link(disk->dir, &led->led_dev->dev->kobj,
+				led->led_dev->name);
+	if (ret != 0)
+		goto error_put_disk;
+
+	ret = sysfs_create_link(led->dir, &disk_to_dev(gd)->kobj,
+				gd->disk_name);
+	if (ret != 0)
+		goto error_remove_link;
+
+	link->disk = disk;
+	link->led = led;
+	hlist_add_head(&link->led_disks_node, &led->disks);
+	hlist_add_head(&link->disk_leds_node, &disk->leds);
+
+	if (ledtrig_blkdev_count == 0) {
+		delay = READ_ONCE(ledtrig_blkdev_interval);
+		WARN_ON(!schedule_delayed_work(&ledtrig_blkdev_work, delay));
+	}
+
+	++ledtrig_blkdev_count;
+
+	return 0;
+
+error_remove_link:
+	sysfs_remove_link(disk->dir, led->led_dev->name);
+error_put_disk:
+	blkdev_put_disk(disk);
+error_free_link:
+	kfree(link);
+error_return:
+	return ret;
+}
+
+static bool blkdev_already_linked(const struct ledtrig_blkdev_led *const led,
+				  const struct gendisk *const gd)
+{
+	const struct ledtrig_blkdev_link *link;
+
+	if (gd->ledtrig == NULL)
+		return false;
+
+	hlist_for_each_entry(link, &gd->ledtrig->leds, disk_leds_node) {
+
+		if (link->led == led) {
+			pr_info("blkdev LED: %s already associated with %s\n",
+				gd->disk_name, led->led_dev->name);
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static int blkdev_disk_add(struct ledtrig_blkdev_led *const led,
+			   const char *const disk_name, const size_t name_len)
+{
+	struct gendisk *gd;
+	int ret;
+
+	ret = mutex_lock_interruptible(&ledtrig_blkdev_mutex);
+	if (ret != 0)
+		goto exit_return;
+
+	gd = ledtrig_blkdev_get_disk(disk_name, name_len);
+	if (gd == NULL) {
+		pr_info("blkdev LED: no such block device %.*s\n",
+			(int)name_len, disk_name);
+		ret = -ENODEV;
+		goto exit_unlock;
+	}
+
+	if (blkdev_already_linked(led, gd)) {
+		ret = -EEXIST;
+		goto exit_put_disk;
+	}
+
+	ret = blkdev_disk_add_locked(led, gd);
+
+exit_put_disk:
+	if (ret != 0)
+		put_disk(gd);
+exit_unlock:
+	mutex_unlock(&ledtrig_blkdev_mutex);
+exit_return:
+	return ret;
+}
-- 
2.31.1

[PATCH 12/18] ledtrig-blkdev: Add function to remove LED/device association

[Ian Pilcher]

Remove symlinks in /sys/class/leds/<led>/block_devices and
/sys/block/<disk>/blkdev_leds

Decrement reference count on /sys/block/<disk>/blkdev_leds
directory (removes directory when empty)

Cancel delayed work if no device is associated with any LED

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 56 +++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index e3169d1f0e38..7edbc48050a5 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -409,3 +409,59 @@ static int blkdev_disk_add(struct ledtrig_blkdev_led *const led,
 exit_return:
 	return ret;
 }
+
+
+/*
+ *
+ *	Disassociate a block device from an LED
+ *
+ */
+
+static void blkdev_disk_del_locked(struct ledtrig_blkdev_led *const led,
+				   struct ledtrig_blkdev_link *const link,
+				   struct ledtrig_blkdev_disk *const disk)
+{
+	--ledtrig_blkdev_count;
+
+	if (ledtrig_blkdev_count == 0)
+		WARN_ON(!cancel_delayed_work_sync(&ledtrig_blkdev_work));
+
+	sysfs_remove_link(led->dir, disk->gd->disk_name);
+	sysfs_remove_link(disk->dir, led->led_dev->name);
+	kobject_put(disk->dir);
+
+	hlist_del(&link->led_disks_node);
+	hlist_del(&link->disk_leds_node);
+	kfree(link);
+
+	if (hlist_empty(&disk->leds)) {
+		disk->gd->ledtrig = NULL;
+		kfree(disk);
+	}
+
+	put_disk(disk->gd);
+}
+
+static void blkdev_disk_delete(struct ledtrig_blkdev_led *const led,
+			       const char *const disk_name,
+			       const size_t name_len)
+{
+	struct ledtrig_blkdev_link *link;
+
+	mutex_lock(&ledtrig_blkdev_mutex);
+
+	hlist_for_each_entry(link, &led->disks, led_disks_node) {
+
+		if (ledtrig_blkdev_streq(link->disk->gd->disk_name,
+						disk_name, name_len)) {
+			blkdev_disk_del_locked(led, link, link->disk);
+			goto exit_unlock;
+		}
+	}
+
+	pr_info("blkdev LED: %.*s not associated with LED %s\n",
+		(int)name_len, disk_name, led->led_dev->name);
+
+exit_unlock:
+	mutex_unlock(&ledtrig_blkdev_mutex);
+}
-- 
2.31.1

[PATCH 13/18] ledtrig-blkdev: Add function to disassociate a device from all LEDs

[Ian Pilcher]

Called when block device is being removed

Called via __ledtrig_blkdev_disk_cleanup function pointer from
ledtrig_blkdev_disk_cleanup() in ledtrig-blkdev-core.c

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 7edbc48050a5..1be9de431c08 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -465,3 +465,20 @@ static void blkdev_disk_delete(struct ledtrig_blkdev_led *const led,
 exit_unlock:
 	mutex_unlock(&ledtrig_blkdev_mutex);
 }
+
+
+/*
+ *
+ *	Disassociate all LEDs from a block device (because it's going away)
+ *
+ */
+
+static void blkdev_disk_cleanup(struct gendisk *const gd)
+{
+	struct ledtrig_blkdev_link *link;
+	struct hlist_node *next;
+
+	hlist_for_each_entry_safe(link, next,
+				  &gd->ledtrig->leds, disk_leds_node)
+		blkdev_disk_del_locked(link->led, link, gd->ledtrig);
+}
-- 
2.31.1

[PATCH 14/18] ledtrig-blkdev: Add function to disassociate an LED from the trigger

[Ian Pilcher]

Remove all device associations with this LED

Remove /sys/class/leds/<led>/block_devices directory

Free per-LED data structure

Decrement module reference count

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 1be9de431c08..b2ec85b805d0 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -482,3 +482,30 @@ static void blkdev_disk_cleanup(struct gendisk *const gd)
 				  &gd->ledtrig->leds, disk_leds_node)
 		blkdev_disk_del_locked(link->led, link, gd->ledtrig);
 }
+
+
+/*
+ *
+ *	Disassociate an LED from the trigger
+ *
+ */
+
+static void blkdev_deactivate(struct led_classdev *const led_dev)
+{
+	struct ledtrig_blkdev_led *const led = led_get_trigger_data(led_dev);
+	struct ledtrig_blkdev_link *link;
+	struct hlist_node *next;
+
+	mutex_lock(&ledtrig_blkdev_mutex);
+
+	hlist_for_each_entry_safe(link, next, &led->disks, led_disks_node)
+		blkdev_disk_del_locked(led, link, link->disk);
+
+	hlist_del(&led->leds_node);
+	kobject_put(led->dir);
+	kfree(led);
+
+	mutex_unlock(&ledtrig_blkdev_mutex);
+
+	module_put(THIS_MODULE);
+}
-- 
2.31.1

[PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Ian Pilcher]

/sys/class/leds/<led>/add_blkdev - to create device/LED associations

/sys/class/leds/<led>/delete_blkdev to remove device/LED associations

For both attributes, accept multiple device names separated by whitespace

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 48 +++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index b2ec85b805d0..db82d37fc721 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -509,3 +509,51 @@ static void blkdev_deactivate(struct led_classdev *const led_dev)
 
 	module_put(THIS_MODULE);
 }
+
+
+/*
+ *
+ *	sysfs attributes to add & delete devices from LEDs
+ *
+ */
+
+static ssize_t blkdev_add_or_del(struct device *const dev,
+				 struct device_attribute *const attr,
+				 const char *const buf, const size_t count);
+
+static struct device_attribute ledtrig_blkdev_attr_add =
+	__ATTR(add_blkdev, 0200, NULL, blkdev_add_or_del);
+
+static struct device_attribute ledtrig_blkdev_attr_del =
+	__ATTR(delete_blkdev, 0200, NULL, blkdev_add_or_del);
+
+static ssize_t blkdev_add_or_del(struct device *const dev,
+				 struct device_attribute *const attr,
+				 const char *const buf, const size_t count)
+{
+	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
+	const char *const disk_name = blkdev_skip_space(buf);
+	const char *const endp = blkdev_find_space(disk_name);
+	const ptrdiff_t name_len = endp - disk_name;	/* always >= 0 */
+	int ret;
+
+	if (name_len == 0) {
+		pr_info("blkdev LED: empty block device name\n");
+		return -EINVAL;
+	}
+
+	if (attr == &ledtrig_blkdev_attr_del) {
+		blkdev_disk_delete(led, disk_name, name_len);
+	} else {	/* attr == &ledtrig_blkdev_attr_add */
+		ret = blkdev_disk_add(led, disk_name, name_len);
+		if (ret != 0)
+			return ret;
+	}
+
+	/*
+	 * Consume everything up to the next non-whitespace token (or the end
+	 * of the input).  Avoids "empty block device name" error if there is
+	 * whitespace (such as a newline) after the last token.
+	 */
+	return blkdev_skip_space(endp) - buf;
+}
-- 
2.31.1

[PATCH 16/18] ledtrig-blkdev: Add blink_time & interval sysfs attributes

[Ian Pilcher]

/sys/class/leds/<led>/blink_time controls - per-LED blink duration

/sys/class/leds/<led>/interval - global frequency with which devices
are checked for activity and LEDs are blinked

Enforce 25 millisecond minimum for both attributes

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 63 +++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index db82d37fc721..b23a5c98de98 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -557,3 +557,66 @@ static ssize_t blkdev_add_or_del(struct device *const dev,
 	 */
 	return blkdev_skip_space(endp) - buf;
 }
+
+
+/*
+ *
+ *	blink_time & interval device attributes
+ *
+ */
+
+static ssize_t blkdev_time_show(struct device *const dev,
+				      struct device_attribute *const attr,
+				      char *const buf);
+
+static ssize_t blkdev_time_store(struct device *const dev,
+				 struct device_attribute *const attr,
+				 const char *const buf, const size_t count);
+
+static struct device_attribute ledtrig_blkdev_attr_blink_time =
+	__ATTR(blink_time, 0644, blkdev_time_show, blkdev_time_store);
+
+static struct device_attribute ledtrig_blkdev_attr_interval =
+	__ATTR(interval, 0644, blkdev_time_show, blkdev_time_store);
+
+static ssize_t blkdev_time_show(struct device *const dev,
+				struct device_attribute *const attr,
+				char *const buf)
+{
+	const struct ledtrig_blkdev_led *const led =
+						led_trigger_get_drvdata(dev);
+	unsigned int value;
+
+	if (attr == &ledtrig_blkdev_attr_blink_time)
+		value = READ_ONCE(led->blink_msec);
+	else	// attr == &ledtrig_blkdev_attr_interval
+		value = jiffies_to_msecs(READ_ONCE(ledtrig_blkdev_interval));
+
+	return sprintf(buf, "%u\n", value);
+}
+
+static ssize_t blkdev_time_store(struct device *const dev,
+				 struct device_attribute *const attr,
+				 const char *const buf, const size_t count)
+{
+	struct ledtrig_blkdev_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 < LEDTRIG_BLKDEV_MIN_TIME) {
+		pr_info("blkdev LED: attempt to set time < %s milliseconds\n",
+			__stringify(LEDTRIG_BLKDEV_MIN_TIME));
+		return -ERANGE;
+	}
+
+	if (attr == &ledtrig_blkdev_attr_blink_time)
+		WRITE_ONCE(led->blink_msec, value);
+	else	// attr == &ledtrig_blkdev_attr_interval
+		WRITE_ONCE(ledtrig_blkdev_interval, msecs_to_jiffies(value));
+
+	return count;
+}
-- 
2.31.1

[PATCH 17/18] ledtrig-blkdev: Add mode (read/write/rw) sysfs attributue

[Ian Pilcher]

Show all modes, with current mode in square brackets, in show function

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 67 +++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index b23a5c98de98..ec167633e329 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -620,3 +620,70 @@ static ssize_t blkdev_time_store(struct device *const dev,
 
 	return count;
 }
+
+
+/*
+ *
+ *	LED mode device attribute
+ *
+ */
+
+static const struct {
+	const char	*name;
+	const char	*show;
+} blkdev_modes[] = {
+	[LEDTRIG_BLKDEV_MODE_RO] = {
+		.name	= "read",
+		.show	= "[read] write rw\n",
+	},
+	[LEDTRIG_BLKDEV_MODE_WO] = {
+		.name	= "write",
+		.show	= "read [write] rw\n",
+	},
+	[LEDTRIG_BLKDEV_MODE_RW] = {
+		.name	= "rw",
+		.show	= "read write [rw]\n",
+	},
+};
+
+static ssize_t blkdev_mode_show(struct device *const dev,
+				struct device_attribute *const attr,
+				char *const buf)
+{
+	const struct ledtrig_blkdev_led *const led =
+					led_trigger_get_drvdata(dev);
+
+	return sprintf(buf, blkdev_modes[READ_ONCE(led->mode)].show);
+}
+
+static ssize_t blkdev_mode_store(struct device *const dev,
+				 struct device_attribute *const attr,
+				 const char *const buf, const size_t count)
+{
+	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
+	const char *const mode_name = blkdev_skip_space(buf);
+	const char *const endp = blkdev_find_space(mode_name);
+	const ptrdiff_t name_len = endp - mode_name;	/* always >= 0 */
+	enum ledtrig_blkdev_mode mode;
+
+	if (name_len == 0) {
+		pr_info("blkdev LED: empty mode\n");
+		return -EINVAL;
+	}
+
+	for (mode = LEDTRIG_BLKDEV_MODE_RO;
+				mode <= LEDTRIG_BLKDEV_MODE_RW; ++mode) {
+
+		if (ledtrig_blkdev_streq(blkdev_modes[mode].name,
+						mode_name, name_len)) {
+			WRITE_ONCE(led->mode, mode);
+			return count;
+		}
+	}
+
+	pr_info("blkdev LED: invalid mode (%.*s)\n", (int)name_len, mode_name);
+	return -EINVAL;
+}
+
+static struct device_attribute ledtrig_blkdev_attr_mode =
+	__ATTR(mode, 0644, blkdev_mode_show, blkdev_mode_store);
-- 
2.31.1

[PATCH 18/18] ledtrig-blkdev: Add initialization & exit functions

[Ian Pilcher]

Init function:
 * Initialize interval (convert default value to jiffies)
 * Initialize __ledtrig_blkdev_disk_cleanup function pointer
 * Register the block device LED trigger

Exit functioon:
 * Unregister the LED trigger
 * Set __ledtrig_blkdev_disk_cleanup back to NULL

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 78 +++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index ec167633e329..e6ff5baada2e 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -687,3 +687,81 @@ static ssize_t blkdev_mode_store(struct device *const dev,
 
 static struct device_attribute ledtrig_blkdev_attr_mode =
 	__ATTR(mode, 0644, blkdev_mode_show, blkdev_mode_store);
+
+
+/*
+ *
+ *	Initialization - register the trigger
+ *
+ */
+
+static struct attribute *ledtrig_blkdev_attrs[] = {
+	&ledtrig_blkdev_attr_add.attr,
+	&ledtrig_blkdev_attr_del.attr,
+	&ledtrig_blkdev_attr_blink_time.attr,
+	&ledtrig_blkdev_attr_interval.attr,
+	&ledtrig_blkdev_attr_mode.attr,
+	NULL
+};
+
+static const struct attribute_group ledtrig_blkdev_attr_group = {
+	.attrs	= ledtrig_blkdev_attrs,
+};
+
+static const struct attribute_group *ledtrig_blkdev_attr_groups[] = {
+	&ledtrig_blkdev_attr_group,
+	NULL
+};
+
+static struct led_trigger ledtrig_blkdev_trigger = {
+	.name		= "blkdev",
+	.activate	= blkdev_activate,
+	.deactivate	= blkdev_deactivate,
+	.groups		= ledtrig_blkdev_attr_groups,
+};
+
+static int __init blkdev_init(void)
+{
+	int ret;
+
+	ret = mutex_lock_interruptible(&ledtrig_blkdev_mutex);
+	if (ret != 0)
+		return ret;
+
+	ledtrig_blkdev_interval = msecs_to_jiffies(LEDTRIG_BLKDEV_INTERVAL);
+	__ledtrig_blkdev_disk_cleanup = blkdev_disk_cleanup;
+
+	/*
+	 * Can't call led_trigger_register() with ledtrig_blkdev_mutex locked.
+	 * If an LED has blkdev as its default_trigger, blkdev_activate() will
+	 * be called for that LED, and it will try to lock the mutex, which will
+	 * hang.
+	 */
+	mutex_unlock(&ledtrig_blkdev_mutex);
+
+	ret = led_trigger_register(&ledtrig_blkdev_trigger);
+	if (ret != 0) {
+		mutex_lock(&ledtrig_blkdev_mutex);
+		__ledtrig_blkdev_disk_cleanup = NULL;
+		mutex_unlock(&ledtrig_blkdev_mutex);
+	}
+
+	return ret;
+}
+module_init(blkdev_init);
+
+static void __exit blkdev_exit(void)
+{
+	mutex_lock(&ledtrig_blkdev_mutex);
+
+	/*
+	 * It's OK to call led_trigger_unregister() with the mutex locked,
+	 * because the module can only be unloaded when no LEDs are using
+	 * the blkdev trigger, so blkdev_deactivate() won't be called.
+	 */
+	led_trigger_unregister(&ledtrig_blkdev_trigger);
+	__ledtrig_blkdev_disk_cleanup = NULL;
+
+	mutex_unlock(&ledtrig_blkdev_mutex);
+}
+module_exit(blkdev_exit);
-- 
2.31.1

Re: [PATCH 17/18] ledtrig-blkdev: Add mode (read/write/rw) sysfs attributue

[Greg KH]

On Fri, Sep 03, 2021 at 03:45:47PM -0500, Ian Pilcher wrote:
> +static ssize_t blkdev_mode_store(struct device *const dev,
> +				 struct device_attribute *const attr,
> +				 const char *const buf, const size_t count)
> +{
> +	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> +	const char *const mode_name = blkdev_skip_space(buf);
> +	const char *const endp = blkdev_find_space(mode_name);
> +	const ptrdiff_t name_len = endp - mode_name;	/* always >= 0 */
> +	enum ledtrig_blkdev_mode mode;
> +
> +	if (name_len == 0) {
> +		pr_info("blkdev LED: empty mode\n");
> +		return -EINVAL;
> +	}
> +
> +	for (mode = LEDTRIG_BLKDEV_MODE_RO;
> +				mode <= LEDTRIG_BLKDEV_MODE_RW; ++mode) {
> +
> +		if (ledtrig_blkdev_streq(blkdev_modes[mode].name,
> +						mode_name, name_len)) {
> +			WRITE_ONCE(led->mode, mode);
> +			return count;
> +		}
> +	}
> +
> +	pr_info("blkdev LED: invalid mode (%.*s)\n", (int)name_len, mode_name);

Please do not use pr_* calls in a driver when you have a struct device.

Also, you are now allowing any user to spam the kernel log, this shold
be a dev_dbg() call at the most, if it is even needed at all.  Same for
the other pr_info() call in this function, please remove them all.

thanks,

greg k-h

Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Greg KH]

On Fri, Sep 03, 2021 at 03:45:45PM -0500, Ian Pilcher wrote:
> /sys/class/leds/<led>/add_blkdev - to create device/LED associations
> 
> /sys/class/leds/<led>/delete_blkdev to remove device/LED associations
> 
> For both attributes, accept multiple device names separated by whitespace
> 
> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-blkdev.c | 48 +++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)

Where are the Documentation/ABI/ updates for these new sysfs files?

thanks,

greg k-h

Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Greg KH]

On Fri, Sep 03, 2021 at 03:45:45PM -0500, Ian Pilcher wrote:
> /sys/class/leds/<led>/add_blkdev - to create device/LED associations
> 
> /sys/class/leds/<led>/delete_blkdev to remove device/LED associations
> 
> For both attributes, accept multiple device names separated by whitespace
> 
> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-blkdev.c | 48 +++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
> index b2ec85b805d0..db82d37fc721 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev.c
> @@ -509,3 +509,51 @@ static void blkdev_deactivate(struct led_classdev *const led_dev)
>  
>  	module_put(THIS_MODULE);
>  }
> +
> +
> +/*
> + *
> + *	sysfs attributes to add & delete devices from LEDs
> + *
> + */
> +
> +static ssize_t blkdev_add_or_del(struct device *const dev,
> +				 struct device_attribute *const attr,
> +				 const char *const buf, const size_t count);
> +
> +static struct device_attribute ledtrig_blkdev_attr_add =
> +	__ATTR(add_blkdev, 0200, NULL, blkdev_add_or_del);
> +
> +static struct device_attribute ledtrig_blkdev_attr_del =
> +	__ATTR(delete_blkdev, 0200, NULL, blkdev_add_or_del);

DEVICE_ATTR_RO()?  Or something like that?  Do not use __ATTR() for
device attributes if at all possible, worst case, use DEVICE_ATTR()
here.

And the mode settings are odd, are you sure you want that?

> +static ssize_t blkdev_add_or_del(struct device *const dev,
> +				 struct device_attribute *const attr,
> +				 const char *const buf, const size_t count)
> +{
> +	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> +	const char *const disk_name = blkdev_skip_space(buf);
> +	const char *const endp = blkdev_find_space(disk_name);
> +	const ptrdiff_t name_len = endp - disk_name;	/* always >= 0 */
> +	int ret;
> +
> +	if (name_len == 0) {
> +		pr_info("blkdev LED: empty block device name\n");

Looks like debugging code, please remove.

And how can this ever happen?

> +		return -EINVAL;
> +	}
> +
> +	if (attr == &ledtrig_blkdev_attr_del) {
> +		blkdev_disk_delete(led, disk_name, name_len);
> +	} else {	/* attr == &ledtrig_blkdev_attr_add */
> +		ret = blkdev_disk_add(led, disk_name, name_len);

Why do you have a single attribute callback that does two totally
different things?  Just have 2 different callback functions please, it
makes things much easier to review and maintain over time.

thanks,

greg k-h

Re: [PATCH 08/18] ledtrig-blkdev: Add miscellaneous helper functions

[Greg KH]

On Fri, Sep 03, 2021 at 03:45:38PM -0500, Ian Pilcher wrote:
> Add blkdev_skip_space() and blkdev_find_space() for parsing writes to
> sysfs attributes
> 
> Add blkdev_read_mode() and blkdev_write_mode() LED comparison helpers
> 
> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-blkdev.c | 44 +++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
> index db8326874400..1f319529c3be 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev.c
> @@ -6,6 +6,7 @@
>   *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
>   */
>  
> +#include <linux/ctype.h>
>  #include <linux/module.h>
>  
>  #include "ledtrig-blkdev.h"
> @@ -66,3 +67,46 @@ static unsigned int ledtrig_blkdev_count;
>  
>  /* How often to check for drive activity - in jiffies */
>  static unsigned int ledtrig_blkdev_interval;
> +
> +
> +/*
> + *
> + *	Miscellaneous helper functions
> + *
> + */
> +
> +/*
> + * Returns a pointer to the first non-whitespace character in s
> + * (or a pointer to the terminating null).
> + */
> +static const char *blkdev_skip_space(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 blkdev_skip_space()).
> + */
> +static const char *blkdev_find_space(const char *s)
> +{
> +	while (*s != 0 && !isspace(*s))
> +		++s;
> +
> +	return s;
> +}

Why are block devices odd and need to have spaces found in their names?

And are you sure we do not already have string functions that do this?

thanks,

greg k-h

Re: [PATCH 09/18] ledtrig-blkdev: Periodically check devices for activity & blink LEDs

[Greg KH]

On Fri, Sep 03, 2021 at 03:45:39PM -0500, Ian Pilcher wrote:
> Use a delayed workqueue to periodically check configured block devices for
> activity since the last check.  Blink LEDs associated with devices on which
> the configured type of activity (read/write) has occurred.
> 
> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-blkdev.c | 88 +++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
> index 1f319529c3be..37ba9bb3542e 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev.c
> @@ -7,7 +7,9 @@
>   */
>  
>  #include <linux/ctype.h>
> +#include <linux/leds.h>
>  #include <linux/module.h>
> +#include <linux/part_stat.h>
>  
>  #include "ledtrig-blkdev.h"
>  
> @@ -68,6 +70,10 @@ static unsigned int ledtrig_blkdev_count;
>  /* How often to check for drive activity - in jiffies */
>  static unsigned int ledtrig_blkdev_interval;
>  
> +/* Delayed work used to periodically check for activity & blink LEDs */
> +static void blkdev_process(struct work_struct *const work);
> +static DECLARE_DELAYED_WORK(ledtrig_blkdev_work, blkdev_process);
> +
>  
>  /*
>   *
> @@ -110,3 +116,85 @@ static bool blkdev_write_mode(const enum ledtrig_blkdev_mode mode)
>  {
>  	return mode != LEDTRIG_BLKDEV_MODE_RO;
>  }
> +
> +
> +/*
> + *
> + *	Periodically check for device acitivity and blink LEDs
> + *
> + */
> +
> +static void blkdev_blink(const struct ledtrig_blkdev_led *const led)
> +{
> +	unsigned long delay_on = READ_ONCE(led->blink_msec);
> +	unsigned long delay_off = 1;	/* 0 leaves LED turned on */
> +
> +	led_blink_set_oneshot(led->led_dev, &delay_on, &delay_off, 0);
> +}
> +
> +static void blkdev_update_disk(struct ledtrig_blkdev_disk *const disk,
> +			       const unsigned int generation)
> +{
> +	const struct block_device *const part0 = disk->gd->part0;
> +	const unsigned long read_ios = part_stat_read(part0, ios[STAT_READ]);
> +	const unsigned long write_ios = part_stat_read(part0, ios[STAT_WRITE])
> +				+ part_stat_read(part0, ios[STAT_DISCARD])
> +				+ part_stat_read(part0, ios[STAT_FLUSH]);
> +
> +	if (disk->read_ios != read_ios) {
> +		disk->read_act = true;
> +		disk->read_ios = read_ios;
> +	} else {
> +		disk->read_act = false;
> +	}
> +
> +	if (disk->write_ios != write_ios) {
> +		disk->write_act = true;
> +		disk->write_ios = write_ios;
> +	} else {
> +		disk->write_act = false;
> +	}
> +
> +	disk->generation = generation;
> +}
> +
> +static void blkdev_process(struct work_struct *const work)
> +{
> +	static unsigned int generation;
> +
> +	struct ledtrig_blkdev_led *led;
> +	struct ledtrig_blkdev_link *link;
> +	unsigned long delay;
> +
> +	if (!mutex_trylock(&ledtrig_blkdev_mutex))
> +		goto exit_reschedule;
> +
> +	hlist_for_each_entry(led, &ledtrig_blkdev_leds, leds_node) {
> +
> +		hlist_for_each_entry(link, &led->disks, led_disks_node) {
> +
> +			struct ledtrig_blkdev_disk *const disk = link->disk;
> +
> +			if (disk->generation != generation)
> +				blkdev_update_disk(disk, generation);
> +
> +			if (disk->read_act && blkdev_read_mode(led->mode)) {
> +				blkdev_blink(led);
> +				break;
> +			}
> +
> +			if (disk->write_act && blkdev_write_mode(led->mode)) {
> +				blkdev_blink(led);
> +				break;
> +			}
> +		}
> +	}
> +
> +	++generation;
> +
> +	mutex_unlock(&ledtrig_blkdev_mutex);
> +
> +exit_reschedule:
> +	delay = READ_ONCE(ledtrig_blkdev_interval);
> +	WARN_ON_ONCE(!schedule_delayed_work(&ledtrig_blkdev_work, delay));

You just rebooted a machine if it hit this :(

Please never use WARN_ON() in new code unless the machine is really
broken and you can not do anything else here.

thanks,

greg k-h

Re: [PATCH 01/18] docs: Add block device (blkdev) LED trigger documentation

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

Hi!

> Add Documentation/ABI/testing/sysfs-class-led-trigger-blkdev to
> document:
> 
>   * /sys/class/leds/<led>/blink_time
>   * /sys/class/leds/<led>/interval
>   * /sys/class/leds/<led>/mode
>   * /sys/class/leds/<led>/add_blkdev
>   * /sys/class/leds/<led>/delete_blkdev
>   * /sys/class/leds/<led>/block_devices
> 
> Add /sys/block/<disk>/blkdev_leds to Documentation/ABI/testing/sysfs-block
> 
> Add overview in Documentation/leds/ledtrig-blkdev.rst

> +What:		/sys/class/leds/<led>/add_blkdev
> +Date:		September 2021
> +Contact:	Ian Pilcher <arequipeno@gmail.com>
> +Description:
> +		Associate a block device with this LED by writing its kernel
> +		name (as shown in /sys/block) to this attribute.  Multiple
> +		device names may be written at once, separated by whitespace.

This is seriously strange interface.

> +What:		/sys/class/leds/<led>/delete_blkdev
> +Date:		September 2021
> +Contact:	Ian Pilcher <arequipeno@gmail.com>
> +Description:
> +		Remove the association between this LED and a block device by
> +		writing the device's kernel name to this attribute.  Multiple
> +		device names may be written at once, separated by whitespace.
> +
> +What:		/sys/class/leds/<led>/block_devices
> +Date:		September 2021
> +Contact:	Ian Pilcher <arequipeno@gmail.com>
> +Description:
> +		Directory containing links to all block devices that are
> +		associated with this LED.

If you have directory with symlinks, why not use symlink() syscall
instead of add_blkdev, and unlink() syscall instead of delete_blkdev?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 00/18] Introduce block device LED trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 297 bytes --]

Hi!

> This patch series adds a new "blkdev" LED trigger for disk (or other block
> device) activity LEDs.
> 
> It has the following functionality.

Correct address for lkml is linux-kernel@vger, not linux@vger.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 03/18] ledtrig-blkdev: Add function placeholders needed by block changes

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 12738 bytes --]

Hi Ian,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on linus/master v5.14 next-20210903]
[cannot apply to pavel-linux-leds/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ian-Pilcher/Introduce-block-device-LED-trigger/20210904-044701
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: arm-randconfig-r032-20210904 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 6fe2beba7d2a41964af658c8c59dd172683ef739)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/555c9bcdaa7524206ca62c64730b800037ffa9a0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ian-Pilcher/Introduce-block-device-LED-trigger/20210904-044701
        git checkout 555c9bcdaa7524206ca62c64730b800037ffa9a0
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=arm SHELL=/bin/bash drivers/power/supply/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/power/supply/ipaq_micro_battery.c:91:12: error: conflicting types for 'get_capacity'
   static int get_capacity(struct power_supply *b)
              ^
   include/linux/genhd.h:253:24: note: previous definition is here
   static inline sector_t get_capacity(struct gendisk *disk)
                          ^
>> drivers/power/supply/ipaq_micro_battery.c:161:30: error: incompatible pointer types passing 'struct power_supply *' to parameter of type 'struct gendisk *' [-Werror,-Wincompatible-pointer-types]
                   val->intval = get_capacity(b);
                                              ^
   include/linux/genhd.h:253:53: note: passing argument to parameter 'disk' here
   static inline sector_t get_capacity(struct gendisk *disk)
                                                       ^
   2 errors generated.


vim +/get_capacity +91 drivers/power/supply/ipaq_micro_battery.c

00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24   90  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  @91  static int get_capacity(struct power_supply *b)
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24   92  {
297d716f6260cc drivers/power/ipaq_micro_battery.c        Krzysztof Kozlowski 2015-03-12   93  	struct micro_battery *mb = dev_get_drvdata(b->dev.parent);
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24   94  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24   95  	switch (mb->flag & 0x07) {
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24   96  	case MICRO_BATT_STATUS_HIGH:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24   97  		return 100;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24   98  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24   99  	case MICRO_BATT_STATUS_LOW:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  100  		return 50;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  101  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  102  	case MICRO_BATT_STATUS_CRITICAL:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  103  		return 5;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  104  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  105  	default:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  106  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  107  	}
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  108  	return 0;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  109  }
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  110  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  111  static int get_status(struct power_supply *b)
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  112  {
297d716f6260cc drivers/power/ipaq_micro_battery.c        Krzysztof Kozlowski 2015-03-12  113  	struct micro_battery *mb = dev_get_drvdata(b->dev.parent);
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  114  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  115  	if (mb->flag == MICRO_BATT_STATUS_UNKNOWN)
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  116  		return POWER_SUPPLY_STATUS_UNKNOWN;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  117  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  118  	if (mb->flag & MICRO_BATT_STATUS_FULL)
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  119  		return POWER_SUPPLY_STATUS_FULL;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  120  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  121  	if ((mb->flag & MICRO_BATT_STATUS_CHARGING) ||
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  122  		(mb->flag & MICRO_BATT_STATUS_CHARGEMAIN))
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  123  		return POWER_SUPPLY_STATUS_CHARGING;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  124  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  125  	return POWER_SUPPLY_STATUS_DISCHARGING;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  126  }
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  127  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  128  static int micro_batt_get_property(struct power_supply *b,
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  129  					enum power_supply_property psp,
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  130  					union power_supply_propval *val)
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  131  {
297d716f6260cc drivers/power/ipaq_micro_battery.c        Krzysztof Kozlowski 2015-03-12  132  	struct micro_battery *mb = dev_get_drvdata(b->dev.parent);
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  133  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  134  	switch (psp) {
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  135  	case POWER_SUPPLY_PROP_TECHNOLOGY:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  136  		switch (mb->chemistry) {
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  137  		case MICRO_BATT_CHEM_NICD:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  138  			val->intval = POWER_SUPPLY_TECHNOLOGY_NiCd;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  139  			break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  140  		case MICRO_BATT_CHEM_NIMH:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  141  			val->intval = POWER_SUPPLY_TECHNOLOGY_NiMH;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  142  			break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  143  		case MICRO_BATT_CHEM_LION:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  144  			val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  145  			break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  146  		case MICRO_BATT_CHEM_LIPOLY:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  147  			val->intval = POWER_SUPPLY_TECHNOLOGY_LIPO;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  148  			break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  149  		default:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  150  			val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  151  			break;
3d32a8437c0510 drivers/power/supply/ipaq_micro_battery.c Chen Zhou           2020-01-15  152  		}
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  153  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  154  	case POWER_SUPPLY_PROP_STATUS:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  155  		val->intval = get_status(b);
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  156  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  157  	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  158  		val->intval = 4700000;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  159  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  160  	case POWER_SUPPLY_PROP_CAPACITY:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24 @161  		val->intval = get_capacity(b);
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  162  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  163  	case POWER_SUPPLY_PROP_TEMP:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  164  		val->intval = mb->temperature;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  165  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  166  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  167  		val->intval = mb->voltage;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  168  		break;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  169  	default:
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  170  		return -EINVAL;
3d32a8437c0510 drivers/power/supply/ipaq_micro_battery.c Chen Zhou           2020-01-15  171  	}
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  172  
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  173  	return 0;
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  174  }
00a588f9d27fc6 drivers/power/ipaq_micro_battery.c        Dmitry Artamonow    2014-07-24  175  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34053 bytes --]

Re: [PATCH 17/18] ledtrig-blkdev: Add mode (read/write/rw) sysfs attributue

[Ian Pilcher]

On 9/4/21 12:57 AM, Greg KH wrote:
> Also, you are now allowing any user to spam the kernel log, this shold
> be a dev_dbg() call at the most, if it is even needed at all.  Same for
> the other pr_info() call in this function, please remove them all.

Greg -

A bit of a "philosophical" question, if I may ...

Is allowing the root user to "spam" the kernel log really a concern?
(The permissions on the attribute don't allow non-root users to write
to it.)

As a system administrator working with a sysfs API (or writing udev
rules to do so), I know that I appreciate having a meaningful message in
the log when something doesn't work.

Given that only the root user can trigger these messages, would you be
OK with dev_info()?

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

Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Ian Pilcher]

On 9/4/21 12:57 AM, Greg KH wrote:
> Where are the Documentation/ABI/ updates for these new sysfs files?

They're in Documentation/ABI/testing/sysfs-class-led-trigger-blkdev,
which is added in the first patch in the series.

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

Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Ian Pilcher]

On 9/4/21 12:59 AM, Greg KH wrote:
> DEVICE_ATTR_RO()?  Or something like that?  Do not use __ATTR() for
> device attributes if at all possible, worst case, use DEVICE_ATTR()
> here.

For some reason, it didn't click until now that these are device
attributes (because I was focused on the fact that I was working on the
LED trigger).

DEVICE_ATTR*() it is.

> And the mode settings are odd, are you sure you want that?

Yes.  These are write-only attributes.

>> +	if (name_len == 0) {
>> +		pr_info("blkdev LED: empty block device name\n");
> 
> Looks like debugging code, please remove.

It's really more of an error message for the system administrator.  So
as with my earlier note, dev_info() would be my preference.

> And how can this ever happen?

The blkdev_skip_space() and blkdev_find_space() calls effectively find
the first non-whitespace token in the buffer (disk_name) and its length
(name_len).  If the buffer only contains whitespace (e.g. echo > $ATTR),
then name_len will be 0.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (attr == &ledtrig_blkdev_attr_del) {
>> +		blkdev_disk_delete(led, disk_name, name_len);
>> +	} else {	/* attr == &ledtrig_blkdev_attr_add */
>> +		ret = blkdev_disk_add(led, disk_name, name_len);
> 
> Why do you have a single attribute callback that does two totally
> different things?  Just have 2 different callback functions please, it
> makes things much easier to review and maintain over time.

Hmmm.  All of the "real work" is done in blkdev_disk_delete() and
blkdev_disk_add().  The store function's only purpose is to parse the
token(s) from the buffer, and that logic is exactly the same for the
two different attributes.

So it's a choice between:

> static ssize_t blkdev_add_or_del(struct device *const dev,
> 				 struct device_attribute *const attr,
> 				 const char *const buf, const size_t count)
> {
> 	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> 	const char *const disk_name = blkdev_skip_space(buf);
> 	const char *const endp = blkdev_find_space(disk_name);
> 	const ptrdiff_t name_len = endp - disk_name;	/* always >= 0 */
> 	int ret;
> 
> 	if (name_len == 0) {
> 		pr_info("blkdev LED: empty block device name\n");
> 		return -EINVAL;
> 	}
> 
> 	if (attr == &ledtrig_blkdev_attr_del) {
> 		blkdev_disk_delete(led, disk_name, name_len);
> 	} else {	/* attr == &ledtrig_blkdev_attr_add */
> 		ret = blkdev_disk_add(led, disk_name, name_len);
> 		if (ret != 0)
> 			return ret;
> 	}
> 
> 	/*
> 	 * Consume everything up to the next non-whitespace token (or the end
> 	 * of the input).  Avoids "empty block device name" error if there is
> 	 * whitespace (such as a newline) after the last token.
> 	 */
> 	return blkdev_skip_space(endp) - buf;
> }

Or:

> static ssize_t blkdev_add(struct device *const dev,
> 			  struct device_attribute *const attr,
> 			  const char *const buf, const size_t count)
> {
> 	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> 	const char *const disk_name = blkdev_skip_space(buf);
> 	const char *const endp = blkdev_find_space(disk_name);
> 	const ptrdiff_t name_len = endp - disk_name;	/* always >= 0 */
> 	int ret;
> 
> 	if (name_len == 0) {
> 		pr_info("blkdev LED: empty block device name\n");
> 		return -EINVAL;
> 	}
> 
> 	ret = blkdev_disk_add(led, disk_name, name_len);
> 	if (ret != 0)
> 		return ret;
> 
> 	/*
> 	 * Consume everything up to the next non-whitespace token (or the end
> 	 * of the input).  Avoids "empty block device name" error if there is
> 	 * whitespace (such as a newline) after the last token.
> 	 */
> 	return blkdev_skip_space(endp) - buf;
> }
> 
> 
> static ssize_t blkdev_del(struct device *const dev,
> 			  struct device_attribute *const attr,
> 			  const char *const buf, const size_t count)
> {
> 	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> 	const char *const disk_name = blkdev_skip_space(buf);
> 	const char *const endp = blkdev_find_space(disk_name);
> 	const ptrdiff_t name_len = endp - disk_name;	/* always >= 0 */
> 	int ret;
> 
> 	if (name_len == 0) {
> 		pr_info("blkdev LED: empty block device name\n");
> 		return -EINVAL;
> 	}
> 
> 	blkdev_disk_delete(led, disk_name, name_len);
> 
> 	/* See comment in blkdev_add() */
> 	return blkdev_skip_space(endp) - buf;
> }

Maybe the right thing to do is to simply add a comment to clarify the
separation (and maybe rename the function as well)?

Thanks!

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

Re: [PATCH 08/18] ledtrig-blkdev: Add miscellaneous helper functions

[Ian Pilcher]

On 9/4/21 1:00 AM, Greg KH wrote:
> Why are block devices odd and need to have spaces found in their names?

They aren't.  The functions are used to identify the block device names
within the buffer that is passed to attribute store functions, which may
contain whitespace.

> And are you sure we do not already have string functions that do this?

I did look a bit.  I can't be sure, but I didn't find anything similar.

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

Re: [PATCH 09/18] ledtrig-blkdev: Periodically check devices for activity & blink LEDs

[Ian Pilcher]

On 9/4/21 1:01 AM, Greg KH wrote:
> Please never use WARN_ON() in new code unless the machine is really
> broken and you can not do anything else here.

Wait what?  I thought that was BUG_ON.

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

Re: [PATCH 01/18] docs: Add block device (blkdev) LED trigger documentation

[Ian Pilcher]

On 9/4/21 1:29 AM, Pavel Machek wrote:
>> +What:		/sys/class/leds/<led>/add_blkdev
>> +Date:		September 2021
>> +Contact:	Ian Pilcher <arequipeno@gmail.com>
>> +Description:
>> +		Associate a block device with this LED by writing its kernel
>> +		name (as shown in /sys/block) to this attribute.  Multiple
>> +		device names may be written at once, separated by whitespace.
> 
> This is seriously strange interface.

It's the netdev-like interface that Marek described in an earlier note
(adapted for the fact that the trigger supports many-to-many
relationships).

> If you have directory with symlinks, why not use symlink() syscall
> instead of add_blkdev, and unlink() syscall instead of delete_blkdev?

I'd actually had that thought as well, but I didn't see any obvious way
to do that in sysfs when I looked.  If you know how to do it or know of
an example, please let me know.

If using symlink()/unlink() isn't an option, do you have a suggestion
for a less seriously strange interface?

Thanks!

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

Re: [PATCH 17/18] ledtrig-blkdev: Add mode (read/write/rw) sysfs attributue

[Greg KH]

On Sat, Sep 04, 2021 at 04:01:56PM -0500, Ian Pilcher wrote:
> On 9/4/21 12:57 AM, Greg KH wrote:
> > Also, you are now allowing any user to spam the kernel log, this shold
> > be a dev_dbg() call at the most, if it is even needed at all.  Same for
> > the other pr_info() call in this function, please remove them all.
> 
> Greg -
> 
> A bit of a "philosophical" question, if I may ...
> 
> Is allowing the root user to "spam" the kernel log really a concern?

Yes.

> (The permissions on the attribute don't allow non-root users to write
> to it.)

Ah, but that was not obvious :)

> As a system administrator working with a sysfs API (or writing udev
> rules to do so), I know that I appreciate having a meaningful message in
> the log when something doesn't work.

That's fine, but do not allow a "normal" user to do so please.

> Given that only the root user can trigger these messages, would you be
> OK with dev_info()?

For a sysfs file failure, use dev_err().  If things are working
properly, your kernel code should be totally silent.

thanks,

greg k-h

Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Greg KH]

On Sat, Sep 04, 2021 at 05:35:29PM -0500, Ian Pilcher wrote:
> On 9/4/21 12:59 AM, Greg KH wrote:
> > DEVICE_ATTR_RO()?  Or something like that?  Do not use __ATTR() for
> > device attributes if at all possible, worst case, use DEVICE_ATTR()
> > here.
> 
> For some reason, it didn't click until now that these are device
> attributes (because I was focused on the fact that I was working on the
> LED trigger).
> 
> DEVICE_ATTR*() it is.
> 
> > And the mode settings are odd, are you sure you want that?
> 
> Yes.  These are write-only attributes.

DEVICE_ATTR_WO() then please.

> > > +	if (name_len == 0) {
> > > +		pr_info("blkdev LED: empty block device name\n");
> > 
> > Looks like debugging code, please remove.
> 
> It's really more of an error message for the system administrator.  So
> as with my earlier note, dev_info() would be my preference.

Nope, dev_err() for real errors please.

> > And how can this ever happen?
> 
> The blkdev_skip_space() and blkdev_find_space() calls effectively find
> the first non-whitespace token in the buffer (disk_name) and its length
> (name_len).  If the buffer only contains whitespace (e.g. echo > $ATTR),
> then name_len will be 0.

That's a crazy interface, as others pointed out, don't do that please.

thanks,

greg k-h

Re: [PATCH 09/18] ledtrig-blkdev: Periodically check devices for activity & blink LEDs

[Greg KH]

On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote:
> On 9/4/21 1:01 AM, Greg KH wrote:
> > Please never use WARN_ON() in new code unless the machine is really
> > broken and you can not do anything else here.
> 
> Wait what?  I thought that was BUG_ON.

Not whan panic-on-warn is set, which is getting more and more common
these days.

thanks,

greg k-h

Re: [PATCH 09/18] ledtrig-blkdev: Periodically check devices for activity & blink LEDs

[Ian Pilcher]

On 9/5/21 9:51 AM, Greg KH wrote:
> On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote:
>> On 9/4/21 1:01 AM, Greg KH wrote:
>>> Please never use WARN_ON() in new code unless the machine is really
>>> broken and you can not do anything else here.
>>
>> Wait what?  I thought that was BUG_ON.
> 
> Not whan panic-on-warn is set, which is getting more and more common
> these days.

Fair enough.  What is the recommend approach to reporting a "this should
never" happen situation these days?

Thanks!

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

Re: [PATCH 09/18] ledtrig-blkdev: Periodically check devices for activity & blink LEDs

[Greg KH]

On Sun, Sep 05, 2021 at 09:56:58AM -0500, Ian Pilcher wrote:
> On 9/5/21 9:51 AM, Greg KH wrote:
> > On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote:
> > > On 9/4/21 1:01 AM, Greg KH wrote:
> > > > Please never use WARN_ON() in new code unless the machine is really
> > > > broken and you can not do anything else here.
> > > 
> > > Wait what?  I thought that was BUG_ON.
> > 
> > Not whan panic-on-warn is set, which is getting more and more common
> > these days.
> 
> Fair enough.  What is the recommend approach to reporting a "this should
> never" happen situation these days?

dev_err() and handle the error properly.

Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Ian Pilcher]

On 9/5/21 9:51 AM, Greg KH wrote:
>> It's really more of an error message for the system administrator.  So
>> as with my earlier note, dev_info() would be my preference.
> 
> Nope, dev_err() for real errors please.

Just to clarify, the error in this case is the system administrator
writing an incorrect value to a sysfs attribute (likely via a udev
rule), i.e. a "pilot error."

One of the reviewers of one of my RFC patch sets commented that those
should be INFO level at most.

So dev_err() or dev_info() for that sort of thing (always given that
only the root user has permission to write to trigger the error
message)?

>> The blkdev_skip_space() and blkdev_find_space() calls effectively find
>> the first non-whitespace token in the buffer (disk_name) and its length
>> (name_len).  If the buffer only contains whitespace (e.g. echo > $ATTR),
>> then name_len will be 0.
> 
> That's a crazy interface, as others pointed out, don't do that please.

As Pavel noted, it would be ideal to use symlink()/unlink() in the LED's
block_devices directory for this.  As far as I know however, sysfs
doesn't support doing that.  I'd be happy to learn otherwise.  I would
also welcome any other suggestions for a better interface for setting up
the many-to-many relationships that the trigger supports.

That said, I don't know what that has to do with blkdev_skip_space() and
blkdev_find_space(), which are just helper functions that I use to parse
the device name out of the buffer passed to the store function.
Ultimately, the store function does need to handle the case where the
system administrator (or a broken udev rule) writes an all-whitespace
string to the attribute.

I will try to restructure the code to make things more clear.

Thanks!

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

Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Greg KH]

On Sun, Sep 05, 2021 at 10:33:08AM -0500, Ian Pilcher wrote:
> On 9/5/21 9:51 AM, Greg KH wrote:
> > > It's really more of an error message for the system administrator.  So
> > > as with my earlier note, dev_info() would be my preference.
> > 
> > Nope, dev_err() for real errors please.
> 
> Just to clarify, the error in this case is the system administrator
> writing an incorrect value to a sysfs attribute (likely via a udev
> rule), i.e. a "pilot error."
> 
> One of the reviewers of one of my RFC patch sets commented that those
> should be INFO level at most.
> 
> So dev_err() or dev_info() for that sort of thing (always given that
> only the root user has permission to write to trigger the error
> message)?

Really you should not have any kernel log messages for invalid data sent
to a sysfs file, just return -EINVAL and be done with it.

> > > The blkdev_skip_space() and blkdev_find_space() calls effectively find
> > > the first non-whitespace token in the buffer (disk_name) and its length
> > > (name_len).  If the buffer only contains whitespace (e.g. echo > $ATTR),
> > > then name_len will be 0.
> > 
> > That's a crazy interface, as others pointed out, don't do that please.
> 
> As Pavel noted, it would be ideal to use symlink()/unlink() in the LED's
> block_devices directory for this.  As far as I know however, sysfs
> doesn't support doing that.  I'd be happy to learn otherwise.  I would
> also welcome any other suggestions for a better interface for setting up
> the many-to-many relationships that the trigger supports.

sysfs does not allow that as that is not what sysfs is for.  Perhaps you
want to use configfs, as that is exactly what that is for.

> That said, I don't know what that has to do with blkdev_skip_space() and
> blkdev_find_space(), which are just helper functions that I use to parse
> the device name out of the buffer passed to the store function.
> Ultimately, the store function does need to handle the case where the
> system administrator (or a broken udev rule) writes an all-whitespace
> string to the attribute.

Handling invalid data is fine, but having to parse multiple values in a
single sysfs file violates the rules of sysfs.  Please use something
else instead.

thanks,

greg k-h

Re: [PATCH 09/18] ledtrig-blkdev: Periodically check devices for activity & blink LEDs

[Eric Biggers]

On Sun, Sep 05, 2021 at 05:12:39PM +0200, Greg KH wrote:
> On Sun, Sep 05, 2021 at 09:56:58AM -0500, Ian Pilcher wrote:
> > On 9/5/21 9:51 AM, Greg KH wrote:
> > > On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote:
> > > > On 9/4/21 1:01 AM, Greg KH wrote:
> > > > > Please never use WARN_ON() in new code unless the machine is really
> > > > > broken and you can not do anything else here.
> > > > 
> > > > Wait what?  I thought that was BUG_ON.
> > > 
> > > Not whan panic-on-warn is set, which is getting more and more common
> > > these days.
> > 
> > Fair enough.  What is the recommend approach to reporting a "this should
> > never" happen situation these days?
> 
> dev_err() and handle the error properly.
> 
> 

WARN_ON is the right choice for reporting recoverable kernel bugs, and BUG_ON
for unrecoverable ones; see the two comments in include/asm-generic/bug.h which
explain this.  Please don't use dev_err() if it's a kernel bug (and not just
unexpected input from userspace or hardware behaving weirdly), as that prevents
the bug from being reported if it occurs.

Greg, you've been corrected on this before, e.g.
https://lore.kernel.org/linux-fsdevel/20210707023548.15872-1-desmondcheongzx@gmail.com/T/#u.
Please stop spreading false information as it is destroying your credibility :-(

- Eric

Re: [PATCH 01/18] docs: Add block device (blkdev) LED trigger documentation

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

Hi!

> >>+What:		/sys/class/leds/<led>/add_blkdev
> >>+Date:		September 2021
> >>+Contact:	Ian Pilcher <arequipeno@gmail.com>
> >>+Description:
> >>+		Associate a block device with this LED by writing its kernel
> >>+		name (as shown in /sys/block) to this attribute.  Multiple
> >>+		device names may be written at once, separated by whitespace.
> >
> >This is seriously strange interface.
> 
> It's the netdev-like interface that Marek described in an earlier note
> (adapted for the fact that the trigger supports many-to-many
> relationships).
> 
> >If you have directory with symlinks, why not use symlink() syscall
> >instead of add_blkdev, and unlink() syscall instead of delete_blkdev?
> 
> I'd actually had that thought as well, but I didn't see any obvious way
> to do that in sysfs when I looked.  If you know how to do it or know of
> an example, please let me know.

We got Greg in the Cc list, he'd be right person to talk to...

> If using symlink()/unlink() isn't an option, do you have a suggestion
> for a less seriously strange interface?

Let us explore this possibility, first.

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 01/18] docs: Add block device (blkdev) LED trigger documentation

[Ian Pilcher]

On 9/5/21 1:42 PM, Pavel Machek wrote:
>> If using symlink()/unlink() isn't an option, do you have a suggestion
>> for a less seriously strange interface?
> 
> Let us explore this possibility, first.

It doesn't look like either sysfs or configfs is set up to react to
symlink()/unlink() calls so, I don't think it's possible to implement
that interface.

I don't personally know of any current sysfs API that manages a set of
things like this, so I don't have anything on which to model an
interface right now.

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