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

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

[Randy Dunlap]

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?


[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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

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.

> >  [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?

> > +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.

> > +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
							
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[stuart hayes]

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!

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

Hi!

> >>>  [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.)

Well, LED subsystem expects to know how many LEDs are there and what
the LEDs are, and expects individual control over them.

"2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
of LED subsystem, so this should be independend.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[stuart hayes]

On 6/2/2021 4:05 AM, Pavel Machek wrote:
> Hi!
> 
>>>>>   [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.)
> 
> Well, LED subsystem expects to know how many LEDs are there and what
> the LEDs are, and expects individual control over them.
> 
> "2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
> of LED subsystem, so this should be independend.
> 
> Best regards,
> 								Pavel
> 

Yes... I had originally submitted this without using the LED subsystem, 
and Greg K-H said I had to (see 
https://www.spinics.net/lists/linux-pci/msg102013.html).  Here's the 
relevant bit.

(Greg K-H:)
>> > And why isn't this code using the existing LED subsystem?  Don't create
>> > random new driver-specific sysfs files that do things the existing class
>> > drivers do please.
>> >
>> 
(Me:)
>> I did consider the LED subsystem, but I don't think it is a great match.
>> >> In spite of the name, this _DSM doesn't directly control individual 
LEDs--it
>> sets the state(s) of the PCI port to which an SSD may be connected, so that
>> it may be conveyed to the user... a processor (or at least some logic) will
>> decide how to show which states are active, probably by setting combinations
>> of LEDs to certain colors or blink patterns, or possibly on some other type
>> of display.
(Greg K-H:)
> If you are allowing LEDs to be controlled by the user, then yes, you
> have to use the LED subsystem as you should never try to create a brand
> new driver-specific user/kernel API just for your tiny driver right?
> Please work with the subsystems we have, they are unified for a reason.

So I'm not sure what to do.

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

Hi!

> > > 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.)
> > 
> > Well, LED subsystem expects to know how many LEDs are there and what
> > the LEDs are, and expects individual control over them.
> > 
> > "2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
> > of LED subsystem, so this should be independend.

> 
> Yes... I had originally submitted this without using the LED subsystem, and
> Greg K-H said I had to (see
> https://www.spinics.net/lists/linux-pci/msg102013.html).  Here's the
> relevant bit.

...

> So I'm not sure what to do.

Explain to Greg that you don't know how particular system implements
the indication. It can be small display, 2 LEDs, 3 LEDs, etc... LED
subsystem expects direct LED control.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[stuart hayes]

On 6/2/2021 5:40 PM, Pavel Machek wrote:
> Hi!
> 
>>>> 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.)
>>>
>>> Well, LED subsystem expects to know how many LEDs are there and what
>>> the LEDs are, and expects individual control over them.
>>>
>>> "2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
>>> of LED subsystem, so this should be independend.
> 
>>
>> Yes... I had originally submitted this without using the LED subsystem, and
>> Greg K-H said I had to (see
>> https://www.spinics.net/lists/linux-pci/msg102013.html).  Here's the
>> relevant bit.
> 
> ...
> 
>> So I'm not sure what to do.
> 
> Explain to Greg that you don't know how particular system implements
> the indication. It can be small display, 2 LEDs, 3 LEDs, etc... LED
> subsystem expects direct LED control.
> 
> Best regards,
> 								Pavel
> 

Is there any chance you would accept drive status LEDs using the LED 
subsystem, if the driver was rewritten to represent each possible drive 
state as a single led_classdev device, each with no special attributes, 
just brightness of 0 or 1?

The drive states are logically each displaying one bit of information to 
the user, and each could actually be physically represented by a 
physical LED (though probably not).  I only combined them into a single 
led_classdev device with an extra attribute because I thought it made 
more sense to do so, but it sounds like I might have made a bad choice 
in doing so.

In support of using the LED subsystem for this--there are other LEDs in 
linux which aren't necessarily always represented by single physical 
LEDs, such as "caps lock" for keyboards.  Virtual consoles display 
keyboard indicators as text in a window rather than an LED--and someone 
_could_ make a weird keyboard that displays the 3 bits of 
caps/scroll/number lock info as a decimal number on a display.  My point 
is that it seems to me that the LED subsystem is already being used for 
things that logically display a bit of information to the user, which 
may or may not be represented by a single LED.  (Ethernet port LEDs are 
another possible example--they might combine link present, link speed, 
and activity all on one LED, by using different colors and flashing 
patterns.)

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

[Marek Behún]

On Tue, 1 Jun 2021 22:18:16 -0500
stuart hayes <stuart.w.hayes@gmail.com> wrote:

> 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."

But you are not displaying unique values. Your example is

# 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

so there are 2 values set (ok and locate). Unique means that only one
can be set.

Question: can this LED be configured by userspace? I mean: can you
configure whether the LED should be on/off, disregarding the SSD state?

I ask because the LED subsystem currently officially does not
support LEDs for which brightness cannot be set by userspace...

If yes, you should implement the .brightness_set() function. (Could you
please also send your patch to the linux-leds mailing list?)

Then you should implement a LED-private trigger for this LED, which,
when enabled, will make the LED follow the SSD state.

The sysfs ABI should probably look like this:

# cd /sys/class/leds/<SSD_LED>
# echo 1 >brightness		# to light the LED on
# echo 0 >brightness		# to light the LED off
# echo ssd_state >trigger	# to make the LED follow SSD states
# ls ssd_state			# list available SSD states
ok  locate  failed  rebuild ...
# cat ssd_state/ok		# check if "ok" state is enabled
0
# echo 1 >ssd_state/ok		# enable "ok" state so that the
				# LED will be on when SSD is in "ok"
				# state
# echo none >trigger		# put the LED back into SW mode

(The name of the trigger does not necessarily have to be "ssd_state".
 Other people should give their opinions about the name.)

Marek

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

[Randy Dunlap]

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