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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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!



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux