[+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-5e... Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.or... 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-b32ef91faf...
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