[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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