Re: [PATCH net-next v2 2/3] dinghai: add MSI-X interrupt pools
flat view
From: Simon Horman <horms@kernel.org>
Date: 2026-09-14 07:20:52
Also in:
lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev/#/patchset/20260911115929435mXJzcTF08xlUAdjnT8qqm@zte.com.cn?part=2 --- dinghai: add MSI-X interrupt pools This commit introduces a driver for ZTE DingHai Ethernet, specifically allocating the fixed MSI-X vector layout of the device. It manages the vectors in per-purpose pools for async event queues and vq queue pairs. [Severity: Medium] The commit message states: manage the vectors in per-purpose pools: one pool for the async event queues and one for the vq queue pairs, with a range reserved for RDMA in between. Was the VQ pool allocation omitted from this patch by mistake? Looking at zxdh_pf_irq_pools_init(), only the async pool is allocated.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/zte/dinghai/zxdh_irq.c b/drivers/net/ethernet/zte/dinghai/zxdh_irq.c new file mode 100644 index 0000000000000..437344329c7e8 --- /dev/null +++ b/drivers/net/ethernet/zte/dinghai/zxdh_irq.c
[ ... ]
quoted hunk ↗ jump to hunk
+static void zxdh_irq_release(struct kref *kref) +{ + struct zxdh_irq *irq = container_of(kref, struct zxdh_irq, refcount); + struct zxdh_irq_pool *pool = irq->pool; + + lockdep_assert_held(&pool->lock); + xa_erase(&pool->irqs, irq->index); + /* free_irq() requires the affinity hint to be cleared before it is + * called; the asymmetry with the set path in zxdh_irq_alloc() is + * intentional. + */ + irq_update_affinity_hint(irq->irqn, NULL); + free_cpumask_var(irq->mask); + free_irq(irq->irqn, irq); + kfree(irq); +}
[Severity: High] Does this code leak the logical counters in pool->irqs_per_cpu? When an IRQ is allocated, zxdh_irq_pool_request_irq() increments pool->irqs_per_cpu[cpu]. It seems zxdh_irq_release() misses a call to zxdh_cpu_put(), which will permanently skew load balancing and trigger the WARN_ON(pool->irqs_per_cpu[cpu]) during zxdh_irq_pool_free(). [ ... ]
quoted hunk ↗ jump to hunk
+static struct zxdh_irq *zxdh_irq_alloc(struct zxdh_irq_pool *pool, int vecidx, + const struct cpumask *affinity) +{ + struct zxdh_core_dev *zxdh_dev = pool->dev; + struct zxdh_irq *irq; + int err; + int cpu; + + irq = kzalloc_obj(*irq, GFP_KERNEL); + if (!irq) + return ERR_PTR(-ENOMEM); + + irq->pool = pool; + irq->irqn = pci_irq_vector(zxdh_dev->pdev, vecidx); + if (irq->irqn < 0) { + err = irq->irqn; + goto err_irqn; + } + + ATOMIC_INIT_NOTIFIER_HEAD(&irq->nh); + snprintf(irq->name, ZXDH_MAX_IRQ_NAME, "async_%d@pci:%s", vecidx, + pci_name(zxdh_dev->pdev));
[Severity: Low] Is this hardcoded name prefix intentional? This ignores the configured pool->name. If this generic library is reused for other pools, such as the VQ pool, they will all show up as "async_" in /proc/interrupts.
quoted hunk ↗ jump to hunk
+ + err = request_irq(irq->irqn, zxdh_irq_int_handler, 0, irq->name, irq); + if (err) { + dev_err(zxdh_dev->device, "request_irq failed: %d\n", err); + goto err_irqn; + } + + if (!zalloc_cpumask_var(&irq->mask, GFP_KERNEL)) { + dev_err(zxdh_dev->device, "zalloc_cpumask_var failed\n"); + err = -ENOMEM; + goto err_cpumask; + } + + if (affinity) { + cpumask_copy(irq->mask, affinity); + } else { + /* No preference requested; spread over all online CPUs. */ + for_each_online_cpu(cpu) + cpumask_set_cpu(cpu, irq->mask); + } + irq_update_affinity_hint(irq->irqn, irq->mask);
[Severity: Medium] Will this actually configure the SMP hardware routing for the IRQ? The driver uses irq_update_affinity_hint() which only sets a userspace hint via procfs. Should this be irq_set_affinity_and_hint() instead to actually enforce the intended hardware IRQ load balancing? [ ... ]
quoted hunk ↗ jump to hunk
+static struct zxdh_irq *zxdh_irq_pool_request_irq(struct zxdh_irq_pool *pool, + const struct cpumask *req_mask) +{ + const struct cpumask *affinity = req_mask; + cpumask_var_t auto_mask; + struct zxdh_irq *irq; + u32 irq_index; + int err; + + if (!zalloc_cpumask_var(&auto_mask, GFP_KERNEL)) + return ERR_PTR(-ENOMEM); + + err = xa_alloc(&pool->irqs, &irq_index, NULL, pool->xa_num_irqs, + GFP_KERNEL);
[Severity: High] Will this xa_alloc() call fail unconditionally? The XArray API strictly forbids NULL entries for allocation because NULL represents an unallocated slot. Passing NULL as the entry triggers a WARN_ON_ONCE(!entry) and returns -EINVAL. [ ... ]
quoted hunk ↗ jump to hunk
+void zxdh_irq_pool_free(struct zxdh_irq_pool *pool) +{ + struct zxdh_irq *irq; + unsigned long index; + u32 cpu; + + /* On a fast teardown the table may still hold IRQs; release + * whatever is left. + */ + mutex_lock(&pool->lock); + xa_for_each(&pool->irqs, index, irq) + zxdh_irq_release(&irq->refcount); + mutex_unlock(&pool->lock);
[Severity: High] Can this manual iteration bypass the reference counting and cause a use-after-free? If zxdh_irq_pool_free() executes while active consumers still hold an IRQ reference during a fast teardown, it will forcefully execute zxdh_irq_release() on the irq object. When the consumer eventually calls zxdh_irq_put() and accesses the freed irq and its freed pool->lock, will this trigger a use-after-free? [ ... ]