Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs

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

On 11/11/2020 1:05 AM, Christoph Hellwig wrote:
 > On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote:
 >> +Date:              October 2020
 >> +Contact:   Stuart Hayes <stuart.w.hayes@gmail.com>
 >> +Description:       If the device supports the ACPI _DSM method to
control the
 >> +           PCIe SSD LED states, ssdleds_supported_states (read only)
 >> +           will show the LED states that are supported by the _DSM.
 >> +
 >> +What:              /sys/bus/pci/devices/.../ssdleds_current_state
 >> +Date:              October 2020
 >> +Contact:   Stuart Hayes <stuart.w.hayes@gmail.com>
 >> +Description:       If the device supports the ACPI _DSM method to
control the
 >> +           PCIe SSD LED states, ssdleds_current_state will show or set
 >> +           the current LED states that are active.
 >
 > Is the supported file really required?  Doesn't the current_state one
 > also show which LEDs exist?
 >

The current_state just shows which LED states are currently active, not
which
are supported by the system.  I guess you could try to set all the states in
current_state and read it back to see which ones went on to see which states
are supported, but I'm not 100% if that would work, plus it might result in
the drive LEDs flashing in a strange way briefly.

 >> +config PCI_SSDLEDS
 >> +   def_bool y if (ACPI && PCI)
 >> +   depends on PCI && ACPI
 >
 > We really should not default new code to y.
 >

Good point, I agree.

 >> +   if (dsm_output->status != 0 &&
 >> +       !(dsm_output->status == 4 && dsm_output->function_specific_err
== 1)) {
 >
 > overly longline.  But to make this a little more obvious, maybe you
 > want to
 >
 >   a) switch on dsm_output->status
 >   b) add symbolic names for the magic numbers
 >

Good feedback, thanks.