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 V1 1/5] irqchip/loongson-eiointc: Fix returned value on
 parsing MADT
To:     loongson-kernel@lists.loongnix.cn
Cc:     Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        loongarch@lists.linux.dev,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Huacai Chen <chenhuacai@loongson.cn>
References: <20230324060854.29375-1-lvjianmin@loongson.cn>
 <20230324060854.29375-2-lvjianmin@loongson.cn> <87h6uagnzo.wl-maz@kernel.org>
From:   Jianmin Lv <lvjianmin@loongson.cn>
Message-ID: <cfc7fd9f-67cf-3140-0ec5-a240a45506b9@loongson.cn>
Date:   Mon, 27 Mar 2023 11:58:57 +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: <87h6uagnzo.wl-maz@kernel.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2023/3/24 下午11:36, Marc Zyngier wrote:
> On Fri, 24 Mar 2023 06:08:50 +0000,
> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>
>> In pch_pic_parse_madt(), a NULL parent pointer will be
>> returned from acpi_get_vec_parent() for second pch-pic domain
>> related to second bridge while calling eiointc_acpi_init() at
>> first time, where the parent of it has not been initialized
>> yet, and will be initialized during second time calling
>> eiointc_acpi_init(). So, it's reasonable to return zero so
>> that failure of acpi_table_parse_madt() will be avoided, or else
>> acpi_cascade_irqdomain_init() will return and initialization of
>> followed pch_msi domain will be skipped.
>>
>> Although it does not matter when pch_msi_parse_madt() returns
>> -EINVAL if no invalid parent is found, it's also reasonable to
>> return zero for that.
>>
>> Change-Id: I4d278534999ec3e5c8db6d40155ba2665d9de86f
>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>> ---
>>   drivers/irqchip/irq-loongson-eiointc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
>> index d15fd38c1756..62a632d73991 100644
>> --- a/drivers/irqchip/irq-loongson-eiointc.c
>> +++ b/drivers/irqchip/irq-loongson-eiointc.c
>> @@ -343,7 +343,7 @@ static int __init pch_pic_parse_madt(union acpi_subtable_headers *header,
>>   	if (parent)
>>   		return pch_pic_acpi_init(parent, pchpic_entry);
>>   
>> -	return -EINVAL;
>> +	return 0;
> 
> Why can't you detect this particular case instead of blindly
> suppressing the error?
> 

For dual-bridges scenario, parent handle for pch_pic IRQ domain will be 
set by acpi_set_vec_parent() during each eiointc initialization. And
the parent handle for one pch_pic will be set during *first* eiointc
entry parsing, and the parent handle for another pch_pic will be
set during *second* eiointc entry parsing. But two pch_pic entries
will be parsed during each eiointc entry parsing, so a NULL parent
pointer must be returned druing first eiointc initialization, which
is reasonable and should not be treated as an error.

The calling stack of pch_pic_parse_madt (where acpi_get_vec_parent is 
called to get parent handle) is like this:

cpuintc_acpi_init
   acpi_cascade_irqdomain_init(in cpuintc driver)
     acpi_table_parse_madt
       eiointc_parse_madt
         eiointc_acpi_init /* this will be called two times 
corresponding  to parsing two eiointc entries in MADT under dual-bridges 
scenario*/
           acpi_set_vec_parent /* set parent handle for one pch_pic 
during first eiointc entry parsing, and set parent handle for another 
pch_pic during second eiointc entry parsing*/
             acpi_cascade_irqdomain_init(in eiointc driver)
               acpi_table_parse_madt
                 pch_pic_parse_madt /* this will be called twice because 
of two pchpic entries, and only one valid parent handle will be returned 
from acpi_get_vec_parent() during first eiointc entry parsing (another 
parent handle is not set yet), so a NULL parent pointer must be returned 
during first eiointc entry parsing, which is reasonable and should not 
be treated as an error.*/

Thanks!

> 	M.
>