Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains

[Hanjun Guo]

On 2022/6/18 18:36, Marc Zyngier wrote:
[...]
> > > > --- a/drivers/acpi/irq.c
> > > > +++ b/drivers/acpi/irq.c
> > > > @@ -12,7 +12,7 @@
> > > >      enum acpi_irq_model_id acpi_irq_model;
> > > >    -static struct fwnode_handle *acpi_gsi_domain_id;
> > > > +static struct fwnode_handle *(*acpi_get_gsi_domain_id)(u32 gsi);
> > > >      /**
> > > >     * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
> > > > @@ -26,10 +26,7 @@
> > > >     */
> > > >    int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> > > >    {
> > > > -	struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> > > > -							DOMAIN_BUS_ANY);
> > > > -
> > > > -	*irq = irq_find_mapping(d, gsi);
> > > > +	*irq = acpi_register_gsi(NULL, gsi, -1, -1);
> > >
> > > What is this?
> > >
> > > - This wasn't part of my initial patch, and randomly changing patches
> > >     without mentioning it isn't acceptable
> > >
> > > - you *cannot* trigger a registration here, as this isn't what the API
> > >     advertises
> > >
> > > - what makes you think that passing random values (NULL, -1... )to
> > >     acpi_register_gsi() is an acceptable thing to do?
> > >
> > > The original patch had:
> > >
> > > @@ -26,8 +26,10 @@ static struct fwnode_handle *acpi_gsi_domain_id;
> > >      */
> > >     int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> > >     {
> > > -	struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> > > -							DOMAIN_BUS_ANY);
> > > +	struct irq_domain *d;
> > > +
> > > +	d = irq_find_matching_fwnode(acpi_get_gsi_domain_id(gsi),
> > > +				     DOMAIN_BUS_ANY);
> > >       	*irq = irq_find_mapping(d, gsi);
> > >     	/*
> > >
> > > and I don't think it needs anything else. If something breaks, let's
> > > discuss it, but don't abuse the API nor the fact that I usually don't
> > > review my own patches to sneak things in...
> >
> > Sorry, Marc, I don't know how to communicate with you for my change
> > here before submitting the patch, maybe I should mention it in the
> > patch commit or code.
>
> It should at least be discussed first, like you are doing it here.
>
> > When I use the patch, I found that acpi_gsi_to_irq in driver/acpi/irq.c
> > only handle existed mapping and will return -EINVAL if mapping not
> > found. When I test on my machine, a calling stack is as following:
> >
> >
> > acpi_bus_init
> > ->acpi_enable_subsystem
> >    ->acpi_ev_install_xrupt_handlers
> >      ->acpi_ev_install_sci_handler
> >        ->acpi_os_install_interrupt_handler
> >          ->acpi_gsi_to_irq
> >
> >
> > the acpi_gsi_to_irq returned -EINVAL because of no mapping found. I
> > looked into acpi_gsi_to_irq of x86, acpi_register_gsi is called in it
> > so that new mapping for gsi is created if no mapping is found.
>
> So it looks like we have a discrepancy between the x86 and ARM on that
> front.
>
> Lorenzo, Hanjun, can you please have a look at this and shed some
> light on what the expected behaviour is? It looks like we never
> encountered an issue with this on arm64, which tends to indicate that
> we don't usually use the above path.

Sorry for the late reply, I just noticed this tomorrow.

As you said, we never encountered Jianmin's issue on ARM64 hardware,
for the call stack which Jianmin shows, acpi_ev_install_xrupt_handlers()
is only called for non-reduced ACPI hardware, but ARM64 is always
defined as reduced ACPI hardware in the ACPI spec, from the first
supported version of ACPI spec for ARM.

Jianmin, is the LoongArch using the redunced hardware mode in ACPI?
if it's using SCI interrupt, I think not, correct me if I'm wrong.

> > I looked into generic acpi_register_gsi, the existed mapping will be
> > checked first by calling irq_find_mapping, so I think calling
> > acpi_register_gsi in acpi_gsi_to_irq can address the problem.
> >
> > But you're right, I'm wrong that I passed random value of -1 to
> > acpi_register_gsi. I don't find a right way to address the problem
> > without changing acpi_gsi_to_irq. I'll continue to work for the
> > problem.
>
> At the very least, this should be indirected so that the existing
> behaviour isn't affected, no matter how badly broken arm64 may or may
> not be here. Please have a look at the patch below that should help
> you with this.

Looks good to me, I will review and test the v13 patch set.

Thanks
Hanjun