Messages in this thread | | | Subject | Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains | From | Hanjun Guo <> | Date | Tue, 28 Jun 2022 17:12:24 +0800 |
| |
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
| |