Re: [PATCH v2] Add support for PCIe SSD status LED management

[stuart hayes <stuart.w.hayes@gmail.com>]

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@vger.kernel.org
>>> +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!