From: Bjorn Helgaas <bhelgaas@google.com>

If devicetree describes power supplies related to a PCI device,
pci_pwrctrl_create_device() previously created a pwrctrl platform device
even if CONFIG_PCI_PWRCTL was not enabled.

When pci_pwrctrl_create_device() creates a pwrctrl device and returns it,
pci_scan_device() doesn't enumerating the PCI device on the assumption that
the pwrctrl core will rescan the bus after turning on the power.  If
CONFIG_PCI_PWRCTL is not enabled, the rescan never happens.

This breaks 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.

If CONFIG_PCI_PWRCTL is not enabled, skip creating the pwrctrl platform
device and scan the device normally.

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
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
This an alternate to
https://lore.kernel.org/r/20250522140326.93869-1-manivannan.sadhasivam@linaro.org

It should accomplish the same thing but I think using #ifdef makes it a
little more visible and easier to see that pci_pwrctrl_create_device() is
only relevant when CONFIG_PCI_PWRCTL is enabled.

 drivers/pci/probe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 364fa2a514f8..855da472b608 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2510,6 +2510,7 @@ EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
 
 static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
 {
+#if defined(CONFIG_PCI_PWRCTL) || defined(CONFIG_PCI_PWRCTL_MODULE)
 	struct pci_host_bridge *host = pci_find_host_bridge(bus);
 	struct platform_device *pdev;
 	struct device_node *np;
@@ -2536,6 +2537,9 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
 	}
 
 	return pdev;
+#else
+	return NULL;
+#endif
 }
 
 /*
-- 
2.43.0

On Fri, May 23, 2025 at 03:17:59PM -0500, Bjorn Helgaas wrote:
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2510,6 +2510,7 @@ EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
>  
>  static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
>  {
> +#if defined(CONFIG_PCI_PWRCTL) || defined(CONFIG_PCI_PWRCTL_MODULE)
>  	struct pci_host_bridge *host = pci_find_host_bridge(bus);
>  	struct platform_device *pdev;
>  	struct device_node *np;
> @@ -2536,6 +2537,9 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
>  	}
>  
>  	return pdev;
> +#else
> +	return NULL;
> +#endif
>  }
[...]
> This an alternate to
> https://lore.kernel.org/r/20250522140326.93869-1-manivannan.sadhasivam@linaro.org
> 
> It should accomplish the same thing but I think using #ifdef makes it a
> little more visible and easier to see that pci_pwrctrl_create_device() is
> only relevant when CONFIG_PCI_PWRCTL is enabled.

Just noting though that section 21 of Documentation/process/coding-style.rst
discourages use of #ifdef and recommends IS_ENABLED() and inline stubs
instead.

Thanks,

Lukas

On Fri, May 23, 2025 at 11:26:15PM +0200, Lukas Wunner wrote:
> On Fri, May 23, 2025 at 03:17:59PM -0500, Bjorn Helgaas wrote:
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2510,6 +2510,7 @@ EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
> >  
> >  static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> >  {
> > +#if defined(CONFIG_PCI_PWRCTL) || defined(CONFIG_PCI_PWRCTL_MODULE)
> >  	struct pci_host_bridge *host = pci_find_host_bridge(bus);
> >  	struct platform_device *pdev;
> >  	struct device_node *np;
> > @@ -2536,6 +2537,9 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
> >  	}
> >  
> >  	return pdev;
> > +#else
> > +	return NULL;
> > +#endif
> >  }
> [...]
> > This an alternate to
> > https://lore.kernel.org/r/20250522140326.93869-1-manivannan.sadhasivam@linaro.org
> > 
> > It should accomplish the same thing but I think using #ifdef makes it a
> > little more visible and easier to see that pci_pwrctrl_create_device() is
> > only relevant when CONFIG_PCI_PWRCTL is enabled.
> 
> Just noting though that section 21 of Documentation/process/coding-style.rst
> discourages use of #ifdef and recommends IS_ENABLED() and inline stubs
> instead.

True, although I kind of object to IS_ENABLED() in cases like this
where what we really want is a no-op function, because IS_ENABLED()
forces me to mentally execute the function to see what's going on.

What I would prefer is something like the first paragraph in that
section: the #ifdef in a header file that declares the function and
defines a no-op stub, with the implementation in some pwrctrl file.

But now since we're considering this for v6.15 in a couple days, we
need a minimal fix.  Mani's original patch is probably the best for
that, and I'm fine with that, although I don't think we should throw
brcmstb under the bus.  The pci_pwrctrl_create_device() assumption
that pwrctrl is enabled is just broken, and I don't think the fix is
to improve pwrctrl and convert drivers.

Maybe we could use Mani's patch with something like my commit log
(it's quite possible I don't understand the situation completely):

  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.  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.

  If CONFIG_PCI_PWRCTL is not enabled, skip creating the pwrctrl
  platform device and scan the device normally.

Looking at pci_pwrctrl_create_device() raised more questions for me:

  - If pwrctrl is enabled, devicetree describes a regulator, and the
    power is already on, do we really want to defer enumeration?

  - If pwrctrl *not* enabled, devicetree describes a regulator, and
    the power is off, is there anything in dmesg that gives us a clue
    about why we don't enumerate the device?  Could/should we emit a
    message as a hint?

  - Creating a pwrctrl device, returning a pointer to it, and then
    throwing that pointer away seems ... weird?  Makes me wonder about
    the lifetime management of that device.

Bjorn

On Fri, May 23, 2025 at 09:42:07PM -0500, Bjorn Helgaas wrote:
> What I would prefer is something like the first paragraph in that
> section: the #ifdef in a header file that declares the function and
> defines a no-op stub, with the implementation in some pwrctrl file.

pci_pwrctrl_create_device() is static, but it is possible to #ifdef
the whole function in the .c file and provide the stub in an #else
branch.  That's easier to follow than #ifdef'ing portions of the
function.

Thanks,

Lukas

On Sat, May 24, 2025 at 08:29:46AM +0200, Lukas Wunner wrote:
> On Fri, May 23, 2025 at 09:42:07PM -0500, Bjorn Helgaas wrote:
> > What I would prefer is something like the first paragraph in that
> > section: the #ifdef in a header file that declares the function and
> > defines a no-op stub, with the implementation in some pwrctrl file.
> 
> pci_pwrctrl_create_device() is static, but it is possible to #ifdef
> the whole function in the .c file and provide the stub in an #else
> branch.  That's easier to follow than #ifdef'ing portions of the
> function.
> 

+1

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On Sat, May 24, 2025 at 02:21:04PM +0530, Manivannan Sadhasivam wrote:
> On Sat, May 24, 2025 at 08:29:46AM +0200, Lukas Wunner wrote:
> > On Fri, May 23, 2025 at 09:42:07PM -0500, Bjorn Helgaas wrote:
> > > What I would prefer is something like the first paragraph in that
> > > section: the #ifdef in a header file that declares the function and
> > > defines a no-op stub, with the implementation in some pwrctrl file.
> > 
> > pci_pwrctrl_create_device() is static, but it is possible to #ifdef
> > the whole function in the .c file and provide the stub in an #else
> > branch.  That's easier to follow than #ifdef'ing portions of the
> > function.
> > 
> 
> +1

I dropped the ball here and didn't get any fix for this in v6.15.

Why do we need pci_pwrctrl_create_device() in drivers/pci/probe.c?
The obvious thing would have been to put the implementation in
drivers/pci/pwrctrl with a stub in drivers/pci/pci.h, so I assume
there's some reason we can't do that?

On 5/27/25 15:25, 'Bjorn Helgaas' via BCM-KERNEL-FEEDBACK-LIST,PDL wrote:
> On Sat, May 24, 2025 at 02:21:04PM +0530, Manivannan Sadhasivam wrote:
>> On Sat, May 24, 2025 at 08:29:46AM +0200, Lukas Wunner wrote:
>>> On Fri, May 23, 2025 at 09:42:07PM -0500, Bjorn Helgaas wrote:
>>>> What I would prefer is something like the first paragraph in that
>>>> section: the #ifdef in a header file that declares the function and
>>>> defines a no-op stub, with the implementation in some pwrctrl file.
>>>
>>> pci_pwrctrl_create_device() is static, but it is possible to #ifdef
>>> the whole function in the .c file and provide the stub in an #else
>>> branch.  That's easier to follow than #ifdef'ing portions of the
>>> function.
>>>
>>
>> +1
> 
> I dropped the ball here and didn't get any fix for this in v6.15.

Any chance we could target a fix to be back ported into 6.15 stable? Do 
you need people to help you test it?
-- 
Florian

On Tue, May 27, 2025 at 05:25:22PM -0500, Bjorn Helgaas wrote:
> On Sat, May 24, 2025 at 02:21:04PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, May 24, 2025 at 08:29:46AM +0200, Lukas Wunner wrote:
> > > On Fri, May 23, 2025 at 09:42:07PM -0500, Bjorn Helgaas wrote:
> > > > What I would prefer is something like the first paragraph in that
> > > > section: the #ifdef in a header file that declares the function and
> > > > defines a no-op stub, with the implementation in some pwrctrl file.
> > > 
> > > pci_pwrctrl_create_device() is static, but it is possible to #ifdef
> > > the whole function in the .c file and provide the stub in an #else
> > > branch.  That's easier to follow than #ifdef'ing portions of the
> > > function.
> > > 
> > 
> > +1
> 
> I dropped the ball here and didn't get any fix for this in v6.15.
> 

That's unfortunate. Let's try to get it backported through stable list.

> Why do we need pci_pwrctrl_create_device() in drivers/pci/probe.c?
> The obvious thing would have been to put the implementation in
> drivers/pci/pwrctrl with a stub in drivers/pci/pci.h, so I assume
> there's some reason we can't do that?

I assumed initially that pwrctrl dependency would always be there with PCI core.
Since now it is clear that we need to build the function conditionally, we could
move it to drivers/pci/pwrctrl/core.c with stub in drivers/pci/pci.h.

I will submit a patch for that.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On Tue, May 27, 2025 at 6:25 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Sat, May 24, 2025 at 02:21:04PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, May 24, 2025 at 08:29:46AM +0200, Lukas Wunner wrote:
> > > On Fri, May 23, 2025 at 09:42:07PM -0500, Bjorn Helgaas wrote:
> > > > What I would prefer is something like the first paragraph in that
> > > > section: the #ifdef in a header file that declares the function and
> > > > defines a no-op stub, with the implementation in some pwrctrl file.
> > >
> > > pci_pwrctrl_create_device() is static, but it is possible to #ifdef
> > > the whole function in the .c file and provide the stub in an #else
> > > branch.  That's easier to follow than #ifdef'ing portions of the
> > > function.
> > >
> >
> > +1
>
> I dropped the ball here and didn't get any fix for this in v6.15.

:-(

>
>
> Why do we need pci_pwrctrl_create_device() in drivers/pci/probe.c?
> The obvious thing would have been to put the implementation in
> drivers/pci/pwrctrl with a stub in drivers/pci/pci.h, so I assume
> there's some reason we can't do that?

I was wondering if we could confine PWRCTL/_SLOT to work on a per PCIe
controller basis.  For example, if we allow the port DT node to have
boolean "pwrctrl;" property, it would direct the PWRCTL code to
operate on the regulators within that node.  This would allow
CONFIG_PWRCTL/_SLOT and the pcie-brcmstb.c way of controlling
regulators to happily coexist.

One could argue that "pwrctrl;" does not describe the HW as a DT
property should, but I think it should be considered nevertheless.

Regards,
Jim Quinlan
Broadcom STB

On Thu, May 29, 2025 at 03:23:29PM -0400, Jim Quinlan wrote:
> On Tue, May 27, 2025 at 6:25 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Sat, May 24, 2025 at 02:21:04PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, May 24, 2025 at 08:29:46AM +0200, Lukas Wunner wrote:
> > > > On Fri, May 23, 2025 at 09:42:07PM -0500, Bjorn Helgaas wrote:
> > > > > What I would prefer is something like the first paragraph in that
> > > > > section: the #ifdef in a header file that declares the function and
> > > > > defines a no-op stub, with the implementation in some pwrctrl file.
> > > >
> > > > pci_pwrctrl_create_device() is static, but it is possible to #ifdef
> > > > the whole function in the .c file and provide the stub in an #else
> > > > branch.  That's easier to follow than #ifdef'ing portions of the
> > > > function.
> > > >
> > >
> > > +1
> >
> > I dropped the ball here and didn't get any fix for this in v6.15.
> 
> :-(
> 
> >
> >
> > Why do we need pci_pwrctrl_create_device() in drivers/pci/probe.c?
> > The obvious thing would have been to put the implementation in
> > drivers/pci/pwrctrl with a stub in drivers/pci/pci.h, so I assume
> > there's some reason we can't do that?
> 
> I was wondering if we could confine PWRCTL/_SLOT to work on a per PCIe
> controller basis.  For example, if we allow the port DT node to have
> boolean "pwrctrl;" property, it would direct the PWRCTL code to
> operate on the regulators within that node.  This would allow
> CONFIG_PWRCTL/_SLOT and the pcie-brcmstb.c way of controlling
> regulators to happily coexist.
> 
> One could argue that "pwrctrl;" does not describe the HW as a DT
> property should, but I think it should be considered nevertheless.
> 

No. There were a lot of attempts previously before the advent of pwrctrl
framework to add a DT property for powering the devices, but DT maintainer
clearly said that there should be no DT property for this purpose. So we came up
with this approach.

- Mani

-- 
மணிவண்ணன் சதாசிவம்