| Message ID | 20210813213653.3760-1-stuart.w.hayes@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v3] Add support for PCIe SSD status LED management | expand |
On Fri, Aug 13, 2021 at 05:36:53PM -0400, Stuart Hayes wrote: > +struct mutex drive_status_dev_list_lock; > +struct list_head drive_status_dev_list; Should be declared static. > +const guid_t pcie_ssd_leds_dsm_guid = > + GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b, > + 0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d); Same. > +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, > +}; Same. > +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); > +} Why is &dsm_drive_status_led_ops passed to add_drive_status_dev()? It's always the same argument. > +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; > +} There's a concurrency issue here: initial_scan_for_leds() uses bus_for_each_dev(), which walks the bus's klist_devices. When a device is added (e.g. hotplugged), that device gets added to the tail of klist_devices. (See call to klist_add_tail() in bus_add_device().) It is thus possible that probe_pdev() is run concurrently for the same device, once by the notifier and once by initial_scan_for_leds(). The problem is that add_drive_status_dev() only checks at the top of the function whether the device has already been added to drive_status_dev_list. It goes on to instantiate the LED and only then adds the device to drive_status_dev_list. It's thus possible that the same LED is instantiated twice which might confuse users. Thanks, Lukas
Hi! > 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. > > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> I believe these are not LEDs, right? This is something that displays information to the user, but how exactly it is implemented is up to BIOS vendor. I don't think it is good fit for LED subsystem. Best regards, Pavel
On 8/14/2021 1:23 AM, Lukas Wunner wrote: > On Fri, Aug 13, 2021 at 05:36:53PM -0400, Stuart Hayes wrote: >> +struct mutex drive_status_dev_list_lock; >> +struct list_head drive_status_dev_list; > > Should be declared static. > >> +const guid_t pcie_ssd_leds_dsm_guid = >> + GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b, >> + 0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d); > > Same. > >> +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, >> +}; > > Same. > Thank you! >> +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); >> +} > > Why is &dsm_drive_status_led_ops passed to add_drive_status_dev()? > It's always the same argument. > Because I hope this will also support NPEM as well, since it is so similar except for using a PCIe extended capability instead of a _DSM method. This will make it very easy to add the support... I just don't have any NPEM hardware yet. >> +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; >> +} > > There's a concurrency issue here: initial_scan_for_leds() uses > bus_for_each_dev(), which walks the bus's klist_devices. When a > device is added (e.g. hotplugged), that device gets added to the > tail of klist_devices. (See call to klist_add_tail() in > bus_add_device().) > > It is thus possible that probe_pdev() is run concurrently for the > same device, once by the notifier and once by initial_scan_for_leds(). > > The problem is that add_drive_status_dev() only checks at the top > of the function whether the device has already been added to > drive_status_dev_list. It goes on to instantiate the LED > and only then adds the device to drive_status_dev_list. > > It's thus possible that the same LED is instantiated twice > which might confuse users. > I missed that, thanks!
On 8/18/2021 2:05 AM, Pavel Machek wrote: > Hi! > >> 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. >> >> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> > > I believe these are not LEDs, right? This is something that displays > information to the user, but how exactly it is implemented is up to > BIOS vendor. > > I don't think it is good fit for LED subsystem. > > Best regards, > Pavel > I think it very likely these will be LEDs on most, if not all, systems (likely enough that the PCI ECN has "LEDs" in the name)... they have been LEDs on every system I've seen. I would suspect that systems which support more than one or two of the states won't have a 1-to-1 mapping of logical LEDs to physical LEDs, though, but might instead use something like IBPI (SFF-8489) to use fewer LEDs and be easier to read at a glance. I'm not sure I understand why the LED subsystem would be that much worse a fit for this than for keyboard (or other) indicator LEDs. I understand that many other indicators are more likely to have each single kernel LED mapped to a single physical LED, but functionally they are both kernel LEDs controlling on/off indicators which are likely displayed on LEDs that are always visible on the system.
On Mon, 2021-10-04 at 12:41 -0500, stuart hayes wrote: > > > On 8/14/2021 1:23 AM, Lukas Wunner wrote: > > On Fri, Aug 13, 2021 at 05:36:53PM -0400, Stuart Hayes wrote: > > > +struct mutex drive_status_dev_list_lock; > > > +struct list_head drive_status_dev_list; > > > > Should be declared static. > > > > > +const guid_t pcie_ssd_leds_dsm_guid = > > > + GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b, > > > + 0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d); > > > > Same. > > > > > +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, > > > +}; > > > > Same. > > > > Thank you! > > > > +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); > > > +} > > > > Why is &dsm_drive_status_led_ops passed to add_drive_status_dev()? > > It's always the same argument. > > > > Because I hope this will also support NPEM as well, since it is so > similar except for using a PCIe extended capability instead of a _DSM > method. This will make it very easy to add the support... I just don't > have any NPEM hardware yet. I'm interested in helping the infrastructure along so it can be reused with the CXL memory expander driver. I expect that like most subsystems PCI does not accept enabling without a user. I.e. the NPEM operations need to be included to prove out the infrastructure is suitable to support both LED mechanisms.
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");
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"); >
[ 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.
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
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
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");
[ 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");
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
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
On Fri, Aug 13, 2021 at 05:36:53PM -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. I think r3.3 has been released, so we can cite that now. > 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. To be OCD, we should tighten up the "ports to which they are connected" part (see details below). > + * drive_status_dev->dev could be the drive itself or its PCIe port I assume you mean "drive_status_dev->pdev" (not "->dev")? "... drive itself or its PCIe port" is ambiguous because switches and endpoints both have Ports. It sounds like this refers to either the endpoint or the switch downstream port leading to it, which would match the PCI Firmware spec language about the _DSM being "under the ACPI object representing the embedded PCIe device/function or under the ACPI PCIe description representing the add-in PCI Express slot." > +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[]; > +}; I think this would be easier to read if you can fit the comments to the right on the same line, e.g., struct pci_dev *pdev; /* dev with controls */ > +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"); pci_dbg() often goes nowhere. Seems like these might be pci_info()? > +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; Looks currently equivalent to simply "return err" (or "return led->dsdev->ops->set_current_states(...)"). Is there a case where the ops might return something positive? > +} > + > +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; > + } Even though the four-line body is technically a single statement, I think braces around those lines would improve readability. > + mutex_unlock(&drive_status_dev_list_lock); > + return NULL; > +} > + > +static void remove_drive_status_dev(struct drive_status_dev *dsdev) > +{ > + if (dsdev) { > + int i; if (!dsdev) return; so you can unindent the usual path. > + > + 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 s/leds/LEDs/ to match other usage. > + */ > + 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)); pci_warn(). Maybe include the led_states[i].name? > + 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) I don't see this restriction in the spec. Did I miss it? > + 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); > +} > + > +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(); This is the only way to deal with (a) making this a module that can be loaded later and (b) handling hot-added devices, so I see why this is here. But it's ugly. IMO, for_each_pci_dev() is a symptom of something that should have been done during enumeration, but wasn't. > + 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"); > -- > 2.27.0 >
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 + help + Driver for PCIe SSD status LED management as described in a PCI + Firmware Specification, Revision 3.2 ECN. + + 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 }, +}; + +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); +} + +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");
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. 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