Date:   Thu, 16 Mar 2023 09:32:35 +0000
Message-ID: <86a60dxcr0.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Janne Grunau <j@jannau.net>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Krzysztof =?UTF-8?B?V2lsY3p5?= =?UTF-8?B?xYRza2k=?= 
        <kw@linux.com>, Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Sven Peter <sven@svenpeter.dev>, linux-pci@vger.kernel.org,
        asahi@lists.linux.dev, linux-kernel@vger.kernel.org,
        Daire McNamara <daire.mcnamara@microchip.com>,
        Conor Dooley <conor.dooley@microchip.com>,
        stable@vger.kernel.org
Subject: Re: [PATCH v2] PCI: apple: Set only available ports up
In-Reply-To: <20230309163935.GA1140101@bhelgaas>
References: <20230307-apple_pcie_disabled_ports-v2-1-c3bd1fd278a4@jannau.net>
        <20230309163935.GA1140101@bhelgaas>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.

-- 
Without deviation from the norm, progress is not possible.