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

[Hanjun Guo]

On 2022/6/28 16:45, Jianmin Lv wrote:
> On 2022/6/28 下午3:42, Hanjun Guo wrote:
> > 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.

What? Tomorrow? more coffee is needed... it's yesterday...

> > 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.
>
> Thanks for your reply, Hanjun, LoongArch uses non-reduced ACPI hardware,
> so SCI interrupt is used, which is different from ARM using reduced 
> hardware.

OK, so for ARM64, it will not call acpi_gsi_to_irq() before the
irqdomain and mapping created.

Thanks
Hanjun