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@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Tested-by: Wendy Wang <wendy.wang@xxxxxxxxx>
> ---
>  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@xxxxxxxxx
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/static-keys.rst?id=v5.13