Message-ID: <b5089477-11a3-a013-79cf-0826809d778a@loongson.cn>
Date:   Mon, 27 Jun 2022 16:00:27 +0800
From:   Jianmin Lv <lvjianmin@...ngson.cn>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
        Hanjun Guo <guohanjun@...wei.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains



On 2022/6/27 下午3:32, Marc Zyngier wrote:
> On Sat, 25 Jun 2022 10:34:34 +0100,
> Jianmin Lv <lvjianmin@...ngson.cn> wrote:
>>
>>
>>
>> On 2022/6/18 下午6:36, Marc Zyngier wrote:
>>> On Wed, 15 Jun 2022 10:28:47 +0100,
>>> Jianmin Lv <lvjianmin@...ngson.cn> wrote:
>>>>
>>>>
>>>>
>>>> On 2022/6/15 下午3:14, Marc Zyngier wrote:
>>>>> On Wed, 15 Jun 2022 07:07:21 +0100,
>>>>> Jianmin Lv <lvjianmin@...ngson.cn> wrote:
>>>>>>
>>>>>> From: Marc Zyngier <maz@...nel.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@...ngson.cn>
>>>>>
>>>>> I don't think this tag is appropriate here.
>>>>>
>>>>>> Signed-off-by: Marc Zyngier <maz@...nel.org>
>>>>>> Cc: Hanjun Guo <guohanjun@...wei.com>
>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
>>>>>> Signed-off-by: Jianmin Lv <lvjianmin@...ngson.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.
>>>
>>> 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@...nel.org>
>>> 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@...ngson.cn>
>>> Signed-off-by: Marc Zyngier <maz@...nel.org>
>>> ---
>>>    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.
> 

Ok, I'll add that to the patch, please check the change in next version.

> .	M.
> 

Powered by blists - more mailing lists