On Fri, May 23 2025 at 18:18, Tianyang Zhang wrote:
>
> -static void avecintc_sync(struct avecintc_data *adata)
> +void avecintc_sync(struct avecintc_data *adata)
> {
> struct pending_list *plist;
>
> @@ -109,7 +99,7 @@ static int avecintc_set_affinity(struct irq_data *data, const struct cpumask *de
> return -EBUSY;
>
> if (cpu_online(adata->cpu) && cpumask_test_cpu(adata->cpu, dest))
> - return 0;
> + return IRQ_SET_MASK_OK_DONE;
This change really wants to be seperate with a proper explanation and
not burried inside of this pile of changes.
> +static inline bool invalid_queue_is_full(int node, u32 *tail)
> +{
> + u32 head;
> +
> + head = read_queue_head(node);
Please move the initialization into the declaration line:
u32 head = read_queue...();
All over the place, where it's the first operation in the code. That
makes the code more dense and easier to follow.
> + *tail = read_queue_tail(node);
> +
> + return !!(head == ((*tail + 1) % INVALID_QUEUE_SIZE));
What's the !! for? A == B is a boolean expression already.
> +}
> +
> +static void invalid_enqueue(struct redirect_queue *rqueue, struct irde_inv_cmd *cmd)
> +{
> + struct irde_inv_cmd *inv_addr;
> + u32 tail;
> +
> + guard(raw_spinlock_irqsave)(&rqueue->lock);
> +
> + while (invalid_queue_is_full(rqueue->node, &tail))
> + cpu_relax();
> +
> + inv_addr = (struct irde_inv_cmd *)(rqueue->base + tail * sizeof(struct irde_inv_cmd));
> + memcpy(inv_addr, cmd, sizeof(struct irde_inv_cmd));
> + tail = (tail + 1) % INVALID_QUEUE_SIZE;
> +
> + /*
> + * The uncache-memory access may have an out of order problem cache-memory access,
> + * so a barrier is needed to ensure tail is valid
> + */
This comment does not make sense at all.
What's the actual uncached vs. cached access problem here? AFAICT it's
all about the ordering of the writes:
You need to ensure that the memcpy() data is visible _before_ the
tail is updated, no?
> + wmb();
> +
> + write_queue_tail(rqueue->node, tail);
> +}
> +static int redirect_table_free(struct redirect_item *item)
That return value is there to be ignored by the only caller, right?
> +{
> + struct redirect_table *ird_table;
> + struct redirect_entry *entry;
> +
> + ird_table = item->table;
> +
> + entry = item->entry;
> + memset(entry, 0, sizeof(struct redirect_entry));
> +
> + scoped_guard(raw_spinlock_irqsave, &ird_table->lock)
> + bitmap_release_region(ird_table->bitmap, item->index, 0);
> +
> + kfree(item->gpid);
> +
> + irde_invlid_entry_node(item);
> +
> + return 0;
> +}
> +
> +static inline void redirect_domain_prepare_entry(struct redirect_item *item,
> + struct avecintc_data *adata)
Please align the argument in the second line properly:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#line-breaks
> +
> +static inline void redirect_ack_irq(struct irq_data *d)
> +{
> +}
> +
> +static inline void redirect_unmask_irq(struct irq_data *d)
> +{
> +}
> +
> +static inline void redirect_mask_irq(struct irq_data *d)
> +{
> +}
These want some explanation why they are empty.
> +
> +static struct irq_chip loongarch_redirect_chip = {
> + .name = "REDIRECT",
> + .irq_ack = redirect_ack_irq,
> + .irq_mask = redirect_mask_irq,
> + .irq_unmask = redirect_unmask_irq,
> + .irq_set_affinity = redirect_set_affinity,
> + .irq_compose_msi_msg = redirect_compose_msi_msg,
> +};
> +out_free_resources:
> + redirect_free_resources(domain, virq, nr_irqs);
> + irq_domain_free_irqs_common(domain, virq, nr_irqs);
> +
> + return -EINVAL;
-ENOMEM?
> +}
> +
> + bitmap = bitmap_zalloc(IRD_ENTRIES, GFP_KERNEL);
> + if (!bitmap) {
> + pr_err("Node [%d] redirect table bitmap alloc pages failed!\n", node);
> + return -ENOMEM;
Leaks pages.
> + }
> +
> + ird_table->bitmap = bitmap;
> + ird_table->nr_ird = IRD_ENTRIES;
> + ird_table->node = node;
> +
> + raw_spin_lock_init(&ird_table->lock);
> +
> + if (redirect_queue_init(node))
> + return -EINVAL;
Leaks pages and bitmap.
> +
> + iocsr_write64(CFG_DISABLE_IDLE, LOONGARCH_IOCSR_REDIRECT_CFG);
> + iocsr_write64(__pa(ird_table->table), LOONGARCH_IOCSR_REDIRECT_TBR);
> +
> + return 0;
> +}
> +#if defined(CONFIG_ACPI)
#ifdef CONFIG_ACPI
> +static int __init redirect_reg_base_init(void)
> +{
> + acpi_status status;
> + uint64_t addr = 0;
What's this initialization for?
> +int __init redirect_acpi_init(struct irq_domain *parent)
> +{
> + struct fwnode_handle *fwnode;
> + struct irq_domain *domain;
> + int ret;
> +
> + fwnode = irq_domain_alloc_named_fwnode("redirect");
> + if (!fwnode) {
> + pr_err("Unable to alloc redirect domain handle\n");
> + goto fail;
> + }
> +
> + domain = irq_domain_create_hierarchy(parent, 0, IRD_ENTRIES, fwnode,
> + &redirect_domain_ops, irde_descs);
Please align the arguments in the second line properly.
> + if (!domain) {
> + pr_err("Unable to alloc redirect domain\n");
> + goto out_free_fwnode;
> + }
> +
> + redirect_domain = domain;
> +
> + ret = redirect_table_init(0);
> + if (ret)
> + goto out_free_table;
> +
> + ret = acpi_cascade_irqdomain_init();
> + if (ret < 0) {
> + pr_err("Failed to cascade IRQ domain, ret=%d\n", ret);
> + goto out_free_table;
> + }
> +
> + cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_REDIRECT_STARTING,
> + "irqchip/loongarch/redirect:starting",
> + redirect_cpu_online, NULL);
Hmm.
> +static int redirect_cpu_online(unsigned int cpu)
> +{
> + int ret, node = cpu_to_node(cpu);
> +
> + if (cpu != cpumask_first(cpumask_of_node(node)))
> + return 0;
> +
> + ret = redirect_table_init(node);
> + if (ret) {
> + redirect_table_fini(node);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
So if you unplug all CPUs of a node and then replug the first CPU in the
node, then this invokes redirect_table_init() unconditionally, which
will unconditionally allocate pages and bitmap again ....
Thanks,
tglx
