| Message ID | 20240703091535.3531-1-spasswolf@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | pci: bus: only call of_platform_populate() if CONFIG_OF is defined | expand |
On Wed, Jul 3, 2024 at 11:15 AM Bert Karwatzki <spasswolf@web.de> wrote: > > If of_platform_populate() is called when CONFIG_OF is not defined this > leads to spurious error messages of the following type: > pci 0000:00:01.1: failed to populate child OF nodes (-19) > pci 0000:00:02.1: failed to populate child OF nodes (-19) > > Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node") > Signed-off-by: Bert Karwatzki <spasswolf@web.de> > --- > drivers/pci/bus.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index e4735428814d..b363010664cd 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -350,6 +350,7 @@ void pci_bus_add_device(struct pci_dev *dev) > > pci_dev_assign_added(dev, true); > > +#ifdef CONFIG_OF > if (pci_is_bridge(dev)) { > retval = of_platform_populate(dev->dev.of_node, NULL, NULL, > &dev->dev); > @@ -357,6 +358,7 @@ void pci_bus_add_device(struct pci_dev *dev) > pci_err(dev, "failed to populate child OF nodes (%d)\n", > retval); > } > +#endif > } > EXPORT_SYMBOL_GPL(pci_bus_add_device); > > -- > 2.45.2 > > The mentioned error message occur on systems without CONFIG_OF, i.e. > x86_64. The call to of_platform_depopulate() in drivers/pci/remove.c > does not need #ifdef CONFIG_OF as the return value is not checked (it > will most likely be optimized away on platforms witout OF where the > of_platform_{de,}populate() functions just return -ENODEV) > > Please CC me when replying, I'm not subscribed to the lists. > > Bert Karwatzki There's a better (less ifdeffery) fix on the list that I'll pick up later today[1]. Bart [1] https://lore.kernel.org/lkml/20240702180839.71491-1-superm1@kernel.org/T/
On Wed, Jul 03, 2024 at 11:32:05AM +0200, Bartosz Golaszewski wrote: > On Wed, Jul 3, 2024 at 11:15 AM Bert Karwatzki <spasswolf@web.de> wrote: > > If of_platform_populate() is called when CONFIG_OF is not defined this > > leads to spurious error messages of the following type: > > pci 0000:00:01.1: failed to populate child OF nodes (-19) > > pci 0000:00:02.1: failed to populate child OF nodes (-19) > > > > Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node") > > Signed-off-by: Bert Karwatzki <spasswolf@web.de> > > --- > > drivers/pci/bus.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > > index e4735428814d..b363010664cd 100644 > > --- a/drivers/pci/bus.c > > +++ b/drivers/pci/bus.c > > @@ -350,6 +350,7 @@ void pci_bus_add_device(struct pci_dev *dev) > > > > pci_dev_assign_added(dev, true); > > > > +#ifdef CONFIG_OF > > if (pci_is_bridge(dev)) { Per section 21 of Documentation/process/coding-style.rst, IS_ENABLED() is strongly preferred to #ifdef. > There's a better (less ifdeffery) fix on the list that I'll pick up > later today[1]. > > [1] https://lore.kernel.org/lkml/20240702180839.71491-1-superm1@kernel.org/T/ That other fix doesn't feel very robust as it depends on of_platform_populate() never returning -ENODEV in the CONFIG_OF=y case. Thanks, Lukas
On Sat, Jul 6, 2024 at 10:43 AM Lukas Wunner <lukas@wunner.de> wrote: > > On Wed, Jul 03, 2024 at 11:32:05AM +0200, Bartosz Golaszewski wrote: > > On Wed, Jul 3, 2024 at 11:15 AM Bert Karwatzki <spasswolf@web.de> wrote: > > > If of_platform_populate() is called when CONFIG_OF is not defined this > > > leads to spurious error messages of the following type: > > > pci 0000:00:01.1: failed to populate child OF nodes (-19) > > > pci 0000:00:02.1: failed to populate child OF nodes (-19) > > > > > > Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node") > > > Signed-off-by: Bert Karwatzki <spasswolf@web.de> > > > --- > > > drivers/pci/bus.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > > > index e4735428814d..b363010664cd 100644 > > > --- a/drivers/pci/bus.c > > > +++ b/drivers/pci/bus.c > > > @@ -350,6 +350,7 @@ void pci_bus_add_device(struct pci_dev *dev) > > > > > > pci_dev_assign_added(dev, true); > > > > > > +#ifdef CONFIG_OF > > > if (pci_is_bridge(dev)) { > > Per section 21 of Documentation/process/coding-style.rst, > IS_ENABLED() is strongly preferred to #ifdef. > > > > There's a better (less ifdeffery) fix on the list that I'll pick up > > later today[1]. > > > > [1] https://lore.kernel.org/lkml/20240702180839.71491-1-superm1@kernel.org/T/ > > That other fix doesn't feel very robust as it depends on > of_platform_populate() never returning -ENODEV in the > CONFIG_OF=y case. > If we even have to play these ifdef games then the stubs for of_platform_populate() are broken and should probably return 0 with !OF as otherwise the stubs themselves are useless. Bart
On Sat, Jul 06, 2024 at 07:12:48PM +0200, Bartosz Golaszewski wrote: > On Sat, Jul 6, 2024 at 10:43???AM Lukas Wunner <lukas@wunner.de> wrote: > > On Wed, Jul 03, 2024 at 11:32:05AM +0200, Bartosz Golaszewski wrote: > > > On Wed, Jul 3, 2024 at 11:15 AM Bert Karwatzki <spasswolf@web.de> wrote: > > > > If of_platform_populate() is called when CONFIG_OF is not defined this > > > > leads to spurious error messages of the following type: > > > > pci 0000:00:01.1: failed to populate child OF nodes (-19) > > > > pci 0000:00:02.1: failed to populate child OF nodes (-19) [...] > > > > --- a/drivers/pci/bus.c > > > > +++ b/drivers/pci/bus.c > > > > @@ -350,6 +350,7 @@ void pci_bus_add_device(struct pci_dev *dev) > > > > > > > > pci_dev_assign_added(dev, true); > > > > > > > > +#ifdef CONFIG_OF > > > > if (pci_is_bridge(dev)) { > > > > > > There's a better (less ifdeffery) fix on the list that I'll pick up > > > later today[1]. > > > > > > [1] https://lore.kernel.org/lkml/20240702180839.71491-1-superm1@kernel.org/T/ > > > > That other fix doesn't feel very robust as it depends on > > of_platform_populate() never returning -ENODEV in the > > CONFIG_OF=y case. > > If we even have to play these ifdef games then the stubs for > of_platform_populate() are broken and should probably return 0 with > !OF as otherwise the stubs themselves are useless. The stub was introduced by commit 964dba283439 ("devicetree: Add empty of_platform_populate() for !CONFIG_OF_ADDRESS (sparc)") some twelve years ago. From a brief look I'm under the impression that the commit used -ENODEV on purpose (instead of 0). There are 100 files in the kernel source tree referencing of_platform_populate(). The majority is probably not compiled in the CONFIG_OF=n case, nevertheless changing the stub risks regressing some of those 100 callers unless great care is observed. Just making this conditional on IS_ENABLED(CONFIG_OF) is the least risky option. Thanks, Lukas
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index e4735428814d..b363010664cd 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -350,6 +350,7 @@ void pci_bus_add_device(struct pci_dev *dev) pci_dev_assign_added(dev, true); +#ifdef CONFIG_OF if (pci_is_bridge(dev)) { retval = of_platform_populate(dev->dev.of_node, NULL, NULL, &dev->dev); @@ -357,6 +358,7 @@ void pci_bus_add_device(struct pci_dev *dev) pci_err(dev, "failed to populate child OF nodes (%d)\n", retval); } +#endif } EXPORT_SYMBOL_GPL(pci_bus_add_device);
If of_platform_populate() is called when CONFIG_OF is not defined this leads to spurious error messages of the following type: pci 0000:00:01.1: failed to populate child OF nodes (-19) pci 0000:00:02.1: failed to populate child OF nodes (-19) Fixes: 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF nodes of the port node") Signed-off-by: Bert Karwatzki <spasswolf@web.de> --- drivers/pci/bus.c | 2 ++ 1 file changed, 2 insertions(+) -- 2.45.2 The mentioned error message occur on systems without CONFIG_OF, i.e. x86_64. The call to of_platform_depopulate() in drivers/pci/remove.c does not need #ifdef CONFIG_OF as the return value is not checked (it will most likely be optimized away on platforms witout OF where the of_platform_{de,}populate() functions just return -ENODEV) Please CC me when replying, I'm not subscribed to the lists. Bert Karwatzki