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

[Marc Zyngier]

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.

	M.

-- 
Without deviation from the norm, progress is not possible.