On Mon, Nov 16, 2020 at 04:25:57PM -0600, Stuart Hayes wrote: > > > On 11/13/20 5:19 PM, Greg Kroah-Hartman wrote: > > On Fri, Nov 13, 2020 at 03:38:42PM -0600, Bjorn Helgaas wrote: > >> [+cc Christoph, Dan, Greg (for "one value per file" question below)] > >> > >> On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote: > >>> This patch will expose the PCIe SSD Status LED Management interface in > >>> sysfs for devices that have the relevant _DSM method, per the "_DSM > >>> additions for PCIe SSD Status LED Management" ECN to the PCI Firmware > >>> Specification revision 3.2. > >>> > >>> The interface is exposed in two sysfs files, ssdleds_supported_states (RO) > >>> and ssdleds_current_state (RW). > >>> > >>> Signed-off-by: Stuart Hayes <stuart.w.hayes@xxxxxxxxx> > >>> --- > >>> > >>> v2: made PCI_SSDLEDS dependent on PCI and ACPI > >>> remove unused variable > >>> warn if call to sysfs_create_group fails > >>> include header file with function prototypes > >>> change logical OR to bitwise > >>> > >>> --- > >>> Documentation/ABI/testing/sysfs-bus-pci | 14 ++ > >>> drivers/pci/Kconfig | 7 + > >>> drivers/pci/Makefile | 1 + > >>> drivers/pci/pci-ssdleds.c | 194 ++++++++++++++++++++++++ > >>> drivers/pci/pci-sysfs.c | 1 + > >>> drivers/pci/pci.h | 11 ++ > >>> 6 files changed, 228 insertions(+) > >>> create mode 100644 drivers/pci/pci-ssdleds.c > >>> > >>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci > >>> index 77ad9ec3c801..18ca73b764ac 100644 > >>> --- a/Documentation/ABI/testing/sysfs-bus-pci > >>> +++ b/Documentation/ABI/testing/sysfs-bus-pci > >>> @@ -366,3 +366,17 @@ Contact: Heiner Kallweit <hkallweit1@xxxxxxxxx> > >>> Description: If ASPM is supported for an endpoint, these files can be > >>> used to disable or enable the individual power management > >>> states. Write y/1/on to enable, n/0/off to disable. > >>> + > >>> +What: /sys/bus/pci/devices/.../ssdleds_supported_states > >>> +Date: October 2020 > >>> +Contact: Stuart Hayes <stuart.w.hayes@xxxxxxxxx> > >>> +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@xxxxxxxxx> > >>> +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. > >>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig > >>> index 0c473d75e625..48049866145f 100644 > >>> --- a/drivers/pci/Kconfig > >>> +++ b/drivers/pci/Kconfig > >>> @@ -182,6 +182,13 @@ config PCI_LABEL > >>> def_bool y if (DMI || ACPI) > >>> select NLS > >>> > >>> +config PCI_SSDLEDS > >>> + def_bool y if (ACPI && PCI) > > > > That only should be set if the machine can not boot without it. > > > > For a blinky light, that's not the case. > > > > 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. > > > > 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. On the system I have, changing the active state(s) sends a > message via IPMI to an embedded processor, which will decide the colors > and/or flashing pattern of the LEDs. So brightness/color/blinking are not > controlled, or even known, by the OS. Then I STRONGLY suggest you change the name of all of this code then, as it is totally confusing. > Also, not all of the states will necessarily always be available. For > example, you may only be able to set the "rebuild" state when a drive is > actually connected to the pci port that has this _DSM, while the "locate" > state might be available all the time. Since there's no notification > if/when the supported states change, I believe that would mean, to implement > this using the LED subsystem, the driver would have to register an "LED" > with the LED subsystem for each possible state, and either make reads or > writes to that state fail if it isn't supported. > > But I will re-write this using the LED subsystem if you think it's a better > fit (though I don't think so). 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. thanks, greg k-h
