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