linux-kernel - Re: [PATCH V8 2/2] irqchip/loongarch-avec: Add AVEC irqchip support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <121628a2-9cc6-a46c-d58e-cacc31d0a240@loongson.cn>
Date: Fri, 9 Aug 2024 17:14:11 +0800
From: Tianyang Zhang <zhangtianyang@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, corbet@....net, alexs@...nel.org,
 kernel@...0n.name, jiaxun.yang@...goat.com, gaoliang@...ngson.cn,
 wangliupu@...ngson.cn, lvjianmin@...ngson.cn, yijun@...ngson.cn,
 mhocko@...e.com, akpm@...ux-foundation.org, dianders@...omium.org,
 maobibo@...ngson.cn, xry111@...111.site, zhaotianrui@...ngson.cn,
 nathan@...nel.org, yangtiezhu@...ngson.cn, zhoubinbin@...ngson.cn,
 loongarch@...ts.linux.dev, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH V8 2/2] irqchip/loongarch-avec: Add AVEC irqchip support

Hi, Huacai

在 2024/8/9 下午5:09, Huacai Chen 写道:
> On Fri, Aug 9, 2024 at 9:50 AM Tianyang Zhang <zhangtianyang@...ngson.cn> wrote:
>> Hi, Huacai
>>
>> 在 2024/8/8 下午4:03, Huacai Chen 写道:
>>> Hi, Tianyang,
>>>
>>> On Thu, Aug 8, 2024 at 2:52 PM Tianyang Zhang <zhangtianyang@...ngson.cn> wrote:
>>>> Hi, Thomas
>>>>
>>>> Thank you for your feedback.
>>>>
>>>> 在 2024/8/8 上午6:01, Thomas Gleixner 写道:
>>>>
>>>>>> +    guard(raw_spinlock_irqsave)(&loongarch_avec.lock);
>>>>>> +
>>>>>> +    for (i = 0; i < nr_irqs; i++) {
>>>>>> +            d = irq_domain_get_irq_data(domain, virq + i);
>>>>>> +            if (d) {
>>>>>> +                    clear_free_vector(d);
>>>>>> +                    irq_domain_reset_irq_data(d);
>>>>>> +
>>>>> Stray newline, but the more important question is what kfree()'s 'd'?
>>>>>
>>>>> AFAICT, nothing. So that's a memory leak, no?
>>>> With my understand , 'd' as 'struct irq_data' can be free at public
>>>> irqdomain process, and really miss a kfree targeting 'struct chip_data'
>>>>>> +static int __init avecintc_init(struct irq_domain *parent)
>>>>>> +{
>>>>>> +    parent_irq = irq_create_mapping(parent, INT_AVEC);
>>>>>> +    if (!parent_irq) {
>>>>>> +            pr_err("Failed to mapping hwirq\n");
>>>>>> +            ret = -EINVAL;
>>>>>> +            goto out_remove_domain;
>>>>>> +    }
>>>>>> +    irq_set_chained_handler_and_data(parent_irq, avecintc_irq_dispatch, NULL);
>>>>>> +
>>>>>> +    ret = irq_matrix_init();
>>>>>> +    if (ret < 0) {
>>>>>> +            pr_err("Failed to init irq matrix\n");
>>>>>> +            goto out_remove_domain;
>>>>> Which still leaves the disfunct chained handler installed and the
>>>>> mapping intact.
>>>> There is indeed a problem here, but we have not found a similar approach
>>>> for reference.
>>>>
>>>> Is it reasonable to replace here with handle_bad_irq in case of failure?
>>>> or is there any other more suitable way. We hope you can give us some
>>>> suggestions, thank you very much
>>> Maybe we can move irq_set_chained_handler_and_data(parent_irq,
>>> avecintc_irq_dispatch, NULL) after the checking of irq_matrix_init().
>>>
>>> Huacai
>> I think is a good idea~~
> Again, you can download patches from here as V9:
> https://github.com/chenhuacai/linux/commits/loongarch-next
>
>
> Huacai
Ok, I will test it as soon as my possible
>
>>>>>> +#endif
>>>>>> +    value = iocsr_read64(LOONGARCH_IOCSR_MISC_FUNC);
>>>>>> +    value |= IOCSR_MISC_FUNC_AVEC_EN;
>>>>>> +    iocsr_write64(value, LOONGARCH_IOCSR_MISC_FUNC);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +
>>>>>> +out_remove_domain:
>>>>>> +    irq_domain_remove(loongarch_avec.domain);
>>>>>> +out_free_handle:
>>>>>> +    irq_domain_free_fwnode(loongarch_avec.fwnode);
>>>>>> +out:
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int __init pch_msi_parse_madt(union acpi_subtable_headers *header,
>>>>>> +                                 const unsigned long end)
>>>>>> +{
>>>>>> +    struct acpi_madt_msi_pic *pchmsi_entry = (struct acpi_madt_msi_pic *)header;
>>>>>> +
>>>>>> +    msi_base_addr = pchmsi_entry->msg_address - AVEC_MSG_OFFSET;
>>>>> What validates that msi_base_addr has none of the lower 16 bits set, as
>>>>> they are required to be zero to make MSI message composing work, right?
>>>> This operation originates from some hardware designs.
>>>>
>>>> In 3C6000, either eiointc or avecintc can be the parent controller for
>>>> MSI interrupts and these two controllers have different MSI msg address.
>>>>
>>>> In our platform design scheme, we fix avec-msg-address to the address of
>>>> (eiointc-msg-address - 0x100000). Therefore, here we need to subtract
>>>> AVEC_MSG_OFFSET from the msg_address obtained by MCFG
>>>>
>>>> The main purpose of the design that users of 3C6000 can freely choose
>>>> the version of the Linux kernel that supports loongarch (regardless of
>>>> whether AVEC is supported or not) without having to change the firmware
>>>>
>>>>
>>>> Thanks again
>>>>
>>>> Tianyang
>>>>
>>