Message-ID: <qy2nfwiu2g7pbzbk37wseapvsen7mx4fgqdkdwjbclsj5dltu5@7o2xtj3qhedm>
Date: Sat, 28 Jun 2025 04:57:26 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: bhelgaas@...gle.com, brgl@...ev.pl, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org, lukas@...ner.de, Jim Quinlan <james.quinlan@...adcom.com>
Subject: Re: [PATCH v3] PCI/pwrctrl: Move pci_pwrctrl_create_device()
 definition to drivers/pci/pwrctrl/

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/

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

Powered by blists - more mailing lists