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

[Jianmin Lv]

On 2022/6/15 下午3:14, Marc Zyngier wrote:
> On Wed, 15 Jun 2022 07:07:21 +0100,
> Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:
> > From: Marc Zyngier <maz@xxxxxxxxxx>
> >
> > In an unfortunate departure from the ACPI spec, the LoongArch
> > architecture split its GSI space across multiple interrupt
> > controllers.
> >
> > In order to be able to reuse sthe core code and prevent
> > architectures from reinventing an already square wheel, offer
> > the arch code the ability to register a dispatcher function
> > that will return the domain fwnode for a given GSI.
> >
> > The ARM GIC drivers are updated to support this (with a single
> > domain, as intended).
> >
> > Co-developed-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
>
> I don't think this tag is appropriate here.
>
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > Cc: Hanjun Guo <guohanjun@xxxxxxxxxx>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Signed-off-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
> > ---
> >   drivers/acpi/irq.c           | 40 +++++++++++++++++++++++-----------------
> >   drivers/irqchip/irq-gic-v3.c | 18 ++++++++++++------
> >   drivers/irqchip/irq-gic.c    | 18 ++++++++++++------
> >   include/linux/acpi.h         |  2 +-
> >   4 files changed, 48 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
> > index c68e694..b7460ab 100644
> > --- 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.


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.


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.

> 	M.