Extend the disk trigger
- to allow configuration of the blinking delays
  and whether the led should be kept on, on idle.
- to allow an individual led to be mapped to an ata port

I would also like to add another patch to this series, only leaving the led
on with invert 1 if also at least one disk is present on the ata port.
The led would then not only indicate activity, but also if a disk is
present.
That is why it is an RFC.

@Damien,Niclas: What would be the most straightforward way of telling
the led trigger if at least one disk is present on the ata port and
notifing it when this changes?

Thanks
- Markus Probst

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Markus Probst (4):
      leds: dt-bindings: add disk trigger led pattern
      leds: dt-bindings: add disk trigger for each ata port
      leds: add delay_on, delay_off and invert attributes to disk trigger
      leds: add disk trigger for each ata port

 Documentation/devicetree/bindings/leds/common.yaml |  10 +-
 drivers/ata/libata-core.c                          |  22 +-
 drivers/leds/trigger/ledtrig-disk.c                | 298 ++++++++++++++++++++-
 drivers/scsi/libsas/sas_ata.c                      |   3 +-
 include/linux/leds.h                               |  16 +-
 include/linux/libata.h                             |   6 +-
 6 files changed, 332 insertions(+), 23 deletions(-)
---
base-commit: 24d479d26b25bce5faea3ddd9fa8f3a6c3129ea7
change-id: 20260123-ledtrig_disk_-1a500531865a

Hello Markus,

On Fri, Jan 23, 2026 at 07:05:03PM +0000, Markus Probst wrote:
> Extend the disk trigger
> - to allow configuration of the blinking delays
>   and whether the led should be kept on, on idle.
> - to allow an individual led to be mapped to an ata port
> 
> I would also like to add another patch to this series, only leaving the led
> on with invert 1 if also at least one disk is present on the ata port.
> The led would then not only indicate activity, but also if a disk is
> present.
> That is why it is an RFC.
> 
> @Damien,Niclas: What would be the most straightforward way of telling
> the led trigger if at least one disk is present on the ata port and
> notifing it when this changes?

Why do we want to have this in kernel space?

Sure, there is already the very simple ledtrig-disk driver.

But I'm not a fan of making the driver more complex.
If we want something more complex than what is already there, then it
is probably much better handled in user space, considering the amount
of possible configuration options.

Basically the same argument as used in:
https://lore.kernel.org/linux-nvme/20220227234258.24619-1-ematsumiya@suse.de/T/#u


Kind regards,
Niklas

On Mon, 2026-01-26 at 10:00 +0100, Niklas Cassel wrote:
> Hello Markus,
> 
> On Fri, Jan 23, 2026 at 07:05:03PM +0000, Markus Probst wrote:
> > Extend the disk trigger
> > - to allow configuration of the blinking delays
> >   and whether the led should be kept on, on idle.
> > - to allow an individual led to be mapped to an ata port
> > 
> > I would also like to add another patch to this series, only leaving the led
> > on with invert 1 if also at least one disk is present on the ata port.
> > The led would then not only indicate activity, but also if a disk is
> > present.
> > That is why it is an RFC.
> > 
> > @Damien,Niclas: What would be the most straightforward way of telling
> > the led trigger if at least one disk is present on the ata port and
> > notifing it when this changes?
> 
> Why do we want to have this in kernel space?
Because there are more than enough devices that could make use of it.

Just search the term "NAS device" and you see rarely any devices for
which this wouldn't be useful.

The only reason the leds work on those devices currently, is because
they get shipped with a custom modified kernel by the manufacturer.
This shouldn't be a requirement for running Linux properly on a NAS
device with disk leds.


> Sure, there is already the very simple ledtrig-disk driver.
> 
> But I'm not a fan of making the driver more complex.
Do you mean the complexity it would introduce in libata or for the led
trigger itself?

At least with the current patches it looks fairly maintainable.
For instance the pattern led trigger is more complex in my opinion.

In the case of libata and the indication for a presence of a disk, I
would suggest that I implement it first and we can see after I have a
working version if it is acceptable or not.

I am still asking for guidance on checking if at least one disk is
present on a ata port.

> If we want something more complex than what is already there, then it
> is probably much better handled in user space, considering the amount
> of possible configuration options.
A userspace daemon by itself is possible, but I don't think it is the
best solution. Having an indicator for disk activity on a per-disk
basis seems like basic led functionality that should be present in the
kernel.

It is a very minor detail, but I would prefer to have "linux,default-
trigger" set on the led in the fwnode and having the functionality
automatically for every linux system on the hardware, instead of having
to deal with a userspace daemon.
If this is the easiest solution for nas manufacturer to do disk leds,
there is a good chance it getting adopted some day in the future by
those manufacturer and thus making it work out of the box when
switching away from their proprietary os.

> 
> Basically the same argument as used in:
> https://lore.kernel.org/linux-nvme/20220227234258.24619-1-ematsumiya@suse.de/T/#u
If I understood it corretly, the argument there is that led code
shouldn't be present in a fast path.

This does not apply to this scenario.

Thanks
- Markus Probst

> 
> 
> Kind regards,
> Niklas

On Mon, Jan 26, 2026 at 10:06:02PM +0000, Markus Probst wrote:
> On Mon, 2026-01-26 at 10:00 +0100, Niklas Cassel wrote:
> > 
> > Why do we want to have this in kernel space?
> Because there are more than enough devices that could make use of it.
> 
> Just search the term "NAS device" and you see rarely any devices for
> which this wouldn't be useful.
> 
> The only reason the leds work on those devices currently, is because
> they get shipped with a custom modified kernel by the manufacturer.
> This shouldn't be a requirement for running Linux properly on a NAS
> device with disk leds.

I understand why you want the feature. I just don't understand why we
should add this feature to the kernel, rather than implement it in
user space.

Having a user space implementation for your feature would also allow
an upstream kernel, without the need for any custom kernel patches.


> > If we want something more complex than what is already there, then it
> > is probably much better handled in user space, considering the amount
> > of possible configuration options.
> A userspace daemon by itself is possible, but I don't think it is the
> best solution. Having an indicator for disk activity on a per-disk
> basis seems like basic led functionality that should be present in the
> kernel.

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


> > Basically the same argument as used in:
> > https://lore.kernel.org/linux-nvme/20220227234258.24619-1-ematsumiya@suse.de/T/#u
> If I understood it corretly, the argument there is that led code
> shouldn't be present in a fast path.
> 
> This does not apply to this scenario.

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.


Kind regards,
Niklas

On Tue, 2026-01-27 at 10:32 +0100, Niklas Cassel wrote:
> On Mon, Jan 26, 2026 at 10:06:02PM +0000, Markus Probst wrote:
> > On Mon, 2026-01-26 at 10:00 +0100, Niklas Cassel wrote:
> > > 
> > > Why do we want to have this in kernel space?
> > Because there are more than enough devices that could make use of it.
> > 
> > Just search the term "NAS device" and you see rarely any devices for
> > which this wouldn't be useful.
> > 
> > The only reason the leds work on those devices currently, is because
> > they get shipped with a custom modified kernel by the manufacturer.
> > This shouldn't be a requirement for running Linux properly on a NAS
> > device with disk leds.
> 
> I understand why you want the feature. I just don't understand why we
> should add this feature to the kernel, rather than implement it in
> user space.
> 
> 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.
> 
> > > If we want something more complex than what is already there, then it
> > > is probably much better handled in user space, considering the amount
> > > of possible configuration options.
> > A userspace daemon by itself is possible, but I don't think it is the
> > best solution. Having an indicator for disk activity on a per-disk
> > basis seems like basic led functionality that should be present in the
> > kernel.
> 
> 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.

> > > Basically the same argument as used in:
> > > https://lore.kernel.org/linux-nvme/20220227234258.24619-1-ematsumiya@suse.de/T/#u
> > If I understood it corretly, the argument there is that led code
> > shouldn't be present in a fast path.
> > 
> > This does not apply to this scenario.
> 
> 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

@Pavel,@Lee: I would like to know your opinion on this.

Thanks
- Markus Probst

> 
> 
> Kind regards,
> Niklas

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. That will allow checking if anything is missing in the kernel
interface to do that nicely.

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


-- 
Damien Le Moal
Western Digital Research

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

On 1/29/26 00:44, Markus Probst wrote:
>> 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.

All of this is not the hot path, so we can work on it.

If new sysfs device attributes for an ATA device, you can add them to
ata_ncq_sdev_attrs in libata-sata.c. These show up as part of the scsi device
attributes, so if you define this well with the scsi side, the same attribute
names can be used for pure scsi devices and ATA devices served with libata.


-- 
Damien Le Moal
Western Digital Research

Hello Markus,

On Wed, Jan 28, 2026 at 03:44:19PM +0000, Markus Probst wrote:
> 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.

Hopefully this might help you:

$ ls -al /sys/class/scsi_device | grep ata
lrwxrwxrwx.  1 root root 0 Jan 29 05:12 4:0:0:0 -> ../../devices/pci0000:00/0000:00:17.0/ata5/host4/target4:0:0/4:0:0:0/scsi_device/4:0:0:0
lrwxrwxrwx.  1 root root 0 Jan 29 05:12 5:0:0:0 -> ../../devices/pci0000:00/0000:00:17.0/ata6/host5/target5:0:0/5:0:0:0/scsi_device/5:0:0:0
lrwxrwxrwx.  1 root root 0 Jan 29 05:12 8:0:0:0 -> ../../devices/pci0000:00/0000:00:17.0/ata9/host8/target8:0:0/8:0:0:0/scsi_device/8:0:0:0
lrwxrwxrwx.  1 root root 0 Jan 29 05:12 9:0:0:0 -> ../../devices/pci0000:00/0000:00:17.0/ata10/host9/target9:0:0/9:0:0:0/scsi_device/9:0:0:0


For a specific device, e.g. 4:0:0:0:

$ realpath /sys/class/scsi_device/4:0:0:0/
/sys/devices/pci0000:00/0000:00:17.0/ata5/host4/target4:0:0/4:0:0:0/scsi_device/4:0:0:0

To get the block device name:
$ ls /sys/class/scsi_device/4:0:0:0/device/block/
sda

or

$ ls /sys/devices/pci0000:00/0000:00:17.0/ata5/host4/target4:0:0/4:0:0:0/scsi_device/4:0:0:0/device/block/
sda


You can parse the port from the path. The above example is port 5.

If using a port multiplier (PM), there can be multiple links/devices per port.


Otherwise, for SATA there should be only one.

$ ls -al /sys/class/ata_port/ata5/device/ | grep link | wc -l
1

$ ls -al /sys/class/ata_port/ata5/device/link5/dev5.0/firmware_node

or

$ ls -al  /sys/class/ata_device/dev5.*/device/firmware_node



For PCI BDF, you can use /dev/disk/by-path/

$ ls -al /dev/disk/by-path/ | grep ata
lrwxrwxrwx.  1 root root   9 Jan 29 05:11 pci-0000:00:17.0-ata-3 -> ../../sda
lrwxrwxrwx.  1 root root   9 Jan 29 05:11 pci-0000:00:17.0-ata-3.0 -> ../../sda
lrwxrwxrwx.  1 root root   9 Jan 29 05:11 pci-0000:00:17.0-ata-4 -> ../../sdb
lrwxrwxrwx.  1 root root   9 Jan 29 05:11 pci-0000:00:17.0-ata-4.0 -> ../../sdb
lrwxrwxrwx.  1 root root   9 Jan 29 05:11 pci-0000:00:17.0-ata-7 -> ../../sdc
lrwxrwxrwx.  1 root root   9 Jan 29 05:11 pci-0000:00:17.0-ata-7.0 -> ../../sdc
lrwxrwxrwx.  1 root root   9 Jan 29 05:11 pci-0000:00:17.0-ata-8 -> ../../sdd
lrwxrwxrwx.  1 root root   9 Jan 29 05:11 pci-0000:00:17.0-ata-8.0 -> ../../sdd

Note that these suffixes do not correlate to the ata port number in /sys/class/ata_*


$ ls -al /sys/class/ata_port/
total 0
drwxr-xr-x.  2 root root 0 Jan 29 05:11 .
drwxr-xr-x. 84 root root 0 Jan 29 05:12 ..
lrwxrwxrwx.  1 root root 0 Nov 19 04:01 ata1 -> ../../devices/pci0000:00/0000:00:11.5/ata1/ata_port/ata1
lrwxrwxrwx.  1 root root 0 Jan 29 05:26 ata10 -> ../../devices/pci0000:00/0000:00:17.0/ata10/ata_port/ata10
lrwxrwxrwx.  1 root root 0 Nov 19 04:01 ata11 -> ../../devices/pci0000:50/0000:50:02.0/0000:51:00.0/ata11/ata_port/ata11
lrwxrwxrwx.  1 root root 0 Nov 19 04:01 ata12 -> ../../devices/pci0000:50/0000:50:02.0/0000:51:00.0/ata12/ata_port/ata12
lrwxrwxrwx.  1 root root 0 Nov 19 04:01 ata13 -> ../../devices/pci0000:50/0000:50:02.0/0000:51:00.0/ata13/ata_port/ata13
lrwxrwxrwx.  1 root root 0 Nov 19 04:01 ata2 -> ../../devices/pci0000:00/0000:00:11.5/ata2/ata_port/ata2
lrwxrwxrwx.  1 root root 0 Jan 29 05:26 ata3 -> ../../devices/pci0000:00/0000:00:17.0/ata3/ata_port/ata3
lrwxrwxrwx.  1 root root 0 Jan 29 05:26 ata4 -> ../../devices/pci0000:00/0000:00:17.0/ata4/ata_port/ata4
lrwxrwxrwx.  1 root root 0 Jan 29 05:26 ata5 -> ../../devices/pci0000:00/0000:00:17.0/ata5/ata_port/ata5
lrwxrwxrwx.  1 root root 0 Jan 29 05:26 ata6 -> ../../devices/pci0000:00/0000:00:17.0/ata6/ata_port/ata6
lrwxrwxrwx.  1 root root 0 Jan 29 05:26 ata7 -> ../../devices/pci0000:00/0000:00:17.0/ata7/ata_port/ata7
lrwxrwxrwx.  1 root root 0 Jan 29 05:26 ata8 -> ../../devices/pci0000:00/0000:00:17.0/ata8/ata_port/ata8
lrwxrwxrwx.  1 root root 0 Jan 29 05:26 ata9 -> ../../devices/pci0000:00/0000:00:17.0/ata9/ata_port/ata9


Kind regards,
Niklas

On 1/26/26 3:00 AM, Niklas Cassel wrote:
> But I'm not a fan of making the driver more complex.
> If we want something more complex than what is already there, then it
> is probably much better handled in user space, considering the amount
> of possible configuration options.
> 
> Basically the same argument as used in:
> https://lore.kernel.org/linux-nvme/20220227234258.24619-1-ematsumiya@suse.de/T/#u

Niklas -

Can you provide some links on how this might be done in userspace?

I've been maintaining my out-of-tree block device trigger for years, to
make the LEDs on my NAS work.

  https://github.com/ipilcher/ledtrig-blkdev/blob/v6.9%2B/drivers/leds/trigger/ledtrig-blkdev.c

I'd love to be able to replace it with something in-tree.

-- 
========================================================================
If your user interface is intuitive in retrospect ... it isn't intuitive
========================================================================

Hello Ian,

On Mon, Jan 26, 2026 at 10:19:07AM -0600, Ian Pilcher wrote:
> On 1/26/26 3:00 AM, Niklas Cassel wrote:
> > But I'm not a fan of making the driver more complex.
> > If we want something more complex than what is already there, then it
> > is probably much better handled in user space, considering the amount
> > of possible configuration options.
> > 
> > Basically the same argument as used in:
> > https://lore.kernel.org/linux-nvme/20220227234258.24619-1-ematsumiya@suse.de/T/#u
> 
> Niklas -
> 
> Can you provide some links on how this might be done in userspace?

See the link to the thread above.

Ming suggests "tracking iostat in a fixed period, and triggering one
led activity if any read/write IO happens during 0.5sec."

There is also a link to an nvme-led-daemon in the thread:
https://github.com/scarlion1/nvme-led-daemon

But considering that it is using:
https://www.kernel.org/doc/Documentation/block/stat.txt

I don't see why it can't be used for any block device.


Kind regards,
Niklas

Hi!

> Extend the disk trigger
> - to allow configuration of the blinking delays
>   and whether the led should be kept on, on idle.
> - to allow an individual led to be mapped to an ata port

I have used led trigger before, and it annoyed me than "constant disk
activity" resulted in "constant LED blinking" instead of "LED
constantly on". I would not mind if that was fixed.

Thanks and best regards,
								Pavel

On Sun, 2026-01-25 at 00:21 +0100, Pavel Machek wrote:
> Hi!
> 
> > Extend the disk trigger
> > - to allow configuration of the blinking delays
> >   and whether the led should be kept on, on idle.
> > - to allow an individual led to be mapped to an ata port
> 
> I have used led trigger before, and it annoyed me than "constant disk
> activity" resulted in "constant LED blinking" instead of "LED
> constantly on". I would not mind if that was fixed.
> 
> Thanks and best regards,
> 								Pavel

This patch series adds support for changing the delay_off value in
blinking.

For having "constant disk activity" => "LED constantly on" the
delay_off value has to be set to 0 ms.

Let me know if I should adjust the default of delay_off from 30 ms to 0
ms.

Thanks
- Markus Probst