[PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bert Karwatzki]

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, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index e4735428814d..3bab78cc68f7 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -350,7 +350,7 @@ void pci_bus_add_device(struct pci_dev *dev)

 	pci_dev_assign_added(dev, true);

-	if (pci_is_bridge(dev)) {
+	if (IS_ENABLED(CONFIG_OF) && pci_is_bridge(dev)) {
 		retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
 					      &dev->dev);
 		if (retval)
--
2.45.2

Just in case this is needed.

Bert Karwatzki

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Lukas Wunner]

On Sun, Jul 07, 2024 at 08:38:28PM +0200, Bert Karwatzki 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>

Reported-by: Praveenkumar Patil <PraveenKumar.Patil@amd.com>
Closes: https://lore.kernel.org/all/20240702173255.39932-1-superm1@kernel.org/
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Cc: Mario Limonciello <mario.limonciello@amd.com>

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Krzysztof Wilczyński]

Hello,

> > 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>
> 
> Reported-by: Praveenkumar Patil <PraveenKumar.Patil@amd.com>
> Closes: https://lore.kernel.org/all/20240702173255.39932-1-superm1@kernel.org/
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> Cc: Mario Limonciello <mario.limonciello@amd.com>

If there aren't any objections, I will take this via the PCI tree, and add
the missing tags.  So, no need to send a new version of this patch.

Thank you for the work here!  Appreciated.

	Krzysztof

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bartosz Golaszewski]

On Mon, 8 Jul 2024 at 02:37, Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Hello,
>
> > > 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>
> >
> > Reported-by: Praveenkumar Patil <PraveenKumar.Patil@amd.com>
> > Closes: https://lore.kernel.org/all/20240702173255.39932-1-superm1@kernel.org/
> > Reviewed-by: Lukas Wunner <lukas@wunner.de>
> > Cc: Mario Limonciello <mario.limonciello@amd.com>
>
> If there aren't any objections, I will take this via the PCI tree, and add
> the missing tags.  So, no need to send a new version of this patch.
>
> Thank you for the work here!  Appreciated.
>
>         Krzysztof

I don't think you can take it via the PCI tree as it depends on the
changes that went via the new pwrseq tree (with Bjorn's blessing).
Please leave your Ack here and I will take it with the other PCI
pwrctl changes.

After the upcoming merge window we should go back to normal.

Bart

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Krzysztof Wilczyński]

[...]
> > If there aren't any objections, I will take this via the PCI tree, and add
> > the missing tags.  So, no need to send a new version of this patch.
> >
> > Thank you for the work here!  Appreciated.
> >
> >         Krzysztof
> 
> I don't think you can take it via the PCI tree as it depends on the
> changes that went via the new pwrseq tree (with Bjorn's blessing).

Aye.

> Please leave your Ack here and I will take it with the other PCI
> pwrctl changes.

Sounds good!  With that...

Acked-by: Krzysztof Wilczyński <kw@linux.com>

> After the upcoming merge window we should go back to normal.

Thank you!

	Krzysztof

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Mario Limonciello]

On 7/8/2024 3:33, Krzysztof Wilczyński wrote:
> [...]
>>> If there aren't any objections, I will take this via the PCI tree, and add
>>> the missing tags.  So, no need to send a new version of this patch.
>>>
>>> Thank you for the work here!  Appreciated.
>>>
>>>          Krzysztof
>>
>> I don't think you can take it via the PCI tree as it depends on the
>> changes that went via the new pwrseq tree (with Bjorn's blessing).
> 
> Aye.
> 
>> Please leave your Ack here and I will take it with the other PCI
>> pwrctl changes.
> 
> Sounds good!  With that...
> 
> Acked-by: Krzysztof Wilczyński <kw@linux.com>
> 
>> After the upcoming merge window we should go back to normal.
> 
> Thank you!
> 
> 	Krzysztof

FWIW this other patch makes it quieter too.

https://lore.kernel.org/linux-pci/20240702180839.71491-1-superm1@kernel.org/

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bartosz Golaszewski]

On Mon, 8 Jul 2024 at 17:29, Mario Limonciello <superm1@kernel.org> wrote:
>
> On 7/8/2024 3:33, Krzysztof Wilczyński wrote:
> > [...]
> >>> If there aren't any objections, I will take this via the PCI tree, and add
> >>> the missing tags.  So, no need to send a new version of this patch.
> >>>
> >>> Thank you for the work here!  Appreciated.
> >>>
> >>>          Krzysztof
> >>
> >> I don't think you can take it via the PCI tree as it depends on the
> >> changes that went via the new pwrseq tree (with Bjorn's blessing).
> >
> > Aye.
> >
> >> Please leave your Ack here and I will take it with the other PCI
> >> pwrctl changes.
> >
> > Sounds good!  With that...
> >
> > Acked-by: Krzysztof Wilczyński <kw@linux.com>
> >
> >> After the upcoming merge window we should go back to normal.
> >
> > Thank you!
> >
> >       Krzysztof
>
> FWIW this other patch makes it quieter too.
>
> https://lore.kernel.org/linux-pci/20240702180839.71491-1-superm1@kernel.org/

I had applied it previously but backed it out in favor of the new one.

Bart

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Manivannan Sadhasivam]

On Mon, Jul 08, 2024 at 05:34:29PM +0200, Bartosz Golaszewski wrote:
> On Mon, 8 Jul 2024 at 17:29, Mario Limonciello <superm1@kernel.org> wrote:
> >
> > On 7/8/2024 3:33, Krzysztof Wilczyński wrote:
> > > [...]
> > >>> If there aren't any objections, I will take this via the PCI tree, and add
> > >>> the missing tags.  So, no need to send a new version of this patch.
> > >>>
> > >>> Thank you for the work here!  Appreciated.
> > >>>
> > >>>          Krzysztof
> > >>
> > >> I don't think you can take it via the PCI tree as it depends on the
> > >> changes that went via the new pwrseq tree (with Bjorn's blessing).
> > >
> > > Aye.
> > >
> > >> Please leave your Ack here and I will take it with the other PCI
> > >> pwrctl changes.
> > >
> > > Sounds good!  With that...
> > >
> > > Acked-by: Krzysztof Wilczyński <kw@linux.com>
> > >
> > >> After the upcoming merge window we should go back to normal.
> > >
> > > Thank you!
> > >
> > >       Krzysztof
> >
> > FWIW this other patch makes it quieter too.
> >
> > https://lore.kernel.org/linux-pci/20240702180839.71491-1-superm1@kernel.org/
> 
> I had applied it previously but backed it out in favor of the new one.
> 

That sounds sensible. The patch referenced above still causes
of_platform_populate() to be called on non-OF platforms, which is not optimal.

- Mani

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

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Mario Limonciello]

On 7/8/2024 10:44, Manivannan Sadhasivam wrote:
> On Mon, Jul 08, 2024 at 05:34:29PM +0200, Bartosz Golaszewski wrote:
>> On Mon, 8 Jul 2024 at 17:29, Mario Limonciello <superm1@kernel.org> wrote:
>>>
>>> On 7/8/2024 3:33, Krzysztof Wilczyński wrote:
>>>> [...]
>>>>>> If there aren't any objections, I will take this via the PCI tree, and add
>>>>>> the missing tags.  So, no need to send a new version of this patch.
>>>>>>
>>>>>> Thank you for the work here!  Appreciated.
>>>>>>
>>>>>>           Krzysztof
>>>>>
>>>>> I don't think you can take it via the PCI tree as it depends on the
>>>>> changes that went via the new pwrseq tree (with Bjorn's blessing).
>>>>
>>>> Aye.
>>>>
>>>>> Please leave your Ack here and I will take it with the other PCI
>>>>> pwrctl changes.
>>>>
>>>> Sounds good!  With that...
>>>>
>>>> Acked-by: Krzysztof Wilczyński <kw@linux.com>
>>>>
>>>>> After the upcoming merge window we should go back to normal.
>>>>
>>>> Thank you!
>>>>
>>>>        Krzysztof
>>>
>>> FWIW this other patch makes it quieter too.
>>>
>>> https://lore.kernel.org/linux-pci/20240702180839.71491-1-superm1@kernel.org/
>>
>> I had applied it previously but backed it out in favor of the new one.
>>
> 
> That sounds sensible. The patch referenced above still causes
> of_platform_populate() to be called on non-OF platforms, which is not optimal.

But couldn't I just as well have CONFIG_OF enabled in my kconfig and get 
the same new noise?

> 
> - Mani
> 

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bartosz Golaszewski]

On Mon, Jul 8, 2024 at 5:46 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 7/8/2024 10:44, Manivannan Sadhasivam wrote:
> > On Mon, Jul 08, 2024 at 05:34:29PM +0200, Bartosz Golaszewski wrote:
> >> On Mon, 8 Jul 2024 at 17:29, Mario Limonciello <superm1@kernel.org> wrote:
> >>>
> >>> On 7/8/2024 3:33, Krzysztof Wilczyński wrote:
> >>>> [...]
> >>>>>> If there aren't any objections, I will take this via the PCI tree, and add
> >>>>>> the missing tags.  So, no need to send a new version of this patch.
> >>>>>>
> >>>>>> Thank you for the work here!  Appreciated.
> >>>>>>
> >>>>>>           Krzysztof
> >>>>>
> >>>>> I don't think you can take it via the PCI tree as it depends on the
> >>>>> changes that went via the new pwrseq tree (with Bjorn's blessing).
> >>>>
> >>>> Aye.
> >>>>
> >>>>> Please leave your Ack here and I will take it with the other PCI
> >>>>> pwrctl changes.
> >>>>
> >>>> Sounds good!  With that...
> >>>>
> >>>> Acked-by: Krzysztof Wilczyński <kw@linux.com>
> >>>>
> >>>>> After the upcoming merge window we should go back to normal.
> >>>>
> >>>> Thank you!
> >>>>
> >>>>        Krzysztof
> >>>
> >>> FWIW this other patch makes it quieter too.
> >>>
> >>> https://lore.kernel.org/linux-pci/20240702180839.71491-1-superm1@kernel.org/
> >>
> >> I had applied it previously but backed it out in favor of the new one.
> >>
> >
> > That sounds sensible. The patch referenced above still causes
> > of_platform_populate() to be called on non-OF platforms, which is not optimal.
>
> But couldn't I just as well have CONFIG_OF enabled in my kconfig and get
> the same new noise?
>

If you have CONFIG_OF enabled then of_platform_populate() will go the
normal path and actually try to populate sub-nodes of the host bridge
node. If there are no OF nodes (not a device-tree system) then it will
fail.

Bart

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Mario Limonciello]

On 7/8/2024 10:49, Bartosz Golaszewski wrote:

> 
> If you have CONFIG_OF enabled then of_platform_populate() will go the
> normal path and actually try to populate sub-nodes of the host bridge
> node. If there are no OF nodes (not a device-tree system) then it will
> fail.
> 
> Bart

So how about keep both patches then?

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bartosz Golaszewski]

On Mon, Jul 8, 2024 at 5:53 PM Mario Limonciello <superm1@kernel.org> wrote:
>
> On 7/8/2024 10:49, Bartosz Golaszewski wrote:
>
> >
> > If you have CONFIG_OF enabled then of_platform_populate() will go the
> > normal path and actually try to populate sub-nodes of the host bridge
> > node. If there are no OF nodes (not a device-tree system) then it will
> > fail.
> >
> > Bart
>
> So how about keep both patches then?

No, it doesn't make sense. If CONFIG_OF is enabled then -ENODEV is a
valid error. I was wrong to apply the previous patch as it would lead
to hiding actual errors on OF-enabled systems.

Bart

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Mario Limonciello]

On 7/8/2024 10:58, Bartosz Golaszewski wrote:
> On Mon, Jul 8, 2024 at 5:53 PM Mario Limonciello <superm1@kernel.org> wrote:
>>
>> On 7/8/2024 10:49, Bartosz Golaszewski wrote:
>>
>>>
>>> If you have CONFIG_OF enabled then of_platform_populate() will go the
>>> normal path and actually try to populate sub-nodes of the host bridge
>>> node. If there are no OF nodes (not a device-tree system) then it will
>>> fail.
>>>
>>> Bart
>>
>> So how about keep both patches then?
> 
> No, it doesn't make sense. If CONFIG_OF is enabled then -ENODEV is a
> valid error. I was wrong to apply the previous patch as it would lead
> to hiding actual errors on OF-enabled systems.
> 
> Bart

Got it; thanks.

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Manivannan Sadhasivam]

On Mon, Jul 08, 2024 at 10:53:18AM -0500, Mario Limonciello wrote:
> On 7/8/2024 10:49, Bartosz Golaszewski wrote:
> 
> > 
> > If you have CONFIG_OF enabled then of_platform_populate() will go the
> > normal path and actually try to populate sub-nodes of the host bridge
> > node. If there are no OF nodes (not a device-tree system) then it will
> > fail.
> > 
> > Bart
> 
> So how about keep both patches then?

Why would anyone running non OF system have CONFIG_OF enabled? If that's the
case, they have to see the error because there would be no DT nodes.

- Mani

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

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Manivannan Sadhasivam]

On Sun, Jul 07, 2024 at 08:38:28PM +0200, Bert Karwatzki 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>

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index e4735428814d..3bab78cc68f7 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -350,7 +350,7 @@ void pci_bus_add_device(struct pci_dev *dev)
> 
>  	pci_dev_assign_added(dev, true);
> 
> -	if (pci_is_bridge(dev)) {
> +	if (IS_ENABLED(CONFIG_OF) && pci_is_bridge(dev)) {
>  		retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
>  					      &dev->dev);
>  		if (retval)
> --
> 2.45.2
> 
> Just in case this is needed.
> 
> Bert Karwatzki
> 

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

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Sun, 07 Jul 2024 20:38:28 +0200, Bert Karwatzki 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")
> 
> [...]

Applied, thanks!

[1/1] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled
      (no commit info)

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bartosz Golaszewski]

On Mon, 8 Jul 2024 at 11:54, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
>
> On Sun, 07 Jul 2024 20:38:28 +0200, Bert Karwatzki 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")
> >
> > [...]
>
> Applied, thanks!
>
> [1/1] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled
>       (no commit info)
>

The commit is here[1], not sure why b4 didn't pick it up.

Bart

[1] https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=pwrseq/for-next&id=10a0e6c2a8fc0d4b7e8e684654e920ea55527f3b

> Best regards,
> --
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bjorn Helgaas]

On Sun, Jul 07, 2024 at 08:38:28PM +0200, Bert Karwatzki 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>

I didn't trace all the history of this patch so I don't know whether
it's in or out.  If it hasn't been applied yet or will be revised, run
"git log --oneline drivers/pci/bus.c" and match the style, e.g.,

  PCI/pwrctl: Call of_platform_populate() only when CONFIG_OF enabled

This would also match the 8fb18619d910 subject so it's more obvious
they are connected.

Bjorn

Re: [PATCH v2] pci: bus: only call of_platform_populate() if CONFIG_OF is enabled

[Bartosz Golaszewski]

On Mon, Jul 8, 2024 at 9:13 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Sun, Jul 07, 2024 at 08:38:28PM +0200, Bert Karwatzki 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>
>
> I didn't trace all the history of this patch so I don't know whether
> it's in or out.  If it hasn't been applied yet or will be revised, run
> "git log --oneline drivers/pci/bus.c" and match the style, e.g.,
>
>   PCI/pwrctl: Call of_platform_populate() only when CONFIG_OF enabled
>
> This would also match the 8fb18619d910 subject so it's more obvious
> they are connected.
>
> Bjorn
>

Good point. I updated the commit title in my pwrseq branch.

Bart