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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux