| Message ID | 20230307-apple_pcie_disabled_ports-v2-1-c3bd1fd278a4@jannau.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI: apple: Set only available ports up | expand |
On Thu, 9 Mar 2023 at 13:43, Janne Grunau <j@jannau.net> 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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > Cc: stable@vger.kernel.org > Reviewed-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Janne Grunau <j@jannau.net> Makes sense. Reviewed-by: Eric Curtin <ecurtin@redhat.com> 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/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > --- > 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 <j@jannau.net> > >
On Thu, Mar 9, 2023 at 7:36 AM Janne Grunau <j@jannau.net> 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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > Cc: stable@vger.kernel.org > Reviewed-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Janne Grunau <j@jannau.net> > --- > Changes in v2: > - rewritten commit message with more details and corrections > - collected Marc's "Reviewed-by:" > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > --- > drivers/pci/controller/pcie-apple.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Unfortunately, this is a common issue... Reviewed-by: Rob Herring <robh@kernel.org> Rob
[+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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > Cc: stable@vger.kernel.org > Reviewed-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Janne Grunau <j@jannau.net> > --- > Changes in v2: > - rewritten commit message with more details and corrections > - collected Marc's "Reviewed-by:" > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > --- > 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
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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > > Cc: stable@vger.kernel.org > > Reviewed-by: Marc Zyngier <maz@kernel.org> > > Signed-off-by: Janne Grunau <j@jannau.net> > > --- > > Changes in v2: > > - rewritten commit message with more details and corrections > > - collected Marc's "Reviewed-by:" > > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > > --- > > 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/20230111125323.1911373-9-daire.mcnamara@microchip.com/ 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.
On Thu, 09 Mar 2023 16:39:35 +0000, Bjorn Helgaas <helgaas@kernel.org> 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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > > Cc: stable@vger.kernel.org > > Reviewed-by: Marc Zyngier <maz@kernel.org> > > Signed-off-by: Janne Grunau <j@jannau.net> > > --- > > Changes in v2: > > - rewritten commit message with more details and corrections > > - collected Marc's "Reviewed-by:" > > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > > --- > > 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.
On Thu, Mar 16, 2023 at 09:32:35AM +0000, Marc Zyngier wrote: > On Thu, 09 Mar 2023 16:39:35 +0000, > Bjorn Helgaas <helgaas@kernel.org> 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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > > > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > > > Cc: stable@vger.kernel.org > > > Reviewed-by: Marc Zyngier <maz@kernel.org> > > > Signed-off-by: Janne Grunau <j@jannau.net> > > > --- > > > Changes in v2: > > > - rewritten commit message with more details and corrections > > > - collected Marc's "Reviewed-by:" > > > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > > > --- > > > 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
On Thu, 16 Mar 2023 10:47:11 +0000, Lorenzo Pieralisi <lpieralisi@kernel.org> 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 <helgaas@kernel.org> 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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > > > > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > > > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > > > > Cc: stable@vger.kernel.org > > > > Reviewed-by: Marc Zyngier <maz@kernel.org> > > > > Signed-off-by: Janne Grunau <j@jannau.net> > > > > --- > > > > Changes in v2: > > > > - rewritten commit message with more details and corrections > > > > - collected Marc's "Reviewed-by:" > > > > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > > > > --- > > > > 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.
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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > > Cc: stable@vger.kernel.org > > Reviewed-by: Marc Zyngier <maz@kernel.org> > > Signed-off-by: Janne Grunau <j@jannau.net> > > --- > > Changes in v2: > > - rewritten commit message with more details and corrections > > - collected Marc's "Reviewed-by:" > > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > > --- > > 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
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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ > > > Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ > > > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") > > > Cc: stable@vger.kernel.org > > > Reviewed-by: Marc Zyngier <maz@kernel.org> > > > Signed-off-by: Janne Grunau <j@jannau.net> > > > --- > > > Changes in v2: > > > - rewritten commit message with more details and corrections > > > - collected Marc's "Reviewed-by:" > > > - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net > > > --- > > > 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
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);
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/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/ Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/ Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up") Cc: stable@vger.kernel.org Reviewed-by: Marc Zyngier <maz@kernel.org> Signed-off-by: Janne Grunau <j@jannau.net> --- Changes in v2: - rewritten commit message with more details and corrections - collected Marc's "Reviewed-by:" - Link to v1: https://lore.kernel.org/r/20230307-apple_pcie_disabled_ports-v1-1-b32ef91faf19@jannau.net --- 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,