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-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.
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