On 6/1/2021 5:38 PM, Pavel Machek wrote:
Hi!
# echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
# cat /sys/class/leds/0000:88:00.0::drive_status/states
This has two problems: ":" already has special meaning in LED name,
and we'd prefer not to have new "states" interface unless absolutely
neccessary.
Regarding the ":"s in the LED name: I didn't specify the name
"0000:88:00.0"--that's just the name of the parent device of the
led_classdev, so it used that by default. I'm not really sure what to
do here. I could check the device name and replace the ":" with
something else like "_". Would it break anything to have extra ":"
characters in the name? If so, maybe led_classdev_register() should
check for ":" in the name and rename it somehow, like it does when it
finds duplicate names?
As for the "states" interface: I don't disagree that it isn't ideal to
have a new "states" interface, but I have struggled to come up with
anything better. I guess the only alternative to having a new attribute
or attributes would be to create up to ten LED classdev devices--one for
each state supported by the device. I did seriously consider this, and
even started to do it that way, but I decided not to for several
reasons: (1) it would clutter /sys/class/leds, (2) it would require
reads and writes to multiple files to change the states, (3) it would be
more cumbersome for the driver, and (4) it would be a bit slower,
because, if, say, ledmon wanted to set certain states, it would probably
just write to the brightness attribute of each of the LEDs, which would
cause the _DSM would get run for each write, instead of just once. And
the _DSM runs IPMI commands, at least on my system, so it is slow...
But, I'm definitely OK rewriting this to register one led_classdev for
each possible state if that's the better way to do this.
[ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
So what does this do? Turns the LED on if driver is in "ok" or
"locate" states?
This would cause the system to somehow show the user that that this
drive (or drive slot) is the one that it wants a person to be able to
physically locate (possibly by flashing a blue LED on/near the drive),
and also that the drive is OK. It would presumably do that by lighting
the LEDs on/near the drive with certain colors and/or flashing patterns,
though it could, in theory, put "OK" in an LCD on the drive slot. How
the states are displayed to the user is beyond the scope of the specs.
(The _DSM and NPEM specs provide for a way to control status LEDs on a
drive or drive slot. Typically drives will have 2 or 3 LEDs that are
illuminated in different colors or flashing patterns to indicate various
states (like those listed in IBPI / SFF-8489), though the _DSM / NPEM
specs do not specify how the states are displayed.)
+Date: April 2021
+Contact: linux-pci@xxxxxxxxxxxxxxx
+Description:
+ This attribute indicates the status states supported by a drive
+ or drive slot's LEDs, as defined in the "_DSM additions for PCIe
+ SSD Status LED Management" ECN to the PCI Firmware Specification
+ Revision 3.2, dated 12 February 2020, and in "Native PCIe
+ Enclosure Management", section 6.29 of the PCIe Base Spec 5.0.
+
+ Only supported states will be shown, and the currently active
+ states are shown in brackets. The active state(s) can be written
+ by echoing a space or comma separated string of states to this
+ attribute. For example:
+
+ # echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
+ # cat /sys/class/leds/0000:88:00.0::drive_status/states
+ [ok] [locate] failed rebuild pfa hotspare ica ifa
This goes against "one value per file", really needs better
description, and probably needs different interface.
This is very similar to how the available I/O schedulers and currently
selected I/O scheduler is displayed in (for example)
/sys/block/sda/queue/scheduler. The only difference is that more than
one state can be active on the LEDs, while only a single I/O scheduler
can be selected at a time.
Both Bjorn Helgaas and Krzysztof Wilczyński had suggested the
scheduler-type interface, so I went with that. In an earlier attempt at
this driver, when Bjorn suggested this, he asked if that would violate
the "one value per file" rule, and Greg K-H responded "That's a valid
way of displaying options for a sysfs file that can be specific unique
values."
(I'm not devoted to this interface, but I can't think of anything
better. I initially had this as simply a number, with the states
defined in the _DSM/NPEM specs, but Bjorn suggested that was a pain to
interpret, especially because the PCI specs aren't public. My second
try, I used a really verbose output copied from acpi's
"debug_layer"--that still used numbers, but defined them... I didn't
much like it, nor did several others who responded, which is why I went
with this interface.)
+config PCIE_SSD_LEDS
+ tristate "PCIe SSD status LED support"
+ depends on ACPI && NEW_LEDS
I expect that should be LEDS_CLASS instead of NEW_LEDS.
Did you test it with NEW_LEDS=y and LEDS_CLASS not set?
[adding Pavel and linux-leds m.l. for other review]
Thank you!
Pavel
Thanks!
