linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
@ 2020-11-10 15:37 Stuart Hayes
  2020-11-11  7:05 ` Christoph Hellwig
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Stuart Hayes @ 2020-11-10 15:37 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, Stuart Hayes

This patch will expose the PCIe SSD Status LED Management interface in
sysfs for devices that have the relevant _DSM method, per the "_DSM
additions for PCIe SSD Status LED Management" ECN to the PCI Firmware
Specification revision 3.2.

The interface is exposed in two sysfs files, ssdleds_supported_states (RO)
and ssdleds_current_state (RW).

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---

v2: made PCI_SSDLEDS dependent on PCI and ACPI
    remove unused variable
    warn if call to sysfs_create_group fails
    include header file with function prototypes
    change logical OR to bitwise

---
 Documentation/ABI/testing/sysfs-bus-pci |  14 ++
 drivers/pci/Kconfig                     |   7 +
 drivers/pci/Makefile                    |   1 +
 drivers/pci/pci-ssdleds.c               | 194 ++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c                 |   1 +
 drivers/pci/pci.h                       |  11 ++
 6 files changed, 228 insertions(+)
 create mode 100644 drivers/pci/pci-ssdleds.c

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 77ad9ec3c801..18ca73b764ac 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -366,3 +366,17 @@ Contact:	Heiner Kallweit <hkallweit1@gmail.com>
 Description:	If ASPM is supported for an endpoint, these files can be
 		used to disable or enable the individual power management
 		states. Write y/1/on to enable, n/0/off to disable.
+
+What:		/sys/bus/pci/devices/.../ssdleds_supported_states
+Date:		October 2020
+Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
+Description:	If the device supports the ACPI _DSM method to control the
+		PCIe SSD LED states, ssdleds_supported_states (read only)
+		will show the LED states that are supported by the _DSM.
+
+What:		/sys/bus/pci/devices/.../ssdleds_current_state
+Date:		October 2020
+Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
+Description:	If the device supports the ACPI _DSM method to control the
+		PCIe SSD LED states, ssdleds_current_state will show or set
+		the current LED states that are active.
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 0c473d75e625..48049866145f 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -182,6 +182,13 @@ config PCI_LABEL
 	def_bool y if (DMI || ACPI)
 	select NLS
 
+config PCI_SSDLEDS
+	def_bool y if (ACPI && PCI)
+	depends on PCI && ACPI
+	help
+	  Enables userspace access to PCIe SSD LED management interface via
+	  sysfs.
+
 config PCI_HYPERV
 	tristate "Hyper-V PCI Frontend"
 	depends on X86_64 && HYPERV && PCI_MSI && PCI_MSI_IRQ_DOMAIN && SYSFS
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 522d2b974e91..75d3d5a3b1ed 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_PCI_ATS)		+= ats.o
 obj-$(CONFIG_PCI_IOV)		+= iov.o
 obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
 obj-$(CONFIG_PCI_LABEL)		+= pci-label.o
+obj-$(CONFIG_PCI_SSDLEDS)	+= pci-ssdleds.o
 obj-$(CONFIG_X86_INTEL_MID)	+= pci-mid.o
 obj-$(CONFIG_PCI_SYSCALL)	+= syscall.o
 obj-$(CONFIG_PCI_STUB)		+= pci-stub.o
diff --git a/drivers/pci/pci-ssdleds.c b/drivers/pci/pci-ssdleds.c
new file mode 100644
index 000000000000..d35e4f3caa71
--- /dev/null
+++ b/drivers/pci/pci-ssdleds.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Provide userspace access to PCIe SSD Status LED Management
+ *
+ * The PCIe spec defines an ACPI _DSM method to allow PCIe SSD status LED
+ * management (see "_DSM additions for PCIe SSD Status LED Management" ECN,
+ * February 12, 2020). This code provides a sysfs interface to this _DSM.
+ *
+ * Copyright (C) 2020 Dell Inc.
+ *
+ */
+
+#include <linux/sysfs.h>
+#include <linux/pci.h>
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include "pci.h"
+
+const guid_t pci_ssdleds_dsm_guid =
+	GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b,
+		  0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d);
+
+#define SSDLEDS_GET_SUPPORTED_STATES_DSM	0x01
+#define SSDLEDS_GET_STATE_DSM			0x02
+#define SSDLEDS_SET_STATE_DSM			0x03
+
+struct pci_ssdleds_dsm_output {
+	u16 status;
+	u8 function_specific_err;
+	u8 vendor_specific_err;
+	u32 state;
+};
+
+static int ssdleds_dsm_set(struct device *dev, const char *buf, u64 dsm_func)
+{
+	acpi_handle handle;
+	union acpi_object *out_obj, arg3[2];
+	struct pci_ssdleds_dsm_output *dsm_output;
+	u32 val;
+	int err;
+
+	handle = ACPI_HANDLE(dev);
+	if (!handle)
+		return -ENODEV;
+
+	err = kstrtou32(buf, 0, &val);
+	if (err || val > U32_MAX)
+		return -EINVAL;
+
+	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 *)&val;
+
+	out_obj = acpi_evaluate_dsm_typed(handle, &pci_ssdleds_dsm_guid, 0x1,
+				      dsm_func, &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 pci_ssdleds_dsm_output *)out_obj->buffer.pointer;
+	/*
+	 * Ignore function specific error bit 1 (some LED state bits weren't
+	 * set), since write was done. User can read current state to see which
+	 * bits were set.
+	 */
+	if (dsm_output->status != 0 &&
+	    !(dsm_output->status == 4 && dsm_output->function_specific_err == 1)) {
+		ACPI_FREE(out_obj);
+		return -EIO;
+	}
+
+	ACPI_FREE(out_obj);
+
+	return 0;
+}
+
+static int ssdleds_dsm_get(struct device *dev, char *buf, u64 dsm_func)
+{
+	acpi_handle handle;
+	union acpi_object *out_obj;
+	struct pci_ssdleds_dsm_output *dsm_output;
+
+	handle = ACPI_HANDLE(dev);
+	if (!handle)
+		return -ENODEV;
+
+	out_obj = acpi_evaluate_dsm_typed(handle, &pci_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 pci_ssdleds_dsm_output *)out_obj->buffer.pointer;
+	if (dsm_output->status != 0) {
+		ACPI_FREE(out_obj);
+		return -EIO;
+	}
+
+	scnprintf(buf, PAGE_SIZE, "%#x\n", dsm_output->state);
+
+	ACPI_FREE(out_obj);
+
+	return strlen(buf) > 0 ? strlen(buf) : -EIO;
+}
+
+static bool device_has_dsm(struct device *dev)
+{
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(dev);
+	if (!handle)
+		return false;
+
+	return !!acpi_check_dsm(handle, &pci_ssdleds_dsm_guid, 0x1,
+		       1 << SSDLEDS_GET_SUPPORTED_STATES_DSM |
+		       1 << SSDLEDS_GET_STATE_DSM |
+		       1 << SSDLEDS_SET_STATE_DSM);
+}
+
+static ssize_t ssdleds_current_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	int ret;
+
+	ret = ssdleds_dsm_set(dev, buf, SSDLEDS_SET_STATE_DSM);
+	return (ret < 0) ? ret : count;
+}
+
+static ssize_t ssdleds_current_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	return ssdleds_dsm_get(dev, buf, SSDLEDS_GET_STATE_DSM);
+}
+
+static ssize_t ssdleds_supported_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	return ssdleds_dsm_get(dev, buf, SSDLEDS_GET_SUPPORTED_STATES_DSM);
+}
+
+static umode_t ssdleds_attr_visible(struct kobject *kobj,
+				    struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	umode_t mode = attr->mode;
+
+	return device_has_dsm(dev) ? mode : 0;
+}
+
+static struct device_attribute ssdleds_attr_current = {
+	.attr = {.name = "ssdleds_current_state", .mode = 0644},
+	.show = ssdleds_current_show,
+	.store = ssdleds_current_store,
+};
+
+static struct device_attribute ssdleds_attr_supported = {
+	.attr = {.name = "ssdleds_supported_states", .mode = 0444},
+	.show = ssdleds_supported_show,
+};
+
+static struct attribute *ssdleds_attributes[] = {
+	&ssdleds_attr_current.attr,
+	&ssdleds_attr_supported.attr,
+	NULL,
+};
+
+static const struct attribute_group ssdleds_attr_group = {
+	.attrs = ssdleds_attributes,
+	.is_visible = ssdleds_attr_visible,
+};
+
+void pci_create_ssdleds_files(struct pci_dev *pdev)
+{
+	if (sysfs_create_group(&pdev->dev.kobj, &ssdleds_attr_group))
+		pci_warn(pdev, "Unable to create ssdleds attributes\n");
+}
+
+void pci_remove_ssdleds_files(struct pci_dev *pdev)
+{
+	sysfs_remove_group(&pdev->dev.kobj, &ssdleds_attr_group);
+}
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d15c881e2e7e..820f32956971 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1377,6 +1377,7 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
 		goto err_rom_file;
 
 	pci_create_firmware_label_files(pdev);
+	pci_create_ssdleds_files(pdev);
 
 	return 0;
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f86cae9aa1f4..8e6883a1b701 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -30,6 +30,17 @@ static inline void pci_remove_firmware_label_files(struct pci_dev *pdev)
 void pci_create_firmware_label_files(struct pci_dev *pdev);
 void pci_remove_firmware_label_files(struct pci_dev *pdev);
 #endif
+
+#if !defined(CONFIG_PCI_SSDLEDS)
+static inline void pci_create_ssdleds_files(struct pci_dev *pdev)
+{ return; }
+static inline void pci_remove_ssdleds_files(struct pci_dev *pdev)
+{ return; }
+#else
+void pci_create_ssdleds_files(struct pci_dev *pdev);
+void pci_remove_ssdleds_files(struct pci_dev *pdev);
+#endif
+
 void pci_cleanup_rom(struct pci_dev *dev);
 
 enum pci_mmap_api {
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
  2020-11-10 15:37 [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs Stuart Hayes
@ 2020-11-11  7:05 ` Christoph Hellwig
  2020-11-12  4:48   ` Stuart Hayes
  2020-11-13 14:14 ` Dan Carpenter
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2020-11-11  7:05 UTC (permalink / raw)
  To: Stuart Hayes; +Cc: Bjorn Helgaas, linux-pci

On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote:
> +Date:		October 2020
> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> +Description:	If the device supports the ACPI _DSM method to control the
> +		PCIe SSD LED states, ssdleds_supported_states (read only)
> +		will show the LED states that are supported by the _DSM.
> +
> +What:		/sys/bus/pci/devices/.../ssdleds_current_state
> +Date:		October 2020
> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> +Description:	If the device supports the ACPI _DSM method to control the
> +		PCIe SSD LED states, ssdleds_current_state will show or set
> +		the current LED states that are active.

Is the supported file really required?  Doesn't the current_state one
also show which LEDs exist?

> +config PCI_SSDLEDS
> +	def_bool y if (ACPI && PCI)
> +	depends on PCI && ACPI

We really should not default new code to y.

> +	if (dsm_output->status != 0 &&
> +	    !(dsm_output->status == 4 && dsm_output->function_specific_err == 1)) {

overly longline.  But to make this a little more obvious, maybe you
want to

 a) switch on dsm_output->status
 b) add symbolic names for the magic numbers

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
  2020-11-11  7:05 ` Christoph Hellwig
@ 2020-11-12  4:48   ` Stuart Hayes
  0 siblings, 0 replies; 9+ messages in thread
From: Stuart Hayes @ 2020-11-12  4:48 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Bjorn Helgaas, linux-pci

On 11/11/2020 1:05 AM, Christoph Hellwig wrote:
 > On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote:
 >> +Date:              October 2020
 >> +Contact:   Stuart Hayes <stuart.w.hayes@gmail.com>
 >> +Description:       If the device supports the ACPI _DSM method to
control the
 >> +           PCIe SSD LED states, ssdleds_supported_states (read only)
 >> +           will show the LED states that are supported by the _DSM.
 >> +
 >> +What:              /sys/bus/pci/devices/.../ssdleds_current_state
 >> +Date:              October 2020
 >> +Contact:   Stuart Hayes <stuart.w.hayes@gmail.com>
 >> +Description:       If the device supports the ACPI _DSM method to
control the
 >> +           PCIe SSD LED states, ssdleds_current_state will show or set
 >> +           the current LED states that are active.
 >
 > Is the supported file really required?  Doesn't the current_state one
 > also show which LEDs exist?
 >

The current_state just shows which LED states are currently active, not
which
are supported by the system.  I guess you could try to set all the states in
current_state and read it back to see which ones went on to see which states
are supported, but I'm not 100% if that would work, plus it might result in
the drive LEDs flashing in a strange way briefly.

 >> +config PCI_SSDLEDS
 >> +   def_bool y if (ACPI && PCI)
 >> +   depends on PCI && ACPI
 >
 > We really should not default new code to y.
 >

Good point, I agree.

 >> +   if (dsm_output->status != 0 &&
 >> +       !(dsm_output->status == 4 && dsm_output->function_specific_err
== 1)) {
 >
 > overly longline.  But to make this a little more obvious, maybe you
 > want to
 >
 >   a) switch on dsm_output->status
 >   b) add symbolic names for the magic numbers
 >

Good feedback, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
  2020-11-10 15:37 [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs Stuart Hayes
  2020-11-11  7:05 ` Christoph Hellwig
@ 2020-11-13 14:14 ` Dan Carpenter
  2020-11-13 21:38 ` Bjorn Helgaas
  2020-11-15  6:38 ` Krzysztof Wilczyński
  3 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2020-11-13 14:14 UTC (permalink / raw)
  To: kbuild, Stuart Hayes, Bjorn Helgaas
  Cc: lkp, Dan Carpenter, kbuild-all, linux-pci, Stuart Hayes

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

Hi Stuart,

url:    https://github.com/0day-ci/linux/commits/Stuart-Hayes/Expose-PCIe-SSD-Status-LED-Management-DSM-in-sysfs/20201111-004557
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-m021-20201111 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

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

smatch warnings:
drivers/pci/pci-ssdleds.c:47 ssdleds_dsm_set() warn: impossible condition '(val > (~0)) => (0-u32max > u32max)'

vim +47 drivers/pci/pci-ssdleds.c

e6eac5bf04a7aca Stuart Hayes 2020-11-10  34  static int ssdleds_dsm_set(struct device *dev, const char *buf, u64 dsm_func)
e6eac5bf04a7aca Stuart Hayes 2020-11-10  35  {
e6eac5bf04a7aca Stuart Hayes 2020-11-10  36  	acpi_handle handle;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  37  	union acpi_object *out_obj, arg3[2];
e6eac5bf04a7aca Stuart Hayes 2020-11-10  38  	struct pci_ssdleds_dsm_output *dsm_output;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  39  	u32 val;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  40  	int err;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  41  
e6eac5bf04a7aca Stuart Hayes 2020-11-10  42  	handle = ACPI_HANDLE(dev);
e6eac5bf04a7aca Stuart Hayes 2020-11-10  43  	if (!handle)
e6eac5bf04a7aca Stuart Hayes 2020-11-10  44  		return -ENODEV;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  45  
e6eac5bf04a7aca Stuart Hayes 2020-11-10  46  	err = kstrtou32(buf, 0, &val);
e6eac5bf04a7aca Stuart Hayes 2020-11-10 @47  	if (err || val > U32_MAX)
                                                           ^^^^^^^^^^^^^
This is not required.  Just "if (err) return err;"'

e6eac5bf04a7aca Stuart Hayes 2020-11-10  48  		return -EINVAL;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  49  
e6eac5bf04a7aca Stuart Hayes 2020-11-10  50  	arg3[0].type = ACPI_TYPE_PACKAGE;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  51  	arg3[0].package.count = 1;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  52  	arg3[0].package.elements = &arg3[1];
e6eac5bf04a7aca Stuart Hayes 2020-11-10  53  
e6eac5bf04a7aca Stuart Hayes 2020-11-10  54  	arg3[1].type = ACPI_TYPE_BUFFER;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  55  	arg3[1].buffer.length = 4;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  56  	arg3[1].buffer.pointer = (u8 *)&val;
e6eac5bf04a7aca Stuart Hayes 2020-11-10  57  
e6eac5bf04a7aca Stuart Hayes 2020-11-10  58  	out_obj = acpi_evaluate_dsm_typed(handle, &pci_ssdleds_dsm_guid, 0x1,

---
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: 35051 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
  2020-11-10 15:37 [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs Stuart Hayes
  2020-11-11  7:05 ` Christoph Hellwig
  2020-11-13 14:14 ` Dan Carpenter
@ 2020-11-13 21:38 ` Bjorn Helgaas
  2020-11-13 23:19   ` Greg Kroah-Hartman
  2020-11-15  6:38 ` Krzysztof Wilczyński
  3 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2020-11-13 21:38 UTC (permalink / raw)
  To: Stuart Hayes
  Cc: Bjorn Helgaas, linux-pci, Christoph Hellwig, Dan Carpenter,
	Greg Kroah-Hartman

[+cc Christoph, Dan, Greg (for "one value per file" question below)]

On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote:
> This patch will expose the PCIe SSD Status LED Management interface in
> sysfs for devices that have the relevant _DSM method, per the "_DSM
> additions for PCIe SSD Status LED Management" ECN to the PCI Firmware
> Specification revision 3.2.
> 
> The interface is exposed in two sysfs files, ssdleds_supported_states (RO)
> and ssdleds_current_state (RW).
> 
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> ---
> 
> v2: made PCI_SSDLEDS dependent on PCI and ACPI
>     remove unused variable
>     warn if call to sysfs_create_group fails
>     include header file with function prototypes
>     change logical OR to bitwise
> 
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  14 ++
>  drivers/pci/Kconfig                     |   7 +
>  drivers/pci/Makefile                    |   1 +
>  drivers/pci/pci-ssdleds.c               | 194 ++++++++++++++++++++++++
>  drivers/pci/pci-sysfs.c                 |   1 +
>  drivers/pci/pci.h                       |  11 ++
>  6 files changed, 228 insertions(+)
>  create mode 100644 drivers/pci/pci-ssdleds.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 77ad9ec3c801..18ca73b764ac 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -366,3 +366,17 @@ Contact:	Heiner Kallweit <hkallweit1@gmail.com>
>  Description:	If ASPM is supported for an endpoint, these files can be
>  		used to disable or enable the individual power management
>  		states. Write y/1/on to enable, n/0/off to disable.
> +
> +What:		/sys/bus/pci/devices/.../ssdleds_supported_states
> +Date:		October 2020
> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> +Description:	If the device supports the ACPI _DSM method to control the
> +		PCIe SSD LED states, ssdleds_supported_states (read only)
> +		will show the LED states that are supported by the _DSM.
> +
> +What:		/sys/bus/pci/devices/.../ssdleds_current_state
> +Date:		October 2020
> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> +Description:	If the device supports the ACPI _DSM method to control the
> +		PCIe SSD LED states, ssdleds_current_state will show or set
> +		the current LED states that are active.
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 0c473d75e625..48049866145f 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -182,6 +182,13 @@ config PCI_LABEL
>  	def_bool y if (DMI || ACPI)
>  	select NLS
>  
> +config PCI_SSDLEDS
> +	def_bool y if (ACPI && PCI)
> +	depends on PCI && ACPI
> +	help
> +	  Enables userspace access to PCIe SSD LED management interface via
> +	  sysfs.
> +
>  config PCI_HYPERV
>  	tristate "Hyper-V PCI Frontend"
>  	depends on X86_64 && HYPERV && PCI_MSI && PCI_MSI_IRQ_DOMAIN && SYSFS
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 522d2b974e91..75d3d5a3b1ed 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_PCI_ATS)		+= ats.o
>  obj-$(CONFIG_PCI_IOV)		+= iov.o
>  obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
>  obj-$(CONFIG_PCI_LABEL)		+= pci-label.o
> +obj-$(CONFIG_PCI_SSDLEDS)	+= pci-ssdleds.o
>  obj-$(CONFIG_X86_INTEL_MID)	+= pci-mid.o
>  obj-$(CONFIG_PCI_SYSCALL)	+= syscall.o
>  obj-$(CONFIG_PCI_STUB)		+= pci-stub.o
> diff --git a/drivers/pci/pci-ssdleds.c b/drivers/pci/pci-ssdleds.c
> new file mode 100644
> index 000000000000..d35e4f3caa71
> --- /dev/null
> +++ b/drivers/pci/pci-ssdleds.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Provide userspace access to PCIe SSD Status LED Management
> + *
> + * The PCIe spec defines an ACPI _DSM method to allow PCIe SSD status LED

s/PCIe spec/PCI Firmware spec/

> + * management (see "_DSM additions for PCIe SSD Status LED Management" ECN,
> + * February 12, 2020). This code provides a sysfs interface to this _DSM.
> + *
> + * Copyright (C) 2020 Dell Inc.
> + *

Unnecessary blank line.

> + */
> +
> +#include <linux/sysfs.h>
> +#include <linux/pci.h>
> +#include <linux/device.h>
> +#include <linux/acpi.h>
> +#include "pci.h"
> +
> +const guid_t pci_ssdleds_dsm_guid =
> +	GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b,
> +		  0x8c, 0xb7, 0x74, 0x7e, 0xd5, 0x1e, 0x19, 0x4d);

Static?

> +#define SSDLEDS_GET_SUPPORTED_STATES_DSM	0x01
> +#define SSDLEDS_GET_STATE_DSM			0x02
> +#define SSDLEDS_SET_STATE_DSM			0x03
> +
> +struct pci_ssdleds_dsm_output {
> +	u16 status;
> +	u8 function_specific_err;
> +	u8 vendor_specific_err;
> +	u32 state;
> +};
> +
> +static int ssdleds_dsm_set(struct device *dev, const char *buf, u64 dsm_func)
> +{
> +	acpi_handle handle;
> +	union acpi_object *out_obj, arg3[2];
> +	struct pci_ssdleds_dsm_output *dsm_output;
> +	u32 val;
> +	int err;
> +
> +	handle = ACPI_HANDLE(dev);
> +	if (!handle)
> +		return -ENODEV;
> +
> +	err = kstrtou32(buf, 0, &val);
> +	if (err || val > U32_MAX)
> +		return -EINVAL;

"buf" is really a sysfs concept, not a _DSM concept, so I would
consider moving the kstrtou32() to the caller.

> +	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 *)&val;
> +
> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_ssdleds_dsm_guid, 0x1,
> +				      dsm_func, &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 pci_ssdleds_dsm_output *)out_obj->buffer.pointer;
> +	/*
> +	 * Ignore function specific error bit 1 (some LED state bits weren't
> +	 * set), since write was done. User can read current state to see which
> +	 * bits were set.
> +	 */
> +	if (dsm_output->status != 0 &&
> +	    !(dsm_output->status == 4 && dsm_output->function_specific_err == 1)) {

It looks like this error check is specific to SSDLEDS_SET_STATE_DSM.

I know this function is currently only called for
SSDLEDS_SET_STATE_DSM, but I think you should either get rid of the
dsm_func argument and specify SSDLEDS_SET_STATE_DSM explicitly in
*this* function, or do this error check in the caller where you *know*
you're passing SSDLEDS_SET_STATE_DSM.

I guess the dsm_output you're checking only exists in this function,
so maybe the former?

> +		ACPI_FREE(out_obj);
> +		return -EIO;
> +	}
> +
> +	ACPI_FREE(out_obj);
> +
> +	return 0;
> +}
> +
> +static int ssdleds_dsm_get(struct device *dev, char *buf, u64 dsm_func)
> +{
> +	acpi_handle handle;
> +	union acpi_object *out_obj;
> +	struct pci_ssdleds_dsm_output *dsm_output;
> +
> +	handle = ACPI_HANDLE(dev);
> +	if (!handle)
> +		return -ENODEV;
> +
> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_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 pci_ssdleds_dsm_output *)out_obj->buffer.pointer;
> +	if (dsm_output->status != 0) {
> +		ACPI_FREE(out_obj);
> +		return -EIO;
> +	}
> +
> +	scnprintf(buf, PAGE_SIZE, "%#x\n", dsm_output->state);

Similarly, I would consider returning dsm_output->state and moving the
scnprintf() so the sysfs-specific stuff is in the caller.

It looks like you just expose the hex value that encodes things like:

  OK
  Locate
  Fail
  Rebuild
  ...

The hex value is sort of a pain to interpret, especially since the PCI
specs are not public.  Maybe consider decoding it?  If you did decode
it, that might be a way to reduce this to a single file instead of
having both "supported_states" and "current_state" -- the single file
could list all the supported states, with the current states
highlighted somehow.

Not sure if that would run afoul of the sysfs "one value per file"
rule.  CC'd Greg in case he wants to chime in.

> +	ACPI_FREE(out_obj);
> +
> +	return strlen(buf) > 0 ? strlen(buf) : -EIO;
> +}
> +
> +static bool device_has_dsm(struct device *dev)
> +{
> +	acpi_handle handle;
> +
> +	handle = ACPI_HANDLE(dev);
> +	if (!handle)
> +		return false;
> +
> +	return !!acpi_check_dsm(handle, &pci_ssdleds_dsm_guid, 0x1,
> +		       1 << SSDLEDS_GET_SUPPORTED_STATES_DSM |
> +		       1 << SSDLEDS_GET_STATE_DSM |
> +		       1 << SSDLEDS_SET_STATE_DSM);
> +}
> +
> +static ssize_t ssdleds_current_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	int ret;
> +
> +	ret = ssdleds_dsm_set(dev, buf, SSDLEDS_SET_STATE_DSM);
> +	return (ret < 0) ? ret : count;
> +}
> +
> +static ssize_t ssdleds_current_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	return ssdleds_dsm_get(dev, buf, SSDLEDS_GET_STATE_DSM);
> +}
> +
> +static ssize_t ssdleds_supported_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	return ssdleds_dsm_get(dev, buf, SSDLEDS_GET_SUPPORTED_STATES_DSM);
> +}
> +
> +static umode_t ssdleds_attr_visible(struct kobject *kobj,
> +				    struct attribute *attr, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	umode_t mode = attr->mode;
> +
> +	return device_has_dsm(dev) ? mode : 0;
> +}
> +
> +static struct device_attribute ssdleds_attr_current = {
> +	.attr = {.name = "ssdleds_current_state", .mode = 0644},
> +	.show = ssdleds_current_show,
> +	.store = ssdleds_current_store,
> +};

Can these use DEVICE_ATTR_RW() and friends?

> +static struct device_attribute ssdleds_attr_supported = {
> +	.attr = {.name = "ssdleds_supported_states", .mode = 0444},
> +	.show = ssdleds_supported_show,
> +};
> +
> +static struct attribute *ssdleds_attributes[] = {
> +	&ssdleds_attr_current.attr,
> +	&ssdleds_attr_supported.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ssdleds_attr_group = {
> +	.attrs = ssdleds_attributes,
> +	.is_visible = ssdleds_attr_visible,
> +};
> +
> +void pci_create_ssdleds_files(struct pci_dev *pdev)
> +{
> +	if (sysfs_create_group(&pdev->dev.kobj, &ssdleds_attr_group))
> +		pci_warn(pdev, "Unable to create ssdleds attributes\n");
> +}
> +
> +void pci_remove_ssdleds_files(struct pci_dev *pdev)
> +{
> +	sysfs_remove_group(&pdev->dev.kobj, &ssdleds_attr_group);
> +}

I'm not a real sysfs expert, but I *think* that if you follow the
pattern in pci_dev_attr_groups[] in pci-sysfs.c, you won't need these
create/remove functions.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index d15c881e2e7e..820f32956971 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1377,6 +1377,7 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
>  		goto err_rom_file;
>  
>  	pci_create_firmware_label_files(pdev);
> +	pci_create_ssdleds_files(pdev);
>  
>  	return 0;
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index f86cae9aa1f4..8e6883a1b701 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -30,6 +30,17 @@ static inline void pci_remove_firmware_label_files(struct pci_dev *pdev)
>  void pci_create_firmware_label_files(struct pci_dev *pdev);
>  void pci_remove_firmware_label_files(struct pci_dev *pdev);
>  #endif
> +
> +#if !defined(CONFIG_PCI_SSDLEDS)
> +static inline void pci_create_ssdleds_files(struct pci_dev *pdev)
> +{ return; }
> +static inline void pci_remove_ssdleds_files(struct pci_dev *pdev)
> +{ return; }
> +#else
> +void pci_create_ssdleds_files(struct pci_dev *pdev);
> +void pci_remove_ssdleds_files(struct pci_dev *pdev);
> +#endif
> +
>  void pci_cleanup_rom(struct pci_dev *dev);
>  
>  enum pci_mmap_api {
> -- 
> 2.18.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
  2020-11-13 21:38 ` Bjorn Helgaas
@ 2020-11-13 23:19   ` Greg Kroah-Hartman
  2020-11-16 22:25     ` Stuart Hayes
  0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-13 23:19 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Stuart Hayes, Bjorn Helgaas, linux-pci, Christoph Hellwig, Dan Carpenter

On Fri, Nov 13, 2020 at 03:38:42PM -0600, Bjorn Helgaas wrote:
> [+cc Christoph, Dan, Greg (for "one value per file" question below)]
> 
> On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote:
> > This patch will expose the PCIe SSD Status LED Management interface in
> > sysfs for devices that have the relevant _DSM method, per the "_DSM
> > additions for PCIe SSD Status LED Management" ECN to the PCI Firmware
> > Specification revision 3.2.
> > 
> > The interface is exposed in two sysfs files, ssdleds_supported_states (RO)
> > and ssdleds_current_state (RW).
> > 
> > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> > ---
> > 
> > v2: made PCI_SSDLEDS dependent on PCI and ACPI
> >     remove unused variable
> >     warn if call to sysfs_create_group fails
> >     include header file with function prototypes
> >     change logical OR to bitwise
> > 
> > ---
> >  Documentation/ABI/testing/sysfs-bus-pci |  14 ++
> >  drivers/pci/Kconfig                     |   7 +
> >  drivers/pci/Makefile                    |   1 +
> >  drivers/pci/pci-ssdleds.c               | 194 ++++++++++++++++++++++++
> >  drivers/pci/pci-sysfs.c                 |   1 +
> >  drivers/pci/pci.h                       |  11 ++
> >  6 files changed, 228 insertions(+)
> >  create mode 100644 drivers/pci/pci-ssdleds.c
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> > index 77ad9ec3c801..18ca73b764ac 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > @@ -366,3 +366,17 @@ Contact:	Heiner Kallweit <hkallweit1@gmail.com>
> >  Description:	If ASPM is supported for an endpoint, these files can be
> >  		used to disable or enable the individual power management
> >  		states. Write y/1/on to enable, n/0/off to disable.
> > +
> > +What:		/sys/bus/pci/devices/.../ssdleds_supported_states
> > +Date:		October 2020
> > +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> > +Description:	If the device supports the ACPI _DSM method to control the
> > +		PCIe SSD LED states, ssdleds_supported_states (read only)
> > +		will show the LED states that are supported by the _DSM.
> > +
> > +What:		/sys/bus/pci/devices/.../ssdleds_current_state
> > +Date:		October 2020
> > +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> > +Description:	If the device supports the ACPI _DSM method to control the
> > +		PCIe SSD LED states, ssdleds_current_state will show or set
> > +		the current LED states that are active.
> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > index 0c473d75e625..48049866145f 100644
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -182,6 +182,13 @@ config PCI_LABEL
> >  	def_bool y if (DMI || ACPI)
> >  	select NLS
> >  
> > +config PCI_SSDLEDS
> > +	def_bool y if (ACPI && PCI)

That only should be set if the machine can not boot without it.

For a blinky light, that's not the case.

And why isn't this code using the existing LED subsystem?  Don't create
random new driver-specific sysfs files that do things the existing class
drivers do please.


> > +static int ssdleds_dsm_get(struct device *dev, char *buf, u64 dsm_func)
> > +{
> > +	acpi_handle handle;
> > +	union acpi_object *out_obj;
> > +	struct pci_ssdleds_dsm_output *dsm_output;
> > +
> > +	handle = ACPI_HANDLE(dev);
> > +	if (!handle)
> > +		return -ENODEV;
> > +
> > +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_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 pci_ssdleds_dsm_output *)out_obj->buffer.pointer;
> > +	if (dsm_output->status != 0) {
> > +		ACPI_FREE(out_obj);
> > +		return -EIO;
> > +	}
> > +
> > +	scnprintf(buf, PAGE_SIZE, "%#x\n", dsm_output->state);
> 
> Similarly, I would consider returning dsm_output->state and moving the
> scnprintf() so the sysfs-specific stuff is in the caller.
> 
> It looks like you just expose the hex value that encodes things like:
> 
>   OK
>   Locate
>   Fail
>   Rebuild
>   ...
> 
> The hex value is sort of a pain to interpret, especially since the PCI
> specs are not public.  Maybe consider decoding it?  If you did decode
> it, that might be a way to reduce this to a single file instead of
> having both "supported_states" and "current_state" -- the single file
> could list all the supported states, with the current states
> highlighted somehow.
> 
> Not sure if that would run afoul of the sysfs "one value per file"
> rule.  CC'd Greg in case he wants to chime in.

That's a valid way of displaying options for a sysfs file that can
be specific unique values.


> > +static struct device_attribute ssdleds_attr_current = {
> > +	.attr = {.name = "ssdleds_current_state", .mode = 0644},
> > +	.show = ssdleds_current_show,
> > +	.store = ssdleds_current_store,
> > +};
> 
> Can these use DEVICE_ATTR_RW() and friends?

They should, never open-code something like this.

> 
> > +static struct device_attribute ssdleds_attr_supported = {
> > +	.attr = {.name = "ssdleds_supported_states", .mode = 0444},
> > +	.show = ssdleds_supported_show,
> > +};

DEVICE_ATTR_RO();

> > +
> > +static struct attribute *ssdleds_attributes[] = {
> > +	&ssdleds_attr_current.attr,
> > +	&ssdleds_attr_supported.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group ssdleds_attr_group = {
> > +	.attrs = ssdleds_attributes,
> > +	.is_visible = ssdleds_attr_visible,
> > +};

ATTRIBUTE_GROUPS()?

> > +void pci_create_ssdleds_files(struct pci_dev *pdev)
> > +{
> > +	if (sysfs_create_group(&pdev->dev.kobj, &ssdleds_attr_group))
> > +		pci_warn(pdev, "Unable to create ssdleds attributes\n");

You just raced userspace and lost.  Just add this to the default groups
for the device and the core will handle it all for you automatically.

Hint, when you find yourself calling a sysfs_*() call from driver code,
that's a huge flag that you are doing something wrong.

> > +void pci_remove_ssdleds_files(struct pci_dev *pdev)
> > +{
> > +	sysfs_remove_group(&pdev->dev.kobj, &ssdleds_attr_group);
> > +}
> 
> I'm not a real sysfs expert, but I *think* that if you follow the
> pattern in pci_dev_attr_groups[] in pci-sysfs.c, you won't need these
> create/remove functions.

Yes, that is true.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
  2020-11-10 15:37 [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs Stuart Hayes
                   ` (2 preceding siblings ...)
  2020-11-13 21:38 ` Bjorn Helgaas
@ 2020-11-15  6:38 ` Krzysztof Wilczyński
  3 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Wilczyński @ 2020-11-15  6:38 UTC (permalink / raw)
  To: Stuart Hayes; +Cc: Bjorn Helgaas, linux-pci

Hi Stuart,

On 20-11-10 09:37:35, Stuart Hayes wrote:

[...]
> +
> +	scnprintf(buf, PAGE_SIZE, "%#x\n", dsm_output->state);
> +
> +	ACPI_FREE(out_obj);
> +
> +	return strlen(buf) > 0 ? strlen(buf) : -EIO;

Question to you - since scnprintf() returns the number of characters
written into a buffer, maybe it be possible to use this return value
instead of using strlen(), what do you think?

Krzysztof

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
  2020-11-13 23:19   ` Greg Kroah-Hartman
@ 2020-11-16 22:25     ` Stuart Hayes
  2020-11-17  7:07       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 9+ messages in thread
From: Stuart Hayes @ 2020-11-16 22:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Bjorn Helgaas
  Cc: Bjorn Helgaas, linux-pci, Christoph Hellwig, Dan Carpenter



On 11/13/20 5:19 PM, Greg Kroah-Hartman wrote:
> On Fri, Nov 13, 2020 at 03:38:42PM -0600, Bjorn Helgaas wrote:
>> [+cc Christoph, Dan, Greg (for "one value per file" question below)]
>>
>> On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote:
>>> This patch will expose the PCIe SSD Status LED Management interface in
>>> sysfs for devices that have the relevant _DSM method, per the "_DSM
>>> additions for PCIe SSD Status LED Management" ECN to the PCI Firmware
>>> Specification revision 3.2.
>>>
>>> The interface is exposed in two sysfs files, ssdleds_supported_states (RO)
>>> and ssdleds_current_state (RW).
>>>
>>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
>>> ---
>>>
>>> v2: made PCI_SSDLEDS dependent on PCI and ACPI
>>>     remove unused variable
>>>     warn if call to sysfs_create_group fails
>>>     include header file with function prototypes
>>>     change logical OR to bitwise
>>>
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-pci |  14 ++
>>>  drivers/pci/Kconfig                     |   7 +
>>>  drivers/pci/Makefile                    |   1 +
>>>  drivers/pci/pci-ssdleds.c               | 194 ++++++++++++++++++++++++
>>>  drivers/pci/pci-sysfs.c                 |   1 +
>>>  drivers/pci/pci.h                       |  11 ++
>>>  6 files changed, 228 insertions(+)
>>>  create mode 100644 drivers/pci/pci-ssdleds.c
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
>>> index 77ad9ec3c801..18ca73b764ac 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-pci
>>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
>>> @@ -366,3 +366,17 @@ Contact:	Heiner Kallweit <hkallweit1@gmail.com>
>>>  Description:	If ASPM is supported for an endpoint, these files can be
>>>  		used to disable or enable the individual power management
>>>  		states. Write y/1/on to enable, n/0/off to disable.
>>> +
>>> +What:		/sys/bus/pci/devices/.../ssdleds_supported_states
>>> +Date:		October 2020
>>> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
>>> +Description:	If the device supports the ACPI _DSM method to control the
>>> +		PCIe SSD LED states, ssdleds_supported_states (read only)
>>> +		will show the LED states that are supported by the _DSM.
>>> +
>>> +What:		/sys/bus/pci/devices/.../ssdleds_current_state
>>> +Date:		October 2020
>>> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
>>> +Description:	If the device supports the ACPI _DSM method to control the
>>> +		PCIe SSD LED states, ssdleds_current_state will show or set
>>> +		the current LED states that are active.
>>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>>> index 0c473d75e625..48049866145f 100644
>>> --- a/drivers/pci/Kconfig
>>> +++ b/drivers/pci/Kconfig
>>> @@ -182,6 +182,13 @@ config PCI_LABEL
>>>  	def_bool y if (DMI || ACPI)
>>>  	select NLS
>>>  
>>> +config PCI_SSDLEDS
>>> +	def_bool y if (ACPI && PCI)
> 
> That only should be set if the machine can not boot without it.
> 
> For a blinky light, that's not the case.
> 
> And why isn't this code using the existing LED subsystem?  Don't create
> random new driver-specific sysfs files that do things the existing class
> drivers do please.
>

I did consider the LED subsystem, but I don't think it is a great match.

In spite of the name, this _DSM doesn't directly control individual LEDs--it
sets the state(s) of the PCI port to which an SSD may be connected, so that
it may be conveyed to the user... a processor (or at least some logic) will
decide how to show which states are active, probably by setting combinations
of LEDs to certain colors or blink patterns, or possibly on some other type
of display.  On the system I have, changing the active state(s) sends a
message via IPMI to an embedded processor, which will decide the colors
and/or flashing pattern of the LEDs.  So brightness/color/blinking are not
controlled, or even known, by the OS.

Also, not all of the states will necessarily always be available.  For
example, you may only be able to set the "rebuild" state when a drive is
actually connected to the pci port that has this _DSM, while the "locate"
state might be available all the time.  Since there's no notification
if/when the supported states change, I believe that would mean, to implement
this using the LED subsystem, the driver would have to register an "LED"
with the LED subsystem for each possible state, and either make reads or
writes to that state fail if it isn't supported.

But I will re-write this using the LED subsystem if you think it's a better
fit (though I don't think so).

> 
>>> +static int ssdleds_dsm_get(struct device *dev, char *buf, u64 dsm_func)
>>> +{
>>> +	acpi_handle handle;
>>> +	union acpi_object *out_obj;
>>> +	struct pci_ssdleds_dsm_output *dsm_output;
>>> +
>>> +	handle = ACPI_HANDLE(dev);
>>> +	if (!handle)
>>> +		return -ENODEV;
>>> +
>>> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_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 pci_ssdleds_dsm_output *)out_obj->buffer.pointer;
>>> +	if (dsm_output->status != 0) {
>>> +		ACPI_FREE(out_obj);
>>> +		return -EIO;
>>> +	}
>>> +
>>> +	scnprintf(buf, PAGE_SIZE, "%#x\n", dsm_output->state);
>>
>> Similarly, I would consider returning dsm_output->state and moving the
>> scnprintf() so the sysfs-specific stuff is in the caller.
>>
>> It looks like you just expose the hex value that encodes things like:
>>
>>   OK
>>   Locate
>>   Fail
>>   Rebuild
>>   ...
>>
>> The hex value is sort of a pain to interpret, especially since the PCI
>> specs are not public.  Maybe consider decoding it?  If you did decode
>> it, that might be a way to reduce this to a single file instead of
>> having both "supported_states" and "current_state" -- the single file
>> could list all the supported states, with the current states
>> highlighted somehow.
>>
>> Not sure if that would run afoul of the sysfs "one value per file"
>> rule.  CC'd Greg in case he wants to chime in.
> 
> That's a valid way of displaying options for a sysfs file that can
> be specific unique values.
> 

Thanks--I used hex to keep it a single value.

I'll try to find a way to highlight the current states as suggested so I can
keep this to a single file.

> 
>>> +static struct device_attribute ssdleds_attr_current = {
>>> +	.attr = {.name = "ssdleds_current_state", .mode = 0644},
>>> +	.show = ssdleds_current_show,
>>> +	.store = ssdleds_current_store,
>>> +};
>>
>> Can these use DEVICE_ATTR_RW() and friends?
> 
> They should, never open-code something like this.
> 
>>
>>> +static struct device_attribute ssdleds_attr_supported = {
>>> +	.attr = {.name = "ssdleds_supported_states", .mode = 0444},
>>> +	.show = ssdleds_supported_show,
>>> +};
> 
> DEVICE_ATTR_RO();
> 
>>> +
>>> +static struct attribute *ssdleds_attributes[] = {
>>> +	&ssdleds_attr_current.attr,
>>> +	&ssdleds_attr_supported.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +static const struct attribute_group ssdleds_attr_group = {
>>> +	.attrs = ssdleds_attributes,
>>> +	.is_visible = ssdleds_attr_visible,
>>> +};
> 
> ATTRIBUTE_GROUPS()?
> 
>>> +void pci_create_ssdleds_files(struct pci_dev *pdev)
>>> +{
>>> +	if (sysfs_create_group(&pdev->dev.kobj, &ssdleds_attr_group))
>>> +		pci_warn(pdev, "Unable to create ssdleds attributes\n");
> 
> You just raced userspace and lost.  Just add this to the default groups
> for the device and the core will handle it all for you automatically.
> 
> Hint, when you find yourself calling a sysfs_*() call from driver code,
> that's a huge flag that you are doing something wrong.
> 
>>> +void pci_remove_ssdleds_files(struct pci_dev *pdev)
>>> +{
>>> +	sysfs_remove_group(&pdev->dev.kobj, &ssdleds_attr_group);
>>> +}
>>
>> I'm not a real sysfs expert, but I *think* that if you follow the
>> pattern in pci_dev_attr_groups[] in pci-sysfs.c, you won't need these
>> create/remove functions.
> 
> Yes, that is true.
> 
> thanks,
> 
> greg k-h
> 

I will try to incorporate the remainder of the feedback, thanks for taking
time to look a this.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs
  2020-11-16 22:25     ` Stuart Hayes
@ 2020-11-17  7:07       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-17  7:07 UTC (permalink / raw)
  To: Stuart Hayes
  Cc: Bjorn Helgaas, Bjorn Helgaas, linux-pci, Christoph Hellwig,
	Dan Carpenter

On Mon, Nov 16, 2020 at 04:25:57PM -0600, Stuart Hayes wrote:
> 
> 
> On 11/13/20 5:19 PM, Greg Kroah-Hartman wrote:
> > On Fri, Nov 13, 2020 at 03:38:42PM -0600, Bjorn Helgaas wrote:
> >> [+cc Christoph, Dan, Greg (for "one value per file" question below)]
> >>
> >> On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote:
> >>> This patch will expose the PCIe SSD Status LED Management interface in
> >>> sysfs for devices that have the relevant _DSM method, per the "_DSM
> >>> additions for PCIe SSD Status LED Management" ECN to the PCI Firmware
> >>> Specification revision 3.2.
> >>>
> >>> The interface is exposed in two sysfs files, ssdleds_supported_states (RO)
> >>> and ssdleds_current_state (RW).
> >>>
> >>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> >>> ---
> >>>
> >>> v2: made PCI_SSDLEDS dependent on PCI and ACPI
> >>>     remove unused variable
> >>>     warn if call to sysfs_create_group fails
> >>>     include header file with function prototypes
> >>>     change logical OR to bitwise
> >>>
> >>> ---
> >>>  Documentation/ABI/testing/sysfs-bus-pci |  14 ++
> >>>  drivers/pci/Kconfig                     |   7 +
> >>>  drivers/pci/Makefile                    |   1 +
> >>>  drivers/pci/pci-ssdleds.c               | 194 ++++++++++++++++++++++++
> >>>  drivers/pci/pci-sysfs.c                 |   1 +
> >>>  drivers/pci/pci.h                       |  11 ++
> >>>  6 files changed, 228 insertions(+)
> >>>  create mode 100644 drivers/pci/pci-ssdleds.c
> >>>
> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> >>> index 77ad9ec3c801..18ca73b764ac 100644
> >>> --- a/Documentation/ABI/testing/sysfs-bus-pci
> >>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> >>> @@ -366,3 +366,17 @@ Contact:	Heiner Kallweit <hkallweit1@gmail.com>
> >>>  Description:	If ASPM is supported for an endpoint, these files can be
> >>>  		used to disable or enable the individual power management
> >>>  		states. Write y/1/on to enable, n/0/off to disable.
> >>> +
> >>> +What:		/sys/bus/pci/devices/.../ssdleds_supported_states
> >>> +Date:		October 2020
> >>> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> >>> +Description:	If the device supports the ACPI _DSM method to control the
> >>> +		PCIe SSD LED states, ssdleds_supported_states (read only)
> >>> +		will show the LED states that are supported by the _DSM.
> >>> +
> >>> +What:		/sys/bus/pci/devices/.../ssdleds_current_state
> >>> +Date:		October 2020
> >>> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> >>> +Description:	If the device supports the ACPI _DSM method to control the
> >>> +		PCIe SSD LED states, ssdleds_current_state will show or set
> >>> +		the current LED states that are active.
> >>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> >>> index 0c473d75e625..48049866145f 100644
> >>> --- a/drivers/pci/Kconfig
> >>> +++ b/drivers/pci/Kconfig
> >>> @@ -182,6 +182,13 @@ config PCI_LABEL
> >>>  	def_bool y if (DMI || ACPI)
> >>>  	select NLS
> >>>  
> >>> +config PCI_SSDLEDS
> >>> +	def_bool y if (ACPI && PCI)
> > 
> > That only should be set if the machine can not boot without it.
> > 
> > For a blinky light, that's not the case.
> > 
> > And why isn't this code using the existing LED subsystem?  Don't create
> > random new driver-specific sysfs files that do things the existing class
> > drivers do please.
> >
> 
> I did consider the LED subsystem, but I don't think it is a great match.
> 
> In spite of the name, this _DSM doesn't directly control individual LEDs--it
> sets the state(s) of the PCI port to which an SSD may be connected, so that
> it may be conveyed to the user... a processor (or at least some logic) will
> decide how to show which states are active, probably by setting combinations
> of LEDs to certain colors or blink patterns, or possibly on some other type
> of display.  On the system I have, changing the active state(s) sends a
> message via IPMI to an embedded processor, which will decide the colors
> and/or flashing pattern of the LEDs.  So brightness/color/blinking are not
> controlled, or even known, by the OS.

Then I STRONGLY suggest you change the name of all of this code then, as
it is totally confusing.

> Also, not all of the states will necessarily always be available.  For
> example, you may only be able to set the "rebuild" state when a drive is
> actually connected to the pci port that has this _DSM, while the "locate"
> state might be available all the time.  Since there's no notification
> if/when the supported states change, I believe that would mean, to implement
> this using the LED subsystem, the driver would have to register an "LED"
> with the LED subsystem for each possible state, and either make reads or
> writes to that state fail if it isn't supported.
> 
> But I will re-write this using the LED subsystem if you think it's a better
> fit (though I don't think so).

If you are allowing LEDs to be controlled by the user, then yes, you
have to use the LED subsystem as you should never try to create a brand
new driver-specific user/kernel API just for your tiny driver right?
Please work with the subsystems we have, they are unified for a reason.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-11-17  7:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-10 15:37 [PATCH v2] Expose PCIe SSD Status LED Management DSM in sysfs Stuart Hayes
2020-11-11  7:05 ` Christoph Hellwig
2020-11-12  4:48   ` Stuart Hayes
2020-11-13 14:14 ` Dan Carpenter
2020-11-13 21:38 ` Bjorn Helgaas
2020-11-13 23:19   ` Greg Kroah-Hartman
2020-11-16 22:25     ` Stuart Hayes
2020-11-17  7:07       ` Greg Kroah-Hartman
2020-11-15  6:38 ` Krzysztof Wilczyński

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).