Messages in this thread | | | Subject | Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains | From | Jianmin Lv <> | Date | Wed, 15 Jun 2022 17:28:47 +0800 |
| |
On 2022/6/15 下午3:14, Marc Zyngier wrote: > On Wed, 15 Jun 2022 07:07:21 +0100, > Jianmin Lv <lvjianmin@loongson.cn> wrote: >> >> From: Marc Zyngier <maz@kernel.org> >> >> 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@loongson.cn> > > I don't think this tag is appropriate here. > >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> Cc: Hanjun Guo <guohanjun@huawei.com> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> >> --- >> 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.
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.
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.
> M. >
| |