From: stuart hayes <email@example.com>
To: Pavel Machek <firstname.lastname@example.org>, Randy Dunlap <email@example.com>
Cc: Bjorn Helgaas <firstname.lastname@example.org>,
email@example.com, Keith Busch <firstname.lastname@example.org>,
Subject: Re: [PATCH v2] Add support for PCIe SSD status LED management
Date: Tue, 1 Jun 2021 22:18:16 -0500 [thread overview]
Message-ID: <email@example.com> (raw)
On 6/1/2021 5:38 PM, Pavel Machek wrote:
>>> # 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
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: firstname.lastname@example.org
>>> + 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
(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!
next prev parent reply other threads:[~2021-06-02 3:18 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-01 20:38 [PATCH v2] Add support for PCIe SSD status LED management Stuart Hayes
2021-06-01 21:20 ` Randy Dunlap
2021-06-01 22:38 ` Pavel Machek
2021-06-02 3:18 ` stuart hayes [this message]
2021-06-02 9:05 ` Pavel Machek
2021-06-02 15:36 ` stuart hayes
2021-06-02 22:40 ` Pavel Machek
2021-06-04 20:13 ` stuart hayes
2021-06-02 16:33 ` Marek Behún
2021-06-01 23:39 ` Randy Dunlap
2021-06-01 23:48 ` kernel test robot
2021-06-02 7:05 ` Lukas Wunner
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).