Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Mon, 6 Nov 2023 16:16:06 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Sunil V L <sunilvl@ventanamicro.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-riscv@lists.infradead.org, linux-acpi@vger.kernel.org,
        linux-pci@vger.kernel.org, linux-serial@vger.kernel.org,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Anup Patel <anup@brainfault.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Conor Dooley <conor.dooley@microchip.com>,
        Andrew Jones <ajones@ventanamicro.com>,
        Atish Kumar Patra <atishp@rivosinc.com>,
        Haibo Xu <haibo1.xu@intel.com>
Subject: Re: [RFC PATCH v2 06/21] RISC-V: Kconfig: Select deferred GSI probe
 for ACPI systems
Message-ID: <20231106221606.GA264641@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZTuzJ1nsicZYp+uh@sunil-laptop>
Precedence: bulk

On Fri, Oct 27, 2023 at 06:25:03PM +0530, Sunil V L wrote:
> On Thu, Oct 26, 2023 at 12:04:08PM -0500, Bjorn Helgaas wrote:
> > On Thu, Oct 26, 2023 at 01:53:29AM +0530, Sunil V L wrote:
> > > On RISC-V platforms, apart from root interrupt controllers (which
> > > provide local interrupts and IPI), other interrupt controllers in the
> > > hierarchy are probed late. Enable this select this CONFIG option for
> > > RISC-V platforms so that device drivers which connect to deferred
> > > interrupt controllers can take appropriate action.
> > 
> > Quite a bit of this series seems related to the question of interrupt
> > controllers being probed "late".
> > 
> > I don't see anything specific about *how* late this might be, but from
> > the use of -EPROBE_DEFER in individual drivers (8250_pnp explicitly,
> > and acpi_register_gsi() and pnp_irq() and acpi_pci_irq_enable(), which
> > are called from driver .probe() paths) it seems like interrupt
> > controllers might be detected even after devices that use them.
> > 
> > That seems like a fairly invasive change to the driver probe flow.
> > If we really need to do that, I think it might merit a little more
> > background as justification since we haven't had to do it for any
> > other arch yet.
> 
> In RISC-V, the APLIC can be a converter from wired (GSI) to MSI interrupts.
> Hence, especially in this mode, it has to be a platform device to use
> device MSI domain. Also, according to Marc Zyngier there is no reason to
> probe interrupt controllers early apart from root controller. So, the
> device drivers which use wired interrupts need to be probed after APLIC.
> 
> The PNP devices and PCI INTx GSI links use either
> acpi_dev_resource_interrupt() (PNP) or acpi_register_gsi() directly
> (PCI). The approach taken here is to follow the example of
> acpi_irq_get() which is enhanced to return EPROBE_DEFER and several
> platform device drivers which use platform_get_irq() seem to be handling
> this already.

This series (patch 04/21 "ACPI: irq: Add support for deferred probe in
acpi_register_gsi()" [1]) makes acpi_register_gsi() return
-EPROBE_DEFER, which percolates up through pci_enable_device().

Maybe that's ok, but this affects *all* PCI drivers, and it's a new
case that did not occur before.  Many drivers emit warning or error
messages for any pci_enable_device() failure, which you probably don't
want in this case, since -EPROBE_DEFER is not really a "failure";
IIUC, it just means "probe again later."

> Using ResourceSource dependency (mbigen uses) in the namespace as part of
> Extended Interrupt Descriptor will not ensure the order since PNP/INTx
> GSI devices don't work with that.

Are these PNP/INTx GSI devices described in ACPI?  In the namespace?
Or in a static table?

> Is there any other better way to create dependency between IO devices
> and the interrupt controllers when interrupt controller itself is a
> platform device? While using core_initcall() for interrupt controllers
> seem to work which forces the interrupt controller to be probed first,
> Marc is not in favor of that approach since it is fragile.

I guess PCI interrupts from the PCI host bridges (PNP0A03 devices)
feed into the APLIC?  And APLIC is described via MADT?  Based on this
series, it looks like this:

    acpi_init
  +   acpi_riscv_init
  +     riscv_acpi_aplic_platform_init
  +       acpi_table_parse_madt(ACPI_MADT_TYPE_APLIC, aplic_parse_madt, 0)
      acpi_scan_init
        acpi_pci_root_init
        acpi_pci_link_init
	acpi_bus_scan             # add PCI host bridges, etc

If that's the sequence, it looks like aplic_parse_madt() should be
called before the PCI host bridges are added.

Or maybe this isn't how the APLICs are enumerated?

Bjorn

[1] https://lore.kernel.org/r/20231025202344.581132-5-sunilvl@ventanamicro.com