[PATCH] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus

[PATCH] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus

[Rafael J. Wysocki]

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>
Tested-by: Wendy Wang <wendy.wang@intel.com>
---
 drivers/pci/controller/vmd.c |   48 ++++++++++++++++++++++++++
 drivers/pci/host-bridge.c    |    1 
 drivers/pci/pci-acpi.c       |   78 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h     |    3 +
 4 files changed, 130 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,49 @@ 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;
+	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 +791,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 +806,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
@@ -1159,6 +1159,72 @@ void acpi_pci_remove_bus(struct pci_bus
 }
 
 /* ACPI bus type */
+
+
+DEFINE_STATIC_KEY_FALSE(pci_acpi_companion_lookup_key);
+static DEFINE_MUTEX(pci_acpi_companion_lookup_mtx);
+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;
+
+	mutex_lock(&pci_acpi_companion_lookup_mtx);
+
+	if (pci_acpi_find_companion_hook) {
+		ret = -EBUSY;
+	} else {
+		pci_acpi_find_companion_hook = func;
+		static_branch_enable(&pci_acpi_companion_lookup_key);
+		ret = 0;
+	}
+
+	mutex_unlock(&pci_acpi_companion_lookup_mtx);
+
+	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)
+{
+	mutex_lock(&pci_acpi_companion_lookup_mtx);
+
+	pci_acpi_find_companion_hook = NULL;
+	static_branch_disable(&pci_acpi_companion_lookup_key);
+
+	mutex_unlock(&pci_acpi_companion_lookup_mtx);
+}
+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 +1232,18 @@ static struct acpi_device *acpi_pci_find
 	bool check_children;
 	u64 addr;
 
+	if (static_branch_unlikely(&pci_acpi_companion_lookup_key)) {
+		mutex_lock(&pci_acpi_companion_lookup_mtx);
+
+		adev = pci_acpi_find_companion_hook ?
+			pci_acpi_find_companion_hook(pci_dev) : NULL;
+
+		mutex_unlock(&pci_acpi_companion_lookup_mtx);
+
+		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) { }

Re: [PATCH] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus

[Bjorn Helgaas]

[+cc Shanker, Amey]

On Fri, Aug 20, 2021 at 06:12:39PM +0200, Rafael J. Wysocki wrote:
> 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>
> Tested-by: Wendy Wang <wendy.wang@intel.com>
> ---
>  drivers/pci/controller/vmd.c |   48 ++++++++++++++++++++++++++
>  drivers/pci/host-bridge.c    |    1 
>  drivers/pci/pci-acpi.c       |   78 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h     |    3 +
>  4 files changed, 130 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,49 @@ 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;
> +	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 +791,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);

This affects the same path as Shanker's patch to set the
ACPI_COMPANION earlier, in pci_setup_device() instead of in
acpi_bind_one() [1].

I think this should still work correctly since vmd_acpi_begin()
inserts the VMD-specific hook in acpi_pci_find_companion() before we
call pci_scan_child_bus() to enumerate devices below VMD.

So when Shanker's new pci_set_acpi_fwnode() calls
acpi_pci_find_companion(), it should get to vmd_acpi_find_companion()
even though it happens earlier than acpi_bind_one().

Just FYI so you can double check.

> @@ -760,6 +806,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
> @@ -1159,6 +1159,72 @@ void acpi_pci_remove_bus(struct pci_bus
>  }
>  
>  /* ACPI bus type */
> +
> +
> +DEFINE_STATIC_KEY_FALSE(pci_acpi_companion_lookup_key);

I don't know anything about static keys, but it looks like it
complicates the code a fair amount, and the doc [2] suggests it's for
performance-sensitive fast path code.

Is this a path that needs that much optimization?  Or is this static
key dance something that's *always* needed when we change a function
pointer at run-time?

> +static DEFINE_MUTEX(pci_acpi_companion_lookup_mtx);
> +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;
> +
> +	mutex_lock(&pci_acpi_companion_lookup_mtx);
> +
> +	if (pci_acpi_find_companion_hook) {
> +		ret = -EBUSY;
> +	} else {
> +		pci_acpi_find_companion_hook = func;
> +		static_branch_enable(&pci_acpi_companion_lookup_key);
> +		ret = 0;
> +	}
> +
> +	mutex_unlock(&pci_acpi_companion_lookup_mtx);
> +
> +	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)
> +{
> +	mutex_lock(&pci_acpi_companion_lookup_mtx);
> +
> +	pci_acpi_find_companion_hook = NULL;
> +	static_branch_disable(&pci_acpi_companion_lookup_key);
> +
> +	mutex_unlock(&pci_acpi_companion_lookup_mtx);
> +}
> +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 +1232,18 @@ static struct acpi_device *acpi_pci_find
>  	bool check_children;
>  	u64 addr;
>  
> +	if (static_branch_unlikely(&pci_acpi_companion_lookup_key)) {
> +		mutex_lock(&pci_acpi_companion_lookup_mtx);
> +
> +		adev = pci_acpi_find_companion_hook ?
> +			pci_acpi_find_companion_hook(pci_dev) : NULL;
> +
> +		mutex_unlock(&pci_acpi_companion_lookup_mtx);
> +
> +		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) { }

[1] https://lore.kernel.org/r/20210817180937.3123-8-ameynarkhede03@gmail.com
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/static-keys.rst?id=v5.13

Re: [PATCH] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus

[Rafael J. Wysocki]

On Fri, Aug 20, 2021 at 7:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Shanker, Amey]
>
> On Fri, Aug 20, 2021 at 06:12:39PM +0200, Rafael J. Wysocki wrote:
> > 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>
> > Tested-by: Wendy Wang <wendy.wang@intel.com>
> > ---
> >  drivers/pci/controller/vmd.c |   48 ++++++++++++++++++++++++++
> >  drivers/pci/host-bridge.c    |    1
> >  drivers/pci/pci-acpi.c       |   78 +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-acpi.h     |    3 +
> >  4 files changed, 130 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,49 @@ 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;
> > +     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 +791,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);
>
> This affects the same path as Shanker's patch to set the
> ACPI_COMPANION earlier, in pci_setup_device() instead of in
> acpi_bind_one() [1].

Thanks for the pointer.

> I think this should still work correctly since vmd_acpi_begin()
> inserts the VMD-specific hook in acpi_pci_find_companion() before we
> call pci_scan_child_bus() to enumerate devices below VMD.
>
> So when Shanker's new pci_set_acpi_fwnode() calls
> acpi_pci_find_companion(), it should get to vmd_acpi_find_companion()
> even though it happens earlier than acpi_bind_one().
>
> Just FYI so you can double check.

Yes, it should still work.

> > @@ -760,6 +806,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
> > @@ -1159,6 +1159,72 @@ void acpi_pci_remove_bus(struct pci_bus
> >  }
> >
> >  /* ACPI bus type */
> > +
> > +
> > +DEFINE_STATIC_KEY_FALSE(pci_acpi_companion_lookup_key);
>
> I don't know anything about static keys, but it looks like it
> complicates the code a fair amount, and the doc [2] suggests it's for
> performance-sensitive fast path code.
>
> Is this a path that needs that much optimization?  Or is this static
> key dance something that's *always* needed when we change a function
> pointer at run-time?

Well, taking the mutex and checking if the hook is present every time
a PCI device is enumerated sounds to me like unnecessary overhead, but
functionally it can work without the static branch just fine.

Using an rwsem instead of the mutex may reduce the overhead, though.

> > +static DEFINE_MUTEX(pci_acpi_companion_lookup_mtx);
> > +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;
> > +
> > +     mutex_lock(&pci_acpi_companion_lookup_mtx);
> > +
> > +     if (pci_acpi_find_companion_hook) {
> > +             ret = -EBUSY;
> > +     } else {
> > +             pci_acpi_find_companion_hook = func;
> > +             static_branch_enable(&pci_acpi_companion_lookup_key);
> > +             ret = 0;
> > +     }
> > +
> > +     mutex_unlock(&pci_acpi_companion_lookup_mtx);
> > +
> > +     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)
> > +{
> > +     mutex_lock(&pci_acpi_companion_lookup_mtx);
> > +
> > +     pci_acpi_find_companion_hook = NULL;
> > +     static_branch_disable(&pci_acpi_companion_lookup_key);
> > +
> > +     mutex_unlock(&pci_acpi_companion_lookup_mtx);
> > +}
> > +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 +1232,18 @@ static struct acpi_device *acpi_pci_find
> >       bool check_children;
> >       u64 addr;
> >
> > +     if (static_branch_unlikely(&pci_acpi_companion_lookup_key)) {
> > +             mutex_lock(&pci_acpi_companion_lookup_mtx);
> > +
> > +             adev = pci_acpi_find_companion_hook ?
> > +                     pci_acpi_find_companion_hook(pci_dev) : NULL;
> > +
> > +             mutex_unlock(&pci_acpi_companion_lookup_mtx);
> > +
> > +             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) { }
>
> [1] https://lore.kernel.org/r/20210817180937.3123-8-ameynarkhede03@gmail.com
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/static-keys.rst?id=v5.13

Re: [PATCH] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus

[Jon Derrick]

> On Aug 20, 2021, at 11:12 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> 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>
> Tested-by: Wendy Wang <wendy.wang@intel.com>
> ---
> drivers/pci/controller/vmd.c |   48 ++++++++++++++++++++++++++
> drivers/pci/host-bridge.c    |    1 
> drivers/pci/pci-acpi.c       |   78 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/pci-acpi.h     |    3 +
> 4 files changed, 130 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,49 @@ 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;
Can we use is_vmd(pci_dev->bus)?

> +
> +    bridge = pci_find_host_bridge(pci_dev->bus);
> +    busnr = pci_dev->bus->number - bridge->bus->number;
This is just the bus->number - vmd->busn_start, correct?


> +    addr = (busnr << 24) | ((u32)pci_dev->devfn << 16) | 0x8000FFFFU;
So the descriptor assumes busnr < 128 and requires an appropriately sized CFGBAR. Client is typically limited to 32 sub device buses but I’m not certain if enterprise will ever need > 128 (256 is allowed).

> +
> +    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 +791,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 +806,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
> @@ -1159,6 +1159,72 @@ void acpi_pci_remove_bus(struct pci_bus
> }
> 
> /* ACPI bus type */
> +
> +
> +DEFINE_STATIC_KEY_FALSE(pci_acpi_companion_lookup_key);
> +static DEFINE_MUTEX(pci_acpi_companion_lookup_mtx);
> +static struct acpi_device *(*pci_acpi_find_companion_hook)(struct pci_dev *);
Wouldn’t a hook list be a better structure? Or is that too heavy handed for the purpose?

> +
> +/**
> + * 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;
> +
> +    mutex_lock(&pci_acpi_companion_lookup_mtx);
> +
> +    if (pci_acpi_find_companion_hook) {
> +        ret = -EBUSY;
> +    } else {
> +        pci_acpi_find_companion_hook = func;
> +        static_branch_enable(&pci_acpi_companion_lookup_key);
> +        ret = 0;
> +    }
> +
> +    mutex_unlock(&pci_acpi_companion_lookup_mtx);
> +
> +    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)
> +{
> +    mutex_lock(&pci_acpi_companion_lookup_mtx);
> +
> +    pci_acpi_find_companion_hook = NULL;
> +    static_branch_disable(&pci_acpi_companion_lookup_key);
> +
> +    mutex_unlock(&pci_acpi_companion_lookup_mtx);
> +}
> +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 +1232,18 @@ static struct acpi_device *acpi_pci_find
>   bool check_children;
>   u64 addr;
> 
> +    if (static_branch_unlikely(&pci_acpi_companion_lookup_key)) {
> +        mutex_lock(&pci_acpi_companion_lookup_mtx);
> +
> +        adev = pci_acpi_find_companion_hook ?
> +            pci_acpi_find_companion_hook(pci_dev) : NULL;
> +
> +        mutex_unlock(&pci_acpi_companion_lookup_mtx);
> +
> +        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) { }

Re: [PATCH] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus

[Rafael J. Wysocki]

On Fri, Aug 20, 2021 at 7:16 PM Jon Derrick <jonathan.derrick@linux.dev> wrote:
>
>
>
> > On Aug 20, 2021, at 11:12 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > 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>
> > Tested-by: Wendy Wang <wendy.wang@intel.com>
> > ---
> > drivers/pci/controller/vmd.c |   48 ++++++++++++++++++++++++++
> > drivers/pci/host-bridge.c    |    1
> > drivers/pci/pci-acpi.c       |   78 +++++++++++++++++++++++++++++++++++++++++++
> > include/linux/pci-acpi.h     |    3 +
> > 4 files changed, 130 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,49 @@ 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;
> Can we use is_vmd(pci_dev->bus)?

Sure.

> > +
> > +    bridge = pci_find_host_bridge(pci_dev->bus);
> > +    busnr = pci_dev->bus->number - bridge->bus->number;
> This is just the bus->number - vmd->busn_start, correct?

Yes, it is, but vmd is not known here AFAICS.

>
> > +    addr = (busnr << 24) | ((u32)pci_dev->devfn << 16) | 0x8000FFFFU;
> So the descriptor assumes busnr < 128 and requires an appropriately sized CFGBAR. Client is typically limited to 32 sub device buses but I’m not certain if enterprise will ever need > 128 (256 is allowed).

It actually assumes busnr < 32.

Well, that's what the formula is, but I need to return NULL if > 31.

I'm not expecting the ACPI objects to be present on servers even.

> > +
> > +    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 +791,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 +806,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
> > @@ -1159,6 +1159,72 @@ void acpi_pci_remove_bus(struct pci_bus
> > }
> >
> > /* ACPI bus type */
> > +
> > +
> > +DEFINE_STATIC_KEY_FALSE(pci_acpi_companion_lookup_key);
> > +static DEFINE_MUTEX(pci_acpi_companion_lookup_mtx);
> > +static struct acpi_device *(*pci_acpi_find_companion_hook)(struct pci_dev *);
> Wouldn’t a hook list be a better structure? Or is that too heavy handed for the purpose?

It might, but would that list ever contain more than 1 entry?  If not,
then it would be a redundant complication IMO.

> > +
> > +/**
> > + * 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;
> > +
> > +    mutex_lock(&pci_acpi_companion_lookup_mtx);
> > +
> > +    if (pci_acpi_find_companion_hook) {
> > +        ret = -EBUSY;
> > +    } else {
> > +        pci_acpi_find_companion_hook = func;
> > +        static_branch_enable(&pci_acpi_companion_lookup_key);
> > +        ret = 0;
> > +    }
> > +
> > +    mutex_unlock(&pci_acpi_companion_lookup_mtx);
> > +
> > +    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)
> > +{
> > +    mutex_lock(&pci_acpi_companion_lookup_mtx);
> > +
> > +    pci_acpi_find_companion_hook = NULL;
> > +    static_branch_disable(&pci_acpi_companion_lookup_key);
> > +
> > +    mutex_unlock(&pci_acpi_companion_lookup_mtx);
> > +}
> > +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 +1232,18 @@ static struct acpi_device *acpi_pci_find
> >   bool check_children;
> >   u64 addr;
> >
> > +    if (static_branch_unlikely(&pci_acpi_companion_lookup_key)) {
> > +        mutex_lock(&pci_acpi_companion_lookup_mtx);
> > +
> > +        adev = pci_acpi_find_companion_hook ?
> > +            pci_acpi_find_companion_hook(pci_dev) : NULL;
> > +
> > +        mutex_unlock(&pci_acpi_companion_lookup_mtx);
> > +
> > +        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) { }

Re: [PATCH] PCI: VMD: ACPI: Make ACPI companion lookup work for VMD bus

[kernel test robot]

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

Hi "Rafael,

I love your patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/PCI-VMD-ACPI-Make-ACPI-companion-lookup-work-for-VMD-bus/20210821-001443
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-s032-20210821 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-348-gf0e6938b-dirty
        # https://github.com/0day-ci/linux/commit/0722ac93bc98deb9a9c445d7dfb78e7e068c9cb0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafael-J-Wysocki/PCI-VMD-ACPI-Make-ACPI-companion-lookup-work-for-VMD-bus/20210821-001443
        git checkout 0722ac93bc98deb9a9c445d7dfb78e7e068c9cb0
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/pci/

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


sparse warnings: (new ones prefixed by >>)
   drivers/pci/pci-acpi.c:1019:64: sparse: sparse: restricted pci_power_t degrades to integer
>> drivers/pci/pci-acpi.c:1164:1: sparse: sparse: symbol 'pci_acpi_companion_lookup_key' was not declared. Should it be static?

Please review and possibly fold the followup patch.

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

[RFC PATCH] PCI: VMD: ACPI: pci_acpi_companion_lookup_key can be static

[kernel test robot]

drivers/pci/pci-acpi.c:1164:1: warning: symbol 'pci_acpi_companion_lookup_key' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 pci-acpi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 535f43034a073..c8718aeafae5d 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1161,7 +1161,7 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
 /* ACPI bus type */
 
 
-DEFINE_STATIC_KEY_FALSE(pci_acpi_companion_lookup_key);
+static DEFINE_STATIC_KEY_FALSE(pci_acpi_companion_lookup_key);
 static DEFINE_MUTEX(pci_acpi_companion_lookup_mtx);
 static struct acpi_device *(*pci_acpi_find_companion_hook)(struct pci_dev *);