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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f5qfkb543al3ihonj7n7fi4pcd3kopavweyo5ixdgv7o3lct4u@r73fkstrcl62>
Date: Wed, 18 Jun 2025 10:35:41 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Lukas Wunner <lukas@...ner.de>, 
	kernel test robot <lkp@...el.com>, bhelgaas@...gle.com, oe-kbuild-all@...ts.linux.dev, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jim Quinlan <james.quinlan@...adcom.com>
Subject: Re: [PATCH v3] PCI/pwrctrl: Move pci_pwrctrl_create_device()
 definition to drivers/pci/pwrctrl/

On Tue, Jun 17, 2025 at 03:44:06PM -0500, Bjorn Helgaas wrote:
> 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@...ner.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?
> 

Only to avoid making the kernel image bigger.

> 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.
> 

Well, pwrctrl is for non-ACPI platforms (mostly OF) and yes, it is parallel to
ACPI in some form, but that is inevitable since OF is just a hardware
description and ACPI is much more than that.

> > > > 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.
> 

Okay.

> > > 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"
> 

Then it will always end up in confusion of where to place the functions and
such. For sure the Kconfig can define what each file stands for, but IMO it
feels weird to have 2 files for the same purpose.

But if you insist, I can go for it.

> 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.
> 

It is 'pci-pwrctrl-core.ko'.

- Mani

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