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

[Thomas Gleixner <tglx@linutronix.de>]

Tianyang!

On Tue, Aug 06 2024 at 19:55, Tianyang Zhang wrote:
> +int __init arch_probe_nr_irqs(void)
> +{
> +	int nr_io_pics = bitmap_weight(loongson_sysconf.cores_io_master, NR_CPUS);
> +
> +	if (!cpu_has_avecint)
> +		nr_irqs = (64 + NR_VECTORS * nr_io_pics);
> +	else
> +		nr_irqs = (64 + NR_VECTORS * (nr_cpu_ids + nr_io_pics));
> +
> +	return NR_IRQS_LEGACY;
> +}
> +
>  void __init init_IRQ(void)
>  {
>  	int i;
> @@ -102,7 +114,7 @@ void __init init_IRQ(void)
>  	mp_ops.init_ipi();
>  #endif
>  
> -	for (i = 0; i < NR_IRQS; i++)
> +	for (i = 0; i < nr_irqs; i++)
>  		irq_set_noprobe(i);

This is wrong because at that point only the legacy interrupts are
allocated. Nothing will set the NOPROBE flag on any interrupt which is
allocated afterwards.

#define ARCH_IRQ_INIT_FLAGS   IRQ_NOPROBE

in asm/hwirq.h and the core will automatically set the flag for all
interrupts and you can remove this gunk completely.

> +static int avecintc_set_affinity(struct irq_data *data, const struct cpumask *dest, bool force)
> +{
> +	unsigned int cpu, ret, vector;
> +	struct avecintc_data *adata;
> +
> +	scoped_guard(raw_spinlock, &loongarch_avec.lock) {
> +		adata = irq_data_get_irq_chip_data(data);
> +
> +		if (adata->vec && cpu_online(adata->cpu) && cpumask_test_cpu(adata->cpu, dest))
> +			return 0;

For invalid vectors you set adata->vec to UINT_MAX, so how is the
adata->vec check here useful? 0 is a valid vector if I'm reading the
code correctly as it's one of the legacy vectors.

> +void complete_irq_moving(void)
> +{
> +	struct pending_list *plist = this_cpu_ptr(&pending_list);
> +	struct avecintc_data *adata, *tdata;
> +	int cpu, vector, bias;
> +	uint64_t isr;
> +
> +	guard(raw_spinlock)(&loongarch_avec.lock);
> +
> +	list_for_each_entry_safe(adata, tdata, &plist->head, entry) {
> +		cpu = adata->prev_cpu;
> +		vector = adata->prev_vec;
> +		bias = vector / VECTORS_PER_REG;
> +		switch (bias) {
> +		case 0:
> +			isr = csr_read64(LOONGARCH_CSR_ISR0);
> +		case 1:
> +			isr = csr_read64(LOONGARCH_CSR_ISR1);
> +		case 2:
> +			isr = csr_read64(LOONGARCH_CSR_ISR2);
> +		case 3:
> +			isr = csr_read64(LOONGARCH_CSR_ISR3);
> +		}

This switch case is broken. It will always read from LOONGARCH_CSR_ISR3,
unless you add a few 'break;' statements, no?

> +		if (isr & (1UL << (vector % VECTORS_PER_REG))) {
> +			mp_ops.send_ipi_single(cpu, ACTION_CLEAR_VECTOR);
> +			continue;
> +		}
> +		list_del(&adata->entry);
> +		irq_matrix_free(loongarch_avec.vector_matrix, cpu, vector, adata->managed);
> +		this_cpu_write(irq_map[vector], NULL);
> +		adata->moving = 0;
> +		adata->prev_cpu = adata->cpu;
> +		adata->prev_vec = adata->vec;
> +	}
> +}
> +#endif
> +
> +static void avecintc_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> +{
> +	struct avecintc_data *adata = irq_data_get_irq_chip_data(d);
> +
> +	msg->address_hi = 0x0;
> +	msg->address_lo = (msi_base_addr | (adata->vec & 0xff) << 4) |
> +			  ((cpu_logical_map(adata->cpu & 0xffff)) << 12);
> +	msg->data = 0x0;
> +}
> +
> +static struct irq_chip avec_irq_controller = {
> +	.name			= "AVECINTC",
> +	.irq_ack		= avecintc_ack_irq,
> +	.irq_mask		= avecintc_mask_irq,
> +	.irq_unmask		= avecintc_unmask_irq,
> +#ifdef CONFIG_SMP
> +	.irq_set_affinity	= avecintc_set_affinity,
> +#endif
> +	.irq_compose_msi_msg	= avecintc_compose_msi_msg,
> +};
> +
> +static void avecintc_irq_dispatch(struct irq_desc *desc)
> +{
> +	unsigned long vector;
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct irq_desc *d;

This still uses random order of variable declarations. See
Documentation...

> +	chained_irq_enter(chip, desc);
> +
> +	while (true) {

        for (;;) {

> +		vector = csr_read64(LOONGARCH_CSR_IRR);

Please move the vector declaration into the scope where it's used

		unsigned long vector = csr_read64(LOONGARCH_CSR_IRR);
...

> +		if (vector & IRR_INVALID_MASK)
> +			break;
> +
> +		vector &= IRR_VECTOR_MASK;
> +
> +		d = this_cpu_read(irq_map[vector]);
> +		if (d) {
> +			generic_handle_irq_desc(d);
> +		} else {
> +			spurious_interrupt();
> +			pr_warn("Unexpected IRQ occurs on CPU#%d [vector %ld]\n", smp_processor_id(), vector);
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int avecintc_domain_alloc(struct irq_domain *domain,
> +				 unsigned int virq, unsigned int nr_irqs, void *arg)
> +{
> +	unsigned int cpu, i, ret;
> +	struct irq_data *irqd;
> +	struct avecintc_data *adata;

Ditto. And move all variables except i into the for loop and 

> +	guard(raw_spinlock_irqsave)(&loongarch_avec.lock);
> +
> +	for (i = 0; i < nr_irqs; i++) {

make that

	for (unsigned int i = 0; ....) {

> +		irqd = irq_domain_get_irq_data(domain, virq + i);
> +		adata = kzalloc(sizeof(*adata), GFP_KERNEL);
> +		if (!adata)
> +			return -ENOMEM;
> +
> +		ret = irq_matrix_alloc(loongarch_avec.vector_matrix, cpu_online_mask, false, &cpu);
> +		if (ret < 0)
> +			return ret;
> +
> +		adata->moving = 0;
> +		adata->prev_cpu = adata->cpu = cpu;
> +		adata->prev_vec = adata->vec = ret;
> +		adata->managed = irqd_affinity_is_managed(irqd);
> +		irq_domain_set_info(domain, virq + i, virq + i, &avec_irq_controller,
> +				    adata, handle_edge_irq, NULL, NULL);
> +		irqd_set_single_target(irqd);
> +		irqd_set_affinity_on_activate(irqd);
> +
> +		per_cpu_ptr(irq_map, adata->cpu)[adata->vec] = irq_data_to_desc(irqd);
> +	}
> +
> +	return 0;
> +}
> +
> +static void clear_free_vector(struct irq_data *irqd)
> +{
> +	bool managed = irqd_affinity_is_managed(irqd);
> +	struct avecintc_data *adata = irq_data_get_irq_chip_data(irqd);

Sigh.

> +	per_cpu(irq_map, adata->cpu)[adata->vec] = NULL;
> +	irq_matrix_free(loongarch_avec.vector_matrix, adata->cpu, adata->vec, managed);

> +static void avecintc_domain_free(struct irq_domain *domain,
> +				 unsigned int virq, unsigned int nr_irqs)
> +{
> +	unsigned int i;
> +	struct irq_data *d;

See comments above...

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

> +		}
> +	}
> +}
> +
> +static const struct irq_domain_ops avecintc_domain_ops = {
> +	.alloc		= avecintc_domain_alloc,
> +	.free		= avecintc_domain_free,
> +	.select		= msi_lib_irq_domain_select,
> +};
> +
> +static int __init irq_matrix_init(void)
> +{
> +	int i;

...

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

> +	}
> +#ifdef CONFIG_SMP
> +	pending_list_init(0);
> +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_AVECINTC_STARTING,
> +					"irqchip/loongarch/avecintc:starting",
> +					avecintc_cpu_online, avecintc_cpu_offline);

And if that would fail, you happily continue despite knowing that SMP
won't work.

But as this can't fail because of '_nocalls' and the decicated state,
the assignment to ret is pointless.

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

> +int __init avecintc_acpi_init(struct irq_domain *parent)
> +{
> +	int ret;
> +
> +	ret = avecintc_init(parent);

        int ret = ....

> +	if (ret < 0) {
> +		pr_err("Failed to init IRQ domain\n");
> +		return ret;
> +	}
> +
> diff --git a/drivers/irqchip/irq-loongson-pch-msi.c b/drivers/irqchip/irq-loongson-pch-msi.c
> index 2242f63c66fc..ec0b3dba238f 100644
> --- a/drivers/irqchip/irq-loongson-pch-msi.c
> +++ b/drivers/irqchip/irq-loongson-pch-msi.c
> @@ -256,11 +256,14 @@ struct fwnode_handle *get_pch_msi_handle(int pci_segment)
>  {
>  	int i;

Please move this into the for() while at it.

> +	if (cpu_has_avecint)
> +		return pch_msi_handle[0];
> +
>  	for (i = 0; i < MAX_IO_PICS; i++) {
>  		if (msi_group[i].pci_segment == pci_segment)
>  			return pch_msi_handle[i];
>  	}
> -	return NULL;
> +	return pch_msi_handle[0];
>  }
>  
>  int __init pch_msi_acpi_init(struct irq_domain *parent,
> @@ -277,4 +280,19 @@ int __init pch_msi_acpi_init(struct irq_domain *parent,
>  
>  	return ret;
>  }
> +
> +int __init pch_msi_acpi_init_v2(struct irq_domain *parent,
> +					struct acpi_madt_msi_pic *acpi_pchmsi)

Is it really so hard to use the 100 character width or to align the
second line properly as documented?

Thanks,

        tglx