This is a significant rewrite of the patchset that I originally posted
back on 28 July. It incorporates almost all of the suggestions that I
received as feedback to my original patchset (largely because I was
finally able to wrap my head around how complex LED triggers work).
One thing that has not changed is that associations between block
devices and LEDs are still set via an attribute on the device, rather
than the LED. This is much simpler, as the device attribute only has
to handle a single value (the name of the associated LED), rather than
potentially handling multiple device names.
More importantly, it avoids the need to iterate through all of the
block devices on the system, searching by name. This was proposed
fairly recently, and the reaction was not positive.[1][2]
I have modeled the interface for the /sys/block/<DEVICE>/led
attribute on the sysfs interface used for selecting a trigger. All
available LEDs (all LEDs associated with the blkdev trigger) are
shown when the attribute is read, with the currently selected LED
enclosed in square brackets ([]).
As before, this is all very new to me (particularly the RCU stuff), so
I welcome feedback.
Thanks!
Changes from the original patchset:
* Use a single complex LED trigger ("blkdev"), rather than multiple
user-defined triggers.
* Configurable blink time and interval for each LED:
/sys/class/leds/<LED>/blink_{on,off}
* Associated block devices linked from LED subdirectory:
/sys/class/leds/<LED>/block_devices
(Avoids violating the "one value per entry" sysfs rule.)
* Device-LED associations set via /sys/block/<DEVICE>/led
* Document all sysfs attributes in Documentation/ABI/testing (but also kept
the overview doc)
* Removed unnecessary [un]likely macros
* Reduced number of "user error" log messages and changed level to INFO
* More detailed commit messages
* Add Kconfig option in final commit
* Use RCU-protected pointer (rather than mutex)
* No in-kernel APIs for now
[1] https://www.spinics.net/lists/linux-leds/msg18256.html
[2] https://www.spinics.net/lists/linux-leds/msg18261.html
Ian Pilcher (10):
docs: Add block device LED trigger documentation
block: Add file (blk-ledtrig.c) for block device LED trigger
implementation
block: Add block device LED trigger fields to gendisk structure
block: Add functions to set & clear block device LEDs
block: Add block device sysfs attribute to set/clear/show LED
block: Add activate and deactivate functions for block device LED
trigger
block: Add sysfs attributes to LEDs associated with blkdev trigger
block: Add init function for block device LED trigger
block: Blink device LED (if any) when request is sent to its driver
block: Add config option to enable block device LED triggers
Documentation/ABI/testing/sysfs-block | 16 +
.../testing/sysfs-class-led-trigger-blkdev | 28 ++
Documentation/block/blk-ledtrig.rst | 79 +++
Documentation/block/index.rst | 1 +
block/Kconfig | 8 +
block/Makefile | 1 +
block/blk-ledtrig.c | 469 ++++++++++++++++++
block/blk-ledtrig.h | 45 ++
block/blk-mq.c | 2 +
block/genhd.c | 11 +
include/linux/genhd.h | 4 +
11 files changed, 664 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev
create mode 100644 Documentation/block/blk-ledtrig.rst
create mode 100644 block/blk-ledtrig.c
create mode 100644 block/blk-ledtrig.h
--
2.31.1
Hello Ian,
thank you for your proposal. Some comments below:
On Sun, 8 Aug 2021 22:32:07 -0500
Ian Pilcher <[email protected]> wrote:
> One thing that has not changed is that associations between block
> devices and LEDs are still set via an attribute on the device, rather
> than the LED. This is much simpler, as the device attribute only has
> to handle a single value (the name of the associated LED), rather than
> potentially handling multiple device names.
It may be simpler, but it is in contrast to how the netdev trigger
works, which already is in upstream for many years. I really think we
should try to have similar sysfs ABIs here. (I understand that the
netdev trigger is currently unable to handle multiple network
interfaces - but it is possible to extend it so.)
> I have modeled the interface for the /sys/block/<DEVICE>/led
> attribute on the sysfs interface used for selecting a trigger. All
> available LEDs (all LEDs associated with the blkdev trigger) are
> shown when the attribute is read, with the currently selected LED
> enclosed in square brackets ([]).
I think it is reasonable to be able to set something like this:
led0 : blink on activity on any of [sda, sdb, sdc]
led1 : blink on activity on sda
led2 : blink on activity on sdb
led3 : blink on activity on sdc
If I am reading your code correctly, it looks that only one LED can be
configured for a block device. Is this true? If so, then the above
configuration cannot be set.
Also you are blinking the LED on any request to the block device. I
would rather expect to be able to set the LED to blink on read and on
write. (And possibly on other functions, like discard, or critical
temperature, or error, ...) I would like to know what other people
think about this.
Marek
On Monday 09 August 2021 20:56:33 Marek Behún wrote:
> Hello Ian,
>
> thank you for your proposal. Some comments below:
>
> On Sun, 8 Aug 2021 22:32:07 -0500
> Ian Pilcher <[email protected]> wrote:
>
> > One thing that has not changed is that associations between block
> > devices and LEDs are still set via an attribute on the device, rather
> > than the LED. This is much simpler, as the device attribute only has
> > to handle a single value (the name of the associated LED), rather than
> > potentially handling multiple device names.
>
> It may be simpler, but it is in contrast to how the netdev trigger
> works, which already is in upstream for many years. I really think we
> should try to have similar sysfs ABIs here. (I understand that the
> netdev trigger is currently unable to handle multiple network
> interfaces - but it is possible to extend it so.)
>
> > I have modeled the interface for the /sys/block/<DEVICE>/led
> > attribute on the sysfs interface used for selecting a trigger. All
> > available LEDs (all LEDs associated with the blkdev trigger) are
> > shown when the attribute is read, with the currently selected LED
> > enclosed in square brackets ([]).
>
> I think it is reasonable to be able to set something like this:
> led0 : blink on activity on any of [sda, sdb, sdc]
> led1 : blink on activity on sda
> led2 : blink on activity on sdb
> led3 : blink on activity on sdc
>
> If I am reading your code correctly, it looks that only one LED can be
> configured for a block device. Is this true? If so, then the above
> configuration cannot be set.
>
> Also you are blinking the LED on any request to the block device. I
> would rather expect to be able to set the LED to blink on read and on
> write. (And possibly on other functions, like discard, or critical
> temperature, or error, ...) I would like to know what other people
> think about this.
Hello!
HP EliteBook laptops had dedicated LED for some kind of error and
encryption indication. And there is kernel acpi/wmi driver which can
control this LED. I do not know if recent HP laptops still have these
LEDs, but I would suggest to design API in a way that would allow to use
these dedicated LEDs for their original "vendor" purpose.
I'm mentioning it just because this functionality and design is already
on existing production mainstream laptops, and not something imaginary.
If Linux distributions are still cooperating with laptop vendors and
doing "official" Linux preloads then they may be interested in having
"native" LED functionality support in kernel.
Marek -
Thanks for taking the time to look at this.
On 8/9/21 1:56 PM, Marek Behún wrote:
> It may be simpler, but it is in contrast to how the netdev trigger
> works, which already is in upstream for many years. I really think we
> should try to have similar sysfs ABIs here. (I understand that the
> netdev trigger is currently unable to handle multiple network
> interfaces - but it is possible to extend it so.)
I'm not unalterably opposed to the idea, but I don't currently see a way
to do that without resolving block devices (struct gendisk) by name, and
that seems to be a no-no.
If you (or anyone else) has a suggestion on how to get around this
obstacle, I'd be willing to give it a shot.
> I think it is reasonable to be able to set something like this:
> led0 : blink on activity on any of [sda, sdb, sdc]
> led1 : blink on activity on sda
> led2 : blink on activity on sdb
> led3 : blink on activity on sdc
>
> If I am reading your code correctly, it looks that only one LED can be
> configured for a block device. Is this true? If so, then the above
> configuration cannot be set.
You're correct that it's not possible with the current code. Multiple
devices can be associated to with a single LED, but there's not
currently a way to drive more than 1 LED from a single device. This
is something that could be changed.
> Also you are blinking the LED on any request to the block device. I
> would rather expect to be able to set the LED to blink on read and on
> write. (And possibly on other functions, like discard, or critical
> temperature, or error, ...) I would like to know what other people
> think about this.
I wanted to keep things as simple as possible for now. I don't think
that there's any particular reason that separated LEDs couldn't be
configured for read and write requests. (It looks like it should be
pretty easy to distinguish reads vs writes in a struct request.)
My feeling is that things like temperature, errors, etc. are better
monitored from user space, as they tend to require actively querying
the drive.
Like you, I'm interested in knowing if there is actually hardware out
there that has separate read/write LEDs.
All in all, I feel like I should be able to implement almost everything
that you've suggested, *if* I can figure out the block device lookup
issue, but I really don't have any ideas on that.
Thanks for your patience and feedback!
--
========================================================================
In Soviet Russia, Google searches you!
========================================================================
On Mon, 9 Aug 2021 14:54:26 -0500
Ian Pilcher <[email protected]> wrote:
> I'm not unalterably opposed to the idea, but I don't currently see a way
> to do that without resolving block devices (struct gendisk) by name, and
> that seems to be a no-no.
So a name like "sda1" is not viable? Why? What about "MAJOR:MINOR"?
I confess that I am not very familiar with internal blkdev API.
Quick look reveals that there is a struct block_device, containing a
member bd_disk, which is a pointer to struct gendisk.
What is the relationship between these two? Can there be a block device
without a gendisk, for example?
Marek
On 8/9/21 5:43 PM, Marek Behún wrote:
> I confess that I am not very familiar with internal blkdev API.
It's mainly a matter of symbol visibility. See this thread from a few
months ago:
https://www.spinics.net/lists/linux-leds/msg18244.html
Now ... my code currently lives in block/, so there isn't actually
anything technically preventing it from iterating through the block
devices.
The reactions to Enzo's patch (which you can see in that thread) make me
think that anything that iterates through all block devices is likely to
be rejected, but maybe I'm reading too much into it.
Greg / Christoph -
(As you were the people who expressed disapproval of Enzo's patch to
export block_class and disk_type ...)
Can you weigh in on the acceptability of iterating through the block
devices (searching by name) from LED trigger code within the block
subsystem (i.e. no new symbols would need to be exported)?
This would allow the trigger to implement the sysfs API that Marek and
Pavel want.
Thanks!
--
========================================================================
In Soviet Russia, Google searches you!
========================================================================
On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
> On 8/9/21 5:43 PM, Marek Beh?n wrote:
> > I confess that I am not very familiar with internal blkdev API.
>
> It's mainly a matter of symbol visibility. See this thread from a few
> months ago:
>
> https://www.spinics.net/lists/linux-leds/msg18244.html
>
> Now ... my code currently lives in block/, so there isn't actually
> anything technically preventing it from iterating through the block
> devices.
>
> The reactions to Enzo's patch (which you can see in that thread) make me
> think that anything that iterates through all block devices is likely to
> be rejected, but maybe I'm reading too much into it.
>
>
> Greg / Christoph -
>
> (As you were the people who expressed disapproval of Enzo's patch to
> export block_class and disk_type ...)
>
> Can you weigh in on the acceptability of iterating through the block
> devices (searching by name) from LED trigger code within the block
> subsystem (i.e. no new symbols would need to be exported)?
>
> This would allow the trigger to implement the sysfs API that Marek and
> Pavel want.
No idea, let's see the change first, we can never promise anything :)
thanks,
greg k-h
On Tue, 10 Aug 2021 08:35:08 +0200
Greg KH <[email protected]> wrote:
> On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
> > On 8/9/21 5:43 PM, Marek Behún wrote:
> > > I confess that I am not very familiar with internal blkdev API.
> >
> > It's mainly a matter of symbol visibility. See this thread from a few
> > months ago:
> >
> > https://www.spinics.net/lists/linux-leds/msg18244.html
> >
> > Now ... my code currently lives in block/, so there isn't actually
> > anything technically preventing it from iterating through the block
> > devices.
> >
> > The reactions to Enzo's patch (which you can see in that thread) make me
> > think that anything that iterates through all block devices is likely to
> > be rejected, but maybe I'm reading too much into it.
> >
> >
> > Greg / Christoph -
> >
> > (As you were the people who expressed disapproval of Enzo's patch to
> > export block_class and disk_type ...)
> >
> > Can you weigh in on the acceptability of iterating through the block
> > devices (searching by name) from LED trigger code within the block
> > subsystem (i.e. no new symbols would need to be exported)?
> >
> > This would allow the trigger to implement the sysfs API that Marek and
> > Pavel want.
>
> No idea, let's see the change first, we can never promise anything :)
Hi Greg,
Can't we use blkdev_get_by_path() (or blk_lookup_devt() with
blkdev_get_by_dev())?
This would open the block device and return a struct block_device *.
When the LED trigger is disabled, it would also have to release the
device.
Marek
On Tue, Aug 10, 2021 at 03:38:40PM +0200, Marek Beh?n wrote:
> On Tue, 10 Aug 2021 08:35:08 +0200
> Greg KH <[email protected]> wrote:
>
> > On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
> > > On 8/9/21 5:43 PM, Marek Beh?n wrote:
> > > > I confess that I am not very familiar with internal blkdev API.
> > >
> > > It's mainly a matter of symbol visibility. See this thread from a few
> > > months ago:
> > >
> > > https://www.spinics.net/lists/linux-leds/msg18244.html
> > >
> > > Now ... my code currently lives in block/, so there isn't actually
> > > anything technically preventing it from iterating through the block
> > > devices.
> > >
> > > The reactions to Enzo's patch (which you can see in that thread) make me
> > > think that anything that iterates through all block devices is likely to
> > > be rejected, but maybe I'm reading too much into it.
> > >
> > >
> > > Greg / Christoph -
> > >
> > > (As you were the people who expressed disapproval of Enzo's patch to
> > > export block_class and disk_type ...)
> > >
> > > Can you weigh in on the acceptability of iterating through the block
> > > devices (searching by name) from LED trigger code within the block
> > > subsystem (i.e. no new symbols would need to be exported)?
> > >
> > > This would allow the trigger to implement the sysfs API that Marek and
> > > Pavel want.
> >
> > No idea, let's see the change first, we can never promise anything :)
>
> Hi Greg,
>
> Can't we use blkdev_get_by_path() (or blk_lookup_devt() with
> blkdev_get_by_dev())?
> This would open the block device and return a struct block_device *.
> When the LED trigger is disabled, it would also have to release the
> device.
But what about when the device is removed from the system first? Be
careful about that...
Anyway, sure, try those functions, I really do not know, all I
originally complained about was those exports which did not need to be
exported.
thanks,
greg k-h
On 8/10/21 9:48 AM, Greg KH wrote:
> But what about when the device is removed from the system first? Be
> careful about that...
>
> Anyway, sure, try those functions, I really do not know, all I
> originally complained about was those exports which did not need to be
> exported.
Sounds good. I'll work something up. (I'm actually thinking that
class_find_device() may be the best way to go, as it grabs a reference
to the device.)
--
========================================================================
In Soviet Russia, Google searches you!
========================================================================
On 8/10/21 11:24 AM, Greg KH wrote:
> There should not be anything "odd" about block devices here, just do
> whatever all other LED drivers do when referencing a device.
AFAIK, the only LED trigger that does anything similar is the netdev
trigger. It uses dev_get_by_name(), which is specific to network
devices.
The block subsystem doesn't appear to have any similar API, which is
why Enzo submitted his patch to export block_class and disk_type back
in April[1], when he wanted to do something similar.
I'm basically bypassing the need to export the symbols, because my
trigger code is actually in the block subsystem, rather than the LEDs
subsystem.
[1] https://www.spinics.net/lists/linux-leds/msg18244.html
--
========================================================================
In Soviet Russia, Google searches you!
========================================================================
On Tue, 10 Aug 2021 18:24:12 +0200
Greg KH <[email protected]> wrote:
> > Sounds good. I'll work something up. (I'm actually thinking that
> > class_find_device() may be the best way to go, as it grabs a reference
> > to the device.)
>
> There should not be anything "odd" about block devices here, just do
> whatever all other LED drivers do when referencing a device.
Ian, look at ledtrig-tty and maybe get inspiration from there :)
Marek
On Tue, Aug 10, 2021 at 10:55:33AM -0500, Ian Pilcher wrote:
> On 8/10/21 9:48 AM, Greg KH wrote:
> > But what about when the device is removed from the system first? Be
> > careful about that...
> >
> > Anyway, sure, try those functions, I really do not know, all I
> > originally complained about was those exports which did not need to be
> > exported.
>
> Sounds good. I'll work something up. (I'm actually thinking that
> class_find_device() may be the best way to go, as it grabs a reference
> to the device.)
There should not be anything "odd" about block devices here, just do
whatever all other LED drivers do when referencing a device.
thanks,
greg k-h
On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
> On 8/9/21 5:43 PM, Marek Beh?n wrote:
>> I confess that I am not very familiar with internal blkdev API.
>
> It's mainly a matter of symbol visibility. See this thread from a few
> months ago:
>
> https://www.spinics.net/lists/linux-leds/msg18244.html
>
> Now ... my code currently lives in block/, so there isn't actually
> anything technically preventing it from iterating through the block
> devices.
>
> The reactions to Enzo's patch (which you can see in that thread) make me
> think that anything that iterates through all block devices is likely to
> be rejected, but maybe I'm reading too much into it.
I think the main issue with this series is that it adds a shitload of
code and a hook in the absolute I/O fastpath for fricking blinkenlights.
I don't think it is even worth wasting time on something this ridiculous.
On Wed, 11 Aug 2021 08:26:42 +0200
Christoph Hellwig <[email protected]> wrote:
> On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote:
> > On 8/9/21 5:43 PM, Marek Behún wrote:
> >> I confess that I am not very familiar with internal blkdev API.
> >
> > It's mainly a matter of symbol visibility. See this thread from a few
> > months ago:
> >
> > https://www.spinics.net/lists/linux-leds/msg18244.html
> >
> > Now ... my code currently lives in block/, so there isn't actually
> > anything technically preventing it from iterating through the block
> > devices.
> >
> > The reactions to Enzo's patch (which you can see in that thread) make me
> > think that anything that iterates through all block devices is likely to
> > be rejected, but maybe I'm reading too much into it.
>
> I think the main issue with this series is that it adds a shitload of
> code and a hook in the absolute I/O fastpath for fricking blinkenlights.
> I don't think it is even worth wasting time on something this ridiculous.
That's why I think we should do this the way the netdev trigger does.
Periodically reading block_device's stats, and if they are greater,
blink the LED.
Marek