On Mon 2021-05-03 13:56:15, Enzo Matsumiya wrote:

On 05/03, Pavel Machek wrote:

quoted

quoted

As already commented on, this for_each_blk() construct is not a good idea.
Infact, I guess it would be better if you could invert the logic:
Not having the block trigger enumerating all devices, but rather let the
devices register with the block trigger.
That would have the benefit that one could choose which block device should
be handled by the LED trigger subsystem, _and_ you would avoid the need for
a for_each_blk() construct.
Thing is, I don't think that all block devices should be handled by the LED
trigger; eg for things like 'loop' or 'ramdisk' it is very

quoted

questionable.

quoted

Downside is that you would need to modify the drivers, but realistically
there are only very few drivers which should be modified; I would go for
nvme-pci and the sd driver for starters. Maybe floppy, but arguably that can
omitted as one has a very good audio indicator for floppy accesses
:-)

And we already have disk activity trigger. Maybe NVMe and SD needs to
be modified to use it?

TBH I haven't thought of that. My initial idea was to actually offer
maximum flexibility to the user, so exposing all block devices on the
system [*], being able to set any LED available as an indicator for each
of those.

But, indeed, just using ledtrig-disk in NVMe and SD might just be
simpler.


[*] - again, I see now this was a bad idea and will be changed in a
possible next version

Sounds like there should be no new version. Modify NVMe/SD if
required, instead.

Oh and disk-activity LED trigger blinks when disk is fully loaded. I
believe that is a bug and I'd not mind if it was fixed. I probably
have local patch that needs cleaning up somewhere.

Best regards,
								Pavel

--