Re: [PATCH v3] PCI/pwrctrl: Move pci_pwrctrl_create_device() definition to drivers/pci/pwrctrl/

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Jun 16, 2025 at 11:02:09AM +0530, Manivannan Sadhasivam wrote:
> pci_pwrctrl_create_device() is a PWRCTRL framework API. So it should be
> built only when CONFIG_PWRCTRL is enabled. Currently, it is built
> independently of CONFIG_PWRCTRL. This creates enumeration failure on
> platforms like brcmstb using out-of-tree devicetree that describes the
> power supplies for endpoints in the PCIe child node, but doesn't use
> PWRCTRL framework to manage the supplies. The controller driver itself
> manages the supplies.
> 
> But in any case, the API should be built only when CONFIG_PWRCTRL is
> enabled. So move its definition to drivers/pci/pwrctrl/core.c and provide
> a stub in drivers/pci/pci.h when CONFIG_PWRCTRL is not enabled. This also
> fixes the enumeration issues on the affected platforms.

Finally circling back to this since I think brcmstb is broken since
v6.15 and we should fix it for v6.16 final.

IIUC, v3 is the current patch and needs at least a fix for the build
issue [1], and I guess the options are:

  1) Make CONFIG_PCI_PWRCTRL bool.  On my x86-64 system
     pci-pwrctrl-core.o is 8880 bytes, which seems like kind of a lot
     when only a few systems need it.

  2) Leave pci_pwrctrl_create_device() in probe.c.  It gets optimized
     away if CONFIG_OF=n because of_pci_find_child_device() returns
     NULL, but still a little ugly for readers.

  3) Put pci_pwrctrl_create_device() in a separate
     drivers/pci/pwrctrl/ file that is always compiled even if PWRCTRL
     itself is a module.  Ugly because then we sort of have two "core"
     files (core.c and whatever new file is always compiled).

And I guess all of these options still depend on CONFIG_PCI_PWRCTRL
not being enabled in a kernel that has brcmstb enabled?  If so, that
seems ugly to me.  We should be able to enable both PWRCTRL and
brcmstb at the same time, e.g., for a single kernel image that works
both on a brcmstb system and a system that needs pwrctrl.

Bjorn

[1] https://lore.kernel.org/r/202506162013.go7YyNYL-lkp@intel.com

> Fixes: 957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
> Reported-by: Jim Quinlan <james.quinlan@broadcom.com>
> Closes: https://lore.kernel.org/r/CA+-6iNwgaByXEYD3j=-+H_PKAxXRU78svPMRHDKKci8AGXAUPg@mail.gmail.com
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
> 
> Changes in v3:
> 
> * Used IS_ENABLED instead of ifdef in drivers/pci/pci.h (Sathyanarayanan)
> 
> Changes in v2:
> 
> * Dropped the unused headers from probe.c (Lukas)
> 
>  drivers/pci/pci.h          |  8 ++++++++
>  drivers/pci/probe.c        | 32 --------------------------------
>  drivers/pci/pwrctrl/core.c | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 44 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 12215ee72afb..22df9e2bfda6 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -1159,4 +1159,12 @@ static inline int pci_msix_write_tph_tag(struct pci_dev *pdev, unsigned int inde
>  	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
>  	 PCI_CONF1_EXT_REG(reg))
>  
> +#if IS_ENABLED(CONFIG_PCI_PWRCTRL)
> +struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus,
> +						  int devfn);
> +#else
> +static inline struct platform_device *
> +pci_pwrctrl_create_device(struct pci_bus *bus, int devfn) { return NULL; }
> +#endif
> +
>  #endif /* DRIVERS_PCI_H */
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4b8693ec9e4c..478e217928a6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -9,8 +9,6 @@
>  #include <linux/pci.h>
>  #include <linux/msi.h>
>  #include <linux/of_pci.h>
> -#include <linux/of_platform.h>
> -#include <linux/platform_device.h>
>  #include <linux/pci_hotplug.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> @@ -2508,36 +2506,6 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>  }
>  EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
>  
> -static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> -{
> -	struct pci_host_bridge *host = pci_find_host_bridge(bus);
> -	struct platform_device *pdev;
> -	struct device_node *np;
> -
> -	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
> -	if (!np || of_find_device_by_node(np))
> -		return NULL;
> -
> -	/*
> -	 * First check whether the pwrctrl device really needs to be created or
> -	 * not. This is decided based on at least one of the power supplies
> -	 * being defined in the devicetree node of the device.
> -	 */
> -	if (!of_pci_supply_present(np)) {
> -		pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
> -		return NULL;
> -	}
> -
> -	/* Now create the pwrctrl device */
> -	pdev = of_platform_device_create(np, NULL, &host->dev);
> -	if (!pdev) {
> -		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for node: %s\n", np->name);
> -		return NULL;
> -	}
> -
> -	return pdev;
> -}
> -
>  /*
>   * Read the config data for a PCI device, sanity-check it,
>   * and fill in the dev structure.
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 6bdbfed584d6..20585b2c3681 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -6,11 +6,47 @@
>  #include <linux/device.h>
>  #include <linux/export.h>
>  #include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
>  #include <linux/pci.h>
>  #include <linux/pci-pwrctrl.h>
> +#include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  
> +#include "../pci.h"
> +
> +struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> +{
> +	struct pci_host_bridge *host = pci_find_host_bridge(bus);
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +
> +	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
> +	if (!np || of_find_device_by_node(np))
> +		return NULL;
> +
> +	/*
> +	 * First check whether the pwrctrl device really needs to be created or
> +	 * not. This is decided based on at least one of the power supplies
> +	 * being defined in the devicetree node of the device.
> +	 */
> +	if (!of_pci_supply_present(np)) {
> +		pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
> +		return NULL;
> +	}
> +
> +	/* Now create the pwrctrl device */
> +	pdev = of_platform_device_create(np, NULL, &host->dev);
> +	if (!pdev) {
> +		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for node: %s\n", np->name);
> +		return NULL;
> +	}
> +
> +	return pdev;
> +}
> +
>  static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
>  			      void *data)
>  {
> -- 
> 2.43.0
>