linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
@ 2021-08-24 14:43 Rafael J. Wysocki
  2021-08-25 11:02 ` Wang, Wendy
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-08-24 14:43 UTC (permalink / raw)
  To: Linux PCI, Jonathan Derrick, Bjorn Helgaas
  Cc: Wendy Wang, Linux ACPI, LKML, Mika Westerberg, David Box

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

On some systems, in order to get to the deepest low-power state of
the platform (which may be necessary to save significant enough
amounts of energy while suspended to idle. for example), devices on
the PCI bus exposed by the VMD driver need to be power-managed via
ACPI.  However, the layout of the ACPI namespace below the VMD
controller device object does not reflect the layout of the PCI bus
under the VMD host bridge, so in order to identify the ACPI companion
objects for the devices on that bus, it is necessary to use a special
_ADR encoding on the ACPI side.  In other words, acpi_pci_find_companion()
does not work for these devices, so it needs to be amended with a
special lookup logic specific to the VMD bus.

Address this issue by allowing the VMD driver to temporarily install
an ACPI companion lookup hook containing the code matching the devices
on the VMD PCI bus with the corresponding objects in the ACPI
namespace.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2:
   * Use a read-write semaphore for hook manipulation protection and
     get rid of the static key present in the previous version.
   * Add a busnr check in vmd_acpi_find_companion().

Wendy, David, please test this one!

---
 drivers/pci/controller/vmd.c |   55 +++++++++++++++++++++++++++++++
 drivers/pci/host-bridge.c    |    1 
 drivers/pci/pci-acpi.c       |   74 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h     |    3 +
 4 files changed, 133 insertions(+)

Index: linux-pm/drivers/pci/controller/vmd.c
===================================================================
--- linux-pm.orig/drivers/pci/controller/vmd.c
+++ linux-pm/drivers/pci/controller/vmd.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/srcu.h>
 #include <linux/rculist.h>
@@ -447,6 +448,56 @@ static struct pci_ops vmd_ops = {
 	.write		= vmd_pci_write,
 };
 
+#ifdef CONFIG_ACPI
+static struct acpi_device *vmd_acpi_find_companion(struct pci_dev *pci_dev)
+{
+	struct pci_host_bridge *bridge;
+	u32 busnr, addr;
+
+	if (pci_dev->bus->ops != &vmd_ops)
+		return NULL;
+
+	bridge = pci_find_host_bridge(pci_dev->bus);
+	busnr = pci_dev->bus->number - bridge->bus->number;
+	/*
+	 * The address computation below is only applicable to relative bus
+	 * numbers below 32.
+	 */
+	if (busnr > 31)
+		return NULL;
+
+	addr = (busnr << 24) | ((u32)pci_dev->devfn << 16) | 0x8000FFFFU;
+
+	dev_dbg(&pci_dev->dev, "Looking for ACPI companion (address 0x%x)\n",
+		addr);
+
+	return acpi_find_child_device(ACPI_COMPANION(bridge->dev.parent), addr,
+				      false);
+}
+
+static bool hook_installed;
+
+static void vmd_acpi_begin(void)
+{
+	if (pci_acpi_set_companion_lookup_hook(vmd_acpi_find_companion))
+		return;
+
+	hook_installed = true;
+}
+
+static void vmd_acpi_end(void)
+{
+	if (!hook_installed)
+		return;
+
+	pci_acpi_clear_companion_lookup_hook();
+	hook_installed = false;
+}
+#else
+static inline void vmd_acpi_begin(void) { }
+static inline void vmd_acpi_end(void) { }
+#endif /* CONFIG_ACPI */
+
 static void vmd_attach_resources(struct vmd_dev *vmd)
 {
 	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1];
@@ -747,6 +798,8 @@ static int vmd_enable_domain(struct vmd_
 	if (vmd->irq_domain)
 		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
 
+	vmd_acpi_begin();
+
 	pci_scan_child_bus(vmd->bus);
 	pci_assign_unassigned_bus_resources(vmd->bus);
 
@@ -760,6 +813,8 @@ static int vmd_enable_domain(struct vmd_
 
 	pci_bus_add_devices(vmd->bus);
 
+	vmd_acpi_end();
+
 	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
 			       "domain"), "Can't create symlink to domain\n");
 	return 0;
Index: linux-pm/drivers/pci/host-bridge.c
===================================================================
--- linux-pm.orig/drivers/pci/host-bridge.c
+++ linux-pm/drivers/pci/host-bridge.c
@@ -23,6 +23,7 @@ struct pci_host_bridge *pci_find_host_br
 
 	return to_pci_host_bridge(root_bus->bridge);
 }
+EXPORT_SYMBOL_GPL(pci_find_host_bridge);
 
 struct device *pci_get_host_bridge_device(struct pci_dev *dev)
 {
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -17,6 +17,7 @@
 #include <linux/pci-acpi.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_qos.h>
+#include <linux/rwsem.h>
 #include "pci.h"
 
 /*
@@ -1159,6 +1160,69 @@ void acpi_pci_remove_bus(struct pci_bus
 }
 
 /* ACPI bus type */
+
+
+static DECLARE_RWSEM(pci_acpi_companion_lookup_sem);
+static struct acpi_device *(*pci_acpi_find_companion_hook)(struct pci_dev *);
+
+/**
+ * pci_acpi_set_companion_lookup_hook - Set ACPI companion lookup callback.
+ * @func: ACPI companion lookup callback pointer or NULL.
+ *
+ * Set a special ACPI companion lookup callback for PCI devices whose companion
+ * objects in the ACPI namespace have _ADR with non-standard bus-device-function
+ * encodings.
+ *
+ * Return 0 on success or a negative error code on failure (in which case no
+ * changes are made).
+ *
+ * The caller is responsible for the appropriate ordering of the invocations of
+ * this function with respect to the enumeration of the PCI devices needing the
+ * callback installed by it.
+ */
+int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *))
+{
+	int ret;
+
+	if (!func)
+		return -EINVAL;
+
+	down_write(&pci_acpi_companion_lookup_sem);
+
+	if (pci_acpi_find_companion_hook) {
+		ret = -EBUSY;
+	} else {
+		pci_acpi_find_companion_hook = func;
+		ret = 0;
+	}
+
+	up_write(&pci_acpi_companion_lookup_sem);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_acpi_set_companion_lookup_hook);
+
+/**
+ * pci_acpi_clear_companion_lookup_hook - Clear ACPI companion lookup callback.
+ *
+ * Clear the special ACPI companion lookup callback previously set by
+ * pci_acpi_set_companion_lookup_hook().  Block until the last running instance
+ * of the callback returns before clearing it.
+ *
+ * The caller is responsible for the appropriate ordering of the invocations of
+ * this function with respect to the enumeration of the PCI devices needing the
+ * callback cleared by it.
+ */
+void pci_acpi_clear_companion_lookup_hook(void)
+{
+	down_write(&pci_acpi_companion_lookup_sem);
+
+	pci_acpi_find_companion_hook = NULL;
+
+	up_write(&pci_acpi_companion_lookup_sem);
+}
+EXPORT_SYMBOL_GPL(pci_acpi_clear_companion_lookup_hook);
+
 static struct acpi_device *acpi_pci_find_companion(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -1166,6 +1230,16 @@ static struct acpi_device *acpi_pci_find
 	bool check_children;
 	u64 addr;
 
+	down_read(&pci_acpi_companion_lookup_sem);
+
+	adev = pci_acpi_find_companion_hook ?
+		pci_acpi_find_companion_hook(pci_dev) : NULL;
+
+	up_read(&pci_acpi_companion_lookup_sem);
+
+	if (adev)
+		return adev;
+
 	check_children = pci_is_bridge(pci_dev);
 	/* Please ref to ACPI spec for the syntax of _ADR */
 	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
Index: linux-pm/include/linux/pci-acpi.h
===================================================================
--- linux-pm.orig/include/linux/pci-acpi.h
+++ linux-pm/include/linux/pci-acpi.h
@@ -122,6 +122,9 @@ static inline void pci_acpi_add_edr_noti
 static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
 #endif /* CONFIG_PCIE_EDR */
 
+int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
+void pci_acpi_clear_companion_lookup_hook(void);
+
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }




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

* RE: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
  2021-08-24 14:43 [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus Rafael J. Wysocki
@ 2021-08-25 11:02 ` Wang, Wendy
  2021-08-25 12:13   ` Rafael J. Wysocki
  2021-08-25 15:00   ` Bjorn Helgaas
  0 siblings, 2 replies; 7+ messages in thread
From: Wang, Wendy @ 2021-08-25 11:02 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux PCI, Derrick, Jonathan, Bjorn Helgaas
  Cc: Linux ACPI, LKML, Mika Westerberg, David Box, Wang, Wendy

Hi Rafael,

Tested this PATCH v2 against intel next v5.12 kernel on ADL-S NVME and SATA storages:

cat /sys/devices/pci0000\:00/0000\:00\:0e.0/firmware_node/path
\_SB_.PC00.VMD0

10000:e0:17.0 SATA controller: Intel Corporation Device 7ae2 (rev 11)
10000:e0:1d.0 System peripheral: Intel Corporation Device 09ab
10000:e0:1d.4 PCI bridge: Intel Corporation Device 7ab4 (rev 11)
10000:e1:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller PM9A1/PM9A3/980PRO

[ 6193.658074] ahci 10000:e0:17.0: PCI PM: Suspend power state: D3hot
[ 6193.658156] nvme 10000:e1:00.0: PCI PM: Suspend power state: D3hot
[ 6193.710883] pcieport 10000:e0:1d.4: PCI PM: Suspend power state: D3cold
[ 6193.730318] vmd 0000:00:0e.0: PCI PM: Suspend power state: D3hot

cat /sys/kernel/debug/pmc_core/substate_residencies
Substate   Residency
S0i2.0     0
S0i2.1     13862128

Thanks!

-----Original Message-----
From: Rafael J. Wysocki <rjw@rjwysocki.net> 
Sent: Tuesday, August 24, 2021 10:44 PM
To: Linux PCI <linux-pci@vger.kernel.org>; Derrick, Jonathan <jonathan.derrick@intel.com>; Bjorn Helgaas <helgaas@kernel.org>
Cc: Wang, Wendy <wendy.wang@intel.com>; Linux ACPI <linux-acpi@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Mika Westerberg <mika.westerberg@linux.intel.com>; David Box <david.e.box@linux.intel.com>
Subject: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

On some systems, in order to get to the deepest low-power state of the platform (which may be necessary to save significant enough amounts of energy while suspended to idle. for example), devices on the PCI bus exposed by the VMD driver need to be power-managed via ACPI.  However, the layout of the ACPI namespace below the VMD controller device object does not reflect the layout of the PCI bus under the VMD host bridge, so in order to identify the ACPI companion objects for the devices on that bus, it is necessary to use a special _ADR encoding on the ACPI side.  In other words, acpi_pci_find_companion() does not work for these devices, so it needs to be amended with a special lookup logic specific to the VMD bus.

Address this issue by allowing the VMD driver to temporarily install an ACPI companion lookup hook containing the code matching the devices on the VMD PCI bus with the corresponding objects in the ACPI namespace.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2:
   * Use a read-write semaphore for hook manipulation protection and
     get rid of the static key present in the previous version.
   * Add a busnr check in vmd_acpi_find_companion().

Wendy, David, please test this one!

---
 drivers/pci/controller/vmd.c |   55 +++++++++++++++++++++++++++++++
 drivers/pci/host-bridge.c    |    1 
 drivers/pci/pci-acpi.c       |   74 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h     |    3 +
 4 files changed, 133 insertions(+)

Index: linux-pm/drivers/pci/controller/vmd.c
===================================================================
--- linux-pm.orig/drivers/pci/controller/vmd.c
+++ linux-pm/drivers/pci/controller/vmd.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/srcu.h>
 #include <linux/rculist.h>
@@ -447,6 +448,56 @@ static struct pci_ops vmd_ops = {
 	.write		= vmd_pci_write,
 };
 
+#ifdef CONFIG_ACPI
+static struct acpi_device *vmd_acpi_find_companion(struct pci_dev 
+*pci_dev) {
+	struct pci_host_bridge *bridge;
+	u32 busnr, addr;
+
+	if (pci_dev->bus->ops != &vmd_ops)
+		return NULL;
+
+	bridge = pci_find_host_bridge(pci_dev->bus);
+	busnr = pci_dev->bus->number - bridge->bus->number;
+	/*
+	 * The address computation below is only applicable to relative bus
+	 * numbers below 32.
+	 */
+	if (busnr > 31)
+		return NULL;
+
+	addr = (busnr << 24) | ((u32)pci_dev->devfn << 16) | 0x8000FFFFU;
+
+	dev_dbg(&pci_dev->dev, "Looking for ACPI companion (address 0x%x)\n",
+		addr);
+
+	return acpi_find_child_device(ACPI_COMPANION(bridge->dev.parent), addr,
+				      false);
+}
+
+static bool hook_installed;
+
+static void vmd_acpi_begin(void)
+{
+	if (pci_acpi_set_companion_lookup_hook(vmd_acpi_find_companion))
+		return;
+
+	hook_installed = true;
+}
+
+static void vmd_acpi_end(void)
+{
+	if (!hook_installed)
+		return;
+
+	pci_acpi_clear_companion_lookup_hook();
+	hook_installed = false;
+}
+#else
+static inline void vmd_acpi_begin(void) { } static inline void 
+vmd_acpi_end(void) { } #endif /* CONFIG_ACPI */
+
 static void vmd_attach_resources(struct vmd_dev *vmd)  {
 	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1]; @@ -747,6 +798,8 @@ static int vmd_enable_domain(struct vmd_
 	if (vmd->irq_domain)
 		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
 
+	vmd_acpi_begin();
+
 	pci_scan_child_bus(vmd->bus);
 	pci_assign_unassigned_bus_resources(vmd->bus);
 
@@ -760,6 +813,8 @@ static int vmd_enable_domain(struct vmd_
 
 	pci_bus_add_devices(vmd->bus);
 
+	vmd_acpi_end();
+
 	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
 			       "domain"), "Can't create symlink to domain\n");
 	return 0;
Index: linux-pm/drivers/pci/host-bridge.c
===================================================================
--- linux-pm.orig/drivers/pci/host-bridge.c
+++ linux-pm/drivers/pci/host-bridge.c
@@ -23,6 +23,7 @@ struct pci_host_bridge *pci_find_host_br
 
 	return to_pci_host_bridge(root_bus->bridge);
 }
+EXPORT_SYMBOL_GPL(pci_find_host_bridge);
 
 struct device *pci_get_host_bridge_device(struct pci_dev *dev)  {
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -17,6 +17,7 @@
 #include <linux/pci-acpi.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_qos.h>
+#include <linux/rwsem.h>
 #include "pci.h"
 
 /*
@@ -1159,6 +1160,69 @@ void acpi_pci_remove_bus(struct pci_bus  }
 
 /* ACPI bus type */
+
+
+static DECLARE_RWSEM(pci_acpi_companion_lookup_sem);
+static struct acpi_device *(*pci_acpi_find_companion_hook)(struct 
+pci_dev *);
+
+/**
+ * pci_acpi_set_companion_lookup_hook - Set ACPI companion lookup callback.
+ * @func: ACPI companion lookup callback pointer or NULL.
+ *
+ * Set a special ACPI companion lookup callback for PCI devices whose 
+companion
+ * objects in the ACPI namespace have _ADR with non-standard 
+bus-device-function
+ * encodings.
+ *
+ * Return 0 on success or a negative error code on failure (in which 
+case no
+ * changes are made).
+ *
+ * The caller is responsible for the appropriate ordering of the 
+invocations of
+ * this function with respect to the enumeration of the PCI devices 
+needing the
+ * callback installed by it.
+ */
+int pci_acpi_set_companion_lookup_hook(struct acpi_device 
+*(*func)(struct pci_dev *)) {
+	int ret;
+
+	if (!func)
+		return -EINVAL;
+
+	down_write(&pci_acpi_companion_lookup_sem);
+
+	if (pci_acpi_find_companion_hook) {
+		ret = -EBUSY;
+	} else {
+		pci_acpi_find_companion_hook = func;
+		ret = 0;
+	}
+
+	up_write(&pci_acpi_companion_lookup_sem);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_acpi_set_companion_lookup_hook);
+
+/**
+ * pci_acpi_clear_companion_lookup_hook - Clear ACPI companion lookup callback.
+ *
+ * Clear the special ACPI companion lookup callback previously set by
+ * pci_acpi_set_companion_lookup_hook().  Block until the last running 
+instance
+ * of the callback returns before clearing it.
+ *
+ * The caller is responsible for the appropriate ordering of the 
+invocations of
+ * this function with respect to the enumeration of the PCI devices 
+needing the
+ * callback cleared by it.
+ */
+void pci_acpi_clear_companion_lookup_hook(void)
+{
+	down_write(&pci_acpi_companion_lookup_sem);
+
+	pci_acpi_find_companion_hook = NULL;
+
+	up_write(&pci_acpi_companion_lookup_sem);
+}
+EXPORT_SYMBOL_GPL(pci_acpi_clear_companion_lookup_hook);
+
 static struct acpi_device *acpi_pci_find_companion(struct device *dev)  {
 	struct pci_dev *pci_dev = to_pci_dev(dev); @@ -1166,6 +1230,16 @@ static struct acpi_device *acpi_pci_find
 	bool check_children;
 	u64 addr;
 
+	down_read(&pci_acpi_companion_lookup_sem);
+
+	adev = pci_acpi_find_companion_hook ?
+		pci_acpi_find_companion_hook(pci_dev) : NULL;
+
+	up_read(&pci_acpi_companion_lookup_sem);
+
+	if (adev)
+		return adev;
+
 	check_children = pci_is_bridge(pci_dev);
 	/* Please ref to ACPI spec for the syntax of _ADR */
 	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
Index: linux-pm/include/linux/pci-acpi.h ===================================================================
--- linux-pm.orig/include/linux/pci-acpi.h
+++ linux-pm/include/linux/pci-acpi.h
@@ -122,6 +122,9 @@ static inline void pci_acpi_add_edr_noti  static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }  #endif /* CONFIG_PCIE_EDR */
 
+int pci_acpi_set_companion_lookup_hook(struct acpi_device 
+*(*func)(struct pci_dev *)); void 
+pci_acpi_clear_companion_lookup_hook(void);
+
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }




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

* Re: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
  2021-08-25 11:02 ` Wang, Wendy
@ 2021-08-25 12:13   ` Rafael J. Wysocki
  2021-08-25 15:00   ` Bjorn Helgaas
  1 sibling, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-08-25 12:13 UTC (permalink / raw)
  To: Wang, Wendy
  Cc: Rafael J. Wysocki, Linux PCI, Derrick, Jonathan, Bjorn Helgaas,
	Linux ACPI, LKML, Mika Westerberg, David Box

On Wed, Aug 25, 2021 at 1:03 PM Wang, Wendy <wendy.wang@intel.com> wrote:
>
> Hi Rafael,
>
> Tested this PATCH v2 against intel next v5.12 kernel on ADL-S NVME and SATA storages:
>
> cat /sys/devices/pci0000\:00/0000\:00\:0e.0/firmware_node/path
> \_SB_.PC00.VMD0
>
> 10000:e0:17.0 SATA controller: Intel Corporation Device 7ae2 (rev 11)
> 10000:e0:1d.0 System peripheral: Intel Corporation Device 09ab
> 10000:e0:1d.4 PCI bridge: Intel Corporation Device 7ab4 (rev 11)
> 10000:e1:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller PM9A1/PM9A3/980PRO
>
> [ 6193.658074] ahci 10000:e0:17.0: PCI PM: Suspend power state: D3hot
> [ 6193.658156] nvme 10000:e1:00.0: PCI PM: Suspend power state: D3hot
> [ 6193.710883] pcieport 10000:e0:1d.4: PCI PM: Suspend power state: D3cold
> [ 6193.730318] vmd 0000:00:0e.0: PCI PM: Suspend power state: D3hot
>
> cat /sys/kernel/debug/pmc_core/substate_residencies
> Substate   Residency
> S0i2.0     0
> S0i2.1     13862128
>
> Thanks!

Thank you!

> -----Original Message-----
> From: Rafael J. Wysocki <rjw@rjwysocki.net>
> Sent: Tuesday, August 24, 2021 10:44 PM
> To: Linux PCI <linux-pci@vger.kernel.org>; Derrick, Jonathan <jonathan.derrick@intel.com>; Bjorn Helgaas <helgaas@kernel.org>
> Cc: Wang, Wendy <wendy.wang@intel.com>; Linux ACPI <linux-acpi@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Mika Westerberg <mika.westerberg@linux.intel.com>; David Box <david.e.box@linux.intel.com>
> Subject: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> On some systems, in order to get to the deepest low-power state of the platform (which may be necessary to save significant enough amounts of energy while suspended to idle. for example), devices on the PCI bus exposed by the VMD driver need to be power-managed via ACPI.  However, the layout of the ACPI namespace below the VMD controller device object does not reflect the layout of the PCI bus under the VMD host bridge, so in order to identify the ACPI companion objects for the devices on that bus, it is necessary to use a special _ADR encoding on the ACPI side.  In other words, acpi_pci_find_companion() does not work for these devices, so it needs to be amended with a special lookup logic specific to the VMD bus.
>
> Address this issue by allowing the VMD driver to temporarily install an ACPI companion lookup hook containing the code matching the devices on the VMD PCI bus with the corresponding objects in the ACPI namespace.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> -> v2:
>    * Use a read-write semaphore for hook manipulation protection and
>      get rid of the static key present in the previous version.
>    * Add a busnr check in vmd_acpi_find_companion().
>
> Wendy, David, please test this one!
>
> ---
>  drivers/pci/controller/vmd.c |   55 +++++++++++++++++++++++++++++++
>  drivers/pci/host-bridge.c    |    1
>  drivers/pci/pci-acpi.c       |   74 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h     |    3 +
>  4 files changed, 133 insertions(+)
>
> Index: linux-pm/drivers/pci/controller/vmd.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/controller/vmd.c
> +++ linux-pm/drivers/pci/controller/vmd.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/srcu.h>
>  #include <linux/rculist.h>
> @@ -447,6 +448,56 @@ static struct pci_ops vmd_ops = {
>         .write          = vmd_pci_write,
>  };
>
> +#ifdef CONFIG_ACPI
> +static struct acpi_device *vmd_acpi_find_companion(struct pci_dev
> +*pci_dev) {
> +       struct pci_host_bridge *bridge;
> +       u32 busnr, addr;
> +
> +       if (pci_dev->bus->ops != &vmd_ops)
> +               return NULL;
> +
> +       bridge = pci_find_host_bridge(pci_dev->bus);
> +       busnr = pci_dev->bus->number - bridge->bus->number;
> +       /*
> +        * The address computation below is only applicable to relative bus
> +        * numbers below 32.
> +        */
> +       if (busnr > 31)
> +               return NULL;
> +
> +       addr = (busnr << 24) | ((u32)pci_dev->devfn << 16) | 0x8000FFFFU;
> +
> +       dev_dbg(&pci_dev->dev, "Looking for ACPI companion (address 0x%x)\n",
> +               addr);
> +
> +       return acpi_find_child_device(ACPI_COMPANION(bridge->dev.parent), addr,
> +                                     false);
> +}
> +
> +static bool hook_installed;
> +
> +static void vmd_acpi_begin(void)
> +{
> +       if (pci_acpi_set_companion_lookup_hook(vmd_acpi_find_companion))
> +               return;
> +
> +       hook_installed = true;
> +}
> +
> +static void vmd_acpi_end(void)
> +{
> +       if (!hook_installed)
> +               return;
> +
> +       pci_acpi_clear_companion_lookup_hook();
> +       hook_installed = false;
> +}
> +#else
> +static inline void vmd_acpi_begin(void) { } static inline void
> +vmd_acpi_end(void) { } #endif /* CONFIG_ACPI */
> +
>  static void vmd_attach_resources(struct vmd_dev *vmd)  {
>         vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1]; @@ -747,6 +798,8 @@ static int vmd_enable_domain(struct vmd_
>         if (vmd->irq_domain)
>                 dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
>
> +       vmd_acpi_begin();
> +
>         pci_scan_child_bus(vmd->bus);
>         pci_assign_unassigned_bus_resources(vmd->bus);
>
> @@ -760,6 +813,8 @@ static int vmd_enable_domain(struct vmd_
>
>         pci_bus_add_devices(vmd->bus);
>
> +       vmd_acpi_end();
> +
>         WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
>                                "domain"), "Can't create symlink to domain\n");
>         return 0;
> Index: linux-pm/drivers/pci/host-bridge.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/host-bridge.c
> +++ linux-pm/drivers/pci/host-bridge.c
> @@ -23,6 +23,7 @@ struct pci_host_bridge *pci_find_host_br
>
>         return to_pci_host_bridge(root_bus->bridge);
>  }
> +EXPORT_SYMBOL_GPL(pci_find_host_bridge);
>
>  struct device *pci_get_host_bridge_device(struct pci_dev *dev)  {
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -17,6 +17,7 @@
>  #include <linux/pci-acpi.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_qos.h>
> +#include <linux/rwsem.h>
>  #include "pci.h"
>
>  /*
> @@ -1159,6 +1160,69 @@ void acpi_pci_remove_bus(struct pci_bus  }
>
>  /* ACPI bus type */
> +
> +
> +static DECLARE_RWSEM(pci_acpi_companion_lookup_sem);
> +static struct acpi_device *(*pci_acpi_find_companion_hook)(struct
> +pci_dev *);
> +
> +/**
> + * pci_acpi_set_companion_lookup_hook - Set ACPI companion lookup callback.
> + * @func: ACPI companion lookup callback pointer or NULL.
> + *
> + * Set a special ACPI companion lookup callback for PCI devices whose
> +companion
> + * objects in the ACPI namespace have _ADR with non-standard
> +bus-device-function
> + * encodings.
> + *
> + * Return 0 on success or a negative error code on failure (in which
> +case no
> + * changes are made).
> + *
> + * The caller is responsible for the appropriate ordering of the
> +invocations of
> + * this function with respect to the enumeration of the PCI devices
> +needing the
> + * callback installed by it.
> + */
> +int pci_acpi_set_companion_lookup_hook(struct acpi_device
> +*(*func)(struct pci_dev *)) {
> +       int ret;
> +
> +       if (!func)
> +               return -EINVAL;
> +
> +       down_write(&pci_acpi_companion_lookup_sem);
> +
> +       if (pci_acpi_find_companion_hook) {
> +               ret = -EBUSY;
> +       } else {
> +               pci_acpi_find_companion_hook = func;
> +               ret = 0;
> +       }
> +
> +       up_write(&pci_acpi_companion_lookup_sem);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_set_companion_lookup_hook);
> +
> +/**
> + * pci_acpi_clear_companion_lookup_hook - Clear ACPI companion lookup callback.
> + *
> + * Clear the special ACPI companion lookup callback previously set by
> + * pci_acpi_set_companion_lookup_hook().  Block until the last running
> +instance
> + * of the callback returns before clearing it.
> + *
> + * The caller is responsible for the appropriate ordering of the
> +invocations of
> + * this function with respect to the enumeration of the PCI devices
> +needing the
> + * callback cleared by it.
> + */
> +void pci_acpi_clear_companion_lookup_hook(void)
> +{
> +       down_write(&pci_acpi_companion_lookup_sem);
> +
> +       pci_acpi_find_companion_hook = NULL;
> +
> +       up_write(&pci_acpi_companion_lookup_sem);
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_clear_companion_lookup_hook);
> +
>  static struct acpi_device *acpi_pci_find_companion(struct device *dev)  {
>         struct pci_dev *pci_dev = to_pci_dev(dev); @@ -1166,6 +1230,16 @@ static struct acpi_device *acpi_pci_find
>         bool check_children;
>         u64 addr;
>
> +       down_read(&pci_acpi_companion_lookup_sem);
> +
> +       adev = pci_acpi_find_companion_hook ?
> +               pci_acpi_find_companion_hook(pci_dev) : NULL;
> +
> +       up_read(&pci_acpi_companion_lookup_sem);
> +
> +       if (adev)
> +               return adev;
> +
>         check_children = pci_is_bridge(pci_dev);
>         /* Please ref to ACPI spec for the syntax of _ADR */
>         addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
> Index: linux-pm/include/linux/pci-acpi.h ===================================================================
> --- linux-pm.orig/include/linux/pci-acpi.h
> +++ linux-pm/include/linux/pci-acpi.h
> @@ -122,6 +122,9 @@ static inline void pci_acpi_add_edr_noti  static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }  #endif /* CONFIG_PCIE_EDR */
>
> +int pci_acpi_set_companion_lookup_hook(struct acpi_device
> +*(*func)(struct pci_dev *)); void
> +pci_acpi_clear_companion_lookup_hook(void);
> +
>  #else  /* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>
>
>

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

* Re: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
  2021-08-25 11:02 ` Wang, Wendy
  2021-08-25 12:13   ` Rafael J. Wysocki
@ 2021-08-25 15:00   ` Bjorn Helgaas
  2021-08-25 15:26     ` Rafael J. Wysocki
  1 sibling, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2021-08-25 15:00 UTC (permalink / raw)
  To: Wang, Wendy
  Cc: Rafael J. Wysocki, Linux PCI, Derrick, Jonathan, Linux ACPI,
	LKML, Mika Westerberg, David Box

On Wed, Aug 25, 2021 at 11:02:47AM +0000, Wang, Wendy wrote:
> Hi Rafael,
> 
> Tested this PATCH v2 against intel next v5.12 kernel on ADL-S NVME and SATA storages:
> 
> cat /sys/devices/pci0000\:00/0000\:00\:0e.0/firmware_node/path
> \_SB_.PC00.VMD0
> 
> 10000:e0:17.0 SATA controller: Intel Corporation Device 7ae2 (rev 11)
> 10000:e0:1d.0 System peripheral: Intel Corporation Device 09ab
> 10000:e0:1d.4 PCI bridge: Intel Corporation Device 7ab4 (rev 11)
> 10000:e1:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller PM9A1/PM9A3/980PRO
> 
> [ 6193.658074] ahci 10000:e0:17.0: PCI PM: Suspend power state: D3hot
> [ 6193.658156] nvme 10000:e1:00.0: PCI PM: Suspend power state: D3hot
> [ 6193.710883] pcieport 10000:e0:1d.4: PCI PM: Suspend power state: D3cold
> [ 6193.730318] vmd 0000:00:0e.0: PCI PM: Suspend power state: D3hot
> 
> cat /sys/kernel/debug/pmc_core/substate_residencies
> Substate   Residency
> S0i2.0     0
> S0i2.1     13862128
> 
> Thanks!

I guess (given Rafael's response) that this is a positive test result,
i.e., you see the desired behavior with this patch?

Bjorn

> -----Original Message-----
> From: Rafael J. Wysocki <rjw@rjwysocki.net> 
> Sent: Tuesday, August 24, 2021 10:44 PM
> To: Linux PCI <linux-pci@vger.kernel.org>; Derrick, Jonathan <jonathan.derrick@intel.com>; Bjorn Helgaas <helgaas@kernel.org>
> Cc: Wang, Wendy <wendy.wang@intel.com>; Linux ACPI <linux-acpi@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Mika Westerberg <mika.westerberg@linux.intel.com>; David Box <david.e.box@linux.intel.com>
> Subject: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> On some systems, in order to get to the deepest low-power state of the platform (which may be necessary to save significant enough amounts of energy while suspended to idle. for example), devices on the PCI bus exposed by the VMD driver need to be power-managed via ACPI.  However, the layout of the ACPI namespace below the VMD controller device object does not reflect the layout of the PCI bus under the VMD host bridge, so in order to identify the ACPI companion objects for the devices on that bus, it is necessary to use a special _ADR encoding on the ACPI side.  In other words, acpi_pci_find_companion() does not work for these devices, so it needs to be amended with a special lookup logic specific to the VMD bus.
> 
> Address this issue by allowing the VMD driver to temporarily install an ACPI companion lookup hook containing the code matching the devices on the VMD PCI bus with the corresponding objects in the ACPI namespace.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2:
>    * Use a read-write semaphore for hook manipulation protection and
>      get rid of the static key present in the previous version.
>    * Add a busnr check in vmd_acpi_find_companion().
> 
> Wendy, David, please test this one!
> 
> ---
>  drivers/pci/controller/vmd.c |   55 +++++++++++++++++++++++++++++++
>  drivers/pci/host-bridge.c    |    1 
>  drivers/pci/pci-acpi.c       |   74 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h     |    3 +
>  4 files changed, 133 insertions(+)
> 
> Index: linux-pm/drivers/pci/controller/vmd.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/controller/vmd.c
> +++ linux-pm/drivers/pci/controller/vmd.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/srcu.h>
>  #include <linux/rculist.h>
> @@ -447,6 +448,56 @@ static struct pci_ops vmd_ops = {
>  	.write		= vmd_pci_write,
>  };
>  
> +#ifdef CONFIG_ACPI
> +static struct acpi_device *vmd_acpi_find_companion(struct pci_dev 
> +*pci_dev) {
> +	struct pci_host_bridge *bridge;
> +	u32 busnr, addr;
> +
> +	if (pci_dev->bus->ops != &vmd_ops)
> +		return NULL;
> +
> +	bridge = pci_find_host_bridge(pci_dev->bus);
> +	busnr = pci_dev->bus->number - bridge->bus->number;
> +	/*
> +	 * The address computation below is only applicable to relative bus
> +	 * numbers below 32.
> +	 */
> +	if (busnr > 31)
> +		return NULL;
> +
> +	addr = (busnr << 24) | ((u32)pci_dev->devfn << 16) | 0x8000FFFFU;
> +
> +	dev_dbg(&pci_dev->dev, "Looking for ACPI companion (address 0x%x)\n",
> +		addr);
> +
> +	return acpi_find_child_device(ACPI_COMPANION(bridge->dev.parent), addr,
> +				      false);
> +}
> +
> +static bool hook_installed;
> +
> +static void vmd_acpi_begin(void)
> +{
> +	if (pci_acpi_set_companion_lookup_hook(vmd_acpi_find_companion))
> +		return;
> +
> +	hook_installed = true;
> +}
> +
> +static void vmd_acpi_end(void)
> +{
> +	if (!hook_installed)
> +		return;
> +
> +	pci_acpi_clear_companion_lookup_hook();
> +	hook_installed = false;
> +}
> +#else
> +static inline void vmd_acpi_begin(void) { } static inline void 
> +vmd_acpi_end(void) { } #endif /* CONFIG_ACPI */
> +
>  static void vmd_attach_resources(struct vmd_dev *vmd)  {
>  	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1]; @@ -747,6 +798,8 @@ static int vmd_enable_domain(struct vmd_
>  	if (vmd->irq_domain)
>  		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
>  
> +	vmd_acpi_begin();
> +
>  	pci_scan_child_bus(vmd->bus);
>  	pci_assign_unassigned_bus_resources(vmd->bus);
>  
> @@ -760,6 +813,8 @@ static int vmd_enable_domain(struct vmd_
>  
>  	pci_bus_add_devices(vmd->bus);
>  
> +	vmd_acpi_end();
> +
>  	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
>  			       "domain"), "Can't create symlink to domain\n");
>  	return 0;
> Index: linux-pm/drivers/pci/host-bridge.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/host-bridge.c
> +++ linux-pm/drivers/pci/host-bridge.c
> @@ -23,6 +23,7 @@ struct pci_host_bridge *pci_find_host_br
>  
>  	return to_pci_host_bridge(root_bus->bridge);
>  }
> +EXPORT_SYMBOL_GPL(pci_find_host_bridge);
>  
>  struct device *pci_get_host_bridge_device(struct pci_dev *dev)  {
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -17,6 +17,7 @@
>  #include <linux/pci-acpi.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_qos.h>
> +#include <linux/rwsem.h>
>  #include "pci.h"
>  
>  /*
> @@ -1159,6 +1160,69 @@ void acpi_pci_remove_bus(struct pci_bus  }
>  
>  /* ACPI bus type */
> +
> +
> +static DECLARE_RWSEM(pci_acpi_companion_lookup_sem);
> +static struct acpi_device *(*pci_acpi_find_companion_hook)(struct 
> +pci_dev *);
> +
> +/**
> + * pci_acpi_set_companion_lookup_hook - Set ACPI companion lookup callback.
> + * @func: ACPI companion lookup callback pointer or NULL.
> + *
> + * Set a special ACPI companion lookup callback for PCI devices whose 
> +companion
> + * objects in the ACPI namespace have _ADR with non-standard 
> +bus-device-function
> + * encodings.
> + *
> + * Return 0 on success or a negative error code on failure (in which 
> +case no
> + * changes are made).
> + *
> + * The caller is responsible for the appropriate ordering of the 
> +invocations of
> + * this function with respect to the enumeration of the PCI devices 
> +needing the
> + * callback installed by it.
> + */
> +int pci_acpi_set_companion_lookup_hook(struct acpi_device 
> +*(*func)(struct pci_dev *)) {
> +	int ret;
> +
> +	if (!func)
> +		return -EINVAL;
> +
> +	down_write(&pci_acpi_companion_lookup_sem);
> +
> +	if (pci_acpi_find_companion_hook) {
> +		ret = -EBUSY;
> +	} else {
> +		pci_acpi_find_companion_hook = func;
> +		ret = 0;
> +	}
> +
> +	up_write(&pci_acpi_companion_lookup_sem);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_set_companion_lookup_hook);
> +
> +/**
> + * pci_acpi_clear_companion_lookup_hook - Clear ACPI companion lookup callback.
> + *
> + * Clear the special ACPI companion lookup callback previously set by
> + * pci_acpi_set_companion_lookup_hook().  Block until the last running 
> +instance
> + * of the callback returns before clearing it.
> + *
> + * The caller is responsible for the appropriate ordering of the 
> +invocations of
> + * this function with respect to the enumeration of the PCI devices 
> +needing the
> + * callback cleared by it.
> + */
> +void pci_acpi_clear_companion_lookup_hook(void)
> +{
> +	down_write(&pci_acpi_companion_lookup_sem);
> +
> +	pci_acpi_find_companion_hook = NULL;
> +
> +	up_write(&pci_acpi_companion_lookup_sem);
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_clear_companion_lookup_hook);
> +
>  static struct acpi_device *acpi_pci_find_companion(struct device *dev)  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev); @@ -1166,6 +1230,16 @@ static struct acpi_device *acpi_pci_find
>  	bool check_children;
>  	u64 addr;
>  
> +	down_read(&pci_acpi_companion_lookup_sem);
> +
> +	adev = pci_acpi_find_companion_hook ?
> +		pci_acpi_find_companion_hook(pci_dev) : NULL;
> +
> +	up_read(&pci_acpi_companion_lookup_sem);
> +
> +	if (adev)
> +		return adev;
> +
>  	check_children = pci_is_bridge(pci_dev);
>  	/* Please ref to ACPI spec for the syntax of _ADR */
>  	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
> Index: linux-pm/include/linux/pci-acpi.h ===================================================================
> --- linux-pm.orig/include/linux/pci-acpi.h
> +++ linux-pm/include/linux/pci-acpi.h
> @@ -122,6 +122,9 @@ static inline void pci_acpi_add_edr_noti  static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }  #endif /* CONFIG_PCIE_EDR */
>  
> +int pci_acpi_set_companion_lookup_hook(struct acpi_device 
> +*(*func)(struct pci_dev *)); void 
> +pci_acpi_clear_companion_lookup_hook(void);
> +
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> 
> 
> 

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

* Re: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
  2021-08-25 15:00   ` Bjorn Helgaas
@ 2021-08-25 15:26     ` Rafael J. Wysocki
  2021-08-26 18:24       ` Jon Derrick
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-08-25 15:26 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Wang, Wendy, Rafael J. Wysocki, Linux PCI, Derrick, Jonathan,
	Linux ACPI, LKML, Mika Westerberg, David Box

On Wed, Aug 25, 2021 at 5:00 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Aug 25, 2021 at 11:02:47AM +0000, Wang, Wendy wrote:
> > Hi Rafael,
> >
> > Tested this PATCH v2 against intel next v5.12 kernel on ADL-S NVME and SATA storages:
> >
> > cat /sys/devices/pci0000\:00/0000\:00\:0e.0/firmware_node/path
> > \_SB_.PC00.VMD0
> >
> > 10000:e0:17.0 SATA controller: Intel Corporation Device 7ae2 (rev 11)
> > 10000:e0:1d.0 System peripheral: Intel Corporation Device 09ab
> > 10000:e0:1d.4 PCI bridge: Intel Corporation Device 7ab4 (rev 11)
> > 10000:e1:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller PM9A1/PM9A3/980PRO
> >
> > [ 6193.658074] ahci 10000:e0:17.0: PCI PM: Suspend power state: D3hot
> > [ 6193.658156] nvme 10000:e1:00.0: PCI PM: Suspend power state: D3hot
> > [ 6193.710883] pcieport 10000:e0:1d.4: PCI PM: Suspend power state: D3cold

This doesn't happen without using the ACPI companion object (the
deepest power state you can get then is D3hot) AFAICS.

> > [ 6193.730318] vmd 0000:00:0e.0: PCI PM: Suspend power state: D3hot
> >
> > cat /sys/kernel/debug/pmc_core/substate_residencies
> > Substate   Residency
> > S0i2.0     0
> > S0i2.1     13862128
> >
> > Thanks!
>
> I guess (given Rafael's response) that this is a positive test result,
> i.e., you see the desired behavior with this patch?

So yes.

> > -----Original Message-----
> > From: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Sent: Tuesday, August 24, 2021 10:44 PM
> > To: Linux PCI <linux-pci@vger.kernel.org>; Derrick, Jonathan <jonathan.derrick@intel.com>; Bjorn Helgaas <helgaas@kernel.org>
> > Cc: Wang, Wendy <wendy.wang@intel.com>; Linux ACPI <linux-acpi@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Mika Westerberg <mika.westerberg@linux.intel.com>; David Box <david.e.box@linux.intel.com>
> > Subject: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > On some systems, in order to get to the deepest low-power state of the platform (which may be necessary to save significant enough amounts of energy while suspended to idle. for example), devices on the PCI bus exposed by the VMD driver need to be power-managed via ACPI.  However, the layout of the ACPI namespace below the VMD controller device object does not reflect the layout of the PCI bus under the VMD host bridge, so in order to identify the ACPI companion objects for the devices on that bus, it is necessary to use a special _ADR encoding on the ACPI side.  In other words, acpi_pci_find_companion() does not work for these devices, so it needs to be amended with a special lookup logic specific to the VMD bus.
> >
> > Address this issue by allowing the VMD driver to temporarily install an ACPI companion lookup hook containing the code matching the devices on the VMD PCI bus with the corresponding objects in the ACPI namespace.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > -> v2:
> >    * Use a read-write semaphore for hook manipulation protection and
> >      get rid of the static key present in the previous version.
> >    * Add a busnr check in vmd_acpi_find_companion().
> >
> > Wendy, David, please test this one!
> >
> > ---
> >  drivers/pci/controller/vmd.c |   55 +++++++++++++++++++++++++++++++
> >  drivers/pci/host-bridge.c    |    1
> >  drivers/pci/pci-acpi.c       |   74 +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-acpi.h     |    3 +
> >  4 files changed, 133 insertions(+)
> >
> > Index: linux-pm/drivers/pci/controller/vmd.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/controller/vmd.c
> > +++ linux-pm/drivers/pci/controller/vmd.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/module.h>
> >  #include <linux/msi.h>
> >  #include <linux/pci.h>
> > +#include <linux/pci-acpi.h>
> >  #include <linux/pci-ecam.h>
> >  #include <linux/srcu.h>
> >  #include <linux/rculist.h>
> > @@ -447,6 +448,56 @@ static struct pci_ops vmd_ops = {
> >       .write          = vmd_pci_write,
> >  };
> >
> > +#ifdef CONFIG_ACPI
> > +static struct acpi_device *vmd_acpi_find_companion(struct pci_dev
> > +*pci_dev) {
> > +     struct pci_host_bridge *bridge;
> > +     u32 busnr, addr;
> > +
> > +     if (pci_dev->bus->ops != &vmd_ops)
> > +             return NULL;
> > +
> > +     bridge = pci_find_host_bridge(pci_dev->bus);
> > +     busnr = pci_dev->bus->number - bridge->bus->number;
> > +     /*
> > +      * The address computation below is only applicable to relative bus
> > +      * numbers below 32.
> > +      */
> > +     if (busnr > 31)
> > +             return NULL;
> > +
> > +     addr = (busnr << 24) | ((u32)pci_dev->devfn << 16) | 0x8000FFFFU;
> > +
> > +     dev_dbg(&pci_dev->dev, "Looking for ACPI companion (address 0x%x)\n",
> > +             addr);
> > +
> > +     return acpi_find_child_device(ACPI_COMPANION(bridge->dev.parent), addr,
> > +                                   false);
> > +}
> > +
> > +static bool hook_installed;
> > +
> > +static void vmd_acpi_begin(void)
> > +{
> > +     if (pci_acpi_set_companion_lookup_hook(vmd_acpi_find_companion))
> > +             return;
> > +
> > +     hook_installed = true;
> > +}
> > +
> > +static void vmd_acpi_end(void)
> > +{
> > +     if (!hook_installed)
> > +             return;
> > +
> > +     pci_acpi_clear_companion_lookup_hook();
> > +     hook_installed = false;
> > +}
> > +#else
> > +static inline void vmd_acpi_begin(void) { } static inline void
> > +vmd_acpi_end(void) { } #endif /* CONFIG_ACPI */
> > +
> >  static void vmd_attach_resources(struct vmd_dev *vmd)  {
> >       vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1]; @@ -747,6 +798,8 @@ static int vmd_enable_domain(struct vmd_
> >       if (vmd->irq_domain)
> >               dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
> >
> > +     vmd_acpi_begin();
> > +
> >       pci_scan_child_bus(vmd->bus);
> >       pci_assign_unassigned_bus_resources(vmd->bus);
> >
> > @@ -760,6 +813,8 @@ static int vmd_enable_domain(struct vmd_
> >
> >       pci_bus_add_devices(vmd->bus);
> >
> > +     vmd_acpi_end();
> > +
> >       WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
> >                              "domain"), "Can't create symlink to domain\n");
> >       return 0;
> > Index: linux-pm/drivers/pci/host-bridge.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/host-bridge.c
> > +++ linux-pm/drivers/pci/host-bridge.c
> > @@ -23,6 +23,7 @@ struct pci_host_bridge *pci_find_host_br
> >
> >       return to_pci_host_bridge(root_bus->bridge);
> >  }
> > +EXPORT_SYMBOL_GPL(pci_find_host_bridge);
> >
> >  struct device *pci_get_host_bridge_device(struct pci_dev *dev)  {
> > Index: linux-pm/drivers/pci/pci-acpi.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci-acpi.c
> > +++ linux-pm/drivers/pci/pci-acpi.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/pci-acpi.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/pm_qos.h>
> > +#include <linux/rwsem.h>
> >  #include "pci.h"
> >
> >  /*
> > @@ -1159,6 +1160,69 @@ void acpi_pci_remove_bus(struct pci_bus  }
> >
> >  /* ACPI bus type */
> > +
> > +
> > +static DECLARE_RWSEM(pci_acpi_companion_lookup_sem);
> > +static struct acpi_device *(*pci_acpi_find_companion_hook)(struct
> > +pci_dev *);
> > +
> > +/**
> > + * pci_acpi_set_companion_lookup_hook - Set ACPI companion lookup callback.
> > + * @func: ACPI companion lookup callback pointer or NULL.
> > + *
> > + * Set a special ACPI companion lookup callback for PCI devices whose
> > +companion
> > + * objects in the ACPI namespace have _ADR with non-standard
> > +bus-device-function
> > + * encodings.
> > + *
> > + * Return 0 on success or a negative error code on failure (in which
> > +case no
> > + * changes are made).
> > + *
> > + * The caller is responsible for the appropriate ordering of the
> > +invocations of
> > + * this function with respect to the enumeration of the PCI devices
> > +needing the
> > + * callback installed by it.
> > + */
> > +int pci_acpi_set_companion_lookup_hook(struct acpi_device
> > +*(*func)(struct pci_dev *)) {
> > +     int ret;
> > +
> > +     if (!func)
> > +             return -EINVAL;
> > +
> > +     down_write(&pci_acpi_companion_lookup_sem);
> > +
> > +     if (pci_acpi_find_companion_hook) {
> > +             ret = -EBUSY;
> > +     } else {
> > +             pci_acpi_find_companion_hook = func;
> > +             ret = 0;
> > +     }
> > +
> > +     up_write(&pci_acpi_companion_lookup_sem);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_acpi_set_companion_lookup_hook);
> > +
> > +/**
> > + * pci_acpi_clear_companion_lookup_hook - Clear ACPI companion lookup callback.
> > + *
> > + * Clear the special ACPI companion lookup callback previously set by
> > + * pci_acpi_set_companion_lookup_hook().  Block until the last running
> > +instance
> > + * of the callback returns before clearing it.
> > + *
> > + * The caller is responsible for the appropriate ordering of the
> > +invocations of
> > + * this function with respect to the enumeration of the PCI devices
> > +needing the
> > + * callback cleared by it.
> > + */
> > +void pci_acpi_clear_companion_lookup_hook(void)
> > +{
> > +     down_write(&pci_acpi_companion_lookup_sem);
> > +
> > +     pci_acpi_find_companion_hook = NULL;
> > +
> > +     up_write(&pci_acpi_companion_lookup_sem);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_acpi_clear_companion_lookup_hook);
> > +
> >  static struct acpi_device *acpi_pci_find_companion(struct device *dev)  {
> >       struct pci_dev *pci_dev = to_pci_dev(dev); @@ -1166,6 +1230,16 @@ static struct acpi_device *acpi_pci_find
> >       bool check_children;
> >       u64 addr;
> >
> > +     down_read(&pci_acpi_companion_lookup_sem);
> > +
> > +     adev = pci_acpi_find_companion_hook ?
> > +             pci_acpi_find_companion_hook(pci_dev) : NULL;
> > +
> > +     up_read(&pci_acpi_companion_lookup_sem);
> > +
> > +     if (adev)
> > +             return adev;
> > +
> >       check_children = pci_is_bridge(pci_dev);
> >       /* Please ref to ACPI spec for the syntax of _ADR */
> >       addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
> > Index: linux-pm/include/linux/pci-acpi.h ===================================================================
> > --- linux-pm.orig/include/linux/pci-acpi.h
> > +++ linux-pm/include/linux/pci-acpi.h
> > @@ -122,6 +122,9 @@ static inline void pci_acpi_add_edr_noti  static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }  #endif /* CONFIG_PCIE_EDR */
> >
> > +int pci_acpi_set_companion_lookup_hook(struct acpi_device
> > +*(*func)(struct pci_dev *)); void
> > +pci_acpi_clear_companion_lookup_hook(void);
> > +
> >  #else        /* CONFIG_ACPI */
> >  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> >
> >
> >

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

* Re: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
  2021-08-25 15:26     ` Rafael J. Wysocki
@ 2021-08-26 18:24       ` Jon Derrick
  2021-09-02 15:57         ` Rafael J. Wysocki
  0 siblings, 1 reply; 7+ messages in thread
From: Jon Derrick @ 2021-08-26 18:24 UTC (permalink / raw)
  To: Rafael J. Wysocki, Bjorn Helgaas
  Cc: Wang, Wendy, Rafael J. Wysocki, Linux PCI, Linux ACPI, LKML,
	Mika Westerberg, David Box



On 8/25/21 9:26 AM, Rafael J. Wysocki wrote:
> On Wed, Aug 25, 2021 at 5:00 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>> On Wed, Aug 25, 2021 at 11:02:47AM +0000, Wang, Wendy wrote:
>>> Hi Rafael,
>>>
>>> Tested this PATCH v2 against intel next v5.12 kernel on ADL-S NVME and SATA storages:
>>>
>>> cat /sys/devices/pci0000\:00/0000\:00\:0e.0/firmware_node/path
>>> \_SB_.PC00.VMD0
>>>
>>> 10000:e0:17.0 SATA controller: Intel Corporation Device 7ae2 (rev 11)
>>> 10000:e0:1d.0 System peripheral: Intel Corporation Device 09ab
>>> 10000:e0:1d.4 PCI bridge: Intel Corporation Device 7ab4 (rev 11)
>>> 10000:e1:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller PM9A1/PM9A3/980PRO
>>>
>>> [ 6193.658074] ahci 10000:e0:17.0: PCI PM: Suspend power state: D3hot
>>> [ 6193.658156] nvme 10000:e1:00.0: PCI PM: Suspend power state: D3hot
>>> [ 6193.710883] pcieport 10000:e0:1d.4: PCI PM: Suspend power state: D3cold
> 
> This doesn't happen without using the ACPI companion object (the
> deepest power state you can get then is D3hot) AFAICS.
> 
>>> [ 6193.730318] vmd 0000:00:0e.0: PCI PM: Suspend power state: D3hot
>>>
>>> cat /sys/kernel/debug/pmc_core/substate_residencies
>>> Substate   Residency
>>> S0i2.0     0
>>> S0i2.1     13862128
>>>
>>> Thanks!
>>
>> I guess (given Rafael's response) that this is a positive test result,
>> i.e., you see the desired behavior with this patch?
> 
> So yes.

LGTM then

Acked-by: Jon Derrick <jonathan.derrick@intel.com>


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

* Re: [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus
  2021-08-26 18:24       ` Jon Derrick
@ 2021-09-02 15:57         ` Rafael J. Wysocki
  0 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-09-02 15:57 UTC (permalink / raw)
  To: Jon Derrick, Linux PCI
  Cc: Rafael J. Wysocki, Bjorn Helgaas, Wang, Wendy, Rafael J. Wysocki,
	Linux ACPI, LKML, Mika Westerberg, David Box

On Thu, Aug 26, 2021 at 8:24 PM Jon Derrick <jonathan.derrick@intel.com> wrote:
>
>
>
> On 8/25/21 9:26 AM, Rafael J. Wysocki wrote:
> > On Wed, Aug 25, 2021 at 5:00 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>
> >> On Wed, Aug 25, 2021 at 11:02:47AM +0000, Wang, Wendy wrote:
> >>> Hi Rafael,
> >>>
> >>> Tested this PATCH v2 against intel next v5.12 kernel on ADL-S NVME and SATA storages:
> >>>
> >>> cat /sys/devices/pci0000\:00/0000\:00\:0e.0/firmware_node/path
> >>> \_SB_.PC00.VMD0
> >>>
> >>> 10000:e0:17.0 SATA controller: Intel Corporation Device 7ae2 (rev 11)
> >>> 10000:e0:1d.0 System peripheral: Intel Corporation Device 09ab
> >>> 10000:e0:1d.4 PCI bridge: Intel Corporation Device 7ab4 (rev 11)
> >>> 10000:e1:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller PM9A1/PM9A3/980PRO
> >>>
> >>> [ 6193.658074] ahci 10000:e0:17.0: PCI PM: Suspend power state: D3hot
> >>> [ 6193.658156] nvme 10000:e1:00.0: PCI PM: Suspend power state: D3hot
> >>> [ 6193.710883] pcieport 10000:e0:1d.4: PCI PM: Suspend power state: D3cold
> >
> > This doesn't happen without using the ACPI companion object (the
> > deepest power state you can get then is D3hot) AFAICS.
> >
> >>> [ 6193.730318] vmd 0000:00:0e.0: PCI PM: Suspend power state: D3hot
> >>>
> >>> cat /sys/kernel/debug/pmc_core/substate_residencies
> >>> Substate   Residency
> >>> S0i2.0     0
> >>> S0i2.1     13862128
> >>>
> >>> Thanks!
> >>
> >> I guess (given Rafael's response) that this is a positive test result,
> >> i.e., you see the desired behavior with this patch?
> >
> > So yes.
>
> LGTM then
>
> Acked-by: Jon Derrick <jonathan.derrick@intel.com>

Thank you!

It doesn't look like there are any concerns regarding this patch, so
I'll queue it up for merging next week.

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

end of thread, other threads:[~2021-09-02 15:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 14:43 [PATCH v2] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus Rafael J. Wysocki
2021-08-25 11:02 ` Wang, Wendy
2021-08-25 12:13   ` Rafael J. Wysocki
2021-08-25 15:00   ` Bjorn Helgaas
2021-08-25 15:26     ` Rafael J. Wysocki
2021-08-26 18:24       ` Jon Derrick
2021-09-02 15:57         ` Rafael J. Wysocki

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).