drivers/pci/pci.h | 8 ++++++++ drivers/pci/probe.c | 32 -------------------------------- drivers/pci/pwrctrl/core.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 32 deletions(-)
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.0On 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
>
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/ -- மணிவண்ணன் சதாசிவம்
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/
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
>
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
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 -- மணிவண்ணன் சதாசிவம்
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
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
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 -- மணிவண்ணன் சதாசிவம்
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 > > -- மணிவண்ணன் சதாசிவம்
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 -- மணிவண்ணன் சதாசிவம்
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
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 -- மணிவண்ணன் சதாசிவம்
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>
© 2016 - 2025 Red Hat, Inc.