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

From: Hanjun Guo
Date: Tue Jun 28 2022 - 05:12:36 EST


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