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

[Jianmin Lv <lvjianmin@loongson.cn>]

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