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@lin... [2] https://lore.kernel.org/linux-pci/20250523201935.1586198-1-helgaas@kernel.or... [3] https://lore.kernel.org/linux-pci/aDFnWhFa9ZGqr67T@wunner.de/ [4] https://lore.kernel.org/linux-pci/20250629190219.GA1717534@bhelgaas/
Thanks,
Lukas