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

["Williams, Dan J" <dan.j.williams@intel.com>]

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");