Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains
To:     Hanjun Guo <guohanjun@huawei.com>, Marc Zyngier <maz@kernel.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Huacai Chen <chenhuacai@loongson.cn>
References: <1655273250-23495-1-git-send-email-lvjianmin@loongson.cn>
 <1655273250-23495-2-git-send-email-lvjianmin@loongson.cn>
 <87k09ipfe2.wl-maz@kernel.org>
 <0247b7d5-aca9-5db1-e712-4783ee672110@loongson.cn>
 <87fsk2p8b5.wl-maz@kernel.org>
 <ae3e0bc7-67c3-8f05-d3ba-98cab9b75117@huawei.com>
From:   Jianmin Lv <lvjianmin@loongson.cn>
Message-ID: <80e06104-718f-01b5-91ce-a51c7151dde8@loongson.cn>
Date:   Tue, 28 Jun 2022 16:45:05 +0800
User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <ae3e0bc7-67c3-8f05-d3ba-98cab9b75117@huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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