linux-kernel - Re: [PATCH v2] PCI/pwrctrl: Skip creating pwrctrl device unless CONFIG_PCI

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

Message-ID: <n23tedrmgzfo7bxe4mbde2rrsayalcz4jya5yopoeahlll3qaw@mpz4oemtyern>
Date: Tue, 1 Jul 2025 17:27:27 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	bhelgaas@...gle.com, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org, Jim Quinlan <james.quinlan@...adcom.com>
Subject: Re: [PATCH v2] PCI/pwrctrl: Skip creating pwrctrl device unless
 CONFIG_PCI_PWRCTRL is enabled

On Tue, Jul 01, 2025 at 09:00:34AM GMT, Lukas Wunner wrote:
> On Tue, Jul 01, 2025 at 12:17:31PM +0530, Manivannan Sadhasivam wrote:
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2508,6 +2508,7 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
> >  }
> >  EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
> >  
> > +#if IS_ENABLED(CONFIG_PCI_PWRCTRL)
> >  static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> >  {
> 
> Hm, why does pci_pwrctrl_create_device() return a pointer, even though the
> sole caller doesn't make any use of it?  Why not return a negative errno?
> 
> Then you could just do this:
> 
> 	if (!IS_ENABLED(CONFIG_PCI_PWRCTRL))
> 		return 0;
> 
> ... at the top of the function and you don't need the extra LoC for the
> empty inline stub.
> 

This is what I initially submitted [1] though that returned NULL, but the idea
was the same. But Bjorn didn't like that.

> Another option is to set "struct pci_dev *pdev = NULL;" and #ifdef the body
> of the function, save for the "return pdev;" at the bottom.
> 

This is similar to what Bjorn submitted [2], but you were in favor of providing
a stub instead [3]. It also looked better to my eyes.

> Of course you could also do:
> 
> 	if (!IS_ENABLED(CONFIG_PCI_PWRCTRL))
> 		return NULL;
> 
> ... at the top of the function, but again, the caller doesn't make any
> use of the returned pointer.
> 

Right. I could make it to return a errno, but that's not the scope of this
patch. Bjorn wanted to have the #ifdef to be guarded to make the compiled out
part more visible [4], so I ended up with this version.

But whatever the style is, we should make sure that the patch lands in 6.16-rcS.
It is taking more time than needed.

- Mani

[1] https://lore.kernel.org/all/20250522140326.93869-1-manivannan.sadhasivam@linaro.org/
[2] https://lore.kernel.org/linux-pci/20250523201935.1586198-1-helgaas@kernel.org/
[3] https://lore.kernel.org/linux-pci/aDFnWhFa9ZGqr67T@wunner.de/
[4] https://lore.kernel.org/linux-pci/20250629190219.GA1717534@bhelgaas/

> Thanks,
> 
> Lukas
> 

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