[PATCH v2 00/15] Introduce block device LED trigger

[PATCH v2 00/15] Introduce block device LED trigger

[Ian Pilcher]

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

* Use correct address for LKML.

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

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

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

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

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

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

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

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

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

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

Notes:
======

* Documentation for all sysfs attributes added in the first patch.

* All patches build without warnings or errors when trigger is disabled,
  modular or built-in.

V1 summary:
===========

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

It has the following functionality.

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

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

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

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

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

* Requires minimal changes to the block subsystem.

  - Adds 1 pointer to struct gendisk,

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

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

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

* The trigger can be mostly built as a module.

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


Ian Pilcher (15):
  docs: Add block device (blkdev) LED trigger documentation
  leds: trigger: blkdev: Add build infrastructure
  leds: trigger: blkdev: Add functions needed by block changes
  block: Add block device LED trigger integrations
  leds: trigger: blkdev: Complete functions called by block subsys
  leds: trigger: blkdev: Add function to get gendisk by name
  leds: trigger: blkdev: Add constants and types
  leds: trigger: blkdev: Add stub LED trigger structure
  leds: trigger: blkdev: Check devices for activity and blink LEDs
  leds: trigger: blkdev: Add LED trigger activate function
  leds: trigger: blkdev: Enable linking block devices to LEDs
  leds: trigger: blkdev: Enable unlinking block devices from LEDs
  leds: trigger: blkdev: Add LED trigger deactivate function
  leds: trigger: blkdev: Add remaining sysfs attributes
  leds: trigger: blkdev: Add disk cleanup and init/exit functions

 Documentation/ABI/testing/sysfs-block         |   9 +
 .../testing/sysfs-class-led-trigger-blkdev    |  46 ++
 Documentation/leds/index.rst                  |   1 +
 Documentation/leds/ledtrig-blkdev.rst         | 138 ++++
 block/genhd.c                                 |   4 +
 drivers/leds/trigger/Kconfig                  |  18 +
 drivers/leds/trigger/Makefile                 |   2 +
 drivers/leds/trigger/ledtrig-blkdev-core.c    |  55 ++
 drivers/leds/trigger/ledtrig-blkdev.c         | 686 ++++++++++++++++++
 drivers/leds/trigger/ledtrig-blkdev.h         |  18 +
 include/linux/genhd.h                         |   3 +
 include/linux/leds.h                          |  20 +
 12 files changed, 1000 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


base-commit: a3fa7a101dcff93791d1b1bdb3affcad1410c8c1
-- 
2.31.1

[PATCH v2 01/15] 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>/link_device
  * /sys/class/leds/<led>/unlink_device
  * /sys/class/leds/<led>/linked_devices

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

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

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 Documentation/ABI/testing/sysfs-block         |   9 ++
 .../testing/sysfs-class-led-trigger-blkdev    |  46 ++++++
 Documentation/leds/index.rst                  |   1 +
 Documentation/leds/ledtrig-blkdev.rst         | 138 ++++++++++++++++++
 4 files changed, 194 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..80d4becc4e6d 100644
--- a/Documentation/ABI/testing/sysfs-block
+++ b/Documentation/ABI/testing/sysfs-block
@@ -328,3 +328,12 @@ Description:
 		does not complete in this time then the block driver timeout
 		handler is invoked. That timeout handler can decide to retry
 		the request, to fail it or to start a device recovery strategy.
+
+What:		/sys/block/<disk>/linked_leds
+Date:		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..9f75d0bccceb
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
@@ -0,0 +1,46 @@
+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>/link_device
+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.
+
+What:		/sys/class/leds/<led>/unlink_device
+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.
+
+What:		/sys/class/leds/<led>/linked_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..a4acc532f31b
--- /dev/null
+++ b/Documentation/leds/ledtrig-blkdev.rst
@@ -0,0 +1,138 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=================================
+Block Device (blkdev) LED Trigger
+=================================
+
+Available when ``CONFIG_LEDS_TRIGGER_BLKDEV=y`` or
+``CONFIG_LEDS_TRIGGER_BLKDEV=m``.
+
+See also:
+
+* ``Documentation/ABI/testing/sysfs-class-led-trigger-blkdev``
+* ``Documentation/ABI/testing/sysfs-block`` (``/sys/block/<disk>/linked_leds``)
+
+Overview
+========
+
+.. note::
+	The examples below use ``<LED>`` to refer to the name of a
+	system-specific LED.  If no suitable LED is available on a test
+	system (in a virtual machine, for example), it is possible to
+	use a userspace LED.  (See ``Documentation/leds/uleds.rst``.)
+
+Verify that the ``blkdev`` LED trigger is available::
+
+	# grep blkdev /sys/class/leds/<LED>/trigger
+	... rfkill-none blkdev
+
+(If the previous command produces no output, you may need to load the trigger
+module - ``modprobe ledtrig_blkdev``.  If the module is not available, check
+the value of ``CONFIG_LEDS_TRIGGER_BLKDEV`` in your kernel configuration.)
+
+Associate the LED with the ``blkdev`` LED trigger::
+
+	# echo blkdev > /sys/class/leds/<LED>/trigger
+
+	# cat /sys/class/leds/<LED>/trigger
+	... rfkill-none [blkdev]
+
+Note that several new device attributes are available in the
+``/sys/class/leds/<LED>`` directory.
+
+* ``link_device`` and ``unlink_device`` are used to manage the set of block
+  devices associated with this LED.  The LED will blink in response to read or
+  write activity on its linked devices.
+
+* ``mode`` is used to control the type of device activity that will cause this
+  LED to blink - read activity, write activity, or both.  (Note that any
+  activity that changes the state of a device's non-volatile storage is
+  considered to be a write.  This includes discard and cache flush requests.)
+
+* ``blink_time`` is the duration (in milliseconds) of each blink of this LED.
+
+* ``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 ``linked_devices`` directory will contain a symbolic link to every device
+  that is associated with this LED.
+
+Link a block device to the LED::
+
+	# echo sda > /sys/class/leds/<LED>/link_device
+
+	# ls /sys/class/leds/<LED>/linked_devices
+	sda
+
+Read and write activity on the device should cause the LED to blink.  The
+duration of each blink (in milliseconds) can be adjusted by setting
+``/sys/class/leds/<LED>/blink_time``.  (But see **interval and blink_time**
+below.)
+
+Associate a second device with the LED::
+
+	# echo sdb > /sys/class/leds/<LED>/link_device
+
+	# ls /sys/class/leds/<LED>/linked_devices
+	sda  sdb
+
+When a block device is linked to one or more LEDs, the LEDs are linked from
+the device's ``linked_leds`` directory::
+
+	# ls /sys/block/sd{a,b}/linked_leds
+	/sys/block/sda/linked_leds:
+	<LED>
+
+	/sys/block/sdb/linked_leds:
+	<LED>
+
+(The ``linked_leds`` directory only exists when the block device is linked to
+at least one LED.)
+
+``interval`` and ``blink_time``
+===============================
+
+* The ``interval`` attribute is a global setting.  Changing the value via
+  ``/sys/class/leds/<LED>/interval`` will change it for all LEDs associated with
+  the ``blkdev`` 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 linked devices), the LED will be on for ``blink_time``
+  milliseconds and off for ``interval - blink_time`` milliseconds.
+
+* The LED subsystem ignores new blink requests for an LED that is already in
+  in the process of blinking, so setting a ``blink_time`` greater than or equal
+  to ``interval`` will cause some blinks to be 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 v2 02/15] leds: trigger: blkdev: Add build infrastructure

[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 declarations

Add Kconfig options - CONFIG_LEDS_TRIGGER_BLKDEV (visible, tristate) and
CONFIG_LEDS_TRIGGER_BLKDEV_CORE (hidden, boolean)

Make CONFIG_LEDS_TRIGGER_BLKDEV select CONFIG_LEDS_TRIGGER_BLKDEV_CORE, so
built-in portion of the trigger is enabled when trigger is either 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..ad49d92713fb 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..bd9e5f09b7e3
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blkdev-core.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ *	Block device LED trigger - 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..b615a8bf38e2
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ *	Block device LED trigger - 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..6264d8a9201b
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-blkdev.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ *	Block device LED trigger
+ *
+ *	Copyright 2021 Ian Pilcher <arequipeno@gmail.com>
+ */
+
+#ifndef __LEDTRIG_BLKDEV_H
+#define __LEDTRIG_BLKDEV_H
+
+#endif	/* __LEDTRIG_BLKDEV_H */
-- 
2.31.1

Re: [PATCH v2 02/15] leds: trigger: blkdev: Add build infrastructure

[Marek Behún]

On Thu,  9 Sep 2021 17:25:00 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> 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 declarations

I have never seen this done this way. Add new files once you have
content for them. I think this entire proposal should be done as one
patch, since it atomically adds functionality.

Now I have to jump between various e-mail when I want to create a mind
map of what this code is doing, and it is annoying.

Please resend this as:
 - one patch adding sysfs docs
 - one patch for the rest

Marek

[PATCH v2 03/15] leds: trigger: blkdev: Add functions needed by block changes

[Ian Pilcher]

Add ledtrig_blkdev_disk_init() and ledtrig_blkdev_disk_cleanup()
placeholders.  (Function bodies 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 bd9e5f09b7e3..b0a455180b05 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..38fb8b6e68fe 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	/* IS_ENABLED(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	/* IS_ENABLED(CONFIG_LEDS_TRIGGER_BLKDEV) */
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
2.31.1

[PATCH v2 04/15] 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

Re: [PATCH v2 04/15] block: Add block device LED trigger integrations

[Chaitanya Kulkarni]

On 9/9/21 3:25 PM, Ian Pilcher wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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>

The commit log doesn't explain that why you need modify the core block
layer API which is highly discouraged.

Why can't ledtrig_blkdev_disk_init() be called before you
call add_disk() in your driver? same goes for the 
ledtrig_blkdev_disk_cleanup(). If there is legit reason you need to
document that.

Re: [PATCH v2 04/15] block: Add block device LED trigger integrations

[Marek Behún]

On Thu,  9 Sep 2021 17:25:02 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> 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()

No, this is not needed. If someone does not use kzalloc(), they should
use it. No need to fix other code here.

Marek

Re: [PATCH v2 04/15] block: Add block device LED trigger integrations

[Ian Pilcher]

On 9/9/21 8:23 PM, Marek Behún wrote:
> On Thu,  9 Sep 2021 17:25:02 -0500
> Ian Pilcher <arequipeno@gmail.com> wrote:
>> 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()
> 
> No, this is not needed. If someone does not use kzalloc(), they should
> use it. No need to fix other code here.

Yeah.  I'm honestly not sure if this is necessary or not, as I don't
know if there are any drivers that actually have this problem.  I
decided to include this for now, because an uninitialized pointer can
cause memory corruption, etc., when the disk cleanup function follows a
garbage pointer.

This recent commit seems to indicate that until recently drivers were
responsible for doing gendisk allocation.

> commit f525464a8000f092c20b00eead3eaa9d849c599e
> Author: Christoph Hellwig <hch@lst.de>
> Date:   Fri May 21 07:50:55 2021 +0200
> 
>     block: add blk_alloc_disk and blk_cleanup_disk APIs
>     
>     Add two new APIs to allocate and free a gendisk including the
>     request_queue for use with BIO based drivers.  This is to avoid
>     boilerplate code in drivers.

Were those drivers expected to use kzalloc() or otherwise zero out the
entire structure?  I really don't know.

I think that it makes sense to defer to the block subsystem maintainers
on this question.

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

Re: [PATCH v2 04/15] block: Add block device LED trigger integrations

[Marek Behún]

On Thu,  9 Sep 2021 17:25:02 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> @@ -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

So in this patch you are adding pointer to a structure, but the
structure is introduced in a later patch (07/15).

Please send these changes as one patch, it is easier to review.

Marek

[PATCH v2 05/15] leds: trigger: blkdev: Complete functions called by block subsys

[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.c      |  1 +
 drivers/leds/trigger/ledtrig-blkdev.h      |  3 +++
 include/linux/leds.h                       |  1 +
 4 files changed, 20 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev-core.c b/drivers/leds/trigger/ledtrig-blkdev-core.c
index b0a455180b05..d7b02e760b06 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.c b/drivers/leds/trigger/ledtrig-blkdev.c
index b615a8bf38e2..7f85e6080ea1 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/leds.h>
 #include <linux/module.h>
 
 #include "ledtrig-blkdev.h"
diff --git a/drivers/leds/trigger/ledtrig-blkdev.h b/drivers/leds/trigger/ledtrig-blkdev.h
index 6264d8a9201b..b1294da17ba3 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 38fb8b6e68fe..3925499c0b3d 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	/* IS_ENABLED(CONFIG_LEDS_TRIGGER_BLKDEV) */
-- 
2.31.1

[PATCH v2 06/15] leds: trigger: 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.

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

diff --git a/drivers/leds/trigger/ledtrig-blkdev-core.c b/drivers/leds/trigger/ledtrig-blkdev-core.c
index d7b02e760b06..5fd741aa14a6 100644
--- a/drivers/leds/trigger/ledtrig-blkdev-core.c
+++ b/drivers/leds/trigger/ledtrig-blkdev-core.c
@@ -33,3 +33,23 @@ void ledtrig_blkdev_disk_cleanup(struct gendisk *const gd)
 
 	mutex_unlock(&ledtrig_blkdev_mutex);
 }
+
+/* class_find_device() callback.  Must be built-in to access disk_type. */
+static int blkdev_match_name(struct device *const dev, const void *const name)
+{
+	return dev->type == &disk_type
+			&& sysfs_streq(dev_to_disk(dev)->disk_name, name);
+}
+
+/* Must be built-in to access block_class */
+struct gendisk *ledtrig_blkdev_get_disk(const char *const name)
+{
+	struct device *dev;
+
+	dev = class_find_device(&block_class, NULL, name, blkdev_match_name);
+	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 b1294da17ba3..a6ff24fad0c2 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.h
+++ b/drivers/leds/trigger/ledtrig-blkdev.h
@@ -12,4 +12,7 @@
 extern struct mutex ledtrig_blkdev_mutex;
 extern void (*__ledtrig_blkdev_disk_cleanup)(struct gendisk *gd);
 
+/* Caller must call put_disk() */
+struct gendisk *ledtrig_blkdev_get_disk(const char *const name);
+
 #endif	/* __LEDTRIG_BLKDEV_H */
-- 
2.31.1

Re: [PATCH v2 06/15] leds: trigger: blkdev: Add function to get gendisk by name

[Greg KH]

On Thu, Sep 09, 2021 at 05:25:04PM -0500, Ian Pilcher wrote:
> 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.
> 
> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-blkdev-core.c | 20 ++++++++++++++++++++
>  drivers/leds/trigger/ledtrig-blkdev.h      |  3 +++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-blkdev-core.c b/drivers/leds/trigger/ledtrig-blkdev-core.c
> index d7b02e760b06..5fd741aa14a6 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev-core.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev-core.c
> @@ -33,3 +33,23 @@ void ledtrig_blkdev_disk_cleanup(struct gendisk *const gd)
>  
>  	mutex_unlock(&ledtrig_blkdev_mutex);
>  }
> +
> +/* class_find_device() callback.  Must be built-in to access disk_type. */
> +static int blkdev_match_name(struct device *const dev, const void *const name)
> +{
> +	return dev->type == &disk_type
> +			&& sysfs_streq(dev_to_disk(dev)->disk_name, name);
> +}
> +
> +/* Must be built-in to access block_class */
> +struct gendisk *ledtrig_blkdev_get_disk(const char *const name)
> +{
> +	struct device *dev;
> +
> +	dev = class_find_device(&block_class, NULL, name, blkdev_match_name);
> +	if (dev == NULL)
> +		return NULL;

You now have bumped the reference count on this structure.  Where do you
decrement it when you are finished with it?

thanks,

greg k-h

Re: [PATCH v2 06/15] leds: trigger: blkdev: Add function to get gendisk by name

[Ian Pilcher]

On 9/10/21 1:45 AM, Greg KH wrote:
> You now have bumped the reference count on this structure.  Where do you
> decrement it when you are finished with it?

That happens when I "unlink" the block device from the LED in
blkdev_disk_unlink_locked() at ledtrig-blkdev.c:410.

(And also in the error path of link_device_store() at
ledtrig-blkdev.c:372.)

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

Re: [PATCH v2 06/15] leds: trigger: blkdev: Add function to get gendisk by name

[Greg KH]

On Fri, Sep 10, 2021 at 10:17:03AM -0500, Ian Pilcher wrote:
> On 9/10/21 1:45 AM, Greg KH wrote:
> > You now have bumped the reference count on this structure.  Where do you
> > decrement it when you are finished with it?
> 
> That happens when I "unlink" the block device from the LED in
> blkdev_disk_unlink_locked() at ledtrig-blkdev.c:410.

I have no context here anymore, so I have no idea if this is really true
:(

Re: [PATCH v2 06/15] leds: trigger: blkdev: Add function to get gendisk by name

[Ian Pilcher]

On 9/10/21 1:45 AM, Greg KH wrote:
> On Thu, Sep 09, 2021 at 05:25:04PM -0500, Ian Pilcher wrote:
>> +/* Must be built-in to access block_class */
>> +struct gendisk *ledtrig_blkdev_get_disk(const char *const name)
>> +{
>> +	struct device *dev;
>> +
>> +	dev = class_find_device(&block_class, NULL, name, blkdev_match_name);
>> +	if (dev == NULL)
>> +		return NULL;
> 
> You now have bumped the reference count on this structure.  Where do you
> decrement it when you are finished with it?

With context this time.  Sorry about that.


That happens when I "unlink" the block device from the LED in
blkdev_disk_unlink_locked() at ledtrig-blkdev.c:410.

(And also in the error path of link_device_store() at
ledtrig-blkdev.c:372.)

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

[PATCH v2 07/15] leds: trigger: blkdev: Add constants and types

[Ian Pilcher]

Avoid bloating gendisk structure by storing per-device trigger data in
ledtrig_blkdev_disk

Support many-to-many device/LED associations with ledtrig_blkdev_link
struct.  Per-device (ledtrig_blkdev_disk) and per-LED (ledtrig_blkdev_led)
structures both include a list of links, so a device can be linked to
multiple LEDs, and an LED can be linked to multiple devices.

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 7f85e6080ea1..38a2cd2df85c 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -15,3 +15,47 @@ 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 & check interval (milliseconds) */
+#define LEDTRIG_BLKDEV_BLINK_MSEC	75
+#define LEDTRIG_BLKDEV_INTERVAL		100
+
+/* Minimum blink time & check interval (milliseconds) */
+#define LEDTRIG_BLKDEV_MIN_BLINK	10
+#define LEDTRIG_BLKDEV_MIN_INT		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 */
+};
+
+/* Per-block device information */
+struct ledtrig_blkdev_disk {
+	struct gendisk		*gd;
+	struct kobject		*dir;		/* linked_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;		/* linked_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;
+};
-- 
2.31.1

[PATCH v2 08/15] leds: trigger: blkdev: Add stub LED trigger structure

[Ian Pilcher]

Needed to avoid unused function warnings/errors from subsequent patches

Make ledtrig_blkdev_trigger non-static for now to avoid unused variable
warning/error

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

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 38a2cd2df85c..53b62e320491 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -59,3 +59,28 @@ struct ledtrig_blkdev_led {
 	struct hlist_node		leds_node;
 	enum ledtrig_blkdev_mode	mode;
 };
+
+
+/*
+ *
+ *	Initialization - register the trigger
+ *
+ */
+
+static struct attribute *ledtrig_blkdev_attrs[] = {
+	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
+};
+
+struct led_trigger ledtrig_blkdev_trigger = {
+	.name		= "blkdev",
+	.groups		= ledtrig_blkdev_attr_groups,
+};
-- 
2.31.1

[PATCH v2 09/15] leds: trigger: blkdev: Check devices for activity and 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 | 103 ++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 53b62e320491..40dc55e5d4f3 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -8,6 +8,7 @@
 
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/part_stat.h>
 
 #include "ledtrig-blkdev.h"
 
@@ -60,6 +61,108 @@ struct ledtrig_blkdev_led {
 	enum ledtrig_blkdev_mode	mode;
 };
 
+/* All LEDs associated with the trigger */
+static HLIST_HEAD(ledtrig_blkdev_leds);
+
+/* 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);
+
+
+/*
+ *
+ *	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 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;
+}
+
+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

Re: [PATCH v2 09/15] leds: trigger: blkdev: Check devices for activity and blink LEDs

[Marek Behún]

On Thu,  9 Sep 2021 17:25:07 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> +static void blkdev_blink(const struct ledtrig_blkdev_led *const led)
> +{

Why are you declaring the led variable as const? This is not needed.
Sure, you do not change it, but I have never seen this being used in
this way in kernel.

> +	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]);

Again, yes, you do not change part0, read_ios or write_ios in this
function, but this does not mean you need to declare them const.

Const is good when you want to declare that a place where a pointer
points to should be constant. You don't need to do it for the pointer
itself, I don't see any benefit from this.

> +
> +	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 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;
> +}

It would be better to simply do the comparison where it is needed,
since it is so short. These functions aren't needed, they do not
shorten code in any significant way, nor do they make it more readable
(in fact, they make it a little less readable).

> +
> +static void blkdev_process(struct work_struct *const work)

You are again using const where it is not needed.
In fact the work_func_t type does not have it:
  typedef void (*work_func_t)(struct work_struct *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;
> +			}

These two blinks should be one blink, i.e.
  if ((read_act && read_mode) || (write_act && write_mode))
    blink();

> +		}
> +	}
> +
> +	++generation;
> +
> +	mutex_unlock(&ledtrig_blkdev_mutex);
> +
> +exit_reschedule:
> +	delay = READ_ONCE(ledtrig_blkdev_interval);
> +	WARN_ON_ONCE(!schedule_delayed_work(&ledtrig_blkdev_work, delay));
> +}
> +
>  
>  /*
>   *

Re: [PATCH v2 09/15] leds: trigger: blkdev: Check devices for activity and blink LEDs

[Marek Behún]

On Thu,  9 Sep 2021 17:25:07 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> +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]);

So your code allows me to use a partition block device (like sda2) to
register with the blkdev LED trigger, but when I do this, the code will
disregard that I just want the LED to blink on activity on that one
partition. Instead you will blink for whole sda, since you are looking
at stats of only part0.

Am I right?

If so, this in unacceptable. The whole point of blkdev trigger is that
you can reliably use it for any block device, and then it will blink
the LED for that device, be it partition or whole disk.

Marek

Re: [PATCH v2 09/15] leds: trigger: blkdev: Check devices for activity and blink LEDs

[Ian Pilcher]

On 9/9/21 9:17 PM, Marek Behún wrote:
> So your code allows me to use a partition block device (like sda2) to
> register with the blkdev LED trigger, but when I do this, the code will
> disregard that I just want the LED to blink on activity on that one
> partition. Instead you will blink for whole sda, since you are looking
> at stats of only part0.
> 
> Am I right?

You can't add partitions, only whole devices.

# echo vda2 > link_device
-bash: echo: write error: No such device

static int blkdev_match_name(struct device *const dev, const void *const 
name)
{
	return dev->type == &disk_type
			&& sysfs_streq(dev_to_disk(dev)->disk_name, name);
}

Partitions fail the dev->type == &disk_type check.

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

Re: [PATCH v2 09/15] leds: trigger: blkdev: Check devices for activity and blink LEDs

[Marek Behún]

On Fri, 10 Sep 2021 10:09:09 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> On 9/9/21 9:17 PM, Marek Behún wrote:
> > So your code allows me to use a partition block device (like sda2) to
> > register with the blkdev LED trigger, but when I do this, the code will
> > disregard that I just want the LED to blink on activity on that one
> > partition. Instead you will blink for whole sda, since you are looking
> > at stats of only part0.
> > 
> > Am I right?  
> 
> You can't add partitions, only whole devices.

But I should be able to, since partition is a block device in /dev.
Any block device from /sys/class/block should be possible to add.

Marek

Re: [PATCH v2 09/15] leds: trigger: blkdev: Check devices for activity and blink LEDs

[Ian Pilcher]

On 9/10/21 10:12 AM, Marek Behún wrote:
> On Fri, 10 Sep 2021 10:09:09 -0500
> Ian Pilcher <arequipeno@gmail.com> wrote:
>> You can't add partitions, only whole devices.
> 
> But I should be able to, since partition is a block device in /dev.
> Any block device from /sys/class/block should be possible to add.

I wasn't aware that was something that you were interested in doing.
This will require working with the block_device structure rather than
the gendisk.

One possible benefit of this change ... Assuming that block_device
structures are always allocated by bdev_alloc() *and* I'm correct in
thinking that this means that they are always allocated from the inode
cache, then they are always zeroed out when allocated, so there won't
be any need to explicitly initialize the pointer.

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

[PATCH v2 10/15] leds: trigger: blkdev: Add LED trigger activate function

[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 | 57 +++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 40dc55e5d4f3..6f78a9515976 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -164,6 +164,62 @@ static void blkdev_process(struct work_struct *const work)
 }
 
 
+/*
+ *
+ *	Associate an LED with the blkdev trigger
+ *
+ */
+
+static int blkdev_activate(struct led_classdev *const led_dev)
+{
+	struct ledtrig_blkdev_led *led;
+	int ret;
+
+	/* Don't allow module to be removed while any LEDs are linked */
+	if (WARN_ON(!try_module_get(THIS_MODULE))) {
+		ret = -ENODEV;		/* Shouldn't ever happen */
+		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("linked_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;
+}
+
+
 /*
  *
  *	Initialization - register the trigger
@@ -185,5 +241,6 @@ static const struct attribute_group *ledtrig_blkdev_attr_groups[] = {
 
 struct led_trigger ledtrig_blkdev_trigger = {
 	.name		= "blkdev",
+	.activate	= blkdev_activate,
 	.groups		= ledtrig_blkdev_attr_groups,
 };
-- 
2.31.1

Re: [PATCH v2 10/15] leds: trigger: blkdev: Add LED trigger activate function

[Greg KH]

On Thu, Sep 09, 2021 at 05:25:08PM -0500, Ian Pilcher wrote:
> 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 | 57 +++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
> index 40dc55e5d4f3..6f78a9515976 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev.c
> @@ -164,6 +164,62 @@ static void blkdev_process(struct work_struct *const work)
>  }
>  
>  
> +/*
> + *
> + *	Associate an LED with the blkdev trigger
> + *
> + */
> +
> +static int blkdev_activate(struct led_classdev *const led_dev)
> +{
> +	struct ledtrig_blkdev_led *led;
> +	int ret;
> +
> +	/* Don't allow module to be removed while any LEDs are linked */
> +	if (WARN_ON(!try_module_get(THIS_MODULE))) {

That pattern is racy and broken and never ever ever add it to the kernel
again please.  All existing in-kernel users of it are also wrong, we
have been removing them for decades now.

> +		ret = -ENODEV;		/* Shouldn't ever happen */
> +		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("linked_devices",
> +					  &led_dev->dev->kobj);

You have created a "raw" kobject in the device tree now, which means
that userspace will not be notified of it and will have a "hole" in it's
knowledge.  Why not just create a named attribute group to this device
instead?

> +	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);

Again, racy and broken, please do not do this.

thanks,

greg k-h

Re: [PATCH v2 10/15] leds: trigger: blkdev: Add LED trigger activate function

[Ian Pilcher]

On 9/10/21 1:47 AM, Greg KH wrote:
>> +	/* Don't allow module to be removed while any LEDs are linked */
>> +	if (WARN_ON(!try_module_get(THIS_MODULE))) {
> 
> That pattern is racy and broken and never ever ever add it to the kernel
> again please.  All existing in-kernel users of it are also wrong, we
> have been removing them for decades now.

OK.  (I was misled by the instances that you haven't gotten to yet.)

> You have created a "raw" kobject in the device tree now, which means
> that userspace will not be notified of it and will have a "hole" in it's
> knowledge.  Why not just create a named attribute group to this device
> instead?

What would I pass as the first argument to sysfs_create_link() in that
case?

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

[PATCH v2 11/15] leds: trigger: blkdev: Enable linking block devices to LEDs

[Ian Pilcher]

Add /sys/class/leds/<led>/link_device sysfs attribute

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 | 160 ++++++++++++++++++++++++++
 1 file changed, 160 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 6f78a9515976..26509837f037 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -71,6 +71,9 @@ static unsigned int ledtrig_blkdev_interval;
 static void blkdev_process(struct work_struct *const work);
 static DECLARE_DELAYED_WORK(ledtrig_blkdev_work, blkdev_process);
 
+/* Total number of device-to-LED associations */
+static unsigned int ledtrig_blkdev_count;
+
 
 /*
  *
@@ -220,6 +223,162 @@ static int blkdev_activate(struct led_classdev *const led_dev)
 }
 
 
+/*
+ *
+ *	link_device sysfs attribute - assocate a block device with this 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("linked_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_link_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)
+			return true;
+	}
+
+	return false;
+}
+
+static ssize_t link_device_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);
+	struct gendisk *gd;
+	int ret;
+
+	ret = mutex_lock_interruptible(&ledtrig_blkdev_mutex);
+	if (ret != 0)
+		goto exit_return;
+
+	gd = ledtrig_blkdev_get_disk(buf);
+	if (gd == NULL) {
+		ret = -ENODEV;
+		goto exit_unlock;
+	}
+
+	if (blkdev_already_linked(led, gd)) {
+		ret = -EEXIST;
+		goto exit_put_disk;
+	}
+
+	ret = blkdev_disk_link_locked(led, gd);
+
+exit_put_disk:
+	if (ret != 0)
+		put_disk(gd);
+exit_unlock:
+	mutex_unlock(&ledtrig_blkdev_mutex);
+exit_return:
+	return (ret == 0) ? count : ret;
+}
+
+static DEVICE_ATTR_WO(link_device);
+
+
 /*
  *
  *	Initialization - register the trigger
@@ -227,6 +386,7 @@ static int blkdev_activate(struct led_classdev *const led_dev)
  */
 
 static struct attribute *ledtrig_blkdev_attrs[] = {
+	&dev_attr_link_device.attr,
 	NULL
 };
 
-- 
2.31.1

Re: [PATCH v2 11/15] leds: trigger: blkdev: Enable linking block devices to LEDs

[Greg KH]

On Thu, Sep 09, 2021 at 05:25:09PM -0500, Ian Pilcher wrote:
> Add /sys/class/leds/<led>/link_device sysfs attribute
> 
> 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 | 160 ++++++++++++++++++++++++++
>  1 file changed, 160 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
> index 6f78a9515976..26509837f037 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev.c
> @@ -71,6 +71,9 @@ static unsigned int ledtrig_blkdev_interval;
>  static void blkdev_process(struct work_struct *const work);
>  static DECLARE_DELAYED_WORK(ledtrig_blkdev_work, blkdev_process);
>  
> +/* Total number of device-to-LED associations */
> +static unsigned int ledtrig_blkdev_count;
> +
>  
>  /*
>   *
> @@ -220,6 +223,162 @@ static int blkdev_activate(struct led_classdev *const led_dev)
>  }
>  
>  
> +/*
> + *
> + *	link_device sysfs attribute - assocate a block device with this 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);

When do you decrement this kobject?

> +		return 0;
> +	}
> +
> +	disk = kmalloc(sizeof(*disk), GFP_KERNEL);
> +	if (disk == NULL)
> +		return -ENOMEM;
> +
> +	dir = kobject_create_and_add("linked_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);

This should happen in the kobject release function, not here, right?

Did you try this out with removable block devices yet?

thanks,

greg k-h

Re: [PATCH v2 11/15] leds: trigger: blkdev: Enable linking block devices to LEDs

[Ian Pilcher]

On 9/10/21 1:48 AM, Greg KH wrote:
>> +/* 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);
> 
> When do you decrement this kobject?

That happens in blkdev_disk_unlink_locked() at line 399.  (Also in the
error path in blkdev_put_disk(), called at line 321.)

Looking at this now, blkdev_disk_unlink_locked() should be calling
blkdev_put_disk(), rather than calling kobject_put() directly.  I'll fix
that.

>> +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);
> 
> This should happen in the kobject release function, not here, right?

If you're referring to the kfree() call, it's freeing the
ledtrig_blkdev_disk structure, not the gendisk.  I use "gd" for gendisk
pointers and "disk" for ledtrig_blkdev_disk pointers.

> Did you try this out with removable block devices yet?

Absolutely.  I've tested removing both block devices and (user space)
LEDs.

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

[PATCH v2 12/15] leds: trigger: blkdev: Enable unlinking block devices from LEDs

[Ian Pilcher]

Add /sys/class/leds/<led>/unlink_device sysfs attribute

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 devices are associated with *any* LEDs

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

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 26509837f037..fff67ebd28f2 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -379,6 +379,62 @@ static ssize_t link_device_store(struct device *const dev,
 static DEVICE_ATTR_WO(link_device);
 
 
+/*
+ *
+ *	unlink_device sysfs attribute - disassociate a device from this LED
+ *
+ */
+
+static void blkdev_disk_unlink_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 ssize_t unlink_device_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);
+	struct ledtrig_blkdev_link *link;
+
+	mutex_lock(&ledtrig_blkdev_mutex);
+
+	hlist_for_each_entry(link, &led->disks, led_disks_node) {
+
+		if (sysfs_streq(link->disk->gd->disk_name, buf)) {
+			blkdev_disk_unlink_locked(led, link, link->disk);
+			break;
+		}
+	}
+
+	mutex_unlock(&ledtrig_blkdev_mutex);
+
+	return count;
+}
+
+static DEVICE_ATTR_WO(unlink_device);
+
+
 /*
  *
  *	Initialization - register the trigger
@@ -387,6 +443,7 @@ static DEVICE_ATTR_WO(link_device);
 
 static struct attribute *ledtrig_blkdev_attrs[] = {
 	&dev_attr_link_device.attr,
+	&dev_attr_unlink_device.attr,
 	NULL
 };
 
-- 
2.31.1

[PATCH v2 13/15] leds: trigger: blkdev: Add LED trigger deactivate function

[Ian Pilcher]

Remove all device links from 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 | 28 +++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index fff67ebd28f2..c7e101935bf6 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -435,6 +435,33 @@ static ssize_t unlink_device_store(struct device *const dev,
 static DEVICE_ATTR_WO(unlink_device);
 
 
+/*
+ *
+ *	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_unlink_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);
+}
+
+
 /*
  *
  *	Initialization - register the trigger
@@ -459,5 +486,6 @@ static const struct attribute_group *ledtrig_blkdev_attr_groups[] = {
 struct led_trigger ledtrig_blkdev_trigger = {
 	.name		= "blkdev",
 	.activate	= blkdev_activate,
+	.deactivate	= blkdev_deactivate,
 	.groups		= ledtrig_blkdev_attr_groups,
 };
-- 
2.31.1

[PATCH v2 14/15] leds: trigger: blkdev: Add remaining 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

/sys/class/leds/<led>/mode - blink LED for reads, writes, or both

When showing mode, show all modes with current value in square brackets

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

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index c7e101935bf6..4b88f877ee81 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -462,6 +462,135 @@ static void blkdev_deactivate(struct led_classdev *const led_dev)
 }
 
 
+/*
+ *
+ *	blink_time sysfs attribute
+ *
+ */
+
+static ssize_t blink_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);
+
+	return sprintf(buf, "%u\n", READ_ONCE(led->blink_msec));
+}
+
+static ssize_t blink_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_BLINK)
+		return -ERANGE;
+
+	WRITE_ONCE(led->blink_msec, value);
+	return count;
+}
+
+static DEVICE_ATTR_RW(blink_time);
+
+
+/*
+ *
+ *	interval sysfs attribute - affects all LEDS
+ *
+ */
+
+static ssize_t interval_show(struct device *const dev,
+			     struct device_attribute *const attr,
+			     char *const buf)
+{
+	return sprintf(buf, "%u\n",
+		       jiffies_to_msecs(READ_ONCE(ledtrig_blkdev_interval)));
+}
+
+static ssize_t interval_store(struct device *const dev,
+			      struct device_attribute *const attr,
+			      const char *const buf, const size_t count)
+{
+	unsigned int value;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &value);
+	if (ret != 0)
+		return ret;
+
+	if (value < LEDTRIG_BLKDEV_MIN_INT)
+		return -ERANGE;
+
+	WRITE_ONCE(ledtrig_blkdev_interval, msecs_to_jiffies(value));
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(interval);
+
+
+/*
+ *
+ *	mode sysfs attribute - blink for reads, writes, or both
+ *
+ */
+
+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 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 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);
+	enum ledtrig_blkdev_mode mode;
+
+	for (mode = LEDTRIG_BLKDEV_MODE_RO;
+				mode <= LEDTRIG_BLKDEV_MODE_RW; ++mode) {
+
+		if (sysfs_streq(blkdev_modes[mode].name, buf)) {
+			WRITE_ONCE(led->mode, mode);
+			return count;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static DEVICE_ATTR_RW(mode);
+
+
 /*
  *
  *	Initialization - register the trigger
@@ -471,6 +600,9 @@ static void blkdev_deactivate(struct led_classdev *const led_dev)
 static struct attribute *ledtrig_blkdev_attrs[] = {
 	&dev_attr_link_device.attr,
 	&dev_attr_unlink_device.attr,
+	&dev_attr_blink_time.attr,
+	&dev_attr_interval.attr,
+	&dev_attr_mode.attr,
 	NULL
 };
 
-- 
2.31.1

[PATCH v2 15/15] leds: trigger: blkdev: Add disk cleanup and init/exit functions

[Ian Pilcher]

Disk cleanup function - blkdev_disk_cleanup():

  * 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

Init function - blkdev_init():

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

Exit function - blkdev_exit():

  * Unregister the LED trigger
  * Set __ledtrig_blkdev_disk_cleanup back to NULL

Make LED trigger struct (ledtrig_blkdev_trigger) static (as it's now
referenced in blkdev_init()).

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

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 4b88f877ee81..96ea694ce29f 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -435,6 +435,23 @@ static ssize_t unlink_device_store(struct device *const dev,
 static DEVICE_ATTR_WO(unlink_device);
 
 
+/*
+ *
+ *	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_unlink_locked(link->led, link, gd->ledtrig);
+}
+
+
 /*
  *
  *	Disassociate an LED from the trigger
@@ -615,9 +632,55 @@ static const struct attribute_group *ledtrig_blkdev_attr_groups[] = {
 	NULL
 };
 
-struct led_trigger ledtrig_blkdev_trigger = {
+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 v2 00/15] Introduce block device LED trigger

[Marek Behún]

Dear Ian,

I have tried to look into this and replied to some of your patches.

There are still many things to do, and I think the reviewing would be
much easier to review if you sent all the code changes as one patch
(since the changes are doing an atomic change: adding support for blkdev
LED trigger). Keep only the sysfs doc change in a separate patch.

You are unnecessary using the const keyword in places where it is not
needed and not customary for Linux kernel codebase. See in another of
my replies.

You are using a weird comment style, i.e.
  /*
   *
   *	Disassociate an LED from the trigger
   *
   */

  static void blkdev_deactivate(struct led_classdev *const led_dev)

Please look at how functions are documented in led-class.c, for example.

There are many other things I would like you to change and fix,
I will comment on them once you send this proposal as two commits:
one sysfs docs change, one code change.

Marek

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

[Ian Pilcher]

On 9/9/21 9:09 PM, Marek Behún wrote:
> I have tried to look into this and replied to some of your patches.
> 
> There are still many things to do, and I think the reviewing would be
> much easier to review if you sent all the code changes as one patch
> (since the changes are doing an atomic change: adding support for blkdev
> LED trigger). Keep only the sysfs doc change in a separate patch.

Marek -

I'll try to get a simplified version out as soon as I can.  It will
probably be 3 patches, because I do think that the block subsystem
changes should be in a separate patch.

(I agree that it will be simpler to review - not to mention easier for
me to create.  Past experience does tell me that there are likely some
folks who will object to that format, however.)

> You are unnecessary using the const keyword in places where it is not
> needed and not customary for Linux kernel codebase. See in another of
> my replies.

I did see that.  I'm a believer in declaring anything that should not
change as const (to the extent that C allows).  It documents the
fact that the value is expected to remain unchanged throughout the
function call, and it enlists the compiler to enforce it.

So while it's true that they aren't necessary, they do result in code
that is at least slightly less likely to be broken by future changes.

> You are using a weird comment style, i.e.
>    /*
>     *
>     *	Disassociate an LED from the trigger
>     *
>     */
> 
>    static void blkdev_deactivate(struct led_classdev *const led_dev)
> 
> Please look at how functions are documented in led-class.c, for example.

Well ... that comment isn't documenting that function.  It's intended to
identify a section of the file whose contents are related.  If there's a
different comment style that I should be using for that purpose, please
let me know.

I'll respond to your other feedback separately.

Thanks for taking your time on this.  I really do appreciate it!

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

Re: [PATCH v2 12/15] leds: trigger: blkdev: Enable unlinking block devices from LEDs

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210909222513.2184795-13-arequipeno@gmail.com>
References: <20210909222513.2184795-13-arequipeno@gmail.com>
TO: Ian Pilcher <arequipeno@gmail.com>
TO: axboe(a)kernel.dk
TO: pavel(a)ucw.cz
CC: linux-leds(a)vger.kernel.org
CC: linux-block(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org
CC: gregkh(a)linuxfoundation.org
CC: kabel(a)kernel.org

Hi Ian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on a3fa7a101dcff93791d1b1bdb3affcad1410c8c1]

url:    https://github.com/0day-ci/linux/commits/Ian-Pilcher/Introduce-block-device-LED-trigger/20210910-062756
base:   a3fa7a101dcff93791d1b1bdb3affcad1410c8c1
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: i386-randconfig-m021-20210912 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

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

smatch warnings:
drivers/leds/trigger/ledtrig-blkdev.c:410 blkdev_disk_unlink_locked() error: dereferencing freed memory 'disk'

vim +/disk +410 drivers/leds/trigger/ledtrig-blkdev.c

4a00ef60e59b58 Ian Pilcher 2021-09-09  380  
4a00ef60e59b58 Ian Pilcher 2021-09-09  381  
66cb682de7e8bd Ian Pilcher 2021-09-09  382  /*
66cb682de7e8bd Ian Pilcher 2021-09-09  383   *
66cb682de7e8bd Ian Pilcher 2021-09-09  384   *	unlink_device sysfs attribute - disassociate a device from this LED
66cb682de7e8bd Ian Pilcher 2021-09-09  385   *
66cb682de7e8bd Ian Pilcher 2021-09-09  386   */
66cb682de7e8bd Ian Pilcher 2021-09-09  387  
66cb682de7e8bd Ian Pilcher 2021-09-09  388  static void blkdev_disk_unlink_locked(struct ledtrig_blkdev_led *const led,
66cb682de7e8bd Ian Pilcher 2021-09-09  389  				      struct ledtrig_blkdev_link *const link,
66cb682de7e8bd Ian Pilcher 2021-09-09  390  				      struct ledtrig_blkdev_disk *const disk)
66cb682de7e8bd Ian Pilcher 2021-09-09  391  {
66cb682de7e8bd Ian Pilcher 2021-09-09  392  	--ledtrig_blkdev_count;
66cb682de7e8bd Ian Pilcher 2021-09-09  393  
66cb682de7e8bd Ian Pilcher 2021-09-09  394  	if (ledtrig_blkdev_count == 0)
66cb682de7e8bd Ian Pilcher 2021-09-09  395  		WARN_ON(!cancel_delayed_work_sync(&ledtrig_blkdev_work));
66cb682de7e8bd Ian Pilcher 2021-09-09  396  
66cb682de7e8bd Ian Pilcher 2021-09-09  397  	sysfs_remove_link(led->dir, disk->gd->disk_name);
66cb682de7e8bd Ian Pilcher 2021-09-09  398  	sysfs_remove_link(disk->dir, led->led_dev->name);
66cb682de7e8bd Ian Pilcher 2021-09-09  399  	kobject_put(disk->dir);
66cb682de7e8bd Ian Pilcher 2021-09-09  400  
66cb682de7e8bd Ian Pilcher 2021-09-09  401  	hlist_del(&link->led_disks_node);
66cb682de7e8bd Ian Pilcher 2021-09-09  402  	hlist_del(&link->disk_leds_node);
66cb682de7e8bd Ian Pilcher 2021-09-09  403  	kfree(link);
66cb682de7e8bd Ian Pilcher 2021-09-09  404  
66cb682de7e8bd Ian Pilcher 2021-09-09  405  	if (hlist_empty(&disk->leds)) {
66cb682de7e8bd Ian Pilcher 2021-09-09  406  		disk->gd->ledtrig = NULL;
66cb682de7e8bd Ian Pilcher 2021-09-09  407  		kfree(disk);
66cb682de7e8bd Ian Pilcher 2021-09-09  408  	}
66cb682de7e8bd Ian Pilcher 2021-09-09  409  
66cb682de7e8bd Ian Pilcher 2021-09-09 @410  	put_disk(disk->gd);
66cb682de7e8bd Ian Pilcher 2021-09-09  411  }
66cb682de7e8bd Ian Pilcher 2021-09-09  412  

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

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

Re: [PATCH v2 12/15] leds: trigger: blkdev: Enable unlinking block devices from LEDs

[Dan Carpenter]

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

Hi Ian,

url:    https://github.com/0day-ci/linux/commits/Ian-Pilcher/Introduce-block-device-LED-trigger/20210910-062756
base:   a3fa7a101dcff93791d1b1bdb3affcad1410c8c1
config: i386-randconfig-m021-20210912 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

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

smatch warnings:
drivers/leds/trigger/ledtrig-blkdev.c:410 blkdev_disk_unlink_locked() error: dereferencing freed memory 'disk'

vim +/disk +410 drivers/leds/trigger/ledtrig-blkdev.c

66cb682de7e8bd Ian Pilcher 2021-09-09  388  static void blkdev_disk_unlink_locked(struct ledtrig_blkdev_led *const led,
66cb682de7e8bd Ian Pilcher 2021-09-09  389  				      struct ledtrig_blkdev_link *const link,
66cb682de7e8bd Ian Pilcher 2021-09-09  390  				      struct ledtrig_blkdev_disk *const disk)
66cb682de7e8bd Ian Pilcher 2021-09-09  391  {
66cb682de7e8bd Ian Pilcher 2021-09-09  392  	--ledtrig_blkdev_count;
66cb682de7e8bd Ian Pilcher 2021-09-09  393  
66cb682de7e8bd Ian Pilcher 2021-09-09  394  	if (ledtrig_blkdev_count == 0)
66cb682de7e8bd Ian Pilcher 2021-09-09  395  		WARN_ON(!cancel_delayed_work_sync(&ledtrig_blkdev_work));
66cb682de7e8bd Ian Pilcher 2021-09-09  396  
66cb682de7e8bd Ian Pilcher 2021-09-09  397  	sysfs_remove_link(led->dir, disk->gd->disk_name);
66cb682de7e8bd Ian Pilcher 2021-09-09  398  	sysfs_remove_link(disk->dir, led->led_dev->name);
66cb682de7e8bd Ian Pilcher 2021-09-09  399  	kobject_put(disk->dir);
66cb682de7e8bd Ian Pilcher 2021-09-09  400  
66cb682de7e8bd Ian Pilcher 2021-09-09  401  	hlist_del(&link->led_disks_node);
66cb682de7e8bd Ian Pilcher 2021-09-09  402  	hlist_del(&link->disk_leds_node);
66cb682de7e8bd Ian Pilcher 2021-09-09  403  	kfree(link);
66cb682de7e8bd Ian Pilcher 2021-09-09  404  
66cb682de7e8bd Ian Pilcher 2021-09-09  405  	if (hlist_empty(&disk->leds)) {
66cb682de7e8bd Ian Pilcher 2021-09-09  406  		disk->gd->ledtrig = NULL;
66cb682de7e8bd Ian Pilcher 2021-09-09  407  		kfree(disk);
                                                              ^^^^
Freed.

66cb682de7e8bd Ian Pilcher 2021-09-09  408  	}
66cb682de7e8bd Ian Pilcher 2021-09-09  409  
66cb682de7e8bd Ian Pilcher 2021-09-09 @410  	put_disk(disk->gd);
                                                         ^^^^^^^^
Dereference after free.

66cb682de7e8bd Ian Pilcher 2021-09-09  411  }

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

Re: [PATCH v2 12/15] leds: trigger: blkdev: Enable unlinking block devices from LEDs

[Dan Carpenter]

Hi Ian,

url:    https://github.com/0day-ci/linux/commits/Ian-Pilcher/Introduce-block-device-LED-trigger/20210910-062756
base:   a3fa7a101dcff93791d1b1bdb3affcad1410c8c1
config: i386-randconfig-m021-20210912 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

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

smatch warnings:
drivers/leds/trigger/ledtrig-blkdev.c:410 blkdev_disk_unlink_locked() error: dereferencing freed memory 'disk'

vim +/disk +410 drivers/leds/trigger/ledtrig-blkdev.c

66cb682de7e8bd Ian Pilcher 2021-09-09  388  static void blkdev_disk_unlink_locked(struct ledtrig_blkdev_led *const led,
66cb682de7e8bd Ian Pilcher 2021-09-09  389  				      struct ledtrig_blkdev_link *const link,
66cb682de7e8bd Ian Pilcher 2021-09-09  390  				      struct ledtrig_blkdev_disk *const disk)
66cb682de7e8bd Ian Pilcher 2021-09-09  391  {
66cb682de7e8bd Ian Pilcher 2021-09-09  392  	--ledtrig_blkdev_count;
66cb682de7e8bd Ian Pilcher 2021-09-09  393  
66cb682de7e8bd Ian Pilcher 2021-09-09  394  	if (ledtrig_blkdev_count == 0)
66cb682de7e8bd Ian Pilcher 2021-09-09  395  		WARN_ON(!cancel_delayed_work_sync(&ledtrig_blkdev_work));
66cb682de7e8bd Ian Pilcher 2021-09-09  396  
66cb682de7e8bd Ian Pilcher 2021-09-09  397  	sysfs_remove_link(led->dir, disk->gd->disk_name);
66cb682de7e8bd Ian Pilcher 2021-09-09  398  	sysfs_remove_link(disk->dir, led->led_dev->name);
66cb682de7e8bd Ian Pilcher 2021-09-09  399  	kobject_put(disk->dir);
66cb682de7e8bd Ian Pilcher 2021-09-09  400  
66cb682de7e8bd Ian Pilcher 2021-09-09  401  	hlist_del(&link->led_disks_node);
66cb682de7e8bd Ian Pilcher 2021-09-09  402  	hlist_del(&link->disk_leds_node);
66cb682de7e8bd Ian Pilcher 2021-09-09  403  	kfree(link);
66cb682de7e8bd Ian Pilcher 2021-09-09  404  
66cb682de7e8bd Ian Pilcher 2021-09-09  405  	if (hlist_empty(&disk->leds)) {
66cb682de7e8bd Ian Pilcher 2021-09-09  406  		disk->gd->ledtrig = NULL;
66cb682de7e8bd Ian Pilcher 2021-09-09  407  		kfree(disk);
                                                              ^^^^
Freed.

66cb682de7e8bd Ian Pilcher 2021-09-09  408  	}
66cb682de7e8bd Ian Pilcher 2021-09-09  409  
66cb682de7e8bd Ian Pilcher 2021-09-09 @410  	put_disk(disk->gd);
                                                         ^^^^^^^^
Dereference after free.

66cb682de7e8bd Ian Pilcher 2021-09-09  411  }

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