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

[Randy Dunlap <rdunlap@infradead.org>]

On 6/1/21 2:20 PM, Randy Dunlap wrote:
> Hi,
> 
> On 6/1/21 1:38 PM, Stuart Hayes wrote:
>> This patch adds support for the PCIe SSD Status LED Management
>> interface, as described in the "_DSM Additions for PCIe SSD Status LED
>> Management" ECN to the PCI Firmware Specification revision 3.2.
>>
>> It will add a single (led_classdev) LED for any PCIe device that has the
>> relevant _DSM--presumably only drives or drive slots will have this. The
>> available and active status states are exposed using attribute "states"
>> under the LED device. Reading this attribute will show the states supported
>> by the interface, and those states which are currently active are shown
>> in brackets, like this:
>>
>>  # 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 invalid disabled
>>
>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
>> ---
>> V2:
>> 	* Simplified interface to a single "states" attribute under the LED
>> 	  classdev using only state names
>> 	* Reworked driver to separate _DSM specific code, so support for
>> 	  NPEM (or other methods) could be easily be added
>> 	* Use BIT macro
>>
>>  .../sysfs-class-led-driver-pcie-ssd-leds      |  18 +
>>  drivers/pci/Kconfig                           |  12 +
>>  drivers/pci/Makefile                          |   1 +
>>  drivers/pci/pcie-ssd-leds.c                   | 457 ++++++++++++++++++
>>  4 files changed, 488 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
>>  create mode 100644 drivers/pci/pcie-ssd-leds.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds b/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
>> new file mode 100644
>> index 000000000000..1f07733b6f35
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
>> @@ -0,0 +1,18 @@
>> +What:		/sys/class/leds/<led>/states
>> +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 invalid disabled
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index 0c473d75e625..f4acf1ad0fb5 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -190,6 +190,18 @@ config PCI_HYPERV
>>  	  The PCI device frontend driver allows the kernel to import arbitrary
>>  	  PCI devices from a PCI backend to support PCI driver domains.
>>  
>> +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?
> 

ERROR: modpost: "led_classdev_register_ext" [drivers/pci/pcie-ssd-leds.ko] undefined!
ERROR: modpost: "led_classdev_unregister" [drivers/pci/pcie-ssd-leds.ko] undefined!

Yes, just change it to depends on LEDS_CLASS instead.

> 
> [adding Pavel and linux-leds m.l. for other review]
> 
>> +	help
>> +	  Driver for PCIe SSD status LED management as described in a PCI
>> +	  Firmware Specification, Revision 3.2 ECN.
>> +
>> +	  When enabled, an LED interface will be created for each PCIe device
>> +	  that has the ACPI method described in the referenced specification,
>> +	  to allow the device status LEDs for that PCIe device (presumably a
>> +	  solid state storage device or its slot) to be seen and controlled.
>> +
>>  choice
>>  	prompt "PCI Express hierarchy optimization setting"
>>  	default PCIE_BUS_DEFAULT
> 
> thanks.
> 


-- 
~Randy