Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains
From: Jianmin Lv
Date: Mon Jun 27 2022 - 04:00:43 EST
On 2022/6/27 下午3:32, Marc Zyngier wrote:
On Sat, 25 Jun 2022 10:34:34 +0100,
Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:
On 2022/6/18 下午6:36, Marc Zyngier wrote:
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));
Hi, Marc
I want to make sure that if acpi_set_gsi_to_irq_fallback should be
implemented in driver/acpi/irq.c as acpi_set_irq_model, e.g.:
void __init acpi_set_gsi_to_irq_fallback(int (*fn)(u32))
{
acpi_gsi_to_irq_fallback = fn;
}
And then, arch related code can call acpi_set_gsi_to_irq_fallback
to register a callback.
Yes. I had something like that, but forgot to add it to the patch,
apparently.
Ok, I'll add that to the patch, please check the change in next version.
M.