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

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

[Williams, Dan J]

On Fri, 2021-08-13 at 17:36 -0400, 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 (led_classdev) LEDs to each PCIe device that has a supported
> _DSM interface (one off/on LED per supported state). Both PCIe storage
> devices, and the ports to which they are connected, can support this
> interface.

Can you describe why this chose the drivers/leds/led-class.c route
instead of drivers/misc/enclosure.c? Or, something simple / open-coded
like drivers/ata/libata-sata.c? If that was already discussed in a
previous posting just point me over there. My initial take away is that
this is spending effort on gear matching with the led_classdev
interface.

The comments that follow are just an initial pass based on being
suprised about the led_classdev choice and the desire for NPEM support.


> 
> 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
> V3:
>         * Changed code to use a single LED per supported state
>         * Moved to drivers/pci/pcie
>         * Changed Kconfig dependency to LEDS_CLASS instead of NEW_LEDS
>         * Added PCI device class check before _DSM presence check
>         * Other cleanups that don't affect the function
> 
> ---
>  drivers/pci/pcie/Kconfig    |  11 +
>  drivers/pci/pcie/Makefile   |   1 +
>  drivers/pci/pcie/ssd-leds.c | 419 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 431 insertions(+)
>  create mode 100644 drivers/pci/pcie/ssd-leds.c
> 
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index 45a2ef702b45..b738d473209f 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -142,3 +142,14 @@ config PCIE_EDR
>           the PCI Firmware Specification r3.2.  Enable this if you want to
>           support hybrid DPC model which uses both firmware and OS to
>           implement DPC.
> +
> +config PCIE_SSD_LEDS
> +       tristate "PCIe SSD status LED support"
> +       depends on ACPI && LEDS_CLASS

This "depends on ACPI" is awkward when this grows NPEM support. I feel
like NPEM is the first class citizen and then ACPI optionally overrides
NPEM support if present.


> +       help
> +         Driver for PCIe SSD status LED management as described in a PCI
> +         Firmware Specification, Revision 3.2 ECN.

The auxiliary bus [1] was recently added as a way for drivers to carve
off functionality into sub-device / sub-driver pairs. One benefit from
the auxiliary bus organization is that the NPEM driver gets a propoer
alias and auto-loading support. As is this driver appears to need to be
manually loaded.

[1]: Documentation/driver-api/auxiliary_bus.rst

> +
> +         When enabled, LED interfaces will be created for supported drive
> +         states for each PCIe device that has the ACPI _DSM method described
> +         in the referenced specification.
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index b2980db88cc0..fbcb8c2d1dc1 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)                += pme.o
>  obj-$(CONFIG_PCIE_DPC)         += dpc.o
>  obj-$(CONFIG_PCIE_PTM)         += ptm.o
>  obj-$(CONFIG_PCIE_EDR)         += edr.o
> +obj-$(CONFIG_PCIE_SSD_LEDS)    += ssd-leds.o
> diff --git a/drivers/pci/pcie/ssd-leds.c b/drivers/pci/pcie/ssd-leds.c
> new file mode 100644
> index 000000000000..cacb77e5da2d
> --- /dev/null
> +++ b/drivers/pci/pcie/ssd-leds.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Module to provide LED interfaces for PCIe SSD status LED states, 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.
> + *
> + * The "_DSM..." spec is functionally similar to Native PCIe Enclosure
> + * Management, but uses a _DSM ACPI method rather than a PCIe extended
> + * capability.
> + *
> + * Copyright (c) 2021 Dell Inc.
> + *
> + * TODO: Add NPEM support
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <uapi/linux/uleds.h>
> +
> +#define DRIVER_NAME    "pcie-ssd-leds"
> +#define DRIVER_VERSION "v1.0"
> +
> +struct led_state {
> +       char *name;
> +       int bit;
> +};
> +
> +static struct led_state led_states[] = {
> +       { .name = "ok",         .bit = 2 },
> +       { .name = "locate",     .bit = 3 },
> +       { .name = "failed",     .bit = 4 },
> +       { .name = "rebuild",    .bit = 5 },
> +       { .name = "pfa",        .bit = 6 },
> +       { .name = "hotspare",   .bit = 7 },
> +       { .name = "ica",        .bit = 8 },
> +       { .name = "ifa",        .bit = 9 },
> +       { .name = "invalid",    .bit = 10 },
> +       { .name = "disabled",   .bit = 11 },
> +};

include/linux/enclosure.h has common ABI definitions of industry
standard enclosure LED settings. The above looks to be open coding the
same?

> +
> +struct drive_status_led_ops {
> +       int (*get_supported_states)(struct pci_dev *pdev, u32 *states);
> +       int (*get_current_states)(struct pci_dev *pdev, u32 *states);
> +       int (*set_current_states)(struct pci_dev *pdev, u32 states);
> +};
> +
> +struct drive_status_state_led {
> +       struct led_classdev cdev;
> +       struct drive_status_dev *dsdev;
> +       int bit;
> +};
> +
> +/*
> + * drive_status_dev->dev could be the drive itself or its PCIe port
> + */
> +struct drive_status_dev {
> +       struct list_head list;
> +       /* PCI device that has the LED controls */
> +       struct pci_dev *pdev;
> +       /* _DSM (or NPEM) LED ops */
> +       struct drive_status_led_ops *ops;
> +       /* currently active states */
> +       u32 states;
> +       int num_leds;
> +       struct drive_status_state_led leds[];
> +};
> +
> +struct mutex drive_status_dev_list_lock;
> +struct list_head drive_status_dev_list;
> +
> +/*
> + * _DSM LED control
> + */
> +const guid_t pcie_ssd_leds_dsm_guid =
> +       GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b,
> +                 0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d);
> +
> +#define GET_SUPPORTED_STATES_DSM       0x01
> +#define GET_STATE_DSM                  0x02
> +#define SET_STATE_DSM                  0x03
> +
> +struct ssdleds_dsm_output {
> +       u16 status;
> +       u8 function_specific_err;
> +       u8 vendor_specific_err;
> +       u32 state;
> +};
> +
> +static void dsm_status_err_print(struct pci_dev *pdev,
> +                                struct ssdleds_dsm_output *output)
> +{
> +       switch (output->status) {
> +       case 0:
> +               break;
> +       case 1:
> +               pci_dbg(pdev, "_DSM not supported\n");
> +               break;
> +       case 2:
> +               pci_dbg(pdev, "_DSM invalid input parameters\n");
> +               break;
> +       case 3:
> +               pci_dbg(pdev, "_DSM communication error\n");
> +               break;
> +       case 4:
> +               pci_dbg(pdev, "_DSM function-specific error 0x%x\n",
> +                       output->function_specific_err);
> +               break;
> +       case 5:
> +               pci_dbg(pdev, "_DSM vendor-specific error 0x%x\n",
> +                       output->vendor_specific_err);
> +               break;
> +       default:
> +               pci_dbg(pdev, "_DSM returned unknown status 0x%x\n",
> +                       output->status);
> +       }
> +}
> +
> +static int dsm_set(struct pci_dev *pdev, u32 value)
> +{
> +       acpi_handle handle;
> +       union acpi_object *out_obj, arg3[2];
> +       struct ssdleds_dsm_output *dsm_output;
> +
> +       handle = ACPI_HANDLE(&pdev->dev);
> +       if (!handle)
> +               return -ENODEV;
> +
> +       arg3[0].type = ACPI_TYPE_PACKAGE;
> +       arg3[0].package.count = 1;
> +       arg3[0].package.elements = &arg3[1];
> +
> +       arg3[1].type = ACPI_TYPE_BUFFER;
> +       arg3[1].buffer.length = 4;
> +       arg3[1].buffer.pointer = (u8 *)&value;
> +
> +       out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssd_leds_dsm_guid,
> +                               1, SET_STATE_DSM, &arg3[0], ACPI_TYPE_BUFFER);
> +       if (!out_obj)
> +               return -EIO;
> +
> +       if (out_obj->buffer.length < 8) {
> +               ACPI_FREE(out_obj);
> +               return -EIO;
> +       }
> +
> +       dsm_output = (struct ssdleds_dsm_output *)out_obj->buffer.pointer;
> +
> +       if (dsm_output->status != 0) {
> +               dsm_status_err_print(pdev, dsm_output);
> +               ACPI_FREE(out_obj);
> +               return -EIO;
> +       }
> +       ACPI_FREE(out_obj);
> +       return 0;
> +}
> +
> +static int dsm_get(struct pci_dev *pdev, u64 dsm_func, u32 *output)
> +{
> +       acpi_handle handle;
> +       union acpi_object *out_obj;
> +       struct ssdleds_dsm_output *dsm_output;
> +
> +       handle = ACPI_HANDLE(&pdev->dev);
> +       if (!handle)
> +               return -ENODEV;
> +
> +       out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssd_leds_dsm_guid, 0x1,
> +                                         dsm_func, NULL, ACPI_TYPE_BUFFER);
> +       if (!out_obj)
> +               return -EIO;
> +
> +       if (out_obj->buffer.length < 8) {
> +               ACPI_FREE(out_obj);
> +               return -EIO;
> +       }
> +
> +       dsm_output = (struct ssdleds_dsm_output *)out_obj->buffer.pointer;
> +       if (dsm_output->status != 0) {
> +               dsm_status_err_print(pdev, dsm_output);
> +               ACPI_FREE(out_obj);
> +               return -EIO;
> +       }
> +
> +       *output = dsm_output->state;
> +       ACPI_FREE(out_obj);
> +       return 0;
> +}
> +
> +static int get_supported_states_dsm(struct pci_dev *pdev, u32 *states)
> +{
> +       return dsm_get(pdev, GET_SUPPORTED_STATES_DSM, states);
> +}
> +
> +static int get_current_states_dsm(struct pci_dev *pdev, u32 *states)
> +{
> +       return dsm_get(pdev, GET_STATE_DSM, states);
> +}
> +
> +static int set_current_states_dsm(struct pci_dev *pdev, u32 states)
> +{
> +       return dsm_set(pdev, states);
> +}
> +
> +static bool pdev_has_dsm(struct pci_dev *pdev)
> +{
> +       acpi_handle handle;
> +
> +       handle = ACPI_HANDLE(&pdev->dev);
> +       if (!handle)
> +               return false;
> +
> +       return acpi_check_dsm(handle, &pcie_ssd_leds_dsm_guid, 0x1,
> +                             1 << GET_SUPPORTED_STATES_DSM ||
> +                             1 << GET_STATE_DSM ||
> +                             1 << SET_STATE_DSM);
> +}
> +
> +struct drive_status_led_ops dsm_drive_status_led_ops = {
> +       .get_supported_states = get_supported_states_dsm,
> +       .get_current_states = get_current_states_dsm,
> +       .set_current_states = set_current_states_dsm,
> +};
> +
> +/*
> + * code not specific to method (_DSM/NPEM)
> + */
> +
> +static int set_brightness(struct led_classdev *led_cdev,
> +                                      enum led_brightness brightness)
> +{
> +       struct drive_status_state_led *led;
> +       int err;
> +
> +       led = container_of(led_cdev, struct drive_status_state_led, cdev);
> +
> +       if (brightness == LED_OFF)
> +               clear_bit(led->bit, (unsigned long *)&(led->dsdev->states));
> +       else
> +               set_bit(led->bit, (unsigned long *)&(led->dsdev->states));
> +       err = led->dsdev->ops->set_current_states(led->dsdev->pdev,
> +                                                 led->dsdev->states);
> +       if (err < 0)
> +               return err;
> +       return 0;
> +}
> +
> +static enum led_brightness get_brightness(struct led_classdev *led_cdev)
> +{
> +       struct drive_status_state_led *led;
> +
> +       led = container_of(led_cdev, struct drive_status_state_led, cdev);
> +       return test_bit(led->bit, (unsigned long *)&led->dsdev->states)
> +               ? LED_ON : LED_OFF;
> +}
> +
> +static struct drive_status_dev *to_drive_status_dev(struct pci_dev *pdev)
> +{
> +       struct drive_status_dev *dsdev;
> +
> +       mutex_lock(&drive_status_dev_list_lock);
> +       list_for_each_entry(dsdev, &drive_status_dev_list, list)
> +               if (pdev == dsdev->pdev) {
> +                       mutex_unlock(&drive_status_dev_list_lock);
> +                       return dsdev;
> +               }
> +       mutex_unlock(&drive_status_dev_list_lock);
> +       return NULL;
> +}
> +
> +static void remove_drive_status_dev(struct drive_status_dev *dsdev)
> +{
> +       if (dsdev) {
> +               int i;
> +
> +               mutex_lock(&drive_status_dev_list_lock);
> +               list_del(&dsdev->list);
> +               mutex_unlock(&drive_status_dev_list_lock);
> +               for (i = 0; i < dsdev->num_leds; i++)
> +                       led_classdev_unregister(&dsdev->leds[i].cdev);
> +               kfree(dsdev);
> +       }
> +}
> +
> +static void add_drive_status_dev(struct pci_dev *pdev,
> +                                struct drive_status_led_ops *ops)
> +{
> +       u32 supported;
> +       int ret, num_leds, i;
> +       struct drive_status_dev *dsdev;
> +       char name[LED_MAX_NAME_SIZE];
> +       struct drive_status_state_led *led;
> +
> +       if (to_drive_status_dev(pdev))
> +               /*
> +                * leds have already been added for this dev
> +                */
> +               return;
> +
> +       if (ops->get_supported_states(pdev, &supported) < 0)
> +               return;
> +       num_leds = hweight32(supported);
> +       if (num_leds == 0)
> +               return;
> +
> +       dsdev = kzalloc(struct_size(dsdev, leds, num_leds), GFP_KERNEL);
> +       if (!dsdev)
> +               return;
> +
> +       dsdev->num_leds = 0;
> +       dsdev->pdev = pdev;
> +       dsdev->ops = ops;
> +       dsdev->states = 0;
> +       if (ops->set_current_states(pdev, dsdev->states)) {
> +               kfree(dsdev);
> +               return;
> +       }
> +       INIT_LIST_HEAD(&dsdev->list);
> +       /*
> +        * add LEDs only for supported states
> +        */
> +       for (i = 0; i < ARRAY_SIZE(led_states); i++) {
> +               if (!test_bit(led_states[i].bit, (unsigned long *)&supported))
> +                       continue;
> +
> +               led = &dsdev->leds[dsdev->num_leds];
> +               led->dsdev = dsdev;
> +               led->bit = led_states[i].bit;
> +
> +               snprintf(name, sizeof(name), "%s::%s",
> +                        pci_name(pdev), led_states[i].name);
> +               led->cdev.name = name;
> +               led->cdev.max_brightness = LED_ON;
> +               led->cdev.brightness_set_blocking = set_brightness;
> +               led->cdev.brightness_get = get_brightness;
> +               ret = 0;
> +               ret = led_classdev_register(&pdev->dev, &led->cdev);
> +               if (ret) {
> +                       pr_warn("Failed to register LEDs for %s\n", pci_name(pdev));
> +                       remove_drive_status_dev(dsdev);
> +                       return;
> +               }
> +               dsdev->num_leds++;
> +       }
> +
> +       mutex_lock(&drive_status_dev_list_lock);
> +       list_add_tail(&dsdev->list, &drive_status_dev_list);
> +       mutex_unlock(&drive_status_dev_list_lock);
> +}
> +
> +/*
> + * code specific to PCIe devices
> + */
> +static void probe_pdev(struct pci_dev *pdev)
> +{
> +       /*
> +        * This is only supported on PCIe storage devices and PCIe ports
> +        */
> +       if (pdev->class != PCI_CLASS_STORAGE_EXPRESS &&
> +           pdev->class != PCI_CLASS_BRIDGE_PCI)
> +               return;
> +       if (pdev_has_dsm(pdev))
> +               add_drive_status_dev(pdev, &dsm_drive_status_led_ops);
> +}
> +
> +static int ssd_leds_pci_bus_notifier_cb(struct notifier_block *nb,
> +                                          unsigned long action, void *data)
> +{
> +       struct pci_dev *pdev = to_pci_dev(data);
> +
> +       if (action == BUS_NOTIFY_ADD_DEVICE)
> +               probe_pdev(pdev);
> +       else if (action == BUS_NOTIFY_DEL_DEVICE)
> +               remove_drive_status_dev(to_drive_status_dev(pdev));
> +       return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block ssd_leds_pci_bus_nb = {
> +       .notifier_call = ssd_leds_pci_bus_notifier_cb,
> +       .priority = INT_MIN,
> +};
> +
> +static void initial_scan_for_leds(void)
> +{
> +       struct pci_dev *pdev = NULL;
> +
> +       for_each_pci_dev(pdev)
> +               probe_pdev(pdev);


This looks odd to me. Why force enable every occurrence these leds, and
do so indepdendently of the bind state of the driver for the associated
PCI device? I would expect that this support would be a library called
by the NVME driver, or the CXL driver. A library just like the
led_classdev infrastructure.


> +}
> +
> +static int __init ssd_leds_init(void)
> +{
> +       mutex_init(&drive_status_dev_list_lock);
> +       INIT_LIST_HEAD(&drive_status_dev_list);
> +
> +       bus_register_notifier(&pci_bus_type, &ssd_leds_pci_bus_nb);
> +       initial_scan_for_leds();
> +       return 0;
> +}
> +
> +static void __exit ssd_leds_exit(void)
> +{
> +       struct drive_status_dev *dsdev, *temp;
> +
> +       bus_unregister_notifier(&pci_bus_type, &ssd_leds_pci_bus_nb);
> +       list_for_each_entry_safe(dsdev, temp, &drive_status_dev_list, list)
> +               remove_drive_status_dev(dsdev);
> +}
> +
> +module_init(ssd_leds_init);
> +module_exit(ssd_leds_exit);
> +
> +MODULE_AUTHOR("Stuart Hayes <stuart.w.hayes@gmail.com>");
> +MODULE_DESCRIPTION("Support for PCIe SSD Status LEDs");
> +MODULE_LICENSE("GPL");

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

[stuart hayes]

On 10/5/2021 12:14 AM, Williams, Dan J wrote:
> On Fri, 2021-08-13 at 17:36 -0400, 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 (led_classdev) LEDs to each PCIe device that has a supported
>> _DSM interface (one off/on LED per supported state). Both PCIe storage
>> devices, and the ports to which they are connected, can support this
>> interface.
> 
> Can you describe why this chose the drivers/leds/led-class.c route
> instead of drivers/misc/enclosure.c? Or, something simple / open-coded
> like drivers/ata/libata-sata.c? If that was already discussed in a
> previous posting just point me over there. My initial take away is that
> this is spending effort on gear matching with the led_classdev
> interface.
> 
> The comments that follow are just an initial pass based on being
> suprised about the led_classdev choice and the desire for NPEM support.
> 
> 

Thank you for the comments, Dan.

I originally did submit something simple that just added a couple of 
sysfs attributes to allow userspace access to the _DSM, but Greg K-H 
said (1) that I shouldn't create new driver-specific sysfs files that do 
things that existing class drivers do, and that if I'm allowing LEDs to 
be controlled by the user, I have to use the LED subsystem, so I went 
with that. (See the end of 
https://patchwork.ozlabs.org/project/linux-pci/patch/20201110153735.58587-1-stuart.w.hayes@gmail.com/)

I tried to make my code very similar to drivers/input/input-leds.c, 
where up to 11 LEDs per input device are registered.  Since NPEM/_DSM 
allows any of the states to be independently set or cleared, and any of 
the states may or may not be supported, it seemed very similar to me 
(except, as Pavel points out, the NPEM/_DSM states probably won't have 
one LED per state like the keyboard LEDs typically do).

I will look into drivers/misc/enclosure.c.  I think I didn't go that 
route initially as it appeared to be quite old and not widely used, and 
the states don't completely overlap.  More about it below.

>>
>> 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
>> V3:
>>          * Changed code to use a single LED per supported state
>>          * Moved to drivers/pci/pcie
>>          * Changed Kconfig dependency to LEDS_CLASS instead of NEW_LEDS
>>          * Added PCI device class check before _DSM presence check
>>          * Other cleanups that don't affect the function
>>
>> ---
>>   drivers/pci/pcie/Kconfig    |  11 +
>>   drivers/pci/pcie/Makefile   |   1 +
>>   drivers/pci/pcie/ssd-leds.c | 419 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 431 insertions(+)
>>   create mode 100644 drivers/pci/pcie/ssd-leds.c
>>
>> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
>> index 45a2ef702b45..b738d473209f 100644
>> --- a/drivers/pci/pcie/Kconfig
>> +++ b/drivers/pci/pcie/Kconfig
>> @@ -142,3 +142,14 @@ config PCIE_EDR
>>            the PCI Firmware Specification r3.2.  Enable this if you want to
>>            support hybrid DPC model which uses both firmware and OS to
>>            implement DPC.
>> +
>> +config PCIE_SSD_LEDS
>> +       tristate "PCIe SSD status LED support"
>> +       depends on ACPI && LEDS_CLASS
> 
> This "depends on ACPI" is awkward when this grows NPEM support. I feel
> like NPEM is the first class citizen and then ACPI optionally overrides
> NPEM support if present.
> 
> 

I didn't actually think about NPEM when I originally submitted this... 
and even now, as I submitted it, it only uses the _DSM method, so it is 
useless without ACPI.  The ACPI dependency could go if NPEM support was 
added, for sure.

>> +       help
>> +         Driver for PCIe SSD status LED management as described in a PCI
>> +         Firmware Specification, Revision 3.2 ECN.
> 
> The auxiliary bus [1] was recently added as a way for drivers to carve
> off functionality into sub-device / sub-driver pairs. One benefit from
> the auxiliary bus organization is that the NPEM driver gets a propoer
> alias and auto-loading support. As is this driver appears to need to be
> manually loaded.
> 
> [1]: Documentation/driver-api/auxiliary_bus.rst
> 

Yes, unfortunately, it would need to be manually loaded.  I flip-flopped 
multiple times on making this just a library that the nvme driver could 
call to register the LEDs (if the _DSM was present).  It would be 
simpler, it wouldn't need a module to be manulaly loaded, it would 
resulted in better LED names (if the _DSM was on the NVMe PCI device), 
and it would mean that the driver that owns the PCI device would also 
own the LEDs.

The only reason I didn't do that, is that the _DSM can be on the PCIe 
port that the NVMe device is connected to, rather than on the NVMe drive 
PCI device.  And I'm not sure how to deal with the latter situation, 
except maybe change the pcie port bus driver to also call in to the 
library to register LEDs somehow.  But maybe it would be worth trying to 
figure that out.

I was not aware of the auxiliary bus, thanks for pointing it out.  I 
glanced over it, and I'm not sure how it would help over just having a 
library that nvme, pcie port bus driver, CXL, or whatever could call into.


>> +
>> +         When enabled, LED interfaces will be created for supported drive
>> +         states for each PCIe device that has the ACPI _DSM method described
>> +         in the referenced specification.
>> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
>> index b2980db88cc0..fbcb8c2d1dc1 100644
>> --- a/drivers/pci/pcie/Makefile
>> +++ b/drivers/pci/pcie/Makefile
>> @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)                += pme.o
>>   obj-$(CONFIG_PCIE_DPC)         += dpc.o
>>   obj-$(CONFIG_PCIE_PTM)         += ptm.o
>>   obj-$(CONFIG_PCIE_EDR)         += edr.o
>> +obj-$(CONFIG_PCIE_SSD_LEDS)    += ssd-leds.o
>> diff --git a/drivers/pci/pcie/ssd-leds.c b/drivers/pci/pcie/ssd-leds.c
>> new file mode 100644
>> index 000000000000..cacb77e5da2d
>> --- /dev/null
>> +++ b/drivers/pci/pcie/ssd-leds.c
>> @@ -0,0 +1,419 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Module to provide LED interfaces for PCIe SSD status LED states, 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.
>> + *
>> + * The "_DSM..." spec is functionally similar to Native PCIe Enclosure
>> + * Management, but uses a _DSM ACPI method rather than a PCIe extended
>> + * capability.
>> + *
>> + * Copyright (c) 2021 Dell Inc.
>> + *
>> + * TODO: Add NPEM support
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pci.h>
>> +#include <uapi/linux/uleds.h>
>> +
>> +#define DRIVER_NAME    "pcie-ssd-leds"
>> +#define DRIVER_VERSION "v1.0"
>> +
>> +struct led_state {
>> +       char *name;
>> +       int bit;
>> +};
>> +
>> +static struct led_state led_states[] = {
>> +       { .name = "ok",         .bit = 2 },
>> +       { .name = "locate",     .bit = 3 },
>> +       { .name = "failed",     .bit = 4 },
>> +       { .name = "rebuild",    .bit = 5 },
>> +       { .name = "pfa",        .bit = 6 },
>> +       { .name = "hotspare",   .bit = 7 },
>> +       { .name = "ica",        .bit = 8 },
>> +       { .name = "ifa",        .bit = 9 },
>> +       { .name = "invalid",    .bit = 10 },
>> +       { .name = "disabled",   .bit = 11 },
>> +};
> 
> include/linux/enclosure.h has common ABI definitions of industry
> standard enclosure LED settings. The above looks to be open coding the
> same?
>

The LED states in inluce/linux/enclosure.h aren't exactly the same... 
there are states defined in NPEM/_DSM that aren't defined in 
enclosure.h.  In addition, while the enclosure driver allows "locate" to 
be controlled independently, it looks like it will only allow a single 
state (unsupported/ok/critical/etc) to be active at a time, while the 
NPEM/_DSM allow all of the state bits to be independently set or 
cleared.  Maybe only one of those states would need to be set at a time, 
I don't know, but that would impose a limitation on what NPEM/_DSM can 
do.  I'll take a closer look at this as an alternative to using 
drivers/leds/led-class.c.


>> +
>> +struct drive_status_led_ops {
>> +       int (*get_supported_states)(struct pci_dev *pdev, u32 *states);
>> +       int (*get_current_states)(struct pci_dev *pdev, u32 *states);
>> +       int (*set_current_states)(struct pci_dev *pdev, u32 states);
>> +};
>> +
>> +struct drive_status_state_led {
>> +       struct led_classdev cdev;
>> +       struct drive_status_dev *dsdev;
>> +       int bit;
>> +};
>> +
>> +/*
>> + * drive_status_dev->dev could be the drive itself or its PCIe port
>> + */
>> +struct drive_status_dev {
>> +       struct list_head list;
>> +       /* PCI device that has the LED controls */
>> +       struct pci_dev *pdev;
>> +       /* _DSM (or NPEM) LED ops */
>> +       struct drive_status_led_ops *ops;
>> +       /* currently active states */
>> +       u32 states;
>> +       int num_leds;
>> +       struct drive_status_state_led leds[];
>> +};
>> +
>> +struct mutex drive_status_dev_list_lock;
>> +struct list_head drive_status_dev_list;
>> +
>> +/*
>> + * _DSM LED control
>> + */
>> +const guid_t pcie_ssd_leds_dsm_guid =
>> +       GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b,
>> +                 0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d);
>> +
>> +#define GET_SUPPORTED_STATES_DSM       0x01
>> +#define GET_STATE_DSM                  0x02
>> +#define SET_STATE_DSM                  0x03
>> +
>> +struct ssdleds_dsm_output {
>> +       u16 status;
>> +       u8 function_specific_err;
>> +       u8 vendor_specific_err;
>> +       u32 state;
>> +};
>> +
>> +static void dsm_status_err_print(struct pci_dev *pdev,
>> +                                struct ssdleds_dsm_output *output)
>> +{
>> +       switch (output->status) {
>> +       case 0:
>> +               break;
>> +       case 1:
>> +               pci_dbg(pdev, "_DSM not supported\n");
>> +               break;
>> +       case 2:
>> +               pci_dbg(pdev, "_DSM invalid input parameters\n");
>> +               break;
>> +       case 3:
>> +               pci_dbg(pdev, "_DSM communication error\n");
>> +               break;
>> +       case 4:
>> +               pci_dbg(pdev, "_DSM function-specific error 0x%x\n",
>> +                       output->function_specific_err);
>> +               break;
>> +       case 5:
>> +               pci_dbg(pdev, "_DSM vendor-specific error 0x%x\n",
>> +                       output->vendor_specific_err);
>> +               break;
>> +       default:
>> +               pci_dbg(pdev, "_DSM returned unknown status 0x%x\n",
>> +                       output->status);
>> +       }
>> +}
>> +
>> +static int dsm_set(struct pci_dev *pdev, u32 value)
>> +{
>> +       acpi_handle handle;
>> +       union acpi_object *out_obj, arg3[2];
>> +       struct ssdleds_dsm_output *dsm_output;
>> +
>> +       handle = ACPI_HANDLE(&pdev->dev);
>> +       if (!handle)
>> +               return -ENODEV;
>> +
>> +       arg3[0].type = ACPI_TYPE_PACKAGE;
>> +       arg3[0].package.count = 1;
>> +       arg3[0].package.elements = &arg3[1];
>> +
>> +       arg3[1].type = ACPI_TYPE_BUFFER;
>> +       arg3[1].buffer.length = 4;
>> +       arg3[1].buffer.pointer = (u8 *)&value;
>> +
>> +       out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssd_leds_dsm_guid,
>> +                               1, SET_STATE_DSM, &arg3[0], ACPI_TYPE_BUFFER);
>> +       if (!out_obj)
>> +               return -EIO;
>> +
>> +       if (out_obj->buffer.length < 8) {
>> +               ACPI_FREE(out_obj);
>> +               return -EIO;
>> +       }
>> +
>> +       dsm_output = (struct ssdleds_dsm_output *)out_obj->buffer.pointer;
>> +
>> +       if (dsm_output->status != 0) {
>> +               dsm_status_err_print(pdev, dsm_output);
>> +               ACPI_FREE(out_obj);
>> +               return -EIO;
>> +       }
>> +       ACPI_FREE(out_obj);
>> +       return 0;
>> +}
>> +
>> +static int dsm_get(struct pci_dev *pdev, u64 dsm_func, u32 *output)
>> +{
>> +       acpi_handle handle;
>> +       union acpi_object *out_obj;
>> +       struct ssdleds_dsm_output *dsm_output;
>> +
>> +       handle = ACPI_HANDLE(&pdev->dev);
>> +       if (!handle)
>> +               return -ENODEV;
>> +
>> +       out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssd_leds_dsm_guid, 0x1,
>> +                                         dsm_func, NULL, ACPI_TYPE_BUFFER);
>> +       if (!out_obj)
>> +               return -EIO;
>> +
>> +       if (out_obj->buffer.length < 8) {
>> +               ACPI_FREE(out_obj);
>> +               return -EIO;
>> +       }
>> +
>> +       dsm_output = (struct ssdleds_dsm_output *)out_obj->buffer.pointer;
>> +       if (dsm_output->status != 0) {
>> +               dsm_status_err_print(pdev, dsm_output);
>> +               ACPI_FREE(out_obj);
>> +               return -EIO;
>> +       }
>> +
>> +       *output = dsm_output->state;
>> +       ACPI_FREE(out_obj);
>> +       return 0;
>> +}
>> +
>> +static int get_supported_states_dsm(struct pci_dev *pdev, u32 *states)
>> +{
>> +       return dsm_get(pdev, GET_SUPPORTED_STATES_DSM, states);
>> +}
>> +
>> +static int get_current_states_dsm(struct pci_dev *pdev, u32 *states)
>> +{
>> +       return dsm_get(pdev, GET_STATE_DSM, states);
>> +}
>> +
>> +static int set_current_states_dsm(struct pci_dev *pdev, u32 states)
>> +{
>> +       return dsm_set(pdev, states);
>> +}
>> +
>> +static bool pdev_has_dsm(struct pci_dev *pdev)
>> +{
>> +       acpi_handle handle;
>> +
>> +       handle = ACPI_HANDLE(&pdev->dev);
>> +       if (!handle)
>> +               return false;
>> +
>> +       return acpi_check_dsm(handle, &pcie_ssd_leds_dsm_guid, 0x1,
>> +                             1 << GET_SUPPORTED_STATES_DSM ||
>> +                             1 << GET_STATE_DSM ||
>> +                             1 << SET_STATE_DSM);
>> +}
>> +
>> +struct drive_status_led_ops dsm_drive_status_led_ops = {
>> +       .get_supported_states = get_supported_states_dsm,
>> +       .get_current_states = get_current_states_dsm,
>> +       .set_current_states = set_current_states_dsm,
>> +};
>> +
>> +/*
>> + * code not specific to method (_DSM/NPEM)
>> + */
>> +
>> +static int set_brightness(struct led_classdev *led_cdev,
>> +                                      enum led_brightness brightness)
>> +{
>> +       struct drive_status_state_led *led;
>> +       int err;
>> +
>> +       led = container_of(led_cdev, struct drive_status_state_led, cdev);
>> +
>> +       if (brightness == LED_OFF)
>> +               clear_bit(led->bit, (unsigned long *)&(led->dsdev->states));
>> +       else
>> +               set_bit(led->bit, (unsigned long *)&(led->dsdev->states));
>> +       err = led->dsdev->ops->set_current_states(led->dsdev->pdev,
>> +                                                 led->dsdev->states);
>> +       if (err < 0)
>> +               return err;
>> +       return 0;
>> +}
>> +
>> +static enum led_brightness get_brightness(struct led_classdev *led_cdev)
>> +{
>> +       struct drive_status_state_led *led;
>> +
>> +       led = container_of(led_cdev, struct drive_status_state_led, cdev);
>> +       return test_bit(led->bit, (unsigned long *)&led->dsdev->states)
>> +               ? LED_ON : LED_OFF;
>> +}
>> +
>> +static struct drive_status_dev *to_drive_status_dev(struct pci_dev *pdev)
>> +{
>> +       struct drive_status_dev *dsdev;
>> +
>> +       mutex_lock(&drive_status_dev_list_lock);
>> +       list_for_each_entry(dsdev, &drive_status_dev_list, list)
>> +               if (pdev == dsdev->pdev) {
>> +                       mutex_unlock(&drive_status_dev_list_lock);
>> +                       return dsdev;
>> +               }
>> +       mutex_unlock(&drive_status_dev_list_lock);
>> +       return NULL;
>> +}
>> +
>> +static void remove_drive_status_dev(struct drive_status_dev *dsdev)
>> +{
>> +       if (dsdev) {
>> +               int i;
>> +
>> +               mutex_lock(&drive_status_dev_list_lock);
>> +               list_del(&dsdev->list);
>> +               mutex_unlock(&drive_status_dev_list_lock);
>> +               for (i = 0; i < dsdev->num_leds; i++)
>> +                       led_classdev_unregister(&dsdev->leds[i].cdev);
>> +               kfree(dsdev);
>> +       }
>> +}
>> +
>> +static void add_drive_status_dev(struct pci_dev *pdev,
>> +                                struct drive_status_led_ops *ops)
>> +{
>> +       u32 supported;
>> +       int ret, num_leds, i;
>> +       struct drive_status_dev *dsdev;
>> +       char name[LED_MAX_NAME_SIZE];
>> +       struct drive_status_state_led *led;
>> +
>> +       if (to_drive_status_dev(pdev))
>> +               /*
>> +                * leds have already been added for this dev
>> +                */
>> +               return;
>> +
>> +       if (ops->get_supported_states(pdev, &supported) < 0)
>> +               return;
>> +       num_leds = hweight32(supported);
>> +       if (num_leds == 0)
>> +               return;
>> +
>> +       dsdev = kzalloc(struct_size(dsdev, leds, num_leds), GFP_KERNEL);
>> +       if (!dsdev)
>> +               return;
>> +
>> +       dsdev->num_leds = 0;
>> +       dsdev->pdev = pdev;
>> +       dsdev->ops = ops;
>> +       dsdev->states = 0;
>> +       if (ops->set_current_states(pdev, dsdev->states)) {
>> +               kfree(dsdev);
>> +               return;
>> +       }
>> +       INIT_LIST_HEAD(&dsdev->list);
>> +       /*
>> +        * add LEDs only for supported states
>> +        */
>> +       for (i = 0; i < ARRAY_SIZE(led_states); i++) {
>> +               if (!test_bit(led_states[i].bit, (unsigned long *)&supported))
>> +                       continue;
>> +
>> +               led = &dsdev->leds[dsdev->num_leds];
>> +               led->dsdev = dsdev;
>> +               led->bit = led_states[i].bit;
>> +
>> +               snprintf(name, sizeof(name), "%s::%s",
>> +                        pci_name(pdev), led_states[i].name);
>> +               led->cdev.name = name;
>> +               led->cdev.max_brightness = LED_ON;
>> +               led->cdev.brightness_set_blocking = set_brightness;
>> +               led->cdev.brightness_get = get_brightness;
>> +               ret = 0;
>> +               ret = led_classdev_register(&pdev->dev, &led->cdev);
>> +               if (ret) {
>> +                       pr_warn("Failed to register LEDs for %s\n", pci_name(pdev));
>> +                       remove_drive_status_dev(dsdev);
>> +                       return;
>> +               }
>> +               dsdev->num_leds++;
>> +       }
>> +
>> +       mutex_lock(&drive_status_dev_list_lock);
>> +       list_add_tail(&dsdev->list, &drive_status_dev_list);
>> +       mutex_unlock(&drive_status_dev_list_lock);
>> +}
>> +
>> +/*
>> + * code specific to PCIe devices
>> + */
>> +static void probe_pdev(struct pci_dev *pdev)
>> +{
>> +       /*
>> +        * This is only supported on PCIe storage devices and PCIe ports
>> +        */
>> +       if (pdev->class != PCI_CLASS_STORAGE_EXPRESS &&
>> +           pdev->class != PCI_CLASS_BRIDGE_PCI)
>> +               return;
>> +       if (pdev_has_dsm(pdev))
>> +               add_drive_status_dev(pdev, &dsm_drive_status_led_ops);
>> +}
>> +
>> +static int ssd_leds_pci_bus_notifier_cb(struct notifier_block *nb,
>> +                                          unsigned long action, void *data)
>> +{
>> +       struct pci_dev *pdev = to_pci_dev(data);
>> +
>> +       if (action == BUS_NOTIFY_ADD_DEVICE)
>> +               probe_pdev(pdev);
>> +       else if (action == BUS_NOTIFY_DEL_DEVICE)
>> +               remove_drive_status_dev(to_drive_status_dev(pdev));
>> +       return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block ssd_leds_pci_bus_nb = {
>> +       .notifier_call = ssd_leds_pci_bus_notifier_cb,
>> +       .priority = INT_MIN,
>> +};
>> +
>> +static void initial_scan_for_leds(void)
>> +{
>> +       struct pci_dev *pdev = NULL;
>> +
>> +       for_each_pci_dev(pdev)
>> +               probe_pdev(pdev);
> 
> 
> This looks odd to me. Why force enable every occurrence these leds, and
> do so indepdendently of the bind state of the driver for the associated
> PCI device? I would expect that this support would be a library called
> by the NVME driver, or the CXL driver. A library just like the
> led_classdev infrastructure.
> 

I guess I didn't know of any reason why they shouldn't be enabled even 
if the nvme driver wasn't bound, if this driver was stand-alone, since 
the LED function doesn't depend on the nvme driver.  But yes, maybe it 
would be better to make this a library that the nvme driver (or pcie 
port bus driver, or CXL driver) calls to register LEDs.

Thank you for taking a look at this... I'm very glad for any help to get 
this done.

> 
>> +}
>> +
>> +static int __init ssd_leds_init(void)
>> +{
>> +       mutex_init(&drive_status_dev_list_lock);
>> +       INIT_LIST_HEAD(&drive_status_dev_list);
>> +
>> +       bus_register_notifier(&pci_bus_type, &ssd_leds_pci_bus_nb);
>> +       initial_scan_for_leds();
>> +       return 0;
>> +}
>> +
>> +static void __exit ssd_leds_exit(void)
>> +{
>> +       struct drive_status_dev *dsdev, *temp;
>> +
>> +       bus_unregister_notifier(&pci_bus_type, &ssd_leds_pci_bus_nb);
>> +       list_for_each_entry_safe(dsdev, temp, &drive_status_dev_list, list)
>> +               remove_drive_status_dev(dsdev);
>> +}
>> +
>> +module_init(ssd_leds_init);
>> +module_exit(ssd_leds_exit);
>> +
>> +MODULE_AUTHOR("Stuart Hayes <stuart.w.hayes@gmail.com>");
>> +MODULE_DESCRIPTION("Support for PCIe SSD Status LEDs");
>> +MODULE_LICENSE("GPL");
> 

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

[Dan Williams]

[ add Mariusz from ledmon tool: https://github.com/intel/ledmon ]

On Tue, Oct 5, 2021 at 7:43 PM stuart hayes <stuart.w.hayes@gmail.com> wrote:
>
>
> On 10/5/2021 12:14 AM, Williams, Dan J wrote:
> > On Fri, 2021-08-13 at 17:36 -0400, 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 (led_classdev) LEDs to each PCIe device that has a supported
> >> _DSM interface (one off/on LED per supported state). Both PCIe storage
> >> devices, and the ports to which they are connected, can support this
> >> interface.
> >
> > Can you describe why this chose the drivers/leds/led-class.c route
> > instead of drivers/misc/enclosure.c? Or, something simple / open-coded
> > like drivers/ata/libata-sata.c? If that was already discussed in a
> > previous posting just point me over there. My initial take away is that
> > this is spending effort on gear matching with the led_classdev
> > interface.
> >
> > The comments that follow are just an initial pass based on being
> > suprised about the led_classdev choice and the desire for NPEM support.
> >
> >
>
> Thank you for the comments, Dan.
>
> I originally did submit something simple that just added a couple of
> sysfs attributes to allow userspace access to the _DSM, but Greg K-H
> said (1) that I shouldn't create new driver-specific sysfs files that do
> things that existing class drivers do, and that if I'm allowing LEDs to
> be controlled by the user, I have to use the LED subsystem, so I went
> with that. (See the end of
> https://patchwork.ozlabs.org/project/linux-pci/patch/20201110153735.58587-1-stuart.w.hayes@gmail.com/)

I agree with the general sentiment to adopt and extend existing ABIs
wherever possible, it's just not clear to me that the LED class driver
is the best fit. The Enclosure class infrastructure, in addition to
having some concept of industry standard LED signalling patterns, also
has the concept of linking back to the controller of the storage
device in question. So my gut feel is that it's a better starting
point. If it has some gaps or mismatches with NPEM concepts then
that's a good reason to extend it versus trying to describe the
storage use case to the generic LED class driver.

> I tried to make my code very similar to drivers/input/input-leds.c,
> where up to 11 LEDs per input device are registered.  Since NPEM/_DSM
> allows any of the states to be independently set or cleared, and any of
> the states may or may not be supported, it seemed very similar to me
> (except, as Pavel points out, the NPEM/_DSM states probably won't have
> one LED per state like the keyboard LEDs typically do).
>
> I will look into drivers/misc/enclosure.c.  I think I didn't go that
> route initially as it appeared to be quite old and not widely used, and
> the states don't completely overlap.  More about it below.

The SCSI SES driver takes full advantage of the ENCLOSURE driver-core,
so I suspect layering NPEM / SSD_LED_DSM on top of ENCLOSURE is a
better fit than layering those same concepts on top of LED. As for
usages beyond SES you can see that ledmon avoided the kernel
altogether for its support via direct userspace configuration cycles,
but that's incompatible with ACPI DSMs that need a kernel driver, and
its incompatible with LOCKDOWN_KERNEL where root is disallowed from
issuing configuration write cycles.

>
> >>
> >> 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
> >> V3:
> >>          * Changed code to use a single LED per supported state
> >>          * Moved to drivers/pci/pcie
> >>          * Changed Kconfig dependency to LEDS_CLASS instead of NEW_LEDS
> >>          * Added PCI device class check before _DSM presence check
> >>          * Other cleanups that don't affect the function
> >>
> >> ---
> >>   drivers/pci/pcie/Kconfig    |  11 +
> >>   drivers/pci/pcie/Makefile   |   1 +
> >>   drivers/pci/pcie/ssd-leds.c | 419 ++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 431 insertions(+)
> >>   create mode 100644 drivers/pci/pcie/ssd-leds.c
> >>
> >> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> >> index 45a2ef702b45..b738d473209f 100644
> >> --- a/drivers/pci/pcie/Kconfig
> >> +++ b/drivers/pci/pcie/Kconfig
> >> @@ -142,3 +142,14 @@ config PCIE_EDR
> >>            the PCI Firmware Specification r3.2.  Enable this if you want to
> >>            support hybrid DPC model which uses both firmware and OS to
> >>            implement DPC.
> >> +
> >> +config PCIE_SSD_LEDS
> >> +       tristate "PCIe SSD status LED support"
> >> +       depends on ACPI && LEDS_CLASS
> >
> > This "depends on ACPI" is awkward when this grows NPEM support. I feel
> > like NPEM is the first class citizen and then ACPI optionally overrides
> > NPEM support if present.
> >
> >
>
> I didn't actually think about NPEM when I originally submitted this...
> and even now, as I submitted it, it only uses the _DSM method, so it is
> useless without ACPI.  The ACPI dependency could go if NPEM support was
> added, for sure.

As far as I can tell from the specification NPEM is the discovery
mechanism for which PCI device to perform the _DSM. Maybe someone who
is more familiar with the intent of the specification can clarify, but
I would expect that the driver for the NPEM for a given storage device
would check it's local NPEM first and then walk up the hierarchy to
find the first NPEM instance enabled by platform firmware, then before
using that instance check if the ACPI _DSM is available for that PCI
device and use that instead. Otherwise, it's not clear to me how
software definitely associates a given control interface to a specific
storage, or memory in the case of CXL, device.

>
> >> +       help
> >> +         Driver for PCIe SSD status LED management as described in a PCI
> >> +         Firmware Specification, Revision 3.2 ECN.
> >
> > The auxiliary bus [1] was recently added as a way for drivers to carve
> > off functionality into sub-device / sub-driver pairs. One benefit from
> > the auxiliary bus organization is that the NPEM driver gets a propoer
> > alias and auto-loading support. As is this driver appears to need to be
> > manually loaded.
> >
> > [1]: Documentation/driver-api/auxiliary_bus.rst
> >
>
> Yes, unfortunately, it would need to be manually loaded.  I flip-flopped
> multiple times on making this just a library that the nvme driver could
> call to register the LEDs (if the _DSM was present).  It would be
> simpler, it wouldn't need a module to be manulaly loaded, it would
> resulted in better LED names (if the _DSM was on the NVMe PCI device),
> and it would mean that the driver that owns the PCI device would also
> own the LEDs.
>
> The only reason I didn't do that, is that the _DSM can be on the PCIe
> port that the NVMe device is connected to, rather than on the NVMe drive
> PCI device.  And I'm not sure how to deal with the latter situation,
> except maybe change the pcie port bus driver to also call in to the
> library to register LEDs somehow.  But maybe it would be worth trying to
> figure that out.

Like I mentioned above, I think it's ok / expected that the client of
this API will kick off a search that starts with the local PCI device
and walks the hierarchy if necessary to the first enabled NPEM
instance.

> I was not aware of the auxiliary bus, thanks for pointing it out.  I
> glanced over it, and I'm not sure how it would help over just having a
> library that nvme, pcie port bus driver, CXL, or whatever could call into.

It helps because it registers a typical device that can autoload the
NPEM driver. So, instead of force loading this module, or adding all
the logic to be called by NVME directly, the NVME driver can simply
register an auxiliary device. The CXL driver can load a similar
auxiliary device. Then the common NPEM driver would be autoloaded in
response to those events, and typical modprobe policy can be used to
filter the driver or apply any other policy upon the arrival of
NPEM-like devices.

> >> +
> >> +         When enabled, LED interfaces will be created for supported drive
> >> +         states for each PCIe device that has the ACPI _DSM method described
> >> +         in the referenced specification.
> >> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> >> index b2980db88cc0..fbcb8c2d1dc1 100644
> >> --- a/drivers/pci/pcie/Makefile
> >> +++ b/drivers/pci/pcie/Makefile
> >> @@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)                += pme.o
> >>   obj-$(CONFIG_PCIE_DPC)         += dpc.o
> >>   obj-$(CONFIG_PCIE_PTM)         += ptm.o
> >>   obj-$(CONFIG_PCIE_EDR)         += edr.o
> >> +obj-$(CONFIG_PCIE_SSD_LEDS)    += ssd-leds.o
> >> diff --git a/drivers/pci/pcie/ssd-leds.c b/drivers/pci/pcie/ssd-leds.c
> >> new file mode 100644
> >> index 000000000000..cacb77e5da2d
> >> --- /dev/null
> >> +++ b/drivers/pci/pcie/ssd-leds.c
> >> @@ -0,0 +1,419 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Module to provide LED interfaces for PCIe SSD status LED states, 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.
> >> + *
> >> + * The "_DSM..." spec is functionally similar to Native PCIe Enclosure
> >> + * Management, but uses a _DSM ACPI method rather than a PCIe extended
> >> + * capability.
> >> + *
> >> + * Copyright (c) 2021 Dell Inc.
> >> + *
> >> + * TODO: Add NPEM support
> >> + */
> >> +
> >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >> +
> >> +#include <linux/acpi.h>
> >> +#include <linux/leds.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/pci.h>
> >> +#include <uapi/linux/uleds.h>
> >> +
> >> +#define DRIVER_NAME    "pcie-ssd-leds"
> >> +#define DRIVER_VERSION "v1.0"
> >> +
> >> +struct led_state {
> >> +       char *name;
> >> +       int bit;
> >> +};
> >> +
> >> +static struct led_state led_states[] = {
> >> +       { .name = "ok",         .bit = 2 },
> >> +       { .name = "locate",     .bit = 3 },
> >> +       { .name = "failed",     .bit = 4 },
> >> +       { .name = "rebuild",    .bit = 5 },
> >> +       { .name = "pfa",        .bit = 6 },
> >> +       { .name = "hotspare",   .bit = 7 },
> >> +       { .name = "ica",        .bit = 8 },
> >> +       { .name = "ifa",        .bit = 9 },
> >> +       { .name = "invalid",    .bit = 10 },
> >> +       { .name = "disabled",   .bit = 11 },
> >> +};
> >
> > include/linux/enclosure.h has common ABI definitions of industry
> > standard enclosure LED settings. The above looks to be open coding the
> > same?
> >
>
> The LED states in inluce/linux/enclosure.h aren't exactly the same...
> there are states defined in NPEM/_DSM that aren't defined in
> enclosure.h.  In addition, while the enclosure driver allows "locate" to
> be controlled independently, it looks like it will only allow a single
> state (unsupported/ok/critical/etc) to be active at a time, while the
> NPEM/_DSM allow all of the state bits to be independently set or
> cleared.  Maybe only one of those states would need to be set at a time,
> I don't know, but that would impose a limitation on what NPEM/_DSM can
> do.  I'll take a closer look at this as an alternative to using
> drivers/leds/led-class.c.

Have a look. Maybe Mariusz can weigh in here with his experience with
ledmon with what capabilities ledmon would need from this driver so we
can decide what if any extensions need to be made to the enclosure
infrastructure?

It's been a while for me [2], but I seem to recall that some of the
enclosure specific functionality can be wrapped by the upper layer
driver like SES.

[2]: https://lore.kernel.org/all/CAPcyv4iaXfgB1PBv5v+dyRrUvQbzAyyFf1Ozsaao0t8K0w9zwg@mail.gmail.com/

[..]
> >> +static void initial_scan_for_leds(void)
> >> +{
> >> +       struct pci_dev *pdev = NULL;
> >> +
> >> +       for_each_pci_dev(pdev)
> >> +               probe_pdev(pdev);
> >
> >
> > This looks odd to me. Why force enable every occurrence these leds, and
> > do so indepdendently of the bind state of the driver for the associated
> > PCI device? I would expect that this support would be a library called
> > by the NVME driver, or the CXL driver. A library just like the
> > led_classdev infrastructure.
> >
>
> I guess I didn't know of any reason why they shouldn't be enabled even
> if the nvme driver wasn't bound, if this driver was stand-alone, since
> the LED function doesn't depend on the nvme driver.  But yes, maybe it
> would be better to make this a library that the nvme driver (or pcie
> port bus driver, or CXL driver) calls to register LEDs.

It's just not clear to me what userspace would do with this randomly
popping up without any clear association to a storage / memory device.
What reason would userpsace have to blink the lights in that
disassociated case?

> Thank you for taking a look at this... I'm very glad for any help to get
> this done.

No worries, thanks for taking this on, happy to help.

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

[Pavel Machek]

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

Hi!

> > Thank you for the comments, Dan.
> >
> > I originally did submit something simple that just added a couple of
> > sysfs attributes to allow userspace access to the _DSM, but Greg K-H
> > said (1) that I shouldn't create new driver-specific sysfs files that do
> > things that existing class drivers do, and that if I'm allowing LEDs to
> > be controlled by the user, I have to use the LED subsystem, so I went
> > with that. (See the end of
> > https://patchwork.ozlabs.org/project/linux-pci/patch/20201110153735.58587-1-stuart.w.hayes@gmail.com/)
> 
> I agree with the general sentiment to adopt and extend existing ABIs
> wherever possible, it's just not clear to me that the LED class driver
> is the best fit. The Enclosure class infrastructure, in addition to

LED drivers are not good fit for this. It is unlikely to be accepted.

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

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

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

[Tkaczyk, Mariusz]

On 06.10.2021 22:15, Dan Williams wrote:
>>>> +static struct led_state led_states[] = {
>>>> +       { .name = "ok",         .bit = 2 },
>>>> +       { .name = "locate",     .bit = 3 },
>>>> +       { .name = "failed",     .bit = 4 },
>>>> +       { .name = "rebuild",    .bit = 5 },
>>>> +       { .name = "pfa",        .bit = 6 },
>>>> +       { .name = "hotspare",   .bit = 7 },
>>>> +       { .name = "ica",        .bit = 8 },
>>>> +       { .name = "ifa",        .bit = 9 },
>>>> +       { .name = "invalid",    .bit = 10 },
>>>> +       { .name = "disabled",   .bit = 11 },
>>>> +};
>>> include/linux/enclosure.h has common ABI definitions of industry
>>> standard enclosure LED settings. The above looks to be open coding the
>>> same?
>>>
>> The LED states in inluce/linux/enclosure.h aren't exactly the same...
>> there are states defined in NPEM/_DSM that aren't defined in
>> enclosure.h.  In addition, while the enclosure driver allows "locate" to
>> be controlled independently, it looks like it will only allow a single
>> state (unsupported/ok/critical/etc) to be active at a time, while the
>> NPEM/_DSM allow all of the state bits to be independently set or
>> cleared.  Maybe only one of those states would need to be set at a time,
>> I don't know, but that would impose a limitation on what NPEM/_DSM can
>> do.  I'll take a closer look at this as an alternative to using
>> drivers/leds/led-class.c.
> Have a look. Maybe Mariusz can weigh in here with his experience with
> ledmon with what capabilities ledmon would need from this driver so we
> can decide what if any extensions need to be made to the enclosure
> infrastructure?

Hmm... In ledmon we are expecting one state to be set at the time. So,
I would expected from kernel to work the same.

Looking into ledmon code, all capabilities from this list could be
used[1].

 >>>> +       { .name = "ok",         .bit = 2 },
 >>>> +       { .name = "locate",     .bit = 3 },
 >>>> +       { .name = "failed",     .bit = 4 },
 >>>> +       { .name = "rebuild",    .bit = 5 },
 >>>> +       { .name = "pfa",        .bit = 6 },
 >>>> +       { .name = "hotspare",   .bit = 7 },
 >>>> +       { .name = "ica",        .bit = 8 },
 >>>> +       { .name = "ifa",        .bit = 9 },

[1]https://github.com/intel/ledmon/blob/master/src/ibpi.h#L60

Thanks,
Mariusz

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

[stuart hayes]

On 10/7/2021 6:32 AM, Tkaczyk, Mariusz wrote:
> On 06.10.2021 22:15, Dan Williams wrote:
>>>>> +static struct led_state led_states[] = {
>>>>> +       { .name = "ok",         .bit = 2 },
>>>>> +       { .name = "locate",     .bit = 3 },
>>>>> +       { .name = "failed",     .bit = 4 },
>>>>> +       { .name = "rebuild",    .bit = 5 },
>>>>> +       { .name = "pfa",        .bit = 6 },
>>>>> +       { .name = "hotspare",   .bit = 7 },
>>>>> +       { .name = "ica",        .bit = 8 },
>>>>> +       { .name = "ifa",        .bit = 9 },
>>>>> +       { .name = "invalid",    .bit = 10 },
>>>>> +       { .name = "disabled",   .bit = 11 },
>>>>> +};
>>>> include/linux/enclosure.h has common ABI definitions of industry
>>>> standard enclosure LED settings. The above looks to be open coding the
>>>> same?
>>>>
>>> The LED states in inluce/linux/enclosure.h aren't exactly the same...
>>> there are states defined in NPEM/_DSM that aren't defined in
>>> enclosure.h.  In addition, while the enclosure driver allows "locate" to
>>> be controlled independently, it looks like it will only allow a single
>>> state (unsupported/ok/critical/etc) to be active at a time, while the
>>> NPEM/_DSM allow all of the state bits to be independently set or
>>> cleared.  Maybe only one of those states would need to be set at a time,
>>> I don't know, but that would impose a limitation on what NPEM/_DSM can
>>> do.  I'll take a closer look at this as an alternative to using
>>> drivers/leds/led-class.c.
>> Have a look. Maybe Mariusz can weigh in here with his experience with
>> ledmon with what capabilities ledmon would need from this driver so we
>> can decide what if any extensions need to be made to the enclosure
>> infrastructure?
> 
> Hmm... In ledmon we are expecting one state to be set at the time. So,
> I would expected from kernel to work the same.
> 
> Looking into ledmon code, all capabilities from this list could be
> used[1].
> 
>  >>>> +       { .name = "ok",         .bit = 2 },
>  >>>> +       { .name = "locate",     .bit = 3 },
>  >>>> +       { .name = "failed",     .bit = 4 },
>  >>>> +       { .name = "rebuild",    .bit = 5 },
>  >>>> +       { .name = "pfa",        .bit = 6 },
>  >>>> +       { .name = "hotspare",   .bit = 7 },
>  >>>> +       { .name = "ica",        .bit = 8 },
>  >>>> +       { .name = "ifa",        .bit = 9 },
> 
> [1]https://github.com/intel/ledmon/blob/master/src/ibpi.h#L60
> 
> Thanks,
> Mariusz

I've reworked the code so it is an auxiliary driver (to nvme, and
hopefully cxl and pcieport, but I haven't added that yet), and so it
registers as an enclosure (instead of using the LED subsystem).

I had no issues with making it an auxiliary driver... that seemed to
work well, and it looks like it should be easy to add cxl & pcieport
support.

Instead of making pcieport driver add an auxiliary device, I could make
this driver check the parent of the NVMe device if it doesn't find the
NPEM extended capability or _DSM on the NVMe PCIe device itself.  (I
didn't do that, because (a) it would be taking control of part of a PCIe
device to which a different driver was attached, and (b) the "invalid"
state isn't usable if the LED driver is only connected by the nvme driver.)

The enclosure driver isn't usable as is.  The "status" attribute for an
enclosure component corresponds to the "status code" in the SES spec (the
enclosure driver appears to be aligned to the SES spec)... the status code
is not what's used to to control the drive state indicators in SES--the
status code is read-only (in the SES spec) and reflects actual hardware
status, and there are other bits to control the indicators.

The enclosure driver currently only creates sysfs attributes for three
of the state indicators (active, locate, and fault).  So I added seven
more sysfs attributes for the other indicators (SES seems to support all
of the NPEM/_DSM indicators other than "invalid").

Below are the patches.  I wanted to post it here before I submit them,
in case this approach doesn't look good to anyone.  I also wonder if a
better name for this driver might be npem_dsm rather than ssd_leds...

Any feedback is appreciated, thank you!


---------------------------------------------------------
Patch to add auxiliary device to NVMe driver:
---------------------------------------------------------

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 456a0e8a5718..d9acb3132f43 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -26,6 +26,7 @@
  #include <linux/io-64-nonatomic-hi-lo.h>
  #include <linux/sed-opal.h>
  #include <linux/pci-p2pdma.h>
+#include <linux/auxiliary_bus.h>
  
  #include "trace.h"
  #include "nvme.h"
@@ -158,8 +159,38 @@ struct nvme_dev {
  	unsigned int nr_poll_queues;
  
  	bool attrs_added;
+
+	/* auxiliary_device for NPEM/_DSM driver for LEDs */
+	struct auxiliary_device ssd_leds_auxdev;
  };
  
+/*
+ * register auxiliary device for PCIe NPEM/_DSM status LEDs
+ */
+static void nvme_release_ssd_leds_aux_device(struct device *dev)
+{
+}
+
+static void nvme_register_ssd_leds_aux_device(struct nvme_dev *dev)
+{
+	struct auxiliary_device *adev;
+	int ret;
+
+	adev = &dev->ssd_leds_auxdev;
+	adev->name = "ssd_leds";
+	adev->dev.parent = dev->dev;
+	adev->dev.release = nvme_release_ssd_leds_aux_device;
+	adev->id = dev->ctrl.instance;
+	
+	ret = auxiliary_device_init(adev);
+	if (ret < 0)
+		return;
+
+	ret = auxiliary_device_add(adev);
+	if (ret)
+		auxiliary_device_uninit(adev);
+}
+
  static int io_queue_depth_set(const char *val, const struct kernel_param *kp)
  {
  	return param_set_uint_minmax(val, kp, NVME_PCI_MIN_QUEUE_SIZE,
@@ -2812,6 +2843,8 @@ static void nvme_reset_work(struct work_struct *work)
  		nvme_unfreeze(&dev->ctrl);
  	}
  
+	nvme_register_ssd_leds_aux_device(dev);
+
  	/*
  	 * If only admin queue live, keep it to do further investigation or
  	 * recovery.

---------------------------------------------------------
Patch to add more LED attributes to the enclosure driver:
---------------------------------------------------------

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index f950d0155876..95f4f500f4af 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -473,30 +473,6 @@ static const char *const enclosure_type[] = {
  	[ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
  };
  
-static ssize_t get_component_fault(struct device *cdev,
-				   struct device_attribute *attr, char *buf)
-{
-	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
-	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-
-	if (edev->cb->get_fault)
-		edev->cb->get_fault(edev, ecomp);
-	return snprintf(buf, 40, "%d\n", ecomp->fault);
-}
-
-static ssize_t set_component_fault(struct device *cdev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t count)
-{
-	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
-	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
-
-	if (edev->cb->set_fault)
-		edev->cb->set_fault(edev, ecomp, val);
-	return count;
-}
-
  static ssize_t get_component_status(struct device *cdev,
  				    struct device_attribute *attr,char *buf)
  {
@@ -531,54 +507,6 @@ static ssize_t set_component_status(struct device *cdev,
  		return -EINVAL;
  }
  
-static ssize_t get_component_active(struct device *cdev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
-	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-
-	if (edev->cb->get_active)
-		edev->cb->get_active(edev, ecomp);
-	return snprintf(buf, 40, "%d\n", ecomp->active);
-}
-
-static ssize_t set_component_active(struct device *cdev,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
-{
-	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
-	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
-
-	if (edev->cb->set_active)
-		edev->cb->set_active(edev, ecomp, val);
-	return count;
-}
-
-static ssize_t get_component_locate(struct device *cdev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
-	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-
-	if (edev->cb->get_locate)
-		edev->cb->get_locate(edev, ecomp);
-	return snprintf(buf, 40, "%d\n", ecomp->locate);
-}
-
-static ssize_t set_component_locate(struct device *cdev,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
-{
-	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
-	struct enclosure_component *ecomp = to_enclosure_component(cdev);
-	int val = simple_strtoul(buf, NULL, 0);
-
-	if (edev->cb->set_locate)
-		edev->cb->set_locate(edev, ecomp, val);
-	return count;
-}
-
  static ssize_t get_component_power_status(struct device *cdev,
  					  struct device_attribute *attr,
  					  char *buf)
@@ -641,30 +569,157 @@ static ssize_t get_component_slot(struct device *cdev,
  	return snprintf(buf, 40, "%d\n", slot);
  }
  
-static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
-		    set_component_fault);
+/*
+ * callbacks for attrs using enum enclosure_component_setting (LEDs)
+ */
+static ssize_t led_show(struct device *cdev,
+			enum enclosure_component_led led,
+			char *buf)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+	if (edev->cb->get_led)
+		edev->cb->get_led(edev, ecomp, led);
+	else
+		/*
+		 * support old callbacks for fault/active/locate
+		 */
+		switch (led) {
+			case ENCLOSURE_LED_FAULT:
+				if (edev->cb->get_fault) {
+					edev->cb->get_fault(edev, ecomp);
+					ecomp->led[led] = ecomp->fault;
+				}
+				break;
+			case ENCLOSURE_LED_ACTIVE:
+				if (edev->cb->get_active) {
+					edev->cb->get_active(edev, ecomp);
+					ecomp->led[led] = ecomp->active;
+				}
+				break;
+			case ENCLOSURE_LED_LOCATE:
+				if (edev->cb->get_locate) {
+					edev->cb->get_locate(edev, ecomp);
+					ecomp->led[led] = ecomp->locate;
+				}
+				break;
+			default:
+		}
+
+	return snprintf(buf, 40, "%d\n", ecomp->led[led]);
+}
+
+static ssize_t led_set(struct device *cdev,
+		       enum enclosure_component_led led,
+		       const char *buf, size_t count)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+	int val = simple_strtoul(buf, NULL, 0);
+
+	if (edev->cb->set_led)
+		edev->cb->set_led(edev, ecomp, led, val);
+	else
+		/*
+		 * support old callbacks for fault/active/locate
+		 */
+		switch (led) {
+			case ENCLOSURE_LED_FAULT:
+				if (edev->cb->set_fault)
+					edev->cb->set_fault(edev, ecomp, val);
+				break;
+			case ENCLOSURE_LED_ACTIVE:
+				if (edev->cb->set_active)
+					edev->cb->set_active(edev, ecomp, val);
+				break;
+			case ENCLOSURE_LED_LOCATE:
+				if (edev->cb->set_locate)
+					edev->cb->set_locate(edev, ecomp, val);
+				break;
+			default:
+		}
+
+	return count;
+}
+
+#define define_led_rw_attr(name)	\
+static DEVICE_ATTR(name, 0644, show_##name, set_##name)
+
+#define LED_ATTR_RW(led_attr, led)		\
+static ssize_t show_##led_attr(struct device *cdev,			\
+			       struct device_attribute *attr,		\
+	       		       char *buf)				\
+{									\
+	return led_show(cdev, led, buf);				\
+}									\
+static ssize_t set_##led_attr(struct device *cdev,			\
+			      struct device_attribute *attr,		\
+			      const char *buf, size_t count)		\
+{									\
+	return led_set(cdev, led, buf, count);				\
+}									\
+define_led_rw_attr(led_attr)
+
  static DEVICE_ATTR(status, S_IRUGO | S_IWUSR, get_component_status,
  		   set_component_status);
-static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, get_component_active,
-		   set_component_active);
-static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
-		   set_component_locate);
  static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
  		   set_component_power_status);
  static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
  static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
+LED_ATTR_RW(fault, ENCLOSURE_LED_FAULT);
+LED_ATTR_RW(active, ENCLOSURE_LED_ACTIVE);
+LED_ATTR_RW(locate, ENCLOSURE_LED_LOCATE);
+LED_ATTR_RW(ok, ENCLOSURE_LED_OK);
+LED_ATTR_RW(rebuild, ENCLOSURE_LED_REBUILD);
+LED_ATTR_RW(prdfail, ENCLOSURE_LED_PRDFAIL);
+LED_ATTR_RW(hotspare, ENCLOSURE_LED_HOTSPARE);
+LED_ATTR_RW(ica, ENCLOSURE_LED_ICA);
+LED_ATTR_RW(ifa, ENCLOSURE_LED_IFA);
+LED_ATTR_RW(disabled, ENCLOSURE_LED_DISABLED);
  
  static struct attribute *enclosure_component_attrs[] = {
  	&dev_attr_fault.attr,
  	&dev_attr_status.attr,
  	&dev_attr_active.attr,
  	&dev_attr_locate.attr,
+	&dev_attr_ok.attr,
+	&dev_attr_rebuild.attr,
+	&dev_attr_prdfail.attr,
+	&dev_attr_hotspare.attr,
+	&dev_attr_ica.attr,
+	&dev_attr_ifa.attr,
+	&dev_attr_disabled.attr,
  	&dev_attr_power_status.attr,
  	&dev_attr_type.attr,
  	&dev_attr_slot.attr,
  	NULL
  };
-ATTRIBUTE_GROUPS(enclosure_component);
+
+static umode_t enclosure_component_visible(struct kobject *kobj,
+					struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct enclosure_component *ecomp = to_enclosure_component(dev);
+
+	/*
+	 * hide attrs that are only available on array devices
+	 */
+	if (ecomp->type != ENCLOSURE_COMPONENT_ARRAY_DEVICE
+	    && (a == &dev_attr_rebuild.attr
+	        || a == &dev_attr_ok.attr
+	        || a == &dev_attr_hotspare.attr
+	        || a == &dev_attr_ifa.attr
+	        || a == &dev_attr_ica.attr))
+			return 0;
+	return a->mode;
+}
+
+static struct attribute_group enclosure_component_group = {
+	.attrs = enclosure_component_attrs,
+	.is_visible = enclosure_component_visible,
+};
+__ATTRIBUTE_GROUPS(enclosure_component);
  
  static int __init enclosure_init(void)
  {
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 1c630e2c2756..db9dff77e595 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -49,6 +49,20 @@ enum enclosure_component_setting {
  	ENCLOSURE_SETTING_BLINK_B_OFF_ON = 7,
  };
  
+enum enclosure_component_led {
+	ENCLOSURE_LED_FAULT,
+	ENCLOSURE_LED_ACTIVE,
+	ENCLOSURE_LED_LOCATE,
+	ENCLOSURE_LED_OK,
+	ENCLOSURE_LED_REBUILD,
+	ENCLOSURE_LED_PRDFAIL,
+	ENCLOSURE_LED_HOTSPARE,
+	ENCLOSURE_LED_ICA,
+	ENCLOSURE_LED_IFA,
+	ENCLOSURE_LED_DISABLED,
+	ENCLOSURE_LED_MAX,
+};
+
  struct enclosure_device;
  struct enclosure_component;
  struct enclosure_component_callbacks {
@@ -72,6 +86,13 @@ struct enclosure_component_callbacks {
  	int (*set_locate)(struct enclosure_device *,
  			  struct enclosure_component *,
  			  enum enclosure_component_setting);
+	void (*get_led)(struct enclosure_device *,
+			struct enclosure_component *,
+			enum enclosure_component_led);
+	int (*set_led)(struct enclosure_device *,
+		       struct enclosure_component *,
+		       enum enclosure_component_led,
+		       enum enclosure_component_setting);
  	void (*get_power_status)(struct enclosure_device *,
  				 struct enclosure_component *);
  	int (*set_power_status)(struct enclosure_device *,
@@ -80,7 +101,6 @@ struct enclosure_component_callbacks {
  	int (*show_id)(struct enclosure_device *, char *buf);
  };
  
-
  struct enclosure_component {
  	void *scratch;
  	struct device cdev;
@@ -90,6 +110,7 @@ struct enclosure_component {
  	int fault;
  	int active;
  	int locate;
+	int led[ENCLOSURE_LED_MAX];
  	int slot;
  	enum enclosure_status status;
  	int power_status;

---------------------------------------------------------
Patch to add LED driver:
---------------------------------------------------------

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 85ba901bc11b..e9c3b7f5236b 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -469,6 +469,17 @@ config HISI_HIKEY_USB
  	  switching between the dual-role USB-C port and the USB-A host ports
  	  using only one USB controller.
  
+config PCIE_SSD_LEDS
+	tristate "PCIe SSD status LED support"
+	depends on ACPI && ENCLOSURE_SERVICES
+	help
+	  Auxiliary driver for PCIe SSD status LED management as described in
+	  a PCI Firmware Specification, Revision 3.2 ECN.
+
+	  When enabled, an enclosure device will be created for each device
+	  that hast has the ACPI _DSM method described in the referenced ECN,
+	  to allow control of LED states.
+
  source "drivers/misc/c2port/Kconfig"
  source "drivers/misc/eeprom/Kconfig"
  source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index a086197af544..8a8cb438a78c 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -59,3 +59,4 @@ obj-$(CONFIG_UACCE)		+= uacce/
  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
  obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
+obj-$(CONFIG_PCIE_SSD_LEDS)	+= ssd-leds.o
diff --git a/drivers/misc/ssd-leds.c b/drivers/misc/ssd-leds.c
new file mode 100644
index 000000000000..59927ebc8aa4
--- /dev/null
+++ b/drivers/misc/ssd-leds.c
@@ -0,0 +1,426 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Module to provide LED interfaces for PCIe SSD status LED states, 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.
+ *
+ * The "_DSM..." spec is functionally similar to Native PCIe Enclosure
+ * Management, but uses a _DSM ACPI method rather than a PCIe extended
+ * capability.
+ *
+ * Copyright (c) 2021 Dell Inc.
+ *
+ * TODO: Add NPEM support
+ *       Add pcieport & cxl support
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/enclosure.h>
+#include <linux/auxiliary_bus.h>
+
+#define DRIVER_NAME	"ssd-leds"
+#define DRIVER_VERSION	"v1.0"
+
+/*
+ * NPEM & _DSM use the same state bits
+ */
+#define	NPEM_STATE_OK		BIT(2)
+#define	NPEM_STATE_LOCATE	BIT(3)
+#define	NPEM_STATE_FAILED	BIT(4)
+#define	NPEM_STATE_REBUILD	BIT(5)
+#define	NPEM_STATE_PFA		BIT(6)  /* predicted failure analysis */
+#define	NPEM_STATE_HOTSPARE	BIT(7)
+#define	NPEM_STATE_ICA		BIT(8)  /* in a critical array */
+#define	NPEM_STATE_IFA		BIT(9)  /* in a failed array */
+#define	NPEM_STATE_INVALID	BIT(10)
+#define	NPEM_STATE_DISABLED	BIT(11)
+
+static u32 to_npem_state[ENCLOSURE_LED_MAX];
+
+/*
+ * ssd_led_dev->dev could be the drive itself or its PCIe port
+ */
+struct ssd_led_dev {
+	struct list_head list;
+	/* PCI device that has the LED controls */
+	struct pci_dev *pdev;
+	/* ops to read/set state (NPEM or _DSM) */
+	struct drive_status_ops *ops;
+	/* enclosure device used as sysfs interface for states */
+	struct enclosure_device *edev;
+	/* latest written states */
+	u32 states;
+	u32 supported_states;
+};
+
+struct drive_status_ops {
+	int (*get_supported_states)(struct ssd_led_dev *sldev);
+	int (*get_current_states)(struct ssd_led_dev *sldev, u32 *states);
+	int (*set_current_states)(struct ssd_led_dev *sldev, u32 states);
+};
+
+static struct mutex drive_status_lock;
+static struct list_head ssd_led_dev_list;
+static int ssd_leds_exiting = 0;
+
+/*
+ * _DSM LED control
+ */
+static const guid_t pcie_ssd_leds_dsm_guid =
+	GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b,
+		  0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d);
+
+#define GET_SUPPORTED_STATES_DSM	0x01
+#define GET_STATE_DSM			0x02
+#define SET_STATE_DSM			0x03
+
+struct ssdleds_dsm_output {
+	u16 status;
+	u8 function_specific_err;
+	u8 vendor_specific_err;
+	u32 state;
+};
+
+static void dsm_status_err_print(struct pci_dev *pdev,
+				 struct ssdleds_dsm_output *output)
+{
+	switch (output->status) {
+	case 0:
+		break;
+	case 1:
+		pci_dbg(pdev, "_DSM not supported\n");
+		break;
+	case 2:
+		pci_dbg(pdev, "_DSM invalid input parameters\n");
+		break;
+	case 3:
+		pci_dbg(pdev, "_DSM communication error\n");
+		break;
+	case 4:
+		pci_dbg(pdev, "_DSM function-specific error 0x%x\n",
+			output->function_specific_err);
+		break;
+	case 5:
+		pci_dbg(pdev, "_DSM vendor-specific error 0x%x\n",
+			output->vendor_specific_err);
+		break;
+	default:
+		pci_dbg(pdev, "_DSM returned unknown status 0x%x\n",
+			output->status);
+	}
+}
+
+static int dsm_set(struct pci_dev *pdev, u32 value)
+{
+	acpi_handle handle;
+	union acpi_object *out_obj, arg3[2];
+	struct ssdleds_dsm_output *dsm_output;
+
+	handle = ACPI_HANDLE(&pdev->dev);
+	if (!handle)
+		return -ENODEV;
+
+	printk("dsm_set pdev %x:%x.%d value %x\n", pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), value);
+
+	arg3[0].type = ACPI_TYPE_PACKAGE;
+	arg3[0].package.count = 1;
+	arg3[0].package.elements = &arg3[1];
+
+	arg3[1].type = ACPI_TYPE_BUFFER;
+	arg3[1].buffer.length = 4;
+	arg3[1].buffer.pointer = (u8 *)&value;
+
+	out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssd_leds_dsm_guid,
+				1, SET_STATE_DSM, &arg3[0], ACPI_TYPE_BUFFER);
+	if (!out_obj)
+		return -EIO;
+
+	if (out_obj->buffer.length < 8) {
+		ACPI_FREE(out_obj);
+		return -EIO;
+	}
+
+	dsm_output = (struct ssdleds_dsm_output *)out_obj->buffer.pointer;
+
+	if (dsm_output->status != 0) {
+		dsm_status_err_print(pdev, dsm_output);
+		ACPI_FREE(out_obj);
+		return -EIO;
+	}
+	ACPI_FREE(out_obj);
+	return 0;
+}
+
+static int dsm_get(struct pci_dev *pdev, u64 dsm_func, u32 *output)
+{
+	acpi_handle handle;
+	union acpi_object *out_obj;
+	struct ssdleds_dsm_output *dsm_output;
+
+	handle = ACPI_HANDLE(&pdev->dev);
+	if (!handle)
+		return -ENODEV;
+
+	printk("dsm_get pdev %x:%x.%d\n", pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+	out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssd_leds_dsm_guid, 0x1,
+					  dsm_func, NULL, ACPI_TYPE_BUFFER);
+	if (!out_obj)
+		return -EIO;
+
+	if (out_obj->buffer.length < 8) {
+		ACPI_FREE(out_obj);
+		return -EIO;
+	}
+
+	dsm_output = (struct ssdleds_dsm_output *)out_obj->buffer.pointer;
+	if (dsm_output->status != 0) {
+		dsm_status_err_print(pdev, dsm_output);
+		ACPI_FREE(out_obj);
+		return -EIO;
+	}
+
+	*output = dsm_output->state;
+	ACPI_FREE(out_obj);
+	printk("...got %x\n", *output);
+	return 0;
+}
+
+static int get_supported_states_dsm(struct ssd_led_dev *sldev)
+{
+	return dsm_get(sldev->pdev, GET_SUPPORTED_STATES_DSM, &sldev->supported_states);
+}
+
+static int get_current_states_dsm(struct ssd_led_dev *sldev, u32 *states)
+{
+	return dsm_get(sldev->pdev, GET_STATE_DSM, states);
+}
+
+static int set_current_states_dsm(struct ssd_led_dev *sldev, u32 states)
+{
+	return dsm_set(sldev->pdev, states);
+}
+
+static bool pdev_has_dsm(struct pci_dev *pdev)
+{
+	acpi_handle handle = ACPI_HANDLE(&pdev->dev);
+
+	if (!handle)
+		return false;
+
+	return acpi_check_dsm(handle, &pcie_ssd_leds_dsm_guid, 0x1,
+			      1 << GET_SUPPORTED_STATES_DSM ||
+			      1 << GET_STATE_DSM ||
+			      1 << SET_STATE_DSM);
+}
+
+static struct drive_status_ops dsm_drive_status_ops = {
+	.get_supported_states = get_supported_states_dsm,
+	.get_current_states = get_current_states_dsm,
+	.set_current_states = set_current_states_dsm,
+};
+
+/*
+ * end of _DSM code
+ */
+
+static void ssd_leds_get_led(struct enclosure_device *edev,
+			     struct enclosure_component *ecomp,
+			     enum enclosure_component_led led)
+{
+	struct ssd_led_dev *sldev = ecomp->scratch;
+	u32 states = 0;
+
+	sldev->ops->get_current_states(sldev, &states);
+	ecomp->led[led] = !!(states & to_npem_state[led]) ?
+		ENCLOSURE_SETTING_ENABLED : ENCLOSURE_SETTING_DISABLED;
+}
+
+static int ssd_leds_set_led(struct enclosure_device *edev,
+			 struct enclosure_component *ecomp,
+			 enum enclosure_component_led led,
+			 enum enclosure_component_setting val)
+{
+	struct ssd_led_dev *sldev = ecomp->scratch;
+	u32 npem_state = to_npem_state[led], states;
+	int rc;
+
+	if (val != ENCLOSURE_SETTING_ENABLED
+	    && val != ENCLOSURE_SETTING_DISABLED)
+		return -EINVAL;
+
+	states = sldev->states & ~npem_state;
+	states |= val == ENCLOSURE_SETTING_ENABLED ? npem_state : 0;
+
+	if ((states & sldev->supported_states) != states) {
+		printk("can't set state %x\n", states);
+		return -EINVAL;
+	}
+
+	rc = sldev->ops->set_current_states(sldev, states);
+	/*
+	 * save last written state so it doesn't have to be re-read via NPEM/
+	 * _DSM on the next write, not only because it is faster, but because
+	 * _DSM doesn't provide a way to know if the last write has completed
+	 */
+	if (rc == 0)
+		sldev->states = states;
+	return rc;
+}
+
+static struct enclosure_component_callbacks ssdleds_cb = {
+	.get_led	= ssd_leds_get_led,
+	.set_led	= ssd_leds_set_led,
+};
+
+static struct ssd_led_dev *to_ssd_led_dev(struct pci_dev *pdev)
+{
+	struct ssd_led_dev *sldev;
+
+	list_for_each_entry(sldev, &ssd_led_dev_list, list)
+		if (pdev == sldev->pdev)
+			return sldev;
+	return NULL;
+}
+
+static void remove_ssd_led_dev(struct ssd_led_dev *sldev)
+{
+	enclosure_unregister(sldev->edev);
+	list_del(&sldev->list);
+	kfree(sldev);
+}
+
+static int add_ssd_led_dev(struct pci_dev *pdev,
+				const char *name,
+				struct drive_status_ops *ops)
+{
+	struct ssd_led_dev *sldev;
+	struct enclosure_device *edev;
+	struct enclosure_component *ecomp;
+	int rc = 0;
+
+	mutex_lock(&drive_status_lock);
+	if (ssd_leds_exiting)
+		goto out_unlock;
+
+	sldev = kzalloc(sizeof(*sldev), GFP_KERNEL);
+	if (!sldev) {
+		rc = -ENOMEM;
+		goto out_unlock;
+	}
+	sldev->pdev = pdev;
+	sldev->ops = ops;
+	sldev->states = 0;
+	INIT_LIST_HEAD(&sldev->list);
+	if (sldev->ops->get_supported_states(sldev) != 0)
+		goto out_free;
+
+	edev = enclosure_register(&pdev->dev, name, 1, &ssdleds_cb);
+	if (!edev)
+		goto out_free;
+
+	ecomp = enclosure_component_alloc(edev, 0, ENCLOSURE_COMPONENT_DEVICE, "leds");
+	if (IS_ERR(ecomp))
+		goto out_unreg;
+
+	ecomp->type = ENCLOSURE_COMPONENT_ARRAY_DEVICE;
+	rc = enclosure_component_register(ecomp);
+	if (rc < 0)
+		goto out_unreg;
+
+	ecomp->scratch = sldev;
+	sldev->edev = edev;
+	list_add_tail(&sldev->list, &ssd_led_dev_list);
+	goto out_unlock;
+
+out_unreg:
+	enclosure_unregister(edev);
+out_free:
+	kfree(sldev);
+out_unlock:
+	mutex_unlock(&drive_status_lock);
+	return rc;
+}
+
+static int ssd_leds_probe(struct auxiliary_device *adev,
+			  const struct auxiliary_device_id *id)
+{
+	struct pci_dev *pdev = to_pci_dev(adev->dev.parent);
+
+	if (pdev_has_dsm(pdev))
+		return add_ssd_led_dev(pdev, dev_name(&adev->dev), &dsm_drive_status_ops);
+	return 0;
+}
+
+static void ssd_leds_remove(struct auxiliary_device *adev)
+{
+	struct pci_dev *pdev = to_pci_dev(adev->dev.parent);
+	struct ssd_led_dev *sldev;
+
+	mutex_lock(&drive_status_lock);
+	sldev = to_ssd_led_dev(pdev);
+	if (sldev)
+		remove_ssd_led_dev(sldev);
+out_unlock:
+	mutex_unlock(&drive_status_lock);
+}
+
+static const struct auxiliary_device_id ssd_leds_id_table[] = {
+	{ .name = "nvme.ssd_leds", },
+//	{ .name = "pcieport.ssd_leds", },
+//	{ .name = "cxl.ssd_leds", },	
+	{},
+};
+
+MODULE_DEVICE_TABLE(auxiliary, ssd_leds_id_table);
+
+static struct auxiliary_driver ssd_leds_driver = {
+	.name = "ssd_leds",
+	.probe = ssd_leds_probe,
+	.remove = ssd_leds_remove,
+	.id_table = ssd_leds_id_table,
+};
+
+static int __init ssd_leds_init(void)
+{
+	mutex_init(&drive_status_lock);
+	INIT_LIST_HEAD(&ssd_led_dev_list);
+
+	to_npem_state[ENCLOSURE_LED_FAULT] = NPEM_STATE_FAILED;
+	to_npem_state[ENCLOSURE_LED_ACTIVE] = 0;
+	to_npem_state[ENCLOSURE_LED_LOCATE] = NPEM_STATE_LOCATE;
+	to_npem_state[ENCLOSURE_LED_OK] = NPEM_STATE_OK;
+	to_npem_state[ENCLOSURE_LED_REBUILD] = NPEM_STATE_REBUILD;
+	to_npem_state[ENCLOSURE_LED_PRDFAIL] = NPEM_STATE_PFA;
+	to_npem_state[ENCLOSURE_LED_HOTSPARE] = NPEM_STATE_HOTSPARE;
+	to_npem_state[ENCLOSURE_LED_ICA] = NPEM_STATE_ICA;
+	to_npem_state[ENCLOSURE_LED_IFA] = NPEM_STATE_IFA;
+	to_npem_state[ENCLOSURE_LED_DISABLED] = NPEM_STATE_DISABLED;
+
+	auxiliary_driver_register(&ssd_leds_driver);
+	return 0;
+}
+
+static void __exit ssd_leds_exit(void)
+{
+	struct ssd_led_dev *sldev, *temp;
+
+	mutex_lock(&drive_status_lock);
+	ssd_leds_exiting = 1;
+	list_for_each_entry_safe(sldev, temp, &ssd_led_dev_list, list)
+		remove_ssd_led_dev(sldev);
+	mutex_unlock(&drive_status_lock);
+	auxiliary_driver_unregister(&ssd_leds_driver);
+}
+
+module_init(ssd_leds_init);
+module_exit(ssd_leds_exit);
+
+MODULE_AUTHOR("Stuart Hayes <stuart.w.hayes@gmail.com>");
+MODULE_DESCRIPTION("Support for PCIe SSD Status LEDs");
+MODULE_LICENSE("GPL");

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

[Dan Williams]

[ add James and Martin in case they see anything here that collides
with the SES driver ]

On Tue, Nov 2, 2021 at 9:34 AM stuart hayes <stuart.w.hayes@gmail.com> wrote:
>
>
>
> On 10/7/2021 6:32 AM, Tkaczyk, Mariusz wrote:
> > On 06.10.2021 22:15, Dan Williams wrote:
> >>>>> +static struct led_state led_states[] = {
> >>>>> +       { .name = "ok",         .bit = 2 },
> >>>>> +       { .name = "locate",     .bit = 3 },
> >>>>> +       { .name = "failed",     .bit = 4 },
> >>>>> +       { .name = "rebuild",    .bit = 5 },
> >>>>> +       { .name = "pfa",        .bit = 6 },
> >>>>> +       { .name = "hotspare",   .bit = 7 },
> >>>>> +       { .name = "ica",        .bit = 8 },
> >>>>> +       { .name = "ifa",        .bit = 9 },
> >>>>> +       { .name = "invalid",    .bit = 10 },
> >>>>> +       { .name = "disabled",   .bit = 11 },
> >>>>> +};
> >>>> include/linux/enclosure.h has common ABI definitions of industry
> >>>> standard enclosure LED settings. The above looks to be open coding the
> >>>> same?
> >>>>
> >>> The LED states in inluce/linux/enclosure.h aren't exactly the same...
> >>> there are states defined in NPEM/_DSM that aren't defined in
> >>> enclosure.h.  In addition, while the enclosure driver allows "locate" to
> >>> be controlled independently, it looks like it will only allow a single
> >>> state (unsupported/ok/critical/etc) to be active at a time, while the
> >>> NPEM/_DSM allow all of the state bits to be independently set or
> >>> cleared.  Maybe only one of those states would need to be set at a time,
> >>> I don't know, but that would impose a limitation on what NPEM/_DSM can
> >>> do.  I'll take a closer look at this as an alternative to using
> >>> drivers/leds/led-class.c.
> >> Have a look. Maybe Mariusz can weigh in here with his experience with
> >> ledmon with what capabilities ledmon would need from this driver so we
> >> can decide what if any extensions need to be made to the enclosure
> >> infrastructure?
> >
> > Hmm... In ledmon we are expecting one state to be set at the time. So,
> > I would expected from kernel to work the same.
> >
> > Looking into ledmon code, all capabilities from this list could be
> > used[1].
> >
> >  >>>> +       { .name = "ok",         .bit = 2 },
> >  >>>> +       { .name = "locate",     .bit = 3 },
> >  >>>> +       { .name = "failed",     .bit = 4 },
> >  >>>> +       { .name = "rebuild",    .bit = 5 },
> >  >>>> +       { .name = "pfa",        .bit = 6 },
> >  >>>> +       { .name = "hotspare",   .bit = 7 },
> >  >>>> +       { .name = "ica",        .bit = 8 },
> >  >>>> +       { .name = "ifa",        .bit = 9 },
> >
> > [1]https://github.com/intel/ledmon/blob/master/src/ibpi.h#L60
> >
> > Thanks,
> > Mariusz
>
> I've reworked the code so it is an auxiliary driver (to nvme, and
> hopefully cxl and pcieport, but I haven't added that yet), and so it
> registers as an enclosure (instead of using the LED subsystem).
>
> I had no issues with making it an auxiliary driver... that seemed to
> work well, and it looks like it should be easy to add cxl & pcieport
> support.

Cool! Thanks for taking this on.

>
> Instead of making pcieport driver add an auxiliary device, I could make
> this driver check the parent of the NVMe device if it doesn't find the
> NPEM extended capability or _DSM on the NVMe PCIe device itself.  (I
> didn't do that, because (a) it would be taking control of part of a PCIe
> device to which a different driver was attached, and (b) the "invalid"
> state isn't usable if the LED driver is only connected by the nvme driver.)

Caveat a) does not sound too bad to me, but b) does sound unfortunate.

What about a mechanism for a downstream driver to register itself
after the fact with the active NPEM instant in its parent topology? At
least I seem to recall either enclosure or ses linking a block-device
to an enclosure slot.

> The enclosure driver isn't usable as is.  The "status" attribute for an
> enclosure component corresponds to the "status code" in the SES spec (the
> enclosure driver appears to be aligned to the SES spec)... the status code
> is not what's used to to control the drive state indicators in SES--the
> status code is read-only (in the SES spec) and reflects actual hardware
> status, and there are other bits to control the indicators.

What does this mean for the NPEM enabling?

> The enclosure driver currently only creates sysfs attributes for three
> of the state indicators (active, locate, and fault).  So I added seven
> more sysfs attributes for the other indicators (SES seems to support all
> of the NPEM/_DSM indicators other than "invalid").
>
> Below are the patches.  I wanted to post it here before I submit them,
> in case this approach doesn't look good to anyone.

If you want to get early opinions an "RFC" prefix on the submission
helps set the expectations of the implementation maturity. It's also
nicer to see patches with changelogs, but no worries, I'll take a look
at the below.

> I also wonder if a
> better name for this driver might be npem_dsm rather than ssd_leds...

I do think dropping the "SSD" connotation is useful. Either NPEM or
PCIE_EM sound ok to me. No need for the _DSM suffix I think, that's
just an implementation detail that platform firmware can get in the
way. I can also imagine a non-ACPI platform doing a similar override,
but it's all NPEM protocol in the end.

>
> Any feedback is appreciated, thank you!
>
>
> ---------------------------------------------------------
> Patch to add auxiliary device to NVMe driver:
> ---------------------------------------------------------
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 456a0e8a5718..d9acb3132f43 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -26,6 +26,7 @@
>   #include <linux/io-64-nonatomic-hi-lo.h>
>   #include <linux/sed-opal.h>
>   #include <linux/pci-p2pdma.h>
> +#include <linux/auxiliary_bus.h>
>
>   #include "trace.h"
>   #include "nvme.h"
> @@ -158,8 +159,38 @@ struct nvme_dev {
>         unsigned int nr_poll_queues;
>
>         bool attrs_added;
> +
> +       /* auxiliary_device for NPEM/_DSM driver for LEDs */
> +       struct auxiliary_device ssd_leds_auxdev;

This auxiliary device needs to be allocated out of line from 'struct
nvme_dev'. The lifetime of 'struct nvme_dev' is shorter than a 'struct
device'. Recall that an auxiliary device embeds a 'struct device', and
a 'struct device' embeds a kobject. The kobject lifetime can be
arbitrarily extended by any random kernel agent taking a reference on
the object. You can likely trigger use after free warnings with this
code if you enable CONFIG_DEBUG_KOBJECT_RELEASE.


>   };
>
> +/*
> + * register auxiliary device for PCIe NPEM/_DSM status LEDs
> + */
> +static void nvme_release_ssd_leds_aux_device(struct device *dev)
> +{

Empty device release method is among the most common bugs that Greg
finds, be sure to fix this up before sending he sees it. The above
suggestion to move the allocation of the device out of line will fix
this up too.

> +}
> +
> +static void nvme_register_ssd_leds_aux_device(struct nvme_dev *dev)
> +{
> +       struct auxiliary_device *adev;
> +       int ret;
> +
> +       adev = &dev->ssd_leds_auxdev;
> +       adev->name = "ssd_leds";
> +       adev->dev.parent = dev->dev;
> +       adev->dev.release = nvme_release_ssd_leds_aux_device;
> +       adev->id = dev->ctrl.instance;
> +
> +       ret = auxiliary_device_init(adev);
> +       if (ret < 0)
> +               return;
> +
> +       ret = auxiliary_device_add(adev);
> +       if (ret)
> +               auxiliary_device_uninit(adev);
> +}
> +
>   static int io_queue_depth_set(const char *val, const struct kernel_param *kp)
>   {
>         return param_set_uint_minmax(val, kp, NVME_PCI_MIN_QUEUE_SIZE,
> @@ -2812,6 +2843,8 @@ static void nvme_reset_work(struct work_struct *work)
>                 nvme_unfreeze(&dev->ctrl);
>         }
>
> +       nvme_register_ssd_leds_aux_device(dev);
> +
>         /*
>          * If only admin queue live, keep it to do further investigation or
>          * recovery.
>
> ---------------------------------------------------------
> Patch to add more LED attributes to the enclosure driver:
> ---------------------------------------------------------
>
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index f950d0155876..95f4f500f4af 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -473,30 +473,6 @@ static const char *const enclosure_type[] = {
>         [ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
>   };
>
> -static ssize_t get_component_fault(struct device *cdev,
> -                                  struct device_attribute *attr, char *buf)
> -{
> -       struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -       struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -
> -       if (edev->cb->get_fault)
> -               edev->cb->get_fault(edev, ecomp);
> -       return snprintf(buf, 40, "%d\n", ecomp->fault);
> -}

This conversion to the LED_ATTR_RW() scheme definitely wants to be its
own lead-in cleanup patch with a changelog before adding the new NPEM
attributes. Don't smoosh it all into one patch when you reformat this
into a patch series.

> -
> -static ssize_t set_component_fault(struct device *cdev,
> -                                  struct device_attribute *attr,
> -                                  const char *buf, size_t count)
> -{
> -       struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -       struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -       int val = simple_strtoul(buf, NULL, 0);
> -
> -       if (edev->cb->set_fault)
> -               edev->cb->set_fault(edev, ecomp, val);
> -       return count;
> -}
> -
>   static ssize_t get_component_status(struct device *cdev,
>                                     struct device_attribute *attr,char *buf)
>   {
> @@ -531,54 +507,6 @@ static ssize_t set_component_status(struct device *cdev,
>                 return -EINVAL;
>   }
>
> -static ssize_t get_component_active(struct device *cdev,
> -                                   struct device_attribute *attr, char *buf)
> -{
> -       struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -       struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -
> -       if (edev->cb->get_active)
> -               edev->cb->get_active(edev, ecomp);
> -       return snprintf(buf, 40, "%d\n", ecomp->active);
> -}
> -
> -static ssize_t set_component_active(struct device *cdev,
> -                                   struct device_attribute *attr,
> -                                   const char *buf, size_t count)
> -{
> -       struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -       struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -       int val = simple_strtoul(buf, NULL, 0);
> -
> -       if (edev->cb->set_active)
> -               edev->cb->set_active(edev, ecomp, val);
> -       return count;
> -}
> -
> -static ssize_t get_component_locate(struct device *cdev,
> -                                   struct device_attribute *attr, char *buf)
> -{
> -       struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -       struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -
> -       if (edev->cb->get_locate)
> -               edev->cb->get_locate(edev, ecomp);
> -       return snprintf(buf, 40, "%d\n", ecomp->locate);
> -}
> -
> -static ssize_t set_component_locate(struct device *cdev,
> -                                   struct device_attribute *attr,
> -                                   const char *buf, size_t count)
> -{
> -       struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> -       struct enclosure_component *ecomp = to_enclosure_component(cdev);
> -       int val = simple_strtoul(buf, NULL, 0);
> -
> -       if (edev->cb->set_locate)
> -               edev->cb->set_locate(edev, ecomp, val);
> -       return count;
> -}
> -
>   static ssize_t get_component_power_status(struct device *cdev,
>                                           struct device_attribute *attr,
>                                           char *buf)
> @@ -641,30 +569,157 @@ static ssize_t get_component_slot(struct device *cdev,
>         return snprintf(buf, 40, "%d\n", slot);
>   }
>
> -static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
> -                   set_component_fault);
> +/*
> + * callbacks for attrs using enum enclosure_component_setting (LEDs)
> + */
> +static ssize_t led_show(struct device *cdev,
> +                       enum enclosure_component_led led,
> +                       char *buf)
> +{
> +       struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> +       struct enclosure_component *ecomp = to_enclosure_component(cdev);
> +
> +       if (edev->cb->get_led)
> +               edev->cb->get_led(edev, ecomp, led);
> +       else
> +               /*
> +                * support old callbacks for fault/active/locate
> +                */
> +               switch (led) {
> +                       case ENCLOSURE_LED_FAULT:
> +                               if (edev->cb->get_fault) {
> +                                       edev->cb->get_fault(edev, ecomp);
> +                                       ecomp->led[led] = ecomp->fault;
> +                               }
> +                               break;
> +                       case ENCLOSURE_LED_ACTIVE:
> +                               if (edev->cb->get_active) {
> +                                       edev->cb->get_active(edev, ecomp);
> +                                       ecomp->led[led] = ecomp->active;
> +                               }
> +                               break;
> +                       case ENCLOSURE_LED_LOCATE:
> +                               if (edev->cb->get_locate) {
> +                                       edev->cb->get_locate(edev, ecomp);
> +                                       ecomp->led[led] = ecomp->locate;
> +                               }
> +                               break;
> +                       default:
> +               }
> +
> +       return snprintf(buf, 40, "%d\n", ecomp->led[led]);
> +}
> +
> +static ssize_t led_set(struct device *cdev,
> +                      enum enclosure_component_led led,
> +                      const char *buf, size_t count)
> +{
> +       struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> +       struct enclosure_component *ecomp = to_enclosure_component(cdev);
> +       int val = simple_strtoul(buf, NULL, 0);
> +
> +       if (edev->cb->set_led)
> +               edev->cb->set_led(edev, ecomp, led, val);
> +       else
> +               /*
> +                * support old callbacks for fault/active/locate
> +                */
> +               switch (led) {
> +                       case ENCLOSURE_LED_FAULT:
> +                               if (edev->cb->set_fault)
> +                                       edev->cb->set_fault(edev, ecomp, val);
> +                               break;
> +                       case ENCLOSURE_LED_ACTIVE:
> +                               if (edev->cb->set_active)
> +                                       edev->cb->set_active(edev, ecomp, val);
> +                               break;
> +                       case ENCLOSURE_LED_LOCATE:
> +                               if (edev->cb->set_locate)
> +                                       edev->cb->set_locate(edev, ecomp, val);
> +                               break;
> +                       default:
> +               }
> +
> +       return count;
> +}
> +
> +#define define_led_rw_attr(name)       \
> +static DEVICE_ATTR(name, 0644, show_##name, set_##name)
> +
> +#define LED_ATTR_RW(led_attr, led)             \
> +static ssize_t show_##led_attr(struct device *cdev,                    \
> +                              struct device_attribute *attr,           \
> +                              char *buf)                               \
> +{                                                                      \
> +       return led_show(cdev, led, buf);                                \
> +}                                                                      \
> +static ssize_t set_##led_attr(struct device *cdev,                     \
> +                             struct device_attribute *attr,            \
> +                             const char *buf, size_t count)            \
> +{                                                                      \
> +       return led_set(cdev, led, buf, count);                          \
> +}                                                                      \
> +define_led_rw_attr(led_attr)
> +
>   static DEVICE_ATTR(status, S_IRUGO | S_IWUSR, get_component_status,
>                    set_component_status);
> -static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, get_component_active,
> -                  set_component_active);
> -static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
> -                  set_component_locate);
>   static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
>                    set_component_power_status);
>   static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
>   static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
> +LED_ATTR_RW(fault, ENCLOSURE_LED_FAULT);
> +LED_ATTR_RW(active, ENCLOSURE_LED_ACTIVE);
> +LED_ATTR_RW(locate, ENCLOSURE_LED_LOCATE);
> +LED_ATTR_RW(ok, ENCLOSURE_LED_OK);
> +LED_ATTR_RW(rebuild, ENCLOSURE_LED_REBUILD);
> +LED_ATTR_RW(prdfail, ENCLOSURE_LED_PRDFAIL);
> +LED_ATTR_RW(hotspare, ENCLOSURE_LED_HOTSPARE);
> +LED_ATTR_RW(ica, ENCLOSURE_LED_ICA);
> +LED_ATTR_RW(ifa, ENCLOSURE_LED_IFA);
> +LED_ATTR_RW(disabled, ENCLOSURE_LED_DISABLED);
>
>   static struct attribute *enclosure_component_attrs[] = {
>         &dev_attr_fault.attr,
>         &dev_attr_status.attr,
>         &dev_attr_active.attr,
>         &dev_attr_locate.attr,
> +       &dev_attr_ok.attr,
> +       &dev_attr_rebuild.attr,
> +       &dev_attr_prdfail.attr,
> +       &dev_attr_hotspare.attr,
> +       &dev_attr_ica.attr,
> +       &dev_attr_ifa.attr,
> +       &dev_attr_disabled.attr,
>         &dev_attr_power_status.attr,
>         &dev_attr_type.attr,
>         &dev_attr_slot.attr,
>         NULL
>   };
> -ATTRIBUTE_GROUPS(enclosure_component);
> +
> +static umode_t enclosure_component_visible(struct kobject *kobj,
> +                                       struct attribute *a, int n)
> +{
> +       struct device *dev = container_of(kobj, struct device, kobj);
> +       struct enclosure_component *ecomp = to_enclosure_component(dev);
> +
> +       /*
> +        * hide attrs that are only available on array devices
> +        */

Isn't this expanding the ABI for enclosures outside of NPEM? Should
there be a new ENCLOSURE_COMPONENT_NPEM or somesuch?

> +       if (ecomp->type != ENCLOSURE_COMPONENT_ARRAY_DEVICE
> +           && (a == &dev_attr_rebuild.attr
> +               || a == &dev_attr_ok.attr
> +               || a == &dev_attr_hotspare.attr
> +               || a == &dev_attr_ifa.attr
> +               || a == &dev_attr_ica.attr))
> +                       return 0;
> +       return a->mode;
> +}
> +
> +static struct attribute_group enclosure_component_group = {

const?

> +       .attrs = enclosure_component_attrs,
> +       .is_visible = enclosure_component_visible,
> +};
> +__ATTRIBUTE_GROUPS(enclosure_component);
>
>   static int __init enclosure_init(void)
>   {
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
> index 1c630e2c2756..db9dff77e595 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -49,6 +49,20 @@ enum enclosure_component_setting {
>         ENCLOSURE_SETTING_BLINK_B_OFF_ON = 7,
>   };
>
> +enum enclosure_component_led {
> +       ENCLOSURE_LED_FAULT,
> +       ENCLOSURE_LED_ACTIVE,
> +       ENCLOSURE_LED_LOCATE,
> +       ENCLOSURE_LED_OK,
> +       ENCLOSURE_LED_REBUILD,
> +       ENCLOSURE_LED_PRDFAIL,
> +       ENCLOSURE_LED_HOTSPARE,
> +       ENCLOSURE_LED_ICA,
> +       ENCLOSURE_LED_IFA,
> +       ENCLOSURE_LED_DISABLED,
> +       ENCLOSURE_LED_MAX,

I didn't quite understand why this needs to be distinct from 'enum
enclosure_status'?

> +};
> +
>   struct enclosure_device;
>   struct enclosure_component;
>   struct enclosure_component_callbacks {
> @@ -72,6 +86,13 @@ struct enclosure_component_callbacks {
>         int (*set_locate)(struct enclosure_device *,
>                           struct enclosure_component *,
>                           enum enclosure_component_setting);
> +       void (*get_led)(struct enclosure_device *,
> +                       struct enclosure_component *,
> +                       enum enclosure_component_led);
> +       int (*set_led)(struct enclosure_device *,
> +                      struct enclosure_component *,
> +                      enum enclosure_component_led,
> +                      enum enclosure_component_setting);
>         void (*get_power_status)(struct enclosure_device *,
>                                  struct enclosure_component *);
>         int (*set_power_status)(struct enclosure_device *,
> @@ -80,7 +101,6 @@ struct enclosure_component_callbacks {
>         int (*show_id)(struct enclosure_device *, char *buf);
>   };
>
> -
>   struct enclosure_component {
>         void *scratch;
>         struct device cdev;
> @@ -90,6 +110,7 @@ struct enclosure_component {
>         int fault;
>         int active;
>         int locate;
> +       int led[ENCLOSURE_LED_MAX];
>         int slot;
>         enum enclosure_status status;
>         int power_status;
>
> ---------------------------------------------------------
> Patch to add LED driver:
> ---------------------------------------------------------
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 85ba901bc11b..e9c3b7f5236b 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -469,6 +469,17 @@ config HISI_HIKEY_USB
>           switching between the dual-role USB-C port and the USB-A host ports
>           using only one USB controller.
>
> +config PCIE_SSD_LEDS
> +       tristate "PCIe SSD status LED support"
> +       depends on ACPI && ENCLOSURE_SERVICES

Let's find a way to gracefully fall back to NPEM-only in the case of
CONFIG_ACPI=n builds. I.e. POWERPC platforms have NPEM but not ACPI.

> +       help
> +         Auxiliary driver for PCIe SSD status LED management as described in
> +         a PCI Firmware Specification, Revision 3.2 ECN.
> +
> +         When enabled, an enclosure device will be created for each device
> +         that hast has the ACPI _DSM method described in the referenced ECN,
> +         to allow control of LED states.
> +
>   source "drivers/misc/c2port/Kconfig"
>   source "drivers/misc/eeprom/Kconfig"
>   source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index a086197af544..8a8cb438a78c 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_UACCE)           += uacce/
>   obj-$(CONFIG_XILINX_SDFEC)    += xilinx_sdfec.o
>   obj-$(CONFIG_HISI_HIKEY_USB)  += hisi_hikey_usb.o
>   obj-$(CONFIG_HI6421V600_IRQ)  += hi6421v600-irq.o
> +obj-$(CONFIG_PCIE_SSD_LEDS)    += ssd-leds.o
> diff --git a/drivers/misc/ssd-leds.c b/drivers/misc/ssd-leds.c
> new file mode 100644
> index 000000000000..59927ebc8aa4
> --- /dev/null
> +++ b/drivers/misc/ssd-leds.c

I think this belongs in drivers/pci/ and / or drivers/acpi/. I.e the
enumeration code could enumerate either an ACPI auxdev or a PCIE
auxdev and register a separate driver accordingly with a shared NPEM
core for the common bits.

> @@ -0,0 +1,426 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Module to provide LED interfaces for PCIe SSD status LED states, 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.
> + *
> + * The "_DSM..." spec is functionally similar to Native PCIe Enclosure
> + * Management, but uses a _DSM ACPI method rather than a PCIe extended
> + * capability.
> + *
> + * Copyright (c) 2021 Dell Inc.
> + *
> + * TODO: Add NPEM support
> + *       Add pcieport & cxl support
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/enclosure.h>
> +#include <linux/auxiliary_bus.h>
> +
> +#define DRIVER_NAME    "ssd-leds"
> +#define DRIVER_VERSION "v1.0"
> +
> +/*
> + * NPEM & _DSM use the same state bits
> + */
> +#define        NPEM_STATE_OK           BIT(2)
> +#define        NPEM_STATE_LOCATE       BIT(3)
> +#define        NPEM_STATE_FAILED       BIT(4)
> +#define        NPEM_STATE_REBUILD      BIT(5)
> +#define        NPEM_STATE_PFA          BIT(6)  /* predicted failure analysis */
> +#define        NPEM_STATE_HOTSPARE     BIT(7)
> +#define        NPEM_STATE_ICA          BIT(8)  /* in a critical array */
> +#define        NPEM_STATE_IFA          BIT(9)  /* in a failed array */
> +#define        NPEM_STATE_INVALID      BIT(10)
> +#define        NPEM_STATE_DISABLED     BIT(11)
> +
> +static u32 to_npem_state[ENCLOSURE_LED_MAX];
> +
> +/*
> + * ssd_led_dev->dev could be the drive itself or its PCIe port
> + */
> +struct ssd_led_dev {
> +       struct list_head list;
> +       /* PCI device that has the LED controls */
> +       struct pci_dev *pdev;
> +       /* ops to read/set state (NPEM or _DSM) */
> +       struct drive_status_ops *ops;
> +       /* enclosure device used as sysfs interface for states */
> +       struct enclosure_device *edev;
> +       /* latest written states */
> +       u32 states;
> +       u32 supported_states;
> +};
> +
> +struct drive_status_ops {
> +       int (*get_supported_states)(struct ssd_led_dev *sldev);
> +       int (*get_current_states)(struct ssd_led_dev *sldev, u32 *states);
> +       int (*set_current_states)(struct ssd_led_dev *sldev, u32 states);
> +};
> +
> +static struct mutex drive_status_lock;
> +static struct list_head ssd_led_dev_list;
> +static int ssd_leds_exiting = 0;
> +
> +/*
> + * _DSM LED control
> + */
> +static const guid_t pcie_ssd_leds_dsm_guid =
> +       GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b,
> +                 0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d);
> +
> +#define GET_SUPPORTED_STATES_DSM       0x01
> +#define GET_STATE_DSM                  0x02
> +#define SET_STATE_DSM                  0x03
> +
> +struct ssdleds_dsm_output {
> +       u16 status;
> +       u8 function_specific_err;
> +       u8 vendor_specific_err;
> +       u32 state;
> +};
> +
> +static void dsm_status_err_print(struct pci_dev *pdev,
> +                                struct ssdleds_dsm_output *output)
> +{
> +       switch (output->status) {
> +       case 0:
> +               break;
> +       case 1:
> +               pci_dbg(pdev, "_DSM not supported\n");
> +               break;
> +       case 2:
> +               pci_dbg(pdev, "_DSM invalid input parameters\n");
> +               break;
> +       case 3:
> +               pci_dbg(pdev, "_DSM communication error\n");
> +               break;
> +       case 4:
> +               pci_dbg(pdev, "_DSM function-specific error 0x%x\n",
> +                       output->function_specific_err);
> +               break;
> +       case 5:
> +               pci_dbg(pdev, "_DSM vendor-specific error 0x%x\n",
> +                       output->vendor_specific_err);
> +               break;
> +       default:
> +               pci_dbg(pdev, "_DSM returned unknown status 0x%x\n",
> +                       output->status);
> +       }
> +}
> +
> +static int dsm_set(struct pci_dev *pdev, u32 value)
> +{
> +       acpi_handle handle;
> +       union acpi_object *out_obj, arg3[2];
> +       struct ssdleds_dsm_output *dsm_output;
> +
> +       handle = ACPI_HANDLE(&pdev->dev);
> +       if (!handle)
> +               return -ENODEV;
> +
> +       printk("dsm_set pdev %x:%x.%d value %x\n", pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), value);
> +
> +       arg3[0].type = ACPI_TYPE_PACKAGE;
> +       arg3[0].package.count = 1;
> +       arg3[0].package.elements = &arg3[1];
> +
> +       arg3[1].type = ACPI_TYPE_BUFFER;
> +       arg3[1].buffer.length = 4;
> +       arg3[1].buffer.pointer = (u8 *)&value;
> +
> +       out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssd_leds_dsm_guid,
> +                               1, SET_STATE_DSM, &arg3[0], ACPI_TYPE_BUFFER);
> +       if (!out_obj)
> +               return -EIO;
> +
> +       if (out_obj->buffer.length < 8) {
> +               ACPI_FREE(out_obj);
> +               return -EIO;
> +       }
> +
> +       dsm_output = (struct ssdleds_dsm_output *)out_obj->buffer.pointer;
> +
> +       if (dsm_output->status != 0) {
> +               dsm_status_err_print(pdev, dsm_output);
> +               ACPI_FREE(out_obj);
> +               return -EIO;
> +       }
> +       ACPI_FREE(out_obj);
> +       return 0;
> +}
> +
> +static int dsm_get(struct pci_dev *pdev, u64 dsm_func, u32 *output)
> +{
> +       acpi_handle handle;
> +       union acpi_object *out_obj;
> +       struct ssdleds_dsm_output *dsm_output;
> +
> +       handle = ACPI_HANDLE(&pdev->dev);
> +       if (!handle)
> +               return -ENODEV;
> +
> +       printk("dsm_get pdev %x:%x.%d\n", pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +
> +       out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssd_leds_dsm_guid, 0x1,
> +                                         dsm_func, NULL, ACPI_TYPE_BUFFER);
> +       if (!out_obj)
> +               return -EIO;
> +
> +       if (out_obj->buffer.length < 8) {
> +               ACPI_FREE(out_obj);
> +               return -EIO;
> +       }
> +
> +       dsm_output = (struct ssdleds_dsm_output *)out_obj->buffer.pointer;
> +       if (dsm_output->status != 0) {
> +               dsm_status_err_print(pdev, dsm_output);
> +               ACPI_FREE(out_obj);
> +               return -EIO;
> +       }
> +
> +       *output = dsm_output->state;
> +       ACPI_FREE(out_obj);
> +       printk("...got %x\n", *output);
> +       return 0;
> +}
> +
> +static int get_supported_states_dsm(struct ssd_led_dev *sldev)
> +{
> +       return dsm_get(sldev->pdev, GET_SUPPORTED_STATES_DSM, &sldev->supported_states);
> +}
> +
> +static int get_current_states_dsm(struct ssd_led_dev *sldev, u32 *states)
> +{
> +       return dsm_get(sldev->pdev, GET_STATE_DSM, states);
> +}
> +
> +static int set_current_states_dsm(struct ssd_led_dev *sldev, u32 states)
> +{
> +       return dsm_set(sldev->pdev, states);
> +}
> +
> +static bool pdev_has_dsm(struct pci_dev *pdev)
> +{
> +       acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> +
> +       if (!handle)
> +               return false;
> +
> +       return acpi_check_dsm(handle, &pcie_ssd_leds_dsm_guid, 0x1,
> +                             1 << GET_SUPPORTED_STATES_DSM ||
> +                             1 << GET_STATE_DSM ||
> +                             1 << SET_STATE_DSM);
> +}
> +
> +static struct drive_status_ops dsm_drive_status_ops = {
> +       .get_supported_states = get_supported_states_dsm,
> +       .get_current_states = get_current_states_dsm,
> +       .set_current_states = set_current_states_dsm,
> +};
> +
> +/*
> + * end of _DSM code
> + */
> +
> +static void ssd_leds_get_led(struct enclosure_device *edev,
> +                            struct enclosure_component *ecomp,
> +                            enum enclosure_component_led led)
> +{
> +       struct ssd_led_dev *sldev = ecomp->scratch;
> +       u32 states = 0;
> +
> +       sldev->ops->get_current_states(sldev, &states);
> +       ecomp->led[led] = !!(states & to_npem_state[led]) ?
> +               ENCLOSURE_SETTING_ENABLED : ENCLOSURE_SETTING_DISABLED;
> +}
> +
> +static int ssd_leds_set_led(struct enclosure_device *edev,
> +                        struct enclosure_component *ecomp,
> +                        enum enclosure_component_led led,
> +                        enum enclosure_component_setting val)
> +{
> +       struct ssd_led_dev *sldev = ecomp->scratch;
> +       u32 npem_state = to_npem_state[led], states;
> +       int rc;
> +
> +       if (val != ENCLOSURE_SETTING_ENABLED
> +           && val != ENCLOSURE_SETTING_DISABLED)
> +               return -EINVAL;
> +
> +       states = sldev->states & ~npem_state;
> +       states |= val == ENCLOSURE_SETTING_ENABLED ? npem_state : 0;
> +
> +       if ((states & sldev->supported_states) != states) {
> +               printk("can't set state %x\n", states);
> +               return -EINVAL;
> +       }
> +
> +       rc = sldev->ops->set_current_states(sldev, states);
> +       /*
> +        * save last written state so it doesn't have to be re-read via NPEM/
> +        * _DSM on the next write, not only because it is faster, but because
> +        * _DSM doesn't provide a way to know if the last write has completed
> +        */
> +       if (rc == 0)
> +               sldev->states = states;
> +       return rc;
> +}
> +
> +static struct enclosure_component_callbacks ssdleds_cb = {
> +       .get_led        = ssd_leds_get_led,
> +       .set_led        = ssd_leds_set_led,
> +};
> +
> +static struct ssd_led_dev *to_ssd_led_dev(struct pci_dev *pdev)
> +{
> +       struct ssd_led_dev *sldev;
> +
> +       list_for_each_entry(sldev, &ssd_led_dev_list, list)
> +               if (pdev == sldev->pdev)
> +                       return sldev;
> +       return NULL;
> +}
> +
> +static void remove_ssd_led_dev(struct ssd_led_dev *sldev)
> +{
> +       enclosure_unregister(sldev->edev);
> +       list_del(&sldev->list);
> +       kfree(sldev);
> +}
> +
> +static int add_ssd_led_dev(struct pci_dev *pdev,
> +                               const char *name,
> +                               struct drive_status_ops *ops)
> +{
> +       struct ssd_led_dev *sldev;
> +       struct enclosure_device *edev;
> +       struct enclosure_component *ecomp;
> +       int rc = 0;
> +
> +       mutex_lock(&drive_status_lock);
> +       if (ssd_leds_exiting)
> +               goto out_unlock;
> +
> +       sldev = kzalloc(sizeof(*sldev), GFP_KERNEL);
> +       if (!sldev) {
> +               rc = -ENOMEM;
> +               goto out_unlock;
> +       }
> +       sldev->pdev = pdev;
> +       sldev->ops = ops;
> +       sldev->states = 0;
> +       INIT_LIST_HEAD(&sldev->list);
> +       if (sldev->ops->get_supported_states(sldev) != 0)
> +               goto out_free;
> +
> +       edev = enclosure_register(&pdev->dev, name, 1, &ssdleds_cb);
> +       if (!edev)
> +               goto out_free;
> +
> +       ecomp = enclosure_component_alloc(edev, 0, ENCLOSURE_COMPONENT_DEVICE, "leds");
> +       if (IS_ERR(ecomp))
> +               goto out_unreg;
> +
> +       ecomp->type = ENCLOSURE_COMPONENT_ARRAY_DEVICE;
> +       rc = enclosure_component_register(ecomp);
> +       if (rc < 0)
> +               goto out_unreg;
> +
> +       ecomp->scratch = sldev;
> +       sldev->edev = edev;
> +       list_add_tail(&sldev->list, &ssd_led_dev_list);

Why not just store sldev in the in the auxdev driver-data. I don't see
a reason for ssd_led_dev_list to exist.

> +       goto out_unlock;
> +
> +out_unreg:
> +       enclosure_unregister(edev);
> +out_free:
> +       kfree(sldev);
> +out_unlock:
> +       mutex_unlock(&drive_status_lock);
> +       return rc;
> +}
> +
> +static int ssd_leds_probe(struct auxiliary_device *adev,
> +                         const struct auxiliary_device_id *id)
> +{
> +       struct pci_dev *pdev = to_pci_dev(adev->dev.parent);
> +
> +       if (pdev_has_dsm(pdev))

This is likely an argument for separate drivers for the NPEM and ACPI
_DSM case, because it should never be the case that the code gets this
far and this check returns false. If this is false the auxdev
shouldn't have been registered in the first instance.

> +               return add_ssd_led_dev(pdev, dev_name(&adev->dev), &dsm_drive_status_ops);
> +       return 0;
> +}
> +
> +static void ssd_leds_remove(struct auxiliary_device *adev)
> +{
> +       struct pci_dev *pdev = to_pci_dev(adev->dev.parent);
> +       struct ssd_led_dev *sldev;
> +
> +       mutex_lock(&drive_status_lock);
> +       sldev = to_ssd_led_dev(pdev);
> +       if (sldev)
> +               remove_ssd_led_dev(sldev);
> +out_unlock:
> +       mutex_unlock(&drive_status_lock);
> +}
> +
> +static const struct auxiliary_device_id ssd_leds_id_table[] = {
> +       { .name = "nvme.ssd_leds", },
> +//     { .name = "pcieport.ssd_leds", },
> +//     { .name = "cxl.ssd_leds", },
> +       {},
> +};
> +
> +MODULE_DEVICE_TABLE(auxiliary, ssd_leds_id_table);
> +
> +static struct auxiliary_driver ssd_leds_driver = {
> +       .name = "ssd_leds",
> +       .probe = ssd_leds_probe,
> +       .remove = ssd_leds_remove,
> +       .id_table = ssd_leds_id_table,
> +};
> +
> +static int __init ssd_leds_init(void)
> +{
> +       mutex_init(&drive_status_lock);
> +       INIT_LIST_HEAD(&ssd_led_dev_list);
> +
> +       to_npem_state[ENCLOSURE_LED_FAULT] = NPEM_STATE_FAILED;
> +       to_npem_state[ENCLOSURE_LED_ACTIVE] = 0;
> +       to_npem_state[ENCLOSURE_LED_LOCATE] = NPEM_STATE_LOCATE;
> +       to_npem_state[ENCLOSURE_LED_OK] = NPEM_STATE_OK;
> +       to_npem_state[ENCLOSURE_LED_REBUILD] = NPEM_STATE_REBUILD;
> +       to_npem_state[ENCLOSURE_LED_PRDFAIL] = NPEM_STATE_PFA;
> +       to_npem_state[ENCLOSURE_LED_HOTSPARE] = NPEM_STATE_HOTSPARE;
> +       to_npem_state[ENCLOSURE_LED_ICA] = NPEM_STATE_ICA;
> +       to_npem_state[ENCLOSURE_LED_IFA] = NPEM_STATE_IFA;
> +       to_npem_state[ENCLOSURE_LED_DISABLED] = NPEM_STATE_DISABLED;
> +
> +       auxiliary_driver_register(&ssd_leds_driver);
> +       return 0;
> +}
> +
> +static void __exit ssd_leds_exit(void)
> +{
> +       struct ssd_led_dev *sldev, *temp;
> +
> +       mutex_lock(&drive_status_lock);
> +       ssd_leds_exiting = 1;
> +       list_for_each_entry_safe(sldev, temp, &ssd_led_dev_list, list)
> +               remove_ssd_led_dev(sldev);
> +       mutex_unlock(&drive_status_lock);
> +       auxiliary_driver_unregister(&ssd_leds_driver);
> +}
> +
> +module_init(ssd_leds_init);
> +module_exit(ssd_leds_exit);
> +
> +MODULE_AUTHOR("Stuart Hayes <stuart.w.hayes@gmail.com>");
> +MODULE_DESCRIPTION("Support for PCIe SSD Status LEDs");
> +MODULE_LICENSE("GPL");

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

[James Bottomley]

On Fri, 2021-11-05 at 19:52 -0700, Dan Williams wrote:
> [ add James and Martin in case they see anything here that collides
> with the SES driver ]

It's a bit difficult to tell among all the quoting.  However, I will
say that if you want to play with drivers/misc/enclosure.c you need at
least to cc the maintainer and preferably linux-scsi because they're
currently the only consumer ... and the enterprise does a lot with ses.

James

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

[Krzysztof Wilczyński]

Hi Stuart,

[...]
> I've reworked the code so it is an auxiliary driver (to nvme, and
> hopefully cxl and pcieport, but I haven't added that yet), and so it
> registers as an enclosure (instead of using the LED subsystem).

Would you be able to post this new reworded driver as either a new
version or a completely new series, or even as an RFC?  It would help
to get a fresh perspective and also make it easier to review it.

	Krzysztof