On Fri, Jul 26 2024 at 18:24, Tianyang Zhang wrote:
> +static int avecintc_set_affinity(struct irq_data *data, const struct cpumask *dest,
> + bool force)
Please align arguments according to documentation. In this case don't
use a line break. Just use the full 100 character width all over the place
> +{
> + unsigned int cpu, ret, vector;
> + unsigned long flags;
> + struct avecintc_data *adata;
Please look for variable declarations in:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
and fix it up all over the place.
> +
> + raw_spin_lock_irqsave(&loongarch_avec.lock, flags);
This does not need irqsave because the function is always called with
interrupts disabled.
> + adata = irq_data_get_irq_chip_data(data);
> +
> + if (adata->vec && cpu_online(adata->cpu) && cpumask_test_cpu(adata->cpu, dest)) {
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> + return 0;
> + }
> +
> + if (adata->moving)
> + return -EBUSY;
This leaks loongarch_avec.lock. Please use
scoped_guard(raw_spin_lock)(&loongarch_avec.lock);
above so you spare all the unlocks and cannot miss one.
> + cpumask_and(&intersect_mask, dest, cpu_online_mask);
> +
> + ret = irq_matrix_alloc(loongarch_avec.vector_matrix, &intersect_mask, false, &cpu);
> + if (ret < 0) {
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> + return ret;
> + }
> +
> + vector = ret;
> + adata->cpu = cpu;
> + adata->vec = vector;
> + per_cpu_ptr(irq_map, adata->cpu)[adata->vec] = irq_data_to_desc(data);
> + avecintc_sync(adata);
> +
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> + irq_data_update_effective_affinity(data, cpumask_of(cpu));
> +
> + return IRQ_SET_MASK_OK;
> +}
> +
> +static int avecintc_cpu_online(unsigned int cpu)
> +{
> + unsigned long flags;
> +
> + if (!loongarch_avec.vector_matrix)
> + return 0;
> +
> + raw_spin_lock_irqsave(&loongarch_avec.lock, flags);
guard(raw_spin_lock)
interrupts are disabled.
> + irq_matrix_online(loongarch_avec.vector_matrix);
> +
> + pending_list_init(cpu);
> +
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> +
> + return 0;
> +}
> +
> +static int avecintc_cpu_offline(unsigned int cpu)
> +{
> + unsigned long flags;
> + struct pending_list *plist = per_cpu_ptr(&pending_list, cpu);
> +
> + if (!loongarch_avec.vector_matrix)
> + return 0;
> +
> + raw_spin_lock_irqsave(&loongarch_avec.lock, flags);
Ditto.
> + if (list_empty(&plist->head))
> + irq_matrix_offline(loongarch_avec.vector_matrix);
> + else
> + pr_warn("CPU#%d advanced vector is busy\n", cpu);
Seriously? You leave the matrix online so allocation can be made from an
offline CPU?
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> +
> + return 0;
> +}
> +
> +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;
> +
> + raw_spin_lock(&loongarch_avec.lock);
guard()
> + 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);
> + }
> +
> + if (isr & (1UL << (vector % VECTORS_PER_REG))) {
> + mp_ops.send_ipi_single(cpu, ACTION_CLEAR_VECTOR);
Is it guaranteed that the device vector is handled _before_ the next
IPI is handled? If not this is a live lock.
> + continue;
> + }
> + list_del(&adata->entry);
> + irq_matrix_free(loongarch_avec.vector_matrix, cpu, vector, adata->managed);
> + this_cpu_write(irq_map[vector], NULL);
> + adata->prev_cpu = adata->cpu;
> + adata->prev_vec = adata->vec;
> + adata->moving = 0;
> + }
> +
> + raw_spin_unlock(&loongarch_avec.lock);
> +}
> +#endif
> +
> +static void avecintc_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> +{
> + struct avecintc_data *adata;
> +
> + adata = irq_data_get_irq_chip_data(d);
Move the assignement up to the declaration.
> + 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;
> +
> + chained_irq_enter(chip, desc);
> +
> + while (true) {
> + 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 {
See bracket rules.
> + 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;
> + unsigned long flags;
> + struct irq_data *irqd;
> + struct avecintc_data *adata;
> +
> + raw_spin_lock_irqsave(&loongarch_avec.lock, flags);
guard(raw_spinlock_irqsave)(....);
> + for (i = 0; i < nr_irqs; i++) {
> + irqd = irq_domain_get_irq_data(domain, virq + i);
> + adata = kzalloc(sizeof(*adata), GFP_KERNEL);
> + if (!adata) {
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> + return -ENOMEM;
> + }
> +
> + ret = irq_matrix_alloc(loongarch_avec.vector_matrix, cpu_online_mask, false, &cpu);
> + if (ret < 0) {
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> + return ret;
> + }
> +
> + 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);
> + adata->moving = 0;
Initialize first before doing enything else.
> + 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);
> + }
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> +
> + 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);
> +
> + per_cpu(irq_map, adata->cpu)[adata->vec] = NULL;
> + irq_matrix_free(loongarch_avec.vector_matrix, adata->cpu, adata->vec, managed);
> + adata->cpu = 0;
0 is a valid CPU number, no?
> + adata->vec = 0;
> +
> +#ifdef CONFIG_SMP
> + if (!adata->moving)
> + return;
> +
> + per_cpu(irq_map, adata->prev_cpu)[adata->prev_vec] = NULL;
> + irq_matrix_free(loongarch_avec.vector_matrix,
> + adata->prev_cpu, adata->prev_vec, adata->managed);
> + adata->moving = 0;
> + adata->prev_vec = 0;
> + adata->prev_cpu = 0;
> + list_del_init(&adata->entry);
> +#endif
> +}
> +
> +static void avecintc_domain_free(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> +{
> + unsigned int i;
> + unsigned long flags;
> + struct irq_data *d;
> +
> + raw_spin_lock_irqsave(&loongarch_avec.lock, flags);
guard()
> + 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);
> +
> + }
> + }
> +
> + raw_spin_unlock_irqrestore(&loongarch_avec.lock, flags);
> +}
> +
> +static const struct irq_domain_ops avecintc_domain_ops = {
> + .alloc = avecintc_domain_alloc,
> + .free = avecintc_domain_free,
> +};
> +
> +static int __init irq_matrix_init(void)
> +{
> + int i;
> +
> + loongarch_avec.vector_matrix = irq_alloc_matrix(NR_VECTORS, 0, NR_VECTORS - 1);
> + if (!loongarch_avec.vector_matrix)
> + return -ENOMEM;
> +
> + for (i = 0; i < NR_LEGACY_VECTORS; i++)
> + irq_matrix_assign_system(loongarch_avec.vector_matrix, i, false);
> +
> + irq_matrix_online(loongarch_avec.vector_matrix);
> +
> + return 0;
> +}
> +
> +static int __init avecintc_init(struct irq_domain *parent)
> +{
> + int ret, parent_irq;
> + unsigned long value;
> +
> + raw_spin_lock_init(&loongarch_avec.lock);
> +
> + loongarch_avec.fwnode = irq_domain_alloc_named_fwnode("AVECINTC");
> + if (!loongarch_avec.fwnode) {
> + pr_err("Unable to allocate domain handle\n");
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + loongarch_avec.domain = irq_domain_create_tree(loongarch_avec.fwnode,
> + &avecintc_domain_ops, NULL);
> + if (!loongarch_avec.domain) {
> + pr_err("Unable to create IRQ domain\n");
> + ret = -ENOMEM;
> + goto out_free_handle;
> + }
> +
> + 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_free_matrix;
> + }
> +#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);
> +#endif
> + value = iocsr_read64(LOONGARCH_IOCSR_MISC_FUNC);
> + value |= IOCSR_MISC_FUNC_AVEC_EN;
> + iocsr_write64(value, LOONGARCH_IOCSR_MISC_FUNC);
> +
> + return ret;
> +
> +out_free_matrix:
> + kfree(loongarch_avec.vector_matrix);
The code above just failed to allocate the matrix, so why are you trying
to free it?
And this happily fails to uninstall the chained handler.
> +out_remove_domain:
> + irq_domain_remove(loongarch_avec.domain);
> +out_free_handle:
> + irq_domain_free_fwnode(loongarch_avec.fwnode);
> +out:
> + return ret;
> +}
> +
> +static struct irq_chip pch_msi_irq_chip_v2 = {
> + .name = "PCH PCI MSI",
> + .irq_ack = irq_chip_ack_parent,
> +};
> +
> +static struct msi_domain_info pch_msi_domain_info_v2 = {
> + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> + MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX,
> + .chip = &pch_msi_irq_chip_v2,
> +};
> +
> +int __init pch_msi_acpi_init_v2(struct irq_domain *parent,
> + struct acpi_madt_msi_pic *acpi_pchmsi)
> +{
> + struct irq_domain *msi_domain;
> +
> + if (pch_msi_handle[0])
> + return 0;
> +
> + pch_msi_handle[0] = irq_domain_alloc_fwnode(&acpi_pchmsi->msg_address);
> + if (!pch_msi_handle[0]) {
> + pr_err("Unable to allocate domain handle\n");
> + return -ENOMEM;
> + }
> +
> + msi_domain = pci_msi_create_irq_domain(pch_msi_handle[0],
> + &pch_msi_domain_info_v2, parent);
Please don't do that. Convert this to use per device MSI domains.
Thanks,
tglx
