Messages in this thread | | | Subject | Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains | From | Jianmin Lv <> | Date | Tue, 28 Jun 2022 16:45:05 +0800 |
| |
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. > > 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.
>> >>> 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. > > Looks good to me, I will review and test the v13 patch set. > > Thanks > Hanjun
| |