Re: [PATCH] PCI/pwrctrl: Skip creating platform device unless CONFIG_PCI_PWRCTL enabled

[Bjorn Helgaas <helgaas@kernel.org>]

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