This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: Use GIC functions to allocate global EQs
This commit replaces the GDMA global interrupt setup code with the new GIC
allocation and release functions to manage interrupt contexts via refcounting.
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 81bf379283663..72a67b0b2b5d4 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1918,30 +1918,13 @@ static int mana_gd_setup_dyn_irqs(struct pci_dev
> *pdev, int nvec)
> * further used in irq_setup()
> */
> for (i = 1; i <= nvec; i++) {
> - gic = kzalloc_obj(*gic);
> + gic = mana_gd_get_gic(gc, false, &i);
> if (!gic) {
> err = -ENOMEM;
> goto free_irq;
> }
> - gic->handler = mana_gd_process_eq_events;
> - INIT_LIST_HEAD(&gic->eq_list);
> - spin_lock_init(&gic->lock);
> -
> - snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_q%d@pci:%s",
> - i - 1, pci_name(pdev));
Does moving the name generation into mana_gd_get_gic() unintentionally change
the IRQ names exposed in /proc/interrupts?
In mana_gd_get_gic(), the names are now generated as "mana_msi%d" using the
raw MSI index:
if (!gic->msi)
snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_hwc@pci:%s",
pci_name(dev));
else
snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_msi%d@pci:%s",
gic->msi, pci_name(dev));
This could break userspace tooling (like irqbalance or monitoring scripts) that
might rely on parsing the "mana_q*" pattern to bind network queues.
[ ... ]
> @@ -1963,19 +1946,11 @@ static int mana_gd_setup_dyn_irqs(struct pci_dev
> *pdev, int nvec)
> kfree(irqs);
> return 0;
>
> -free_current_gic:
> - kfree(gic);
> free_irq:
> for (i -= 1; i > 0; i--) {
> irq = pci_irq_vector(pdev, i);
> - gic = xa_load(&gc->irq_contexts, i);
> - if (WARN_ON(!gic))
> - continue;
> -
> irq_update_affinity_hint(irq, NULL);
> - free_irq(irq, gic);
> - xa_erase(&gc->irq_contexts, i);
> - kfree(gic);
> + mana_gd_put_gic(gc, false, i);
> }
> kfree(irqs);
> return err;
[ ... ]
> @@ -2139,26 +2085,20 @@ static int mana_gd_setup_remaining_irqs(struct
> pci_dev *pdev)
> static void mana_gd_remove_irqs(struct pci_dev *pdev)
> {
> struct gdma_context *gc = pci_get_drvdata(pdev);
> - struct gdma_irq_context *gic;
> int irq, i;
>
> if (gc->max_num_msix < 1)
> return;
>
> for (i = 0; i < gc->max_num_msix; i++) {
> - irq = pci_irq_vector(pdev, i);
> - if (irq < 0)
> - continue;
> -
> - gic = xa_load(&gc->irq_contexts, i);
> - if (WARN_ON(!gic))
> + if (!xa_load(&gc->irq_contexts, i))
> continue;
>
> /* Need to clear the hint before free_irq */
> + irq = pci_irq_vector(pdev, i);
> irq_update_affinity_hint(irq, NULL);
> - free_irq(irq, gic);
> - xa_erase(&gc->irq_contexts, i);
> - kfree(gic);
> +
> + mana_gd_put_gic(gc, false, i);
> }
Is it safe to unconditionally call irq_update_affinity_hint() in these cleanup
paths?
Now that IRQs can be shared and are reference counted via mana_gd_put_gic(),
unconditionally clearing the affinity hint here could disrupt the routing for
any remaining active users sharing this IRQ. mana_gd_put_gic() already clears
the affinity hint internally when the refcount reaches zero.
Also, since the "if (irq < 0)" check was removed in mana_gd_remove_irqs(), if
pci_irq_vector() returns a negative error like -EINVAL, we would pass a
negative value to irq_update_affinity_hint(), which expects an unsigned int.
