Re: [PATCH RFC 0/4] leds: extend disk trigger

[Markus Probst <markus.probst@posteo.de>]

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

On Wed, 2026-01-28 at 15:34 +0900, Damien Le Moal wrote:
> On 1/28/26 12:34 AM, Markus Probst wrote:
> > > Having a user space implementation for your feature would also allow
> > > an upstream kernel, without the need for any custom kernel patches.
> > Only because it can be done in userspace, doesn't mean it should be.
> 
> Yes it should. Maintaining userspace is far easier than forcing kernel changes
> onto users to get blinking LEDs. So unless you have a very strong argument for
> doing it in the kernel, userspace is probably the right approach. That will
> apply to any block device, and not just ATA devices. E.g. NAS with M.2 NVMe
> storage can work with the same.
> 
> > > There seems to be existing user space applications that handles this,
> > > I think both the daemon I linked to before, which uses /sys/block/<dev>/stat
> > > which is thus per device and not per port, and e.g. this:
> > > https://linux.die.net/man/8/ledmon
> > > https://github.com/md-raid-utilities/ledmon
> > > https://github.com/md-raid-utilities/ledmon/blob/main/src/lib/ahci.c
> > As far as I can tell, this daemon doesn't actually use the LED
> > Subsystem, but instead leds directly connected to the storage
> > controller.
> > But yes, I would be capable of coding such daemon.
> 
> Then let's try.
I will give it a try.

@Ian: I will notify you, once there is a working version.

> That will allow checking if anything is missing in the kernel
> interface to do that nicely.
There is.

I noticed for leds, that the fwnode path isn't exposed in sysfs.
"/sys/class/leds/<name>/device/firmware_node/path" exists, but points
to the parent device.

Something similar with scsi and ata exists. scsi doesn't expose the
firmware_node and there is no symlink (or other connection that I am
ware of) between scsi_* and ata_* in sysfs. This means, I cannot map a
fwnode path to a block device.

If I want to distribute a pre-defined config for such led userspace
daemon alongside the ACPI Overlay for a specific NAS model, I need an
identifier that is equal across all devices with that specific NAS
model.

This is less of an issue for leds, but given that leds could be renamed
on name collisions the issue still exists.

Thanks
- Markus Probst

> 
> > > I think my main concern is that I don't think we should bloat the kernel
> > > for a complex feature that can just as well be implemented in user space.
> > It is still unclear to me if you worry about the complexity in
> > drivers/ata/libata-* or drivers/leds/trigger/ledtrig-disk.c
> 
> It is not so much about complexity but rather the fact that controlling
> blinking LEDs in the hot IO path is not desirable. While SATA HDDs will be less
> sensible to the delays caused by the calls to the LED control functions
> compared to fast NVMe SSDs, we do also need to think about much faster SATA
> SSDs. So instead of risking performance regressions, let's try to do this in
> userspace first.
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]