Re: [PATCH v2] PCI/pwrctrl: Skip creating pwrctrl device unless CONFIG_PCI_PWRCTRL is enabled

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Bart, Krzysztof, update Mani's addr to kernel.org]

On Tue, Jul 01, 2025 at 12:17:31PM +0530, Manivannan Sadhasivam wrote:
> If devicetree describes power supplies related to a PCI device, we
> previously created a pwrctrl device even if CONFIG_PCI_PWRCTL was
> not enabled.
> 
> When pci_pwrctrl_create_device() creates and returns a pwrctrl device,
> pci_scan_device() doesn't enumerate the PCI device. It assumes the pwrctrl
> core will rescan the bus after turning on the power. However, if
> CONFIG_PCI_PWRCTL is not enabled, the rescan never happens.
> 
> This may break PCI enumeration on any system that describes power supplies
> in devicetree but does not use pwrctrl. Jim reported that some brcmstb
> platforms break this way.
> 
> While the actual fix would be to convert all the platforms to use pwrctrl
> framework, we also need to skip creating the pwrctrl device if
> CONFIG_PCI_PWRCTL is not enabled and let the PCI core scan the device
> normally (assuming it is already powered on or by the controller driver).

I'm fine with this change, but I think the commit log leaves the wrong
impression.  If CONFIG_PCI_PWRCTRL is not enabled, we shouldn't do
anything related to it, independent of what other platforms or drivers
do.

So I wouldn't describe this as "the actual fix is converting all
platforms to use pwrctrl."  Even if all platforms use pwrctrl, we
*still* shouldn't run pci_pwrctrl_create_device() unless
CONFIG_PCI_PWRCTRL is enabled.

I think all we need to say is something like this:

  We only need pci_pwrctrl_create_device() when CONFIG_PCI_PWRCTRL is
  enabled.  Compile it out when CONFIG_PCI_PWRCTRL is not enabled.

> Cc: stable@vger.kernel.org # 6.15
> Fixes: 957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")

Not sure about this.  If the problem we're solving is "we run pwrctrl
code when CONFIG_PCI_PWRCTRL is not enabled," 957f40d039a9 is not the
commit that added that behavior.

Maybe 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF
nodes of the port node") would be more appropriate?

> Reported-by: Jim Quinlan <james.quinlan@broadcom.com>
> Closes: https://lore.kernel.org/r/CA+-6iNwgaByXEYD3j=-+H_PKAxXRU78svPMRHDKKci8AGXAUPg@mail.gmail.com

I'm also not sure this really merits a "Closes:" tag.  All this does
is enable a workaround (disable CONFIG_PCI_PWRCTRL when brcmstb is
enabled).  That's not a fix because we *should* be able to enable both
pwrctrl and brcmstb at the same time.

If 2489eeb777af ("PCI/pwrctrl: Skip scanning for the device further if
pwrctrl device is created") was purely an optimization (see
https://lore.kernel.org/r/20250701203526.GA1849466@bhelgaas), I think
I would:

  - Revert 2489eeb777af with a stable tag for v6.15, and

  - Apply this patch with a Fixes: 8fb18619d910 ("PCI/pwrctl: Create
    platform devices for child OF nodes of the port node") but no
    stable tag.  8fb18619d910 appeared in v6.11 and the "don't enable
    CONFIG_PCI_PWRCTRL" workaround was enough for brcmstb until
    2489eeb777af, so if we revert 2489eeb777af, we wouldn't need to
    backport *this* patch.

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> 
> Changes in v2:
> 
> * Used the stub instead of returning NULL inside the function
> 
>  drivers/pci/probe.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4b8693ec9e4c..e6a34db77826 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2508,6 +2508,7 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>  }
>  EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
>  
> +#if IS_ENABLED(CONFIG_PCI_PWRCTRL)
>  static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
>  {
>  	struct pci_host_bridge *host = pci_find_host_bridge(bus);
> @@ -2537,6 +2538,12 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
>  
>  	return pdev;
>  }
> +#else
> +static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> +{
> +	return NULL;
> +}
> +#endif
>  
>  /*
>   * Read the config data for a PCI device, sanity-check it,
> -- 
> 2.43.0
> {