[PATCH v2] Add support for PCIe SSD status LED management

[PATCH v2] Add support for PCIe SSD status LED management

[Stuart Hayes]

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 a single (led_classdev) LED for any PCIe device that has the
relevant _DSM--presumably only drives or drive slots will have this. The
available and active status states are exposed using attribute "states"
under the LED device. Reading this attribute will show the states supported
by the interface, and those states which are currently active are shown
in brackets, like this:

 # echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
 # cat /sys/class/leds/0000:88:00.0::drive_status/states
 [ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled

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

 .../sysfs-class-led-driver-pcie-ssd-leds      |  18 +
 drivers/pci/Kconfig                           |  12 +
 drivers/pci/Makefile                          |   1 +
 drivers/pci/pcie-ssd-leds.c                   | 457 ++++++++++++++++++
 4 files changed, 488 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
 create mode 100644 drivers/pci/pcie-ssd-leds.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds b/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
new file mode 100644
index 000000000000..1f07733b6f35
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
@@ -0,0 +1,18 @@
+What:		/sys/class/leds/<led>/states
+Date:		April 2021
+Contact:	linux-pci@vger.kernel.org
+Description:
+		This attribute indicates the status states supported by a drive
+		or drive slot's LEDs, 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, and in "Native PCIe
+		Enclosure Management", section 6.29 of the PCIe Base Spec 5.0.
+
+		Only supported states will be shown, and the currently active
+		states are shown in brackets.  The active state(s) can be written
+		by echoing a space or comma separated string of states to this
+		attribute.  For example:
+
+		# echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
+		# cat /sys/class/leds/0000:88:00.0::drive_status/states
+		[ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 0c473d75e625..f4acf1ad0fb5 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -190,6 +190,18 @@ config PCI_HYPERV
 	  The PCI device frontend driver allows the kernel to import arbitrary
 	  PCI devices from a PCI backend to support PCI driver domains.
 
+config PCIE_SSD_LEDS
+	tristate "PCIe SSD status LED support"
+	depends on ACPI && NEW_LEDS
+	help
+	  Driver for PCIe SSD status LED management as described in a PCI
+	  Firmware Specification, Revision 3.2 ECN.
+
+	  When enabled, an LED interface will be created for each PCIe device
+	  that has the ACPI method described in the referenced specification,
+	  to allow the device status LEDs for that PCIe device (presumably a
+	  solid state storage device or its slot) to be seen and controlled.
+
 choice
 	prompt "PCI Express hierarchy optimization setting"
 	default PCIE_BUS_DEFAULT
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index d62c4ac4ae1b..ea0a0f351ad2 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_PCI_PF_STUB)	+= pci-pf-stub.o
 obj-$(CONFIG_PCI_ECAM)		+= ecam.o
 obj-$(CONFIG_PCI_P2PDMA)	+= p2pdma.o
 obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
+obj-$(CONFIG_PCIE_SSD_LEDS)	+= pcie-ssd-leds.o
 
 # Endpoint library must be initialized before its users
 obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
diff --git a/drivers/pci/pcie-ssd-leds.c b/drivers/pci/pcie-ssd-leds.c
new file mode 100644
index 000000000000..4a78ff598e09
--- /dev/null
+++ b/drivers/pci/pcie-ssd-leds.c
@@ -0,0 +1,457 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Module to provide LED interface control 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.
+ *
+ * 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;
+	u32 mask;
+};
+
+static struct led_state led_states[] = {
+	{ .name = "ok",			.mask = BIT(2) },
+	{ .name = "locate",		.mask = BIT(3) },
+	{ .name = "failed",		.mask = BIT(4) },
+	{ .name = "rebuild",		.mask = BIT(5) },
+	{ .name = "pfa",		.mask = BIT(6) },
+	{ .name = "hotspare",		.mask = BIT(7) },
+	{ .name = "ica",		.mask = BIT(8) },
+	{ .name = "ifa",		.mask = BIT(9) },
+	{ .name = "invalid",		.mask = BIT(10) },
+	{ .name = "disabled",		.mask = BIT(11) },
+};
+
+struct drive_status_led_ops {
+	int (*get_supported_states)(struct device *dev, u32 *states);
+	int (*get_current_states)(struct device *dev, u32 *states);
+	int (*set_current_states)(struct device *dev, u32 states);
+};
+
+/*
+ * one drive_status_dev struct for each drive/slot device with status LEDs
+ */
+struct drive_status_dev {
+	struct list_head drive_list;
+	struct device *dev;
+	struct drive_status_led_ops *ops;
+	u32 supported_states;
+	struct led_classdev led_cdev;
+	enum led_brightness brightness;
+};
+
+struct mutex drive_list_lock;
+struct list_head drive_list;
+
+/*
+ * code specific to _DSM method
+ */
+const guid_t pcie_ssdleds_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 device *dev,
+			     struct ssdleds_dsm_output *output)
+{
+	switch (output->status) {
+	case 0:
+		break;
+	case 1:
+		dev_dbg(dev, "_DSM not supported\n");
+		break;
+	case 2:
+		dev_dbg(dev, "_DSM invalid input parameters\n");
+		break;
+	case 3:
+		dev_dbg(dev, "_DSM communication error\n");
+		break;
+	case 4:
+		dev_dbg(dev, "_DSM function-specific error 0x%x\n",
+			output->function_specific_err);
+		break;
+	case 5:
+		dev_dbg(dev, "_DSM vendor-specific error 0x%x\n",
+			output->vendor_specific_err);
+		break;
+	default:
+		dev_dbg(dev, "_DSM returned unknown status 0x%x\n",
+			output->status);
+	}
+}
+
+static int dsm_set(struct device *dev, u32 value)
+{
+	acpi_handle handle;
+	union acpi_object *out_obj, arg3[2];
+	struct ssdleds_dsm_output *dsm_output;
+
+	handle = ACPI_HANDLE(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_ssdleds_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(dev, dsm_output);
+		ACPI_FREE(out_obj);
+		return -EIO;
+	}
+	ACPI_FREE(out_obj);
+	return 0;
+}
+
+static int dsm_get(struct device *dev, u64 dsm_func, u32 *output)
+{
+	acpi_handle handle;
+	union acpi_object *out_obj;
+	struct ssdleds_dsm_output *dsm_output;
+
+	handle = ACPI_HANDLE(dev);
+	if (!handle)
+		return -ENODEV;
+
+	out_obj = acpi_evaluate_dsm_typed(handle, &pcie_ssdleds_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(dev, 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 device *dev, u32 *states)
+{
+	return dsm_get(dev, GET_SUPPORTED_STATES_DSM, states);
+}
+
+static int get_current_states_dsm(struct device *dev, u32 *states)
+{
+	return dsm_get(dev, GET_STATE_DSM, states);
+}
+
+static int set_current_states_dsm(struct device *dev, u32 states)
+{
+	return dsm_set(dev, 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_ssdleds_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 ssize_t states_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct drive_status_dev *dsdev;
+	u32 val;
+	int err, i, res = 0;
+
+	dsdev = container_of(led_cdev, struct drive_status_dev, led_cdev);
+
+	err = dsdev->ops->get_current_states(dsdev->dev, &val);
+	if (err < 0)
+		return err;
+
+	for (i = 0; i < ARRAY_SIZE(led_states); i++) {
+		if (led_states[i].mask & dsdev->supported_states & val)
+			res += sysfs_emit_at(buf, res, "[%s] ",
+					     led_states[i].name);
+		else if (led_states[i].mask & dsdev->supported_states)
+			res += sysfs_emit_at(buf, res, "%s",
+					     led_states[i].name);
+	}
+	res += sysfs_emit_at(buf, res, "\n");
+	return res;
+}
+
+static ssize_t states_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct drive_status_dev *dsdev;
+	u32 states = 0;
+	int err;
+
+	/*
+	 * parse input
+	 *
+	 * If multiple states are being set, they should be separated by
+	 * spaces or commas.  Input buffer may end with `\n`.
+	 */
+	while (*buf) {
+		size_t len;
+		int i;
+
+		while (*buf == ' ' || *buf == ',' || *buf == '\n')
+			buf++;
+		len = strcspn(buf, " ,\n");
+		if (!len)
+			break;
+		for (i = 0; i < ARRAY_SIZE(led_states); i++) {
+			if (!strncmp(buf, led_states[i].name, len)) {
+				states |= led_states[i].mask;
+				buf += len;
+				break;
+			}
+		}
+		if (i == ARRAY_SIZE(led_states))
+			return -EINVAL;
+	}
+
+	/*
+	 * set states
+	 */
+	dsdev = container_of(led_cdev, struct drive_status_dev, led_cdev);
+	if (states & ~dsdev->supported_states)
+		return -EINVAL;
+	if (states)
+		dsdev->brightness = LED_ON;
+
+	err = dsdev->ops->set_current_states(dsdev->dev, states);
+	if (err < 0)
+		return err;
+
+	return size;
+}
+static DEVICE_ATTR_RW(states);
+
+static struct attribute *drive_status_attrs[] = {
+	&dev_attr_states.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(drive_status);
+
+static int drive_status_set_brightness(struct led_classdev *led_cdev,
+				  enum led_brightness brightness)
+{
+	struct drive_status_dev *dsdev;
+	int err;
+
+	dsdev = container_of(led_cdev, struct drive_status_dev, led_cdev);
+	dsdev->brightness = brightness;
+
+	if (brightness == LED_OFF) {
+		err = dsdev->ops->set_current_states(dsdev->dev, 0);
+		if (err < 0)
+			return err;
+	}
+	return 0;
+}
+
+static enum led_brightness drive_status_get_brightness(struct led_classdev *led_cdev)
+{
+	struct drive_status_dev *dsdev;
+
+	dsdev = container_of(led_cdev, struct drive_status_dev, led_cdev);
+	return dsdev->brightness;
+}
+
+static struct drive_status_dev *to_drive_status_dev(struct device *dev)
+{
+	struct drive_status_dev *dsdev;
+
+	mutex_lock(&drive_list_lock);
+	list_for_each_entry(dsdev, &drive_list, drive_list)
+		if (dev == dsdev->dev) {
+			mutex_unlock(&drive_list_lock);
+			return dsdev;
+		}
+	mutex_unlock(&drive_list_lock);
+	return NULL;
+}
+
+static void remove_drive_status_dev(struct drive_status_dev *dsdev)
+{
+	if (dsdev) {
+		mutex_lock(&drive_list_lock);
+		list_del(&dsdev->drive_list);
+		mutex_unlock(&drive_list_lock);
+		led_classdev_unregister(&dsdev->led_cdev);
+		kfree(dsdev);
+	}
+}
+
+static void add_drive_status_dev(struct device *dev,
+				 struct drive_status_led_ops *ops)
+{
+	u32 supported_states;
+	int ret;
+	struct drive_status_dev *dsdev;
+	char name[LED_MAX_NAME_SIZE];
+
+	if (to_drive_status_dev(dev))
+		/*
+		 * led has already been added for this dev
+		 */
+		return;
+
+	if (ops->get_supported_states(dev, &supported_states) < 0)
+		return;
+
+	dsdev = kzalloc(sizeof(*dsdev), GFP_KERNEL);
+	if (!dsdev)
+		return;
+
+	dsdev->dev = dev;
+	dsdev->ops = ops;
+	dsdev->supported_states = supported_states;
+	dsdev->brightness = LED_ON;
+	snprintf(name, sizeof(name), "%s::%s",
+		 dev_name(dev), "drive_status");
+
+	dsdev->led_cdev.name = name;
+	dsdev->led_cdev.max_brightness = LED_ON;
+	dsdev->led_cdev.brightness_set_blocking = drive_status_set_brightness;
+	dsdev->led_cdev.brightness_get = drive_status_get_brightness;
+	dsdev->led_cdev.groups = drive_status_groups;
+	ret = led_classdev_register(dev, &dsdev->led_cdev);
+	if (ret) {
+		pr_warn("Failed to register LED %s\n", dsdev->led_cdev.name);
+		remove_drive_status_dev(dsdev);
+		return;
+	}
+
+	mutex_lock(&drive_list_lock);
+	list_add_tail(&dsdev->drive_list, &drive_list);
+	mutex_unlock(&drive_list_lock);
+}
+
+/*
+ * code specific to PCIe devices
+ */
+static void probe_pdev(struct pci_dev *pdev)
+{
+	if (pdev_has_dsm(pdev))
+		add_drive_status_dev(&pdev->dev, &dsm_drive_status_led_ops);
+}
+
+static int pciessdleds_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->dev));
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block pciessdleds_pci_bus_nb = {
+	.notifier_call = pciessdleds_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 pciessdleds_init(void)
+{
+	mutex_init(&drive_list_lock);
+	INIT_LIST_HEAD(&drive_list);
+
+	bus_register_notifier(&pci_bus_type, &pciessdleds_pci_bus_nb);
+	initial_scan_for_leds();
+	return 0;
+}
+
+static void __exit pciessdleds_exit(void)
+{
+	struct drive_status_dev *dsdev, *temp;
+
+	bus_unregister_notifier(&pci_bus_type, &pciessdleds_pci_bus_nb);
+	list_for_each_entry_safe(dsdev, temp, &drive_list, drive_list)
+		remove_drive_status_dev(dsdev);
+}
+
+module_init(pciessdleds_init);
+module_exit(pciessdleds_exit);
+
+MODULE_AUTHOR("Stuart Hayes <stuart.w.hayes@gmail.com>");
+MODULE_DESCRIPTION("Support for PCIe SSD Status LED Management _DSM");
+MODULE_LICENSE("GPL");
-- 
2.27.0

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

[Randy Dunlap]

Hi,

On 6/1/21 1:38 PM, 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 a single (led_classdev) LED for any PCIe device that has the
> relevant _DSM--presumably only drives or drive slots will have this. The
> available and active status states are exposed using attribute "states"
> under the LED device. Reading this attribute will show the states supported
> by the interface, and those states which are currently active are shown
> in brackets, like this:
> 
>  # echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
>  # cat /sys/class/leds/0000:88:00.0::drive_status/states
>  [ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
> 
> 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
> 
>  .../sysfs-class-led-driver-pcie-ssd-leds      |  18 +
>  drivers/pci/Kconfig                           |  12 +
>  drivers/pci/Makefile                          |   1 +
>  drivers/pci/pcie-ssd-leds.c                   | 457 ++++++++++++++++++
>  4 files changed, 488 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
>  create mode 100644 drivers/pci/pcie-ssd-leds.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds b/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
> new file mode 100644
> index 000000000000..1f07733b6f35
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
> @@ -0,0 +1,18 @@
> +What:		/sys/class/leds/<led>/states
> +Date:		April 2021
> +Contact:	linux-pci@vger.kernel.org
> +Description:
> +		This attribute indicates the status states supported by a drive
> +		or drive slot's LEDs, 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, and in "Native PCIe
> +		Enclosure Management", section 6.29 of the PCIe Base Spec 5.0.
> +
> +		Only supported states will be shown, and the currently active
> +		states are shown in brackets.  The active state(s) can be written
> +		by echoing a space or comma separated string of states to this
> +		attribute.  For example:
> +
> +		# echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
> +		# cat /sys/class/leds/0000:88:00.0::drive_status/states
> +		[ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 0c473d75e625..f4acf1ad0fb5 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -190,6 +190,18 @@ config PCI_HYPERV
>  	  The PCI device frontend driver allows the kernel to import arbitrary
>  	  PCI devices from a PCI backend to support PCI driver domains.
>  
> +config PCIE_SSD_LEDS
> +	tristate "PCIe SSD status LED support"
> +	depends on ACPI && NEW_LEDS

I expect that should be LEDS_CLASS instead of NEW_LEDS.
Did you test it with NEW_LEDS=y and LEDS_CLASS not set?


[adding Pavel and linux-leds m.l. for other review]

> +	help
> +	  Driver for PCIe SSD status LED management as described in a PCI
> +	  Firmware Specification, Revision 3.2 ECN.
> +
> +	  When enabled, an LED interface will be created for each PCIe device
> +	  that has the ACPI method described in the referenced specification,
> +	  to allow the device status LEDs for that PCIe device (presumably a
> +	  solid state storage device or its slot) to be seen and controlled.
> +
>  choice
>  	prompt "PCI Express hierarchy optimization setting"
>  	default PCIE_BUS_DEFAULT

thanks.
-- 
~Randy

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

[Pavel Machek]

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

Hi!

> >  # echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
> >  # cat /sys/class/leds/0000:88:00.0::drive_status/states

This has two problems: ":" already has special meaning in LED name,
and we'd prefer not to have new "states" interface unless absolutely
neccessary.

> >  [ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled

So what does this do? Turns the LED on if driver is in "ok" or
"locate" states?

> > +Date:		April 2021
> > +Contact:	linux-pci@vger.kernel.org
> > +Description:
> > +		This attribute indicates the status states supported by a drive
> > +		or drive slot's LEDs, 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, and in "Native PCIe
> > +		Enclosure Management", section 6.29 of the PCIe Base Spec 5.0.
> > +
> > +		Only supported states will be shown, and the currently active
> > +		states are shown in brackets.  The active state(s) can be written
> > +		by echoing a space or comma separated string of states to this
> > +		attribute.  For example:
> > +
> > +		# echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
> > +		# cat /sys/class/leds/0000:88:00.0::drive_status/states
> > +		[ok] [locate] failed rebuild pfa hotspare ica ifa

This goes against "one value per file", really needs better
description, and probably needs different interface.

> > +config PCIE_SSD_LEDS
> > +	tristate "PCIe SSD status LED support"
> > +	depends on ACPI && NEW_LEDS
> 
> I expect that should be LEDS_CLASS instead of NEW_LEDS.
> Did you test it with NEW_LEDS=y and LEDS_CLASS not set?
> 
> 
> [adding Pavel and linux-leds m.l. for other review]

Thank you!
							Pavel
							
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[stuart hayes]

On 6/1/2021 5:38 PM, Pavel Machek wrote:
> Hi!
> 
>>>   # echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
>>>   # cat /sys/class/leds/0000:88:00.0::drive_status/states
> 
> This has two problems: ":" already has special meaning in LED name,
> and we'd prefer not to have new "states" interface unless absolutely
> neccessary.
> 

Regarding the ":"s in the LED name: I didn't specify the name 
"0000:88:00.0"--that's just the name of the parent device of the 
led_classdev, so it used that by default.  I'm not really sure what to 
do here.  I could check the device name and replace the ":" with 
something else like "_".  Would it break anything to have extra ":" 
characters in the name?  If so, maybe led_classdev_register() should 
check for ":" in the name and rename it somehow, like it does when it 
finds duplicate names?

As for the "states" interface:  I don't disagree that it isn't ideal to 
have a new "states" interface, but I have struggled to come up with 
anything better.  I guess the only alternative to having a new attribute 
or attributes would be to create up to ten LED classdev devices--one for 
each state supported by the device.  I did seriously consider this, and 
even started to do it that way, but I decided not to for several 
reasons:  (1) it would clutter /sys/class/leds, (2) it would require 
reads and writes to multiple files to change the states, (3) it would be 
more cumbersome for the driver, and (4) it would be a bit slower, 
because, if, say, ledmon wanted to set certain states, it would probably 
just write to the brightness attribute of each of the LEDs, which would 
cause the _DSM would get run for each write, instead of just once.  And 
the _DSM runs IPMI commands, at least on my system, so it is slow...

But, I'm definitely OK rewriting this to register one led_classdev for 
each possible state if that's the better way to do this.


>>>   [ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
> 
> So what does this do? Turns the LED on if driver is in "ok" or
> "locate" states?
> 

This would cause the system to somehow show the user that that this 
drive (or drive slot) is the one that it wants a person to be able to 
physically locate (possibly by flashing a blue LED on/near the drive), 
and also that the drive is OK.  It would presumably do that by lighting 
the LEDs on/near the drive with certain colors and/or flashing patterns, 
though it could, in theory, put "OK" in an LCD on the drive slot.  How 
the states are displayed to the user is beyond the scope of the specs.

(The _DSM and NPEM specs provide for a way to control status LEDs on a 
drive or drive slot.  Typically drives will have 2 or 3 LEDs that are 
illuminated in different colors or flashing patterns to indicate various 
states (like those listed in IBPI / SFF-8489), though the _DSM / NPEM 
specs do not specify how the states are displayed.)


>>> +Date:		April 2021
>>> +Contact:	linux-pci@vger.kernel.org
>>> +Description:
>>> +		This attribute indicates the status states supported by a drive
>>> +		or drive slot's LEDs, 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, and in "Native PCIe
>>> +		Enclosure Management", section 6.29 of the PCIe Base Spec 5.0.
>>> +
>>> +		Only supported states will be shown, and the currently active
>>> +		states are shown in brackets.  The active state(s) can be written
>>> +		by echoing a space or comma separated string of states to this
>>> +		attribute.  For example:
>>> +
>>> +		# echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
>>> +		# cat /sys/class/leds/0000:88:00.0::drive_status/states
>>> +		[ok] [locate] failed rebuild pfa hotspare ica ifa
> 
> This goes against "one value per file", really needs better
> description, and probably needs different interface.
> 

This is very similar to how the available I/O schedulers and currently 
selected I/O scheduler is displayed in (for example) 
/sys/block/sda/queue/scheduler.  The only difference is that more than 
one state can be active on the LEDs, while only a single I/O scheduler 
can be selected at a time.

Both Bjorn Helgaas and Krzysztof Wilczyński had suggested the 
scheduler-type interface, so I went with that.  In an earlier attempt at 
this driver, when Bjorn suggested this, he asked if that would violate 
the "one value per file" rule, and Greg K-H responded "That's a valid 
way of displaying options for a sysfs file that can be specific unique 
values."

(I'm not devoted to this interface, but I can't think of anything 
better.  I initially had this as simply a number, with the states 
defined in the _DSM/NPEM specs, but Bjorn suggested that was a pain to 
interpret, especially because the PCI specs aren't public.  My second 
try, I used a really verbose output copied from acpi's 
"debug_layer"--that still used numbers, but defined them... I didn't 
much like it, nor did several others who responded, which is why I went 
with this interface.)

>>> +config PCIE_SSD_LEDS
>>> +	tristate "PCIe SSD status LED support"
>>> +	depends on ACPI && NEW_LEDS
>>
>> I expect that should be LEDS_CLASS instead of NEW_LEDS.
>> Did you test it with NEW_LEDS=y and LEDS_CLASS not set?
>>
>>
>> [adding Pavel and linux-leds m.l. for other review]
> 
> Thank you!
> 							Pavel
> 							
> 

Thanks!

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

[Pavel Machek]

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

Hi!

> >>>  [ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
> >
> >So what does this do? Turns the LED on if driver is in "ok" or
> >"locate" states?
> >
> 
> This would cause the system to somehow show the user that that this drive
> (or drive slot) is the one that it wants a person to be able to physically
> locate (possibly by flashing a blue LED on/near the drive), and also that
> the drive is OK.  It would presumably do that by lighting the LEDs on/near
> the drive with certain colors and/or flashing patterns, though it could, in
> theory, put "OK" in an LCD on the drive slot.  How the states are displayed
> to the user is beyond the scope of the specs.
> 
> (The _DSM and NPEM specs provide for a way to control status LEDs on a drive
> or drive slot.  Typically drives will have 2 or 3 LEDs that are illuminated
> in different colors or flashing patterns to indicate various states (like
> those listed in IBPI / SFF-8489), though the _DSM / NPEM specs do not
> specify how the states are displayed.)

Well, LED subsystem expects to know how many LEDs are there and what
the LEDs are, and expects individual control over them.

"2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
of LED subsystem, so this should be independend.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[stuart hayes]

On 6/2/2021 4:05 AM, Pavel Machek wrote:
> Hi!
> 
>>>>>   [ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
>>>
>>> So what does this do? Turns the LED on if driver is in "ok" or
>>> "locate" states?
>>>
>>
>> This would cause the system to somehow show the user that that this drive
>> (or drive slot) is the one that it wants a person to be able to physically
>> locate (possibly by flashing a blue LED on/near the drive), and also that
>> the drive is OK.  It would presumably do that by lighting the LEDs on/near
>> the drive with certain colors and/or flashing patterns, though it could, in
>> theory, put "OK" in an LCD on the drive slot.  How the states are displayed
>> to the user is beyond the scope of the specs.
>>
>> (The _DSM and NPEM specs provide for a way to control status LEDs on a drive
>> or drive slot.  Typically drives will have 2 or 3 LEDs that are illuminated
>> in different colors or flashing patterns to indicate various states (like
>> those listed in IBPI / SFF-8489), though the _DSM / NPEM specs do not
>> specify how the states are displayed.)
> 
> Well, LED subsystem expects to know how many LEDs are there and what
> the LEDs are, and expects individual control over them.
> 
> "2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
> of LED subsystem, so this should be independend.
> 
> Best regards,
> 								Pavel
> 

Yes... I had originally submitted this without using the LED subsystem, 
and Greg K-H said I had to (see 
https://www.spinics.net/lists/linux-pci/msg102013.html).  Here's the 
relevant bit.

(Greg K-H:)
>> > And why isn't this code using the existing LED subsystem?  Don't create
>> > random new driver-specific sysfs files that do things the existing class
>> > drivers do please.
>> >
>> 
(Me:)
>> I did consider the LED subsystem, but I don't think it is a great match.
>> >> In spite of the name, this _DSM doesn't directly control individual 
LEDs--it
>> sets the state(s) of the PCI port to which an SSD may be connected, so that
>> it may be conveyed to the user... a processor (or at least some logic) will
>> decide how to show which states are active, probably by setting combinations
>> of LEDs to certain colors or blink patterns, or possibly on some other type
>> of display.
(Greg K-H:)
> If you are allowing LEDs to be controlled by the user, then yes, you
> have to use the LED subsystem as you should never try to create a brand
> new driver-specific user/kernel API just for your tiny driver right?
> Please work with the subsystems we have, they are unified for a reason.

So I'm not sure what to do.

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

[Pavel Machek]

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

Hi!

> > > This would cause the system to somehow show the user that that this drive
> > > (or drive slot) is the one that it wants a person to be able to physically
> > > locate (possibly by flashing a blue LED on/near the drive), and also that
> > > the drive is OK.  It would presumably do that by lighting the LEDs on/near
> > > the drive with certain colors and/or flashing patterns, though it could, in
> > > theory, put "OK" in an LCD on the drive slot.  How the states are displayed
> > > to the user is beyond the scope of the specs.
> > > 
> > > (The _DSM and NPEM specs provide for a way to control status LEDs on a drive
> > > or drive slot.  Typically drives will have 2 or 3 LEDs that are illuminated
> > > in different colors or flashing patterns to indicate various states (like
> > > those listed in IBPI / SFF-8489), though the _DSM / NPEM specs do not
> > > specify how the states are displayed.)
> > 
> > Well, LED subsystem expects to know how many LEDs are there and what
> > the LEDs are, and expects individual control over them.
> > 
> > "2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
> > of LED subsystem, so this should be independend.

> 
> Yes... I had originally submitted this without using the LED subsystem, and
> Greg K-H said I had to (see
> https://www.spinics.net/lists/linux-pci/msg102013.html).  Here's the
> relevant bit.

...

> So I'm not sure what to do.

Explain to Greg that you don't know how particular system implements
the indication. It can be small display, 2 LEDs, 3 LEDs, etc... LED
subsystem expects direct LED control.

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

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

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

[stuart hayes]

On 6/2/2021 5:40 PM, Pavel Machek wrote:
> Hi!
> 
>>>> This would cause the system to somehow show the user that that this drive
>>>> (or drive slot) is the one that it wants a person to be able to physically
>>>> locate (possibly by flashing a blue LED on/near the drive), and also that
>>>> the drive is OK.  It would presumably do that by lighting the LEDs on/near
>>>> the drive with certain colors and/or flashing patterns, though it could, in
>>>> theory, put "OK" in an LCD on the drive slot.  How the states are displayed
>>>> to the user is beyond the scope of the specs.
>>>>
>>>> (The _DSM and NPEM specs provide for a way to control status LEDs on a drive
>>>> or drive slot.  Typically drives will have 2 or 3 LEDs that are illuminated
>>>> in different colors or flashing patterns to indicate various states (like
>>>> those listed in IBPI / SFF-8489), though the _DSM / NPEM specs do not
>>>> specify how the states are displayed.)
>>>
>>> Well, LED subsystem expects to know how many LEDs are there and what
>>> the LEDs are, and expects individual control over them.
>>>
>>> "2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
>>> of LED subsystem, so this should be independend.
> 
>>
>> Yes... I had originally submitted this without using the LED subsystem, and
>> Greg K-H said I had to (see
>> https://www.spinics.net/lists/linux-pci/msg102013.html).  Here's the
>> relevant bit.
> 
> ...
> 
>> So I'm not sure what to do.
> 
> Explain to Greg that you don't know how particular system implements
> the indication. It can be small display, 2 LEDs, 3 LEDs, etc... LED
> subsystem expects direct LED control.
> 
> Best regards,
> 								Pavel
> 

Is there any chance you would accept drive status LEDs using the LED 
subsystem, if the driver was rewritten to represent each possible drive 
state as a single led_classdev device, each with no special attributes, 
just brightness of 0 or 1?

The drive states are logically each displaying one bit of information to 
the user, and each could actually be physically represented by a 
physical LED (though probably not).  I only combined them into a single 
led_classdev device with an extra attribute because I thought it made 
more sense to do so, but it sounds like I might have made a bad choice 
in doing so.

In support of using the LED subsystem for this--there are other LEDs in 
linux which aren't necessarily always represented by single physical 
LEDs, such as "caps lock" for keyboards.  Virtual consoles display 
keyboard indicators as text in a window rather than an LED--and someone 
_could_ make a weird keyboard that displays the 3 bits of 
caps/scroll/number lock info as a decimal number on a display.  My point 
is that it seems to me that the LED subsystem is already being used for 
things that logically display a bit of information to the user, which 
may or may not be represented by a single LED.  (Ethernet port LEDs are 
another possible example--they might combine link present, link speed, 
and activity all on one LED, by using different colors and flashing 
patterns.)

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

[Marek Behún]

On Tue, 1 Jun 2021 22:18:16 -0500
stuart hayes <stuart.w.hayes@gmail.com> wrote:

> Both Bjorn Helgaas and Krzysztof Wilczyński had suggested the 
> scheduler-type interface, so I went with that.  In an earlier attempt
> at this driver, when Bjorn suggested this, he asked if that would
> violate the "one value per file" rule, and Greg K-H responded "That's
> a valid way of displaying options for a sysfs file that can be
> specific unique values."

But you are not displaying unique values. Your example is

# echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
# cat /sys/class/leds/0000:88:00.0::drive_status/states
[ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled

so there are 2 values set (ok and locate). Unique means that only one
can be set.

Question: can this LED be configured by userspace? I mean: can you
configure whether the LED should be on/off, disregarding the SSD state?

I ask because the LED subsystem currently officially does not
support LEDs for which brightness cannot be set by userspace...

If yes, you should implement the .brightness_set() function. (Could you
please also send your patch to the linux-leds mailing list?)

Then you should implement a LED-private trigger for this LED, which,
when enabled, will make the LED follow the SSD state.

The sysfs ABI should probably look like this:

# cd /sys/class/leds/<SSD_LED>
# echo 1 >brightness		# to light the LED on
# echo 0 >brightness		# to light the LED off
# echo ssd_state >trigger	# to make the LED follow SSD states
# ls ssd_state			# list available SSD states
ok  locate  failed  rebuild ...
# cat ssd_state/ok		# check if "ok" state is enabled
0
# echo 1 >ssd_state/ok		# enable "ok" state so that the
				# LED will be on when SSD is in "ok"
				# state
# echo none >trigger		# put the LED back into SW mode

(The name of the trigger does not necessarily have to be "ssd_state".
 Other people should give their opinions about the name.)

Marek

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

[Randy Dunlap]

On 6/1/21 2:20 PM, Randy Dunlap wrote:
> Hi,
> 
> On 6/1/21 1:38 PM, 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 a single (led_classdev) LED for any PCIe device that has the
>> relevant _DSM--presumably only drives or drive slots will have this. The
>> available and active status states are exposed using attribute "states"
>> under the LED device. Reading this attribute will show the states supported
>> by the interface, and those states which are currently active are shown
>> in brackets, like this:
>>
>>  # echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
>>  # cat /sys/class/leds/0000:88:00.0::drive_status/states
>>  [ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
>>
>> 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
>>
>>  .../sysfs-class-led-driver-pcie-ssd-leds      |  18 +
>>  drivers/pci/Kconfig                           |  12 +
>>  drivers/pci/Makefile                          |   1 +
>>  drivers/pci/pcie-ssd-leds.c                   | 457 ++++++++++++++++++
>>  4 files changed, 488 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
>>  create mode 100644 drivers/pci/pcie-ssd-leds.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds b/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
>> new file mode 100644
>> index 000000000000..1f07733b6f35
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-pcie-ssd-leds
>> @@ -0,0 +1,18 @@
>> +What:		/sys/class/leds/<led>/states
>> +Date:		April 2021
>> +Contact:	linux-pci@vger.kernel.org
>> +Description:
>> +		This attribute indicates the status states supported by a drive
>> +		or drive slot's LEDs, 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, and in "Native PCIe
>> +		Enclosure Management", section 6.29 of the PCIe Base Spec 5.0.
>> +
>> +		Only supported states will be shown, and the currently active
>> +		states are shown in brackets.  The active state(s) can be written
>> +		by echoing a space or comma separated string of states to this
>> +		attribute.  For example:
>> +
>> +		# echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
>> +		# cat /sys/class/leds/0000:88:00.0::drive_status/states
>> +		[ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index 0c473d75e625..f4acf1ad0fb5 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -190,6 +190,18 @@ config PCI_HYPERV
>>  	  The PCI device frontend driver allows the kernel to import arbitrary
>>  	  PCI devices from a PCI backend to support PCI driver domains.
>>  
>> +config PCIE_SSD_LEDS
>> +	tristate "PCIe SSD status LED support"
>> +	depends on ACPI && NEW_LEDS
> 
> I expect that should be LEDS_CLASS instead of NEW_LEDS.
> Did you test it with NEW_LEDS=y and LEDS_CLASS not set?
> 

ERROR: modpost: "led_classdev_register_ext" [drivers/pci/pcie-ssd-leds.ko] undefined!
ERROR: modpost: "led_classdev_unregister" [drivers/pci/pcie-ssd-leds.ko] undefined!

Yes, just change it to depends on LEDS_CLASS instead.

> 
> [adding Pavel and linux-leds m.l. for other review]
> 
>> +	help
>> +	  Driver for PCIe SSD status LED management as described in a PCI
>> +	  Firmware Specification, Revision 3.2 ECN.
>> +
>> +	  When enabled, an LED interface will be created for each PCIe device
>> +	  that has the ACPI method described in the referenced specification,
>> +	  to allow the device status LEDs for that PCIe device (presumably a
>> +	  solid state storage device or its slot) to be seen and controlled.
>> +
>>  choice
>>  	prompt "PCI Express hierarchy optimization setting"
>>  	default PCIE_BUS_DEFAULT
> 
> thanks.
> 


-- 
~Randy

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

[kernel test robot]

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

Hi Stuart,

I love your patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on v5.13-rc4 next-20210601]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Stuart-Hayes/Add-support-for-PCIe-SSD-status-LED-management/20210602-044006
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: ia64-randconfig-r013-20210602 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/a18fb85f5775efb60fa2f1bb6473eeafe1bf722a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Stuart-Hayes/Add-support-for-PCIe-SSD-status-LED-management/20210602-044006
        git checkout a18fb85f5775efb60fa2f1bb6473eeafe1bf722a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ia64-linux-ld: drivers/pci/pcie-ssd-leds.o: in function `remove_drive_status_dev.part.0':
>> pcie-ssd-leds.c:(.text+0x872): undefined reference to `led_classdev_unregister'
   ia64-linux-ld: drivers/pci/pcie-ssd-leds.o: in function `probe_pdev':
>> pcie-ssd-leds.c:(.text+0xd92): undefined reference to `led_classdev_register_ext'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33853 bytes --]

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

[Lukas Wunner]

On Tue, Jun 01, 2021 at 04:38:20PM -0400, Stuart Hayes wrote:
> --- /dev/null
> +++ b/drivers/pci/pcie-ssd-leds.c

Since this is a PCIe-specific feature, it should probably live in the
pcie/ subdirectory.  Or in drivers/nvme/.


> +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_ssdleds_dsm_guid, 0x1,
> +			      1 << GET_SUPPORTED_STATES_DSM ||
> +			      1 << GET_STATE_DSM ||
> +			      1 << SET_STATE_DSM);
> +}

Would it be possible to bail out early if pdev->class !=
PCI_CLASS_STORAGE_EXPRESS (or something like that), thus avoiding
the overhead of an ACPI namespace search for *every* PCI device?

Thanks,

Lukas