[+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@xxxxxxxxxxxxxxx # 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@xxxxxxxxxxxx>
> Closes: https://lore.kernel.org/r/CA+-6iNwgaByXEYD3j=-+H_PKAxXRU78svPMRHDKKci8AGXAUPg@xxxxxxxxxxxxxx
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@xxxxxxxxxx>
> ---
>
> 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
> {
