Re: [PATCH v3] PCI/pwrctrl: Move pci_pwrctrl_create_device() definition to drivers/pci/pwrctrl/

[Bjorn Helgaas <helgaas@kernel.org>]

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
> 
> -- மணிவண்ணன் சதாசிவம்