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

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

[Manivannan Sadhasivam]

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.

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

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

[Bartosz Golaszewski]

On Mon, Jun 16, 2025 at 7:32 AM Manivannan Sadhasivam <mani@kernel.org> 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.
>
> 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>
> ---

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

[kernel test robot]

Hi Manivannan,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.16-rc2 next-20250616]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam/PCI-pwrctrl-Move-pci_pwrctrl_create_device-definition-to-drivers-pci-pwrctrl/20250616-133314
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250616053209.13045-1-mani%40kernel.org
patch subject: [PATCH v3] PCI/pwrctrl: Move pci_pwrctrl_create_device() definition to drivers/pci/pwrctrl/
config: i386-buildonly-randconfig-001-20250616 (https://download.01.org/0day-ci/archive/20250616/202506162013.go7YyNYL-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250616/202506162013.go7YyNYL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506162013.go7YyNYL-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/pci/probe.o: in function `pci_scan_single_device':
>> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Manivannan Sadhasivam]

On Mon, Jun 16, 2025 at 08:21:20PM +0800, kernel test robot wrote:
> Hi Manivannan,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on pci/next]
> [also build test ERROR on pci/for-linus linus/master v6.16-rc2 next-20250616]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam/PCI-pwrctrl-Move-pci_pwrctrl_create_device-definition-to-drivers-pci-pwrctrl/20250616-133314
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> patch link:    https://lore.kernel.org/r/20250616053209.13045-1-mani%40kernel.org
> patch subject: [PATCH v3] PCI/pwrctrl: Move pci_pwrctrl_create_device() definition to drivers/pci/pwrctrl/
> config: i386-buildonly-randconfig-001-20250616 (https://download.01.org/0day-ci/archive/20250616/202506162013.go7YyNYL-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250616/202506162013.go7YyNYL-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202506162013.go7YyNYL-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    ld: drivers/pci/probe.o: in function `pci_scan_single_device':
> >> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'

Hmm, so we cannot have a built-in driver depend on a module...

Bartosz, should we make CONFIG_PCI_PWRCTRL bool then? We can still allow the
individual pwrctrl drivers be tristate.

- Mani

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

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

[Lukas Wunner]

On Mon, Jun 16, 2025 at 06:07:48PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jun 16, 2025 at 08:21:20PM +0800, kernel test robot wrote:
> >    ld: drivers/pci/probe.o: in function `pci_scan_single_device':
> > >> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'
> 
> Hmm, so we cannot have a built-in driver depend on a module...

Does it work if you export pci_pwrctrl_create_device()?

Thanks,

Lukas

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

[Manivannan Sadhasivam]

On Mon, Jun 16, 2025 at 02:52:03PM +0200, Lukas Wunner wrote:
> On Mon, Jun 16, 2025 at 06:07:48PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jun 16, 2025 at 08:21:20PM +0800, kernel test robot wrote:
> > >    ld: drivers/pci/probe.o: in function `pci_scan_single_device':
> > > >> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'
> > 
> > Hmm, so we cannot have a built-in driver depend on a module...
> 
> Does it work if you export pci_pwrctrl_create_device()?
> 

No it doesn't. And that's expected since the export is usually *for* the
modules and not the other way around.

- Mani

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

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

[Lukas Wunner]

On Mon, Jun 16, 2025 at 06:07:48PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jun 16, 2025 at 08:21:20PM +0800, kernel test robot wrote:
> >    ld: drivers/pci/probe.o: in function `pci_scan_single_device':
> > >> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'
> 
> Hmm, so we cannot have a built-in driver depend on a module...
> 
> Bartosz, should we make CONFIG_PCI_PWRCTRL bool then? We can still allow the
> individual pwrctrl drivers be tristate.

I guess the alternative is to just leave it in probe.c.  The function is
optimized away in the CONFIG_OF=n case because of_pci_find_child_device()
returns NULL.  It's unpleasant that it lives outside of pwrctrl/core.c,
but it doesn't occupy any space in the compiled kernel at least on non-OF
(e.g. ACPI) platforms.

Thanks,

Lukas

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

[Bartosz Golaszewski]

On Mon, Jun 16, 2025 at 3:16 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Mon, Jun 16, 2025 at 06:07:48PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jun 16, 2025 at 08:21:20PM +0800, kernel test robot wrote:
> > >    ld: drivers/pci/probe.o: in function `pci_scan_single_device':
> > > >> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'
> >
> > Hmm, so we cannot have a built-in driver depend on a module...
> >
> > Bartosz, should we make CONFIG_PCI_PWRCTRL bool then? We can still allow the
> > individual pwrctrl drivers be tristate.
>
> I guess the alternative is to just leave it in probe.c.  The function is
> optimized away in the CONFIG_OF=n case because of_pci_find_child_device()
> returns NULL.  It's unpleasant that it lives outside of pwrctrl/core.c,
> but it doesn't occupy any space in the compiled kernel at least on non-OF
> (e.g. ACPI) platforms.
>

And there's a third option of having this function live in a separate
.c file under drivers/pci/pwrctl/ that would be always built-in even
if PWRCTL itself is a module. The best/worst of two worlds? :)

Bart

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

[Manivannan Sadhasivam]

On Mon, Jun 16, 2025 at 03:30:27PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jun 16, 2025 at 3:16 PM Lukas Wunner <lukas@wunner.de> wrote:
> >
> > On Mon, Jun 16, 2025 at 06:07:48PM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Jun 16, 2025 at 08:21:20PM +0800, kernel test robot wrote:
> > > >    ld: drivers/pci/probe.o: in function `pci_scan_single_device':
> > > > >> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'
> > >
> > > Hmm, so we cannot have a built-in driver depend on a module...
> > >
> > > Bartosz, should we make CONFIG_PCI_PWRCTRL bool then? We can still allow the
> > > individual pwrctrl drivers be tristate.
> >
> > I guess the alternative is to just leave it in probe.c.  The function is
> > optimized away in the CONFIG_OF=n case because of_pci_find_child_device()
> > returns NULL.  It's unpleasant that it lives outside of pwrctrl/core.c,
> > but it doesn't occupy any space in the compiled kernel at least on non-OF
> > (e.g. ACPI) platforms.
> >
> 
> And there's a third option of having this function live in a separate
> .c file under drivers/pci/pwrctl/ that would be always built-in even
> if PWRCTL itself is a module. The best/worst of two worlds? :)
> 

I would try to avoid the third option at any cost ;) Because the pwrctrl/core.c
would no longer be the 'core' and the code structure would look distorted.

Let's see what Bjorn thinks about option 1 and 2. I did not account for the
built-in vs modular dependency when Bjorn asked me if the move was possible. And
I now vaguely remember that I kept it in probe.c initially because of this
dependency.
 
- Mani

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

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

[Bjorn Helgaas]

On Mon, Jun 16, 2025 at 07:14:50PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jun 16, 2025 at 03:30:27PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Jun 16, 2025 at 3:16 PM Lukas Wunner <lukas@wunner.de> wrote:
> > >
> > > On Mon, Jun 16, 2025 at 06:07:48PM +0530, Manivannan Sadhasivam wrote:
> > > > On Mon, Jun 16, 2025 at 08:21:20PM +0800, kernel test robot wrote:
> > > > >    ld: drivers/pci/probe.o: in function `pci_scan_single_device':
> > > > > >> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'
> > > >
> > > > Hmm, so we cannot have a built-in driver depend on a module...
> > > >
> > > > Bartosz, should we make CONFIG_PCI_PWRCTRL bool then? We can
> > > > still allow the individual pwrctrl drivers be tristate.

I think I'm OK with making CONFIG_PCI_PWRCTRL bool.  What is the
argument for making it a module?

When pwrctrl is a module, it seems like we have no way to even
indicate to the user that "there might be PCI devices here that could
be powered on and enumerated."  That feels like information users
ought to be able to get.

I do wonder if we're building a structure parallel to ACPI
functionality and whether there could/should be some approach that
unifies both.  But that's a tangent to this current issue.

> > > I guess the alternative is to just leave it in probe.c.  The
> > > function is optimized away in the CONFIG_OF=n case because
> > > of_pci_find_child_device() returns NULL.  It's unpleasant that
> > > it lives outside of pwrctrl/core.c, but it doesn't occupy any
> > > space in the compiled kernel at least on non-OF (e.g. ACPI)
> > > platforms.

I don't like having pci_pwrctrl_create_device() in drivers/pci/probe.c
and relying on the compiler to optimize it out when
of_pci_find_child_device() returns NULL.  This is in the fundamental
device enumeration path, and I think it's unnecessary confusion for
every non-OF reader.

> > And there's a third option of having this function live in a
> > separate .c file under drivers/pci/pwrctl/ that would be always
> > built-in even if PWRCTL itself is a module. The best/worst of two
> > worlds? :)
> 
> I would try to avoid the third option at any cost ;) Because the
> pwrctrl/core.c would no longer be the 'core' and the code structure
> would look distorted.

I don't really see adding problem with a file in drivers/pci/pwrctrl/.

Whether it should be "core.c" or not, I dunno.  I think "core.c" could
make sense for things that must be present always, e.g.,
pci_pwrctrl_create_device(), and the driver itself could be
"pwrctrl.c"

If pwrctrl is built as a module, what is the module name?  I assume it
must be "pwrctrl", though Kconfig doesn't mention it and I don't grok
enough Makefile to figure it out.  "core" would be useless in the
print_modules() output.

> Let's see what Bjorn thinks about option 1 and 2. I did not account
> for the built-in vs modular dependency when Bjorn asked me if the
> move was possible. And I now vaguely remember that I kept it in
> probe.c initially because of this dependency.
>  
> - Mani
> 
> -- மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Tue, Jun 17, 2025 at 03:44:06PM -0500, Bjorn Helgaas wrote:
> On Mon, Jun 16, 2025 at 07:14:50PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jun 16, 2025 at 03:30:27PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Jun 16, 2025 at 3:16 PM Lukas Wunner <lukas@wunner.de> wrote:
> > > >
> > > > On Mon, Jun 16, 2025 at 06:07:48PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Mon, Jun 16, 2025 at 08:21:20PM +0800, kernel test robot wrote:
> > > > > >    ld: drivers/pci/probe.o: in function `pci_scan_single_device':
> > > > > > >> probe.c:(.text+0x2400): undefined reference to `pci_pwrctrl_create_device'
> > > > >
> > > > > Hmm, so we cannot have a built-in driver depend on a module...
> > > > >
> > > > > Bartosz, should we make CONFIG_PCI_PWRCTRL bool then? We can
> > > > > still allow the individual pwrctrl drivers be tristate.
> 
> I think I'm OK with making CONFIG_PCI_PWRCTRL bool.  What is the
> argument for making it a module?
> 

Only to avoid making the kernel image bigger.

> When pwrctrl is a module, it seems like we have no way to even
> indicate to the user that "there might be PCI devices here that could
> be powered on and enumerated."  That feels like information users
> ought to be able to get.
> 
> I do wonder if we're building a structure parallel to ACPI
> functionality and whether there could/should be some approach that
> unifies both.  But that's a tangent to this current issue.
> 

Well, pwrctrl is for non-ACPI platforms (mostly OF) and yes, it is parallel to
ACPI in some form, but that is inevitable since OF is just a hardware
description and ACPI is much more than that.

> > > > I guess the alternative is to just leave it in probe.c.  The
> > > > function is optimized away in the CONFIG_OF=n case because
> > > > of_pci_find_child_device() returns NULL.  It's unpleasant that
> > > > it lives outside of pwrctrl/core.c, but it doesn't occupy any
> > > > space in the compiled kernel at least on non-OF (e.g. ACPI)
> > > > platforms.
> 
> I don't like having pci_pwrctrl_create_device() in drivers/pci/probe.c
> and relying on the compiler to optimize it out when
> of_pci_find_child_device() returns NULL.  This is in the fundamental
> device enumeration path, and I think it's unnecessary confusion for
> every non-OF reader.
> 

Okay.

> > > And there's a third option of having this function live in a
> > > separate .c file under drivers/pci/pwrctl/ that would be always
> > > built-in even if PWRCTL itself is a module. The best/worst of two
> > > worlds? :)
> > 
> > I would try to avoid the third option at any cost ;) Because the
> > pwrctrl/core.c would no longer be the 'core' and the code structure
> > would look distorted.
> 
> I don't really see adding problem with a file in drivers/pci/pwrctrl/.
> 
> Whether it should be "core.c" or not, I dunno.  I think "core.c" could
> make sense for things that must be present always, e.g.,
> pci_pwrctrl_create_device(), and the driver itself could be
> "pwrctrl.c"
> 

Then it will always end up in confusion of where to place the functions and
such. For sure the Kconfig can define what each file stands for, but IMO it
feels weird to have 2 files for the same purpose.

But if you insist, I can go for it.

> If pwrctrl is built as a module, what is the module name?  I assume it
> must be "pwrctrl", though Kconfig doesn't mention it and I don't grok
> enough Makefile to figure it out.  "core" would be useless in the
> print_modules() output.
> 

It is 'pci-pwrctrl-core.ko'.

- Mani

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

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 6379 bytes --]

On Mon, Jun 16, 2025 at 1:32 AM Manivannan Sadhasivam <mani@kernel.org> 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.
>
> 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:

Hi Mani,

Thanks for working on this.  How/where should I be applying these
patches?  I've been using torvald's latest master but I get failed
hunks and have to manually fix them up.

At any rate I'll be testing whatever you are posting.

Thanks,
Jim Quinilan
Broadcom STB/CM

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4197 bytes --]

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

[Bjorn Helgaas]

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
> 

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

[Manivannan Sadhasivam]

On Fri, Jun 27, 2025 at 05:45:02PM -0500, Bjorn Helgaas wrote:
> 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.
> 

Yes! Sorry for the delay. The fact that I switched the job and had to attend OSS
NA prevented me from reworking this patch.

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

I guess, we could go with option 3 if you prefer. We could rename the existing
pwrctrl/core.c to pwrctrl/pwrctrl.c and move the definition of
pci_pwrctrl_create_device() to new pwrctrl/core.c. The new file will depend on
HAVE_PWRCTRL, which is bool.

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

Right, that would be the end goal. As I explained in the reply to the bug report
[1], this patch will serve as an interim workaround. Once my pwrctrl rework
(which I didn't submit yet) is merged, I will move this driver to use the
pwrctrl framework.

The fact that I missed this driver in the first place during the previous rework
of the pwrctrl framework is due to devicetree being kept out-of-tree for this
platform.

- Mani

[1] https://lore.kernel.org/all/vazxuov2hdk5sezrk7a5qfuclv2s3wo5sxhfwuo3o4uedsdlqv@po55ny24ctne/

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

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

[Bjorn Helgaas]

On Sat, Jun 28, 2025 at 04:57:26AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jun 27, 2025 at 05:45:02PM -0500, Bjorn Helgaas wrote:
> > 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.
> 
> Yes! Sorry for the delay. The fact that I switched the job and had
> to attend OSS NA prevented me from reworking this patch.
> 
> > 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).
> 
> I guess, we could go with option 3 if you prefer. We could rename
> the existing pwrctrl/core.c to pwrctrl/pwrctrl.c and move the
> definition of pci_pwrctrl_create_device() to new pwrctrl/core.c. The
> new file will depend on HAVE_PWRCTRL, which is bool.

I think I forgot to mention that option 2 still requires a patch to
wrap pci_pwrctrl_create_device() with some sort of #ifdef for
CONFIG_PCI_PWRCTRL, right?  Seems like we need that regardless of the
brcmstb situation so that we don't create pwrctrl devices when
CONFIG_OF=y and CONFIG_PCI_PWRCTRL=n.

That seems like a straightforward solution and the #ifdef would
address my readability concern even though the code stays in probe.c.

> > 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.
> 
> Right, that would be the end goal. As I explained in the reply to
> the bug report [1], this patch will serve as an interim workaround.
> Once my pwrctrl rework (which I didn't submit yet) is merged, I will
> move this driver to use the pwrctrl framework.

OK, so for now, Jim would still need to ensure CONFIG_PCI_PWRCTRL=n
when brcmstb is enabled, but we do have a plan to adapt brcmstb work
with pwrctrl.

> [1]
> https://lore.kernel.org/all/vazxuov2hdk5sezrk7a5qfuclv2s3wo5sxhfwuo3o4uedsdlqv@po55ny24ctne/