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

[Tianyang Zhang]

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

> > +#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