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

[Tianyang Zhang]

Hi, Huacai

在 2024/8/8 下午4:03, Huacai Chen 写道:
> Hi, Tianyang,
>
> On Thu, Aug 8, 2024 at 2:52 PM Tianyang Zhang <zhangtianyang@xxxxxxxxxxx> 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~~
> > > > +#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