Message-ID: <20f855baaa7c36010eab9997a2f43b4f62be726b.camel@posteo.de>
Date: Tue, 27 Jan 2026 15:34:11 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Niklas Cassel <cassel@...nel.org>
Cc: Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>, Rob Herring
	 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley	
 <conor+dt@...nel.org>, Jacek Anaszewski <jacek.anaszewski@...il.com>,
 Damien Le Moal <dlemoal@...nel.org>, John Garry <john.g.garry@...cle.com>,
 Jason Yan <yanaijie@...wei.com>,  "James E.J. Bottomley"
 <James.Bottomley@...senpartnership.com>, "Martin K. Petersen"
 <martin.petersen@...cle.com>, Pavel Machek	 <pavel@....cz>,
 linux-leds@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org, 
	linux-scsi@...r.kernel.org, Ian Pilcher <arequipeno@...il.com>
Subject: Re: [PATCH RFC 0/4] leds: extend disk trigger

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

Download attachment "signature.asc" of type "application/pgp-signature" (871 bytes)

Powered by blists - more mailing lists