Re: [RFC PATCH v2 06/21] RISC-V: Kconfig: Select deferred GSI probe for ACPI systems

[Bjorn Helgaas]

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@xxxxxxxxxxxxxxxx