[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250629190219.GA1717534@bhelgaas>
Date: Sun, 29 Jun 2025 14:02:19 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Manivannan Sadhasivam <mani@...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 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/
Powered by blists - more mailing lists