[v2] PCI: apple: Set only available ports up

Commit Message

Janne Grunau March 9, 2023, 1:36 p.m. UTC

Fixes following warning inside of_irq_parse_raw() called from the common
PCI device probe path.

  /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
  WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
  ...
  Call trace:
   of_irq_parse_raw+0x5fc/0x724
   of_irq_parse_and_map_pci+0x128/0x1d8
   pci_assign_irq+0xc8/0x140
   pci_device_probe+0x70/0x188
   really_probe+0x178/0x418
   __driver_probe_device+0x120/0x188
   driver_probe_device+0x48/0x22c
   __device_attach_driver+0x134/0x1d8
   bus_for_each_drv+0x8c/0xd8
   __device_attach+0xdc/0x1d0
   device_attach+0x20/0x2c
   pci_bus_add_device+0x5c/0xc0
   pci_bus_add_devices+0x58/0x88
   pci_host_probe+0x124/0x178
   pci_host_common_probe+0x124/0x198 [pci_host_common]
   apple_pcie_probe+0x108/0x16c [pcie_apple]
   platform_probe+0xb4/0xdc

This became apparent after disabling unused PCIe ports in the Apple
silicon device trees instead of deleting them.

Use for_each_available_child_of_node instead of for_each_child_of_node
which takes the "status" property into account.

Link: https://lore.kernel.org/asahi/[e-mail]/
Link: https://lore.kernel.org/asahi/[e-mail]/
Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
Cc: [e-mail]
Reviewed-by: Marc Zyngier <[e-mail]>
Signed-off-by: Janne Grunau <[e-mail]>
---
Changes in v2:
- rewritten commit message with more details and corrections
- collected Marc's "Reviewed-by:"
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 drivers/pci/controller/pcie-apple.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
change-id: 20230307-apple_pcie_disabled_ports-0c17fb7a4738

Best regards,

Comments

Eric Curtin March 9, 2023, 2:18 p.m. UTC | #1

On Thu, 9 Mar 2023 at 13:43, Janne Grunau <[e-mail]> wrote:
>
> Fixes following warning inside of_irq_parse_raw() called from the common
> PCI device probe path.
>
>   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
>   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
>   ...
>   Call trace:
>    of_irq_parse_raw+0x5fc/0x724
>    of_irq_parse_and_map_pci+0x128/0x1d8
>    pci_assign_irq+0xc8/0x140
>    pci_device_probe+0x70/0x188
>    really_probe+0x178/0x418
>    __driver_probe_device+0x120/0x188
>    driver_probe_device+0x48/0x22c
>    __device_attach_driver+0x134/0x1d8
>    bus_for_each_drv+0x8c/0xd8
>    __device_attach+0xdc/0x1d0
>    device_attach+0x20/0x2c
>    pci_bus_add_device+0x5c/0xc0
>    pci_bus_add_devices+0x58/0x88
>    pci_host_probe+0x124/0x178
>    pci_host_common_probe+0x124/0x198 [pci_host_common]
>    apple_pcie_probe+0x108/0x16c [pcie_apple]
>    platform_probe+0xb4/0xdc
>
> This became apparent after disabling unused PCIe ports in the Apple
> silicon device trees instead of deleting them.
>
> Use for_each_available_child_of_node instead of for_each_child_of_node
> which takes the "status" property into account.
>
> Link: https://lore.kernel.org/asahi/[e-mail]/
> Link: https://lore.kernel.org/asahi/[e-mail]/
> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> Cc: [e-mail]
> Reviewed-by: Marc Zyngier <[e-mail]>
> Signed-off-by: Janne Grunau <[e-mail]>

Makes sense.

Reviewed-by: Eric Curtin <[e-mail]>

Is mise le meas/Regards,

Eric Curtin

> ---
> Changes in v2:
> - rewritten commit message with more details and corrections
> - collected Marc's "Reviewed-by:"
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  drivers/pci/controller/pcie-apple.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 66f37e403a09..f8670a032f7a 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>         cfg->priv = pcie;
>         INIT_LIST_HEAD(&pcie->ports);
>
> -       for_each_child_of_node(dev->of_node, of_port) {
> +       for_each_available_child_of_node(dev->of_node, of_port) {
>                 ret = apple_pcie_setup_port(pcie, of_port);
>                 if (ret) {
>                         dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
>
> ---
> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
> change-id: 20230307-apple_pcie_disabled_ports-0c17fb7a4738
>
> Best regards,
> --
> Janne Grunau <[e-mail]>
>
>

Rob Herring March 9, 2023, 3:19 p.m. UTC | #2

On Thu, Mar 9, 2023 at 7:36 AM Janne Grunau <[e-mail]> wrote:
>
> Fixes following warning inside of_irq_parse_raw() called from the common
> PCI device probe path.
>
>   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
>   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
>   ...
>   Call trace:
>    of_irq_parse_raw+0x5fc/0x724
>    of_irq_parse_and_map_pci+0x128/0x1d8
>    pci_assign_irq+0xc8/0x140
>    pci_device_probe+0x70/0x188
>    really_probe+0x178/0x418
>    __driver_probe_device+0x120/0x188
>    driver_probe_device+0x48/0x22c
>    __device_attach_driver+0x134/0x1d8
>    bus_for_each_drv+0x8c/0xd8
>    __device_attach+0xdc/0x1d0
>    device_attach+0x20/0x2c
>    pci_bus_add_device+0x5c/0xc0
>    pci_bus_add_devices+0x58/0x88
>    pci_host_probe+0x124/0x178
>    pci_host_common_probe+0x124/0x198 [pci_host_common]
>    apple_pcie_probe+0x108/0x16c [pcie_apple]
>    platform_probe+0xb4/0xdc
>
> This became apparent after disabling unused PCIe ports in the Apple
> silicon device trees instead of deleting them.
>
> Use for_each_available_child_of_node instead of for_each_child_of_node
> which takes the "status" property into account.
>
> Link: https://lore.kernel.org/asahi/[e-mail]/
> Link: https://lore.kernel.org/asahi/[e-mail]/
> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> Cc: [e-mail]
> Reviewed-by: Marc Zyngier <[e-mail]>
> Signed-off-by: Janne Grunau <[e-mail]>
> ---
> Changes in v2:
> - rewritten commit message with more details and corrections
> - collected Marc's "Reviewed-by:"
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  drivers/pci/controller/pcie-apple.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Unfortunately, this is a common issue...

Reviewed-by: Rob Herring <[e-mail]>

Rob

Bjorn Helgaas March 9, 2023, 4:39 p.m. UTC | #3

[+cc Daire, Conor for apple/microchip use of ECAM .init() method]

On Thu, Mar 09, 2023 at 02:36:24PM +0100, Janne Grunau wrote:
> Fixes following warning inside of_irq_parse_raw() called from the common
> PCI device probe path.
> 
>   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
>   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724

Based on this commit log, I assume this patch only fixes the warning,
and the system *works* just fine either way.  If that's the case, it's
debatable whether it meets the stable kernel criteria, although the
documented criteria are much stricter than what happens in practice.

>   ...
>   Call trace:
>    of_irq_parse_raw+0x5fc/0x724
>    of_irq_parse_and_map_pci+0x128/0x1d8
>    pci_assign_irq+0xc8/0x140
>    pci_device_probe+0x70/0x188
>    really_probe+0x178/0x418
>    __driver_probe_device+0x120/0x188
>    driver_probe_device+0x48/0x22c
>    __device_attach_driver+0x134/0x1d8
>    bus_for_each_drv+0x8c/0xd8
>    __device_attach+0xdc/0x1d0
>    device_attach+0x20/0x2c
>    pci_bus_add_device+0x5c/0xc0
>    pci_bus_add_devices+0x58/0x88
>    pci_host_probe+0x124/0x178
>    pci_host_common_probe+0x124/0x198 [pci_host_common]
>    apple_pcie_probe+0x108/0x16c [pcie_apple]
>    platform_probe+0xb4/0xdc
> 
> This became apparent after disabling unused PCIe ports in the Apple
> silicon device trees instead of deleting them.
> 
> Use for_each_available_child_of_node instead of for_each_child_of_node
> which takes the "status" property into account.
> 
> Link: https://lore.kernel.org/asahi/[e-mail]/
> Link: https://lore.kernel.org/asahi/[e-mail]/
> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> Cc: [e-mail]
> Reviewed-by: Marc Zyngier <[e-mail]>
> Signed-off-by: Janne Grunau <[e-mail]>
> ---
> Changes in v2:
> - rewritten commit message with more details and corrections
> - collected Marc's "Reviewed-by:"
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  drivers/pci/controller/pcie-apple.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 66f37e403a09..f8670a032f7a 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  	cfg->priv = pcie;
>  	INIT_LIST_HEAD(&pcie->ports);
>  
> -	for_each_child_of_node(dev->of_node, of_port) {
> +	for_each_available_child_of_node(dev->of_node, of_port) {
>  		ret = apple_pcie_setup_port(pcie, of_port);
>  		if (ret) {
>  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);

Is this change still needed after 6fffbc7ae137 ("PCI: Honor firmware's
device disabled status")?  This is a generic problem, and it would be
a lot nicer if we had a generic solution.  But I assume it *is* still
needed because Rob gave his Reviewed-by.

Not related to this patch, but this function looks funny to me.  Most
pci_ecam_ops.init functions just set up ECAM-related things.

In addition to ECAM stuff, apple_pcie_init() and mc_platform_init()
also initialize IRQs, clocks, and resets.

Maybe we shoehorn the IRQ, clock, reset setup into pci_ecam_ops.init
because we lack a generic hook for doing those things, but it seems a
little muddy conceptually.

Bjorn

Conor Dooley March 9, 2023, 7:48 p.m. UTC | #4

On Thu, Mar 09, 2023 at 10:39:35AM -0600, Bjorn Helgaas wrote:
> [+cc Daire, Conor for apple/microchip use of ECAM .init() method]

It's probably really a Daire question, although he is off in the weeds
at the moment..
> 
> On Thu, Mar 09, 2023 at 02:36:24PM +0100, Janne Grunau wrote:
> > Fixes following warning inside of_irq_parse_raw() called from the common
> > PCI device probe path.
> > 
> >   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
> >   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
> 
> Based on this commit log, I assume this patch only fixes the warning,
> and the system *works* just fine either way.  If that's the case, it's
> debatable whether it meets the stable kernel criteria, although the
> documented criteria are much stricter than what happens in practice.
> 
> >   ...
> >   Call trace:
> >    of_irq_parse_raw+0x5fc/0x724
> >    of_irq_parse_and_map_pci+0x128/0x1d8
> >    pci_assign_irq+0xc8/0x140
> >    pci_device_probe+0x70/0x188
> >    really_probe+0x178/0x418
> >    __driver_probe_device+0x120/0x188
> >    driver_probe_device+0x48/0x22c
> >    __device_attach_driver+0x134/0x1d8
> >    bus_for_each_drv+0x8c/0xd8
> >    __device_attach+0xdc/0x1d0
> >    device_attach+0x20/0x2c
> >    pci_bus_add_device+0x5c/0xc0
> >    pci_bus_add_devices+0x58/0x88
> >    pci_host_probe+0x124/0x178
> >    pci_host_common_probe+0x124/0x198 [pci_host_common]
> >    apple_pcie_probe+0x108/0x16c [pcie_apple]
> >    platform_probe+0xb4/0xdc
> > 
> > This became apparent after disabling unused PCIe ports in the Apple
> > silicon device trees instead of deleting them.
> > 
> > Use for_each_available_child_of_node instead of for_each_child_of_node
> > which takes the "status" property into account.
> > 
> > Link: https://lore.kernel.org/asahi/[e-mail]/
> > Link: https://lore.kernel.org/asahi/[e-mail]/
> > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> > Cc: [e-mail]
> > Reviewed-by: Marc Zyngier <[e-mail]>
> > Signed-off-by: Janne Grunau <[e-mail]>
> > ---
> > Changes in v2:
> > - rewritten commit message with more details and corrections
> > - collected Marc's "Reviewed-by:"
> > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > ---
> >  drivers/pci/controller/pcie-apple.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index 66f37e403a09..f8670a032f7a 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> >  	cfg->priv = pcie;
> >  	INIT_LIST_HEAD(&pcie->ports);
> >  
> > -	for_each_child_of_node(dev->of_node, of_port) {
> > +	for_each_available_child_of_node(dev->of_node, of_port) {
> >  		ret = apple_pcie_setup_port(pcie, of_port);
> >  		if (ret) {
> >  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
> 
> Is this change still needed after 6fffbc7ae137 ("PCI: Honor firmware's
> device disabled status")?  This is a generic problem, and it would be
> a lot nicer if we had a generic solution.  But I assume it *is* still
> needed because Rob gave his Reviewed-by.
> 
> Not related to this patch, but this function looks funny to me.  Most
> pci_ecam_ops.init functions just set up ECAM-related things.
> 
> In addition to ECAM stuff, apple_pcie_init() and mc_platform_init()
> also initialize IRQs, clocks, and resets.
> 
> Maybe we shoehorn the IRQ, clock, reset setup into pci_ecam_ops.init
> because we lack a generic hook for doing those things, but it seems a
> little muddy conceptually.

"We", and that's very much the royal variety, were in the middle of
re-working some of this stuff actually.

At the moment we're using pci_host_common_probe() as
platform_driver.probe, but Daire has a patchset where he introduced an
mc_host_probe() instead, which sets up the clocks.
The interrupt setup is still done in the pci_ecam_ops.init function
though, although with a comment added noting that "Address translation
is up; safe to enable interrupts". The most relevant patch in that
series is:
https://lore.kernel.org/linux-pci/[e-mail]/

Robin/Lorenzo had some comments about the dt parsing that needed
investigation & Daire's off in the weeds, but hopefully we'll get another
version of that stuff out once he gets back.
That'll at least move the clock setup out of the ecam bits and provide a
reason for why we're doing the interrupt setup in pci_ecam_ops.init.

FWIW, doing it this way was review feedback at some stage during the
upstreaming process for the driver:
https://lore.kernel.org/linux-pci/20200709200055.GA763222@bogus/
Originally, Daire was doing this stuff in platform_device.probe but that
was 3 years ago, so maybe the winds have changed since then!

Hope that helps?
Conor.

Marc Zyngier March 16, 2023, 9:32 a.m. UTC | #5

On Thu, 09 Mar 2023 16:39:35 +0000,
Bjorn Helgaas <[e-mail]> wrote:
> 
> [+cc Daire, Conor for apple/microchip use of ECAM .init() method]
> 
> On Thu, Mar 09, 2023 at 02:36:24PM +0100, Janne Grunau wrote:
> > Fixes following warning inside of_irq_parse_raw() called from the common
> > PCI device probe path.
> > 
> >   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
> >   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
> 
> Based on this commit log, I assume this patch only fixes the warning,
> and the system *works* just fine either way.  If that's the case, it's
> debatable whether it meets the stable kernel criteria, although the
> documented criteria are much stricter than what happens in practice.
> 
> >   ...
> >   Call trace:
> >    of_irq_parse_raw+0x5fc/0x724
> >    of_irq_parse_and_map_pci+0x128/0x1d8
> >    pci_assign_irq+0xc8/0x140
> >    pci_device_probe+0x70/0x188
> >    really_probe+0x178/0x418
> >    __driver_probe_device+0x120/0x188
> >    driver_probe_device+0x48/0x22c
> >    __device_attach_driver+0x134/0x1d8
> >    bus_for_each_drv+0x8c/0xd8
> >    __device_attach+0xdc/0x1d0
> >    device_attach+0x20/0x2c
> >    pci_bus_add_device+0x5c/0xc0
> >    pci_bus_add_devices+0x58/0x88
> >    pci_host_probe+0x124/0x178
> >    pci_host_common_probe+0x124/0x198 [pci_host_common]
> >    apple_pcie_probe+0x108/0x16c [pcie_apple]
> >    platform_probe+0xb4/0xdc
> > 
> > This became apparent after disabling unused PCIe ports in the Apple
> > silicon device trees instead of deleting them.
> > 
> > Use for_each_available_child_of_node instead of for_each_child_of_node
> > which takes the "status" property into account.
> > 
> > Link: https://lore.kernel.org/asahi/[e-mail]/
> > Link: https://lore.kernel.org/asahi/[e-mail]/
> > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> > Cc: [e-mail]
> > Reviewed-by: Marc Zyngier <[e-mail]>
> > Signed-off-by: Janne Grunau <[e-mail]>
> > ---
> > Changes in v2:
> > - rewritten commit message with more details and corrections
> > - collected Marc's "Reviewed-by:"
> > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > ---
> >  drivers/pci/controller/pcie-apple.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index 66f37e403a09..f8670a032f7a 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> >  	cfg->priv = pcie;
> >  	INIT_LIST_HEAD(&pcie->ports);
> >  
> > -	for_each_child_of_node(dev->of_node, of_port) {
> > +	for_each_available_child_of_node(dev->of_node, of_port) {
> >  		ret = apple_pcie_setup_port(pcie, of_port);
> >  		if (ret) {
> >  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
> 
> Is this change still needed after 6fffbc7ae137 ("PCI: Honor firmware's
> device disabled status")?  This is a generic problem, and it would be
> a lot nicer if we had a generic solution.  But I assume it *is* still
> needed because Rob gave his Reviewed-by.

I'm not sure this is addressing the same issue. The way I read it, the
patch you mention here allows a PCI device to be disabled in firmware,
even if it could otherwise be probed.

What this patch does is to prevent root ports that exist in the HW but
that have been disabled from being probed. Same concept, only at a
different level.

> Not related to this patch, but this function looks funny to me.  Most
> pci_ecam_ops.init functions just set up ECAM-related things.
> 
> In addition to ECAM stuff, apple_pcie_init() and mc_platform_init()
> also initialize IRQs, clocks, and resets.

And more. We also initialise the RID-to-SID mapping that control the
view the downstream IOMMU has of the devices controlled by the root
port.

> Maybe we shoehorn the IRQ, clock, reset setup into pci_ecam_ops.init
> because we lack a generic hook for doing those things, but it seems a
> little muddy conceptually.

Indeed. I used this callback as it was convenient ordering wise, but
this is conceptually a platform init thing.  The current state of the
ECAM setup doesn't allow any other callback that would suit the
context.

	M.

Lorenzo Pieralisi March 16, 2023, 10:47 a.m. UTC | #6

On Thu, Mar 16, 2023 at 09:32:35AM +0000, Marc Zyngier wrote:
> On Thu, 09 Mar 2023 16:39:35 +0000,
> Bjorn Helgaas <[e-mail]> wrote:
> > 
> > [+cc Daire, Conor for apple/microchip use of ECAM .init() method]
> > 
> > On Thu, Mar 09, 2023 at 02:36:24PM +0100, Janne Grunau wrote:
> > > Fixes following warning inside of_irq_parse_raw() called from the common
> > > PCI device probe path.
> > > 
> > >   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
> > >   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
> > 
> > Based on this commit log, I assume this patch only fixes the warning,
> > and the system *works* just fine either way.  If that's the case, it's
> > debatable whether it meets the stable kernel criteria, although the
> > documented criteria are much stricter than what happens in practice.
> > 
> > >   ...
> > >   Call trace:
> > >    of_irq_parse_raw+0x5fc/0x724
> > >    of_irq_parse_and_map_pci+0x128/0x1d8
> > >    pci_assign_irq+0xc8/0x140
> > >    pci_device_probe+0x70/0x188
> > >    really_probe+0x178/0x418
> > >    __driver_probe_device+0x120/0x188
> > >    driver_probe_device+0x48/0x22c
> > >    __device_attach_driver+0x134/0x1d8
> > >    bus_for_each_drv+0x8c/0xd8
> > >    __device_attach+0xdc/0x1d0
> > >    device_attach+0x20/0x2c
> > >    pci_bus_add_device+0x5c/0xc0
> > >    pci_bus_add_devices+0x58/0x88
> > >    pci_host_probe+0x124/0x178
> > >    pci_host_common_probe+0x124/0x198 [pci_host_common]
> > >    apple_pcie_probe+0x108/0x16c [pcie_apple]
> > >    platform_probe+0xb4/0xdc
> > > 
> > > This became apparent after disabling unused PCIe ports in the Apple
> > > silicon device trees instead of deleting them.
> > > 
> > > Use for_each_available_child_of_node instead of for_each_child_of_node
> > > which takes the "status" property into account.
> > > 
> > > Link: https://lore.kernel.org/asahi/[e-mail]/
> > > Link: https://lore.kernel.org/asahi/[e-mail]/
> > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> > > Cc: [e-mail]
> > > Reviewed-by: Marc Zyngier <[e-mail]>
> > > Signed-off-by: Janne Grunau <[e-mail]>
> > > ---
> > > Changes in v2:
> > > - rewritten commit message with more details and corrections
> > > - collected Marc's "Reviewed-by:"
> > > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > > ---
> > >  drivers/pci/controller/pcie-apple.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > > index 66f37e403a09..f8670a032f7a 100644
> > > --- a/drivers/pci/controller/pcie-apple.c
> > > +++ b/drivers/pci/controller/pcie-apple.c
> > > @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> > >  	cfg->priv = pcie;
> > >  	INIT_LIST_HEAD(&pcie->ports);
> > >  
> > > -	for_each_child_of_node(dev->of_node, of_port) {
> > > +	for_each_available_child_of_node(dev->of_node, of_port) {
> > >  		ret = apple_pcie_setup_port(pcie, of_port);
> > >  		if (ret) {
> > >  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
> > 
> > Is this change still needed after 6fffbc7ae137 ("PCI: Honor firmware's
> > device disabled status")?  This is a generic problem, and it would be
> > a lot nicer if we had a generic solution.  But I assume it *is* still
> > needed because Rob gave his Reviewed-by.
> 
> I'm not sure this is addressing the same issue. The way I read it, the
> patch you mention here allows a PCI device to be disabled in firmware,
> even if it could otherwise be probed.
> 
> What this patch does is to prevent root ports that exist in the HW but
> that have been disabled from being probed. Same concept, only at a
> different level.

A root port is a PCI device though and that's what's causing the warning
AFAIK (? it is triggered on the root port PCI device pci_assign_irq()
call), I am not sure the root port DT node is associated with the root
port PCI device correctly, which might explain why, even after
6fffbc7ae137, the PCI enumeration code is adding the root port PCI
device to the PCI tree.

Is the dts available anywhere ? How are root ports described in it ?

Lorenzo

Marc Zyngier March 16, 2023, 11:18 a.m. UTC | #7

On Thu, 16 Mar 2023 10:47:11 +0000,
Lorenzo Pieralisi <[e-mail]> wrote:
> 
> On Thu, Mar 16, 2023 at 09:32:35AM +0000, Marc Zyngier wrote:
> > On Thu, 09 Mar 2023 16:39:35 +0000,
> > Bjorn Helgaas <[e-mail]> wrote:
> > > 
> > > [+cc Daire, Conor for apple/microchip use of ECAM .init() method]
> > > 
> > > On Thu, Mar 09, 2023 at 02:36:24PM +0100, Janne Grunau wrote:
> > > > Fixes following warning inside of_irq_parse_raw() called from the common
> > > > PCI device probe path.
> > > > 
> > > >   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
> > > >   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
> > > 
> > > Based on this commit log, I assume this patch only fixes the warning,
> > > and the system *works* just fine either way.  If that's the case, it's
> > > debatable whether it meets the stable kernel criteria, although the
> > > documented criteria are much stricter than what happens in practice.
> > > 
> > > >   ...
> > > >   Call trace:
> > > >    of_irq_parse_raw+0x5fc/0x724
> > > >    of_irq_parse_and_map_pci+0x128/0x1d8
> > > >    pci_assign_irq+0xc8/0x140
> > > >    pci_device_probe+0x70/0x188
> > > >    really_probe+0x178/0x418
> > > >    __driver_probe_device+0x120/0x188
> > > >    driver_probe_device+0x48/0x22c
> > > >    __device_attach_driver+0x134/0x1d8
> > > >    bus_for_each_drv+0x8c/0xd8
> > > >    __device_attach+0xdc/0x1d0
> > > >    device_attach+0x20/0x2c
> > > >    pci_bus_add_device+0x5c/0xc0
> > > >    pci_bus_add_devices+0x58/0x88
> > > >    pci_host_probe+0x124/0x178
> > > >    pci_host_common_probe+0x124/0x198 [pci_host_common]
> > > >    apple_pcie_probe+0x108/0x16c [pcie_apple]
> > > >    platform_probe+0xb4/0xdc
> > > > 
> > > > This became apparent after disabling unused PCIe ports in the Apple
> > > > silicon device trees instead of deleting them.
> > > > 
> > > > Use for_each_available_child_of_node instead of for_each_child_of_node
> > > > which takes the "status" property into account.
> > > > 
> > > > Link: https://lore.kernel.org/asahi/[e-mail]/
> > > > Link: https://lore.kernel.org/asahi/[e-mail]/
> > > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> > > > Cc: [e-mail]
> > > > Reviewed-by: Marc Zyngier <[e-mail]>
> > > > Signed-off-by: Janne Grunau <[e-mail]>
> > > > ---
> > > > Changes in v2:
> > > > - rewritten commit message with more details and corrections
> > > > - collected Marc's "Reviewed-by:"
> > > > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > > > ---
> > > >  drivers/pci/controller/pcie-apple.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > > > index 66f37e403a09..f8670a032f7a 100644
> > > > --- a/drivers/pci/controller/pcie-apple.c
> > > > +++ b/drivers/pci/controller/pcie-apple.c
> > > > @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> > > >  	cfg->priv = pcie;
> > > >  	INIT_LIST_HEAD(&pcie->ports);
> > > >  
> > > > -	for_each_child_of_node(dev->of_node, of_port) {
> > > > +	for_each_available_child_of_node(dev->of_node, of_port) {
> > > >  		ret = apple_pcie_setup_port(pcie, of_port);
> > > >  		if (ret) {
> > > >  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
> > > 
> > > Is this change still needed after 6fffbc7ae137 ("PCI: Honor firmware's
> > > device disabled status")?  This is a generic problem, and it would be
> > > a lot nicer if we had a generic solution.  But I assume it *is* still
> > > needed because Rob gave his Reviewed-by.
> > 
> > I'm not sure this is addressing the same issue. The way I read it, the
> > patch you mention here allows a PCI device to be disabled in firmware,
> > even if it could otherwise be probed.
> > 
> > What this patch does is to prevent root ports that exist in the HW but
> > that have been disabled from being probed. Same concept, only at a
> > different level.
> 
> A root port is a PCI device though and that's what's causing the warning
> AFAIK (? it is triggered on the root port PCI device pci_assign_irq()
> call),

As usual, there are two sides to things. The root ports are also part
of a platform device, and this what this patch uses.

> I am not sure the root port DT node is associated with the root
> port PCI device correctly, which might explain why, even after
> 6fffbc7ae137, the PCI enumeration code is adding the root port PCI
> device to the PCI tree.

I didn't say that commit did not suppress the warning. I haven't
tested it the first place because I really need all the PCIe ports I
can get on the machines I have.

It just feels to me that they are tracking different things.

> Is the dts available anywhere ? How are root ports described in it ?

arch/arm64/boot/dts/apple/t8103.dtsi
Documentation/devicetree/bindings/pci/apple,pcie.yaml

	M.

Janne Grunau March 16, 2023, 9:22 p.m. UTC | #8

On 2023-03-09 10:39:35 -0600, Bjorn Helgaas wrote:
> [+cc Daire, Conor for apple/microchip use of ECAM .init() method]
> 
> On Thu, Mar 09, 2023 at 02:36:24PM +0100, Janne Grunau wrote:
> > Fixes following warning inside of_irq_parse_raw() called from the common
> > PCI device probe path.
> > 
> >   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
> >   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
> 
> Based on this commit log, I assume this patch only fixes the warning,
> and the system *works* just fine either way.  If that's the case, it's
> debatable whether it meets the stable kernel criteria, although the
> documented criteria are much stricter than what happens in practice.

Yes, it fixes only the warning and hides devices. The present devices 
still work. I confused myself with the submitted M2 devicetree. It 
missed information in the dt node of an disabled PCIe port breaking 
probing of the pcie controller.
 
I agree that the Cc: stable is not necessary. Please drop it or tell me 
to resend the change.
 
> >   ...
> >   Call trace:
> >    of_irq_parse_raw+0x5fc/0x724
> >    of_irq_parse_and_map_pci+0x128/0x1d8
> >    pci_assign_irq+0xc8/0x140
> >    pci_device_probe+0x70/0x188
> >    really_probe+0x178/0x418
> >    __driver_probe_device+0x120/0x188
> >    driver_probe_device+0x48/0x22c
> >    __device_attach_driver+0x134/0x1d8
> >    bus_for_each_drv+0x8c/0xd8
> >    __device_attach+0xdc/0x1d0
> >    device_attach+0x20/0x2c
> >    pci_bus_add_device+0x5c/0xc0
> >    pci_bus_add_devices+0x58/0x88
> >    pci_host_probe+0x124/0x178
> >    pci_host_common_probe+0x124/0x198 [pci_host_common]
> >    apple_pcie_probe+0x108/0x16c [pcie_apple]
> >    platform_probe+0xb4/0xdc
> > 
> > This became apparent after disabling unused PCIe ports in the Apple
> > silicon device trees instead of deleting them.
> > 
> > Use for_each_available_child_of_node instead of for_each_child_of_node
> > which takes the "status" property into account.
> > 
> > Link: https://lore.kernel.org/asahi/[e-mail]/
> > Link: https://lore.kernel.org/asahi/[e-mail]/
> > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> > Cc: [e-mail]
> > Reviewed-by: Marc Zyngier <[e-mail]>
> > Signed-off-by: Janne Grunau <[e-mail]>
> > ---
> > Changes in v2:
> > - rewritten commit message with more details and corrections
> > - collected Marc's "Reviewed-by:"
> > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > ---
> >  drivers/pci/controller/pcie-apple.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index 66f37e403a09..f8670a032f7a 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> >  	cfg->priv = pcie;
> >  	INIT_LIST_HEAD(&pcie->ports);
> >  
> > -	for_each_child_of_node(dev->of_node, of_port) {
> > +	for_each_available_child_of_node(dev->of_node, of_port) {
> >  		ret = apple_pcie_setup_port(pcie, of_port);
> >  		if (ret) {
> >  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
> 
> Is this change still needed after 6fffbc7ae137 ("PCI: Honor firmware's
> device disabled status")?  This is a generic problem, and it would be
> a lot nicer if we had a generic solution.  But I assume it *is* still
> needed because Rob gave his Reviewed-by.

6fffbc7ae137 avoids the warning and hides the disabled ports as well. I 
think we want to keep this change however in the hope that avoiding the 
port setup of disbled ports saves energy.

Thanks

Janne

Lorenzo Pieralisi March 17, 2023, 9:12 a.m. UTC | #9

On Thu, Mar 16, 2023 at 10:22:17PM +0100, Janne Grunau wrote:
> On 2023-03-09 10:39:35 -0600, Bjorn Helgaas wrote:
> > [+cc Daire, Conor for apple/microchip use of ECAM .init() method]
> > 
> > On Thu, Mar 09, 2023 at 02:36:24PM +0100, Janne Grunau wrote:
> > > Fixes following warning inside of_irq_parse_raw() called from the common
> > > PCI device probe path.
> > > 
> > >   /soc/pcie@690000000/pci@1,0 interrupt-map failed, using interrupt-controller
> > >   WARNING: CPU: 4 PID: 252 at drivers/of/irq.c:279 of_irq_parse_raw+0x5fc/0x724
> > 
> > Based on this commit log, I assume this patch only fixes the warning,
> > and the system *works* just fine either way.  If that's the case, it's
> > debatable whether it meets the stable kernel criteria, although the
> > documented criteria are much stricter than what happens in practice.
> 
> Yes, it fixes only the warning and hides devices. The present devices 
> still work. I confused myself with the submitted M2 devicetree. It 
> missed information in the dt node of an disabled PCIe port breaking 
> probing of the pcie controller.
>  
> I agree that the Cc: stable is not necessary. Please drop it or tell me 
> to resend the change.

I think this change is not needed as it stands. If the goal is
to disable *probing* some root ports for power efficiency, we
need a different commit log altogether, see below.

> > >   ...
> > >   Call trace:
> > >    of_irq_parse_raw+0x5fc/0x724
> > >    of_irq_parse_and_map_pci+0x128/0x1d8
> > >    pci_assign_irq+0xc8/0x140
> > >    pci_device_probe+0x70/0x188
> > >    really_probe+0x178/0x418
> > >    __driver_probe_device+0x120/0x188
> > >    driver_probe_device+0x48/0x22c
> > >    __device_attach_driver+0x134/0x1d8
> > >    bus_for_each_drv+0x8c/0xd8
> > >    __device_attach+0xdc/0x1d0
> > >    device_attach+0x20/0x2c
> > >    pci_bus_add_device+0x5c/0xc0
> > >    pci_bus_add_devices+0x58/0x88
> > >    pci_host_probe+0x124/0x178
> > >    pci_host_common_probe+0x124/0x198 [pci_host_common]
> > >    apple_pcie_probe+0x108/0x16c [pcie_apple]
> > >    platform_probe+0xb4/0xdc
> > > 
> > > This became apparent after disabling unused PCIe ports in the Apple
> > > silicon device trees instead of deleting them.
> > > 
> > > Use for_each_available_child_of_node instead of for_each_child_of_node
> > > which takes the "status" property into account.
> > > 
> > > Link: https://lore.kernel.org/asahi/[e-mail]/
> > > Link: https://lore.kernel.org/asahi/[e-mail]/
> > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> > > Cc: [e-mail]
> > > Reviewed-by: Marc Zyngier <[e-mail]>
> > > Signed-off-by: Janne Grunau <[e-mail]>
> > > ---
> > > Changes in v2:
> > > - rewritten commit message with more details and corrections
> > > - collected Marc's "Reviewed-by:"
> > > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > > ---
> > >  drivers/pci/controller/pcie-apple.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > > index 66f37e403a09..f8670a032f7a 100644
> > > --- a/drivers/pci/controller/pcie-apple.c
> > > +++ b/drivers/pci/controller/pcie-apple.c
> > > @@ -783,7 +783,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> > >  	cfg->priv = pcie;
> > >  	INIT_LIST_HEAD(&pcie->ports);
> > >  
> > > -	for_each_child_of_node(dev->of_node, of_port) {
> > > +	for_each_available_child_of_node(dev->of_node, of_port) {
> > >  		ret = apple_pcie_setup_port(pcie, of_port);
> > >  		if (ret) {
> > >  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
> > 
> > Is this change still needed after 6fffbc7ae137 ("PCI: Honor firmware's
> > device disabled status")?  This is a generic problem, and it would be
> > a lot nicer if we had a generic solution.  But I assume it *is* still
> > needed because Rob gave his Reviewed-by.
> 
> 6fffbc7ae137 avoids the warning and hides the disabled ports as well. I 
> think we want to keep this change however in the hope that avoiding the 
> port setup of disbled ports saves energy.

That's fine but the commit log must be rewritten, it is a completely
different aim than the one stated in the current commit log and it
is not stable material.

Thanks,
Lorenzo

Patch

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 66f37e403a09..f8670a032f7a 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -783,7 +783,7 @@  static int apple_pcie_init(struct pci_config_window *cfg)
 	cfg->priv = pcie;
 	INIT_LIST_HEAD(&pcie->ports);
 
-	for_each_child_of_node(dev->of_node, of_port) {
+	for_each_available_child_of_node(dev->of_node, of_port) {
 		ret = apple_pcie_setup_port(pcie, of_port);
 		if (ret) {
 			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);