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

[Marc Zyngier]

On Wed, 15 Jun 2022 10:28:47 +0100,
Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:
> 
> 
> 
> 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.

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.

> 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.

Thanks,

	M.

From 3e6b87ea49473d0eb384f42e76d584a1495a538c Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@xxxxxxxxxx>
Date: Sat, 18 Jun 2022 11:29:33 +0100
Subject: [PATCH] ACPI: irq: Allow acpi_gsi_to_irq() to have an arch-specific
 fallback

It appears that the generic version of acpi_gsi_to_irq() doesn't
fallback to establishing a mapping if there is no pre-existing
one while the x86 version does.

While arm64 seems unaffected by it, LoongArch is relying on the x86
behaviour. In an effort to prevent new architectures from reinventing
the proverbial wheel, provide an optional callback that the arch code
can set to restore the x86 behaviour.

Hopefully we can eventually get rid of this in the future once
the expected behaviour has been clarified.

Reported-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
---
 drivers/acpi/irq.c   | 8 ++++++--
 include/linux/acpi.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
index 6e1633ac1756..66c5f01995d0 100644
--- a/drivers/acpi/irq.c
+++ b/drivers/acpi/irq.c
@@ -13,6 +13,7 @@
 enum acpi_irq_model_id acpi_irq_model;
 
 static struct fwnode_handle *(*acpi_get_gsi_domain_id)(u32 gsi);
+static int (*acpi_gsi_to_irq_fallback)(u32 gsi);
 
 /**
  * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
@@ -33,9 +34,12 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
 
 	*irq = irq_find_mapping(d, gsi);
 	/*
-	 * *irq == 0 means no mapping, that should
-	 * be reported as a failure
+	 * *irq == 0 means no mapping, that should be reported as a
+	 * failure, unless there is an arch-specific fallback handler.
 	 */
+	if (!*irq && acpi_gsi_to_irq_fallback)
+		*irq = acpi_gsi_to_irq_fallback(gsi);
+
 	return (*irq > 0) ? 0 : -EINVAL;
 }
 EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 957e23f727ea..71d3719e3ec4 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -357,6 +357,7 @@ int acpi_isa_irq_to_gsi (unsigned isa_irq, u32 *gsi);
 
 void acpi_set_irq_model(enum acpi_irq_model_id model,
 			struct fwnode_handle *(*)(u32));
+void acpi_set_gsi_to_irq_fallback(int (*)(u32));
 
 struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
 					     unsigned int size,
-- 
2.34.1


-- 
Without deviation from the norm, progress is not possible.