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 patch replaces the inline GDMA global interrupt setup in
mana_gd_setup_dyn_irqs(), mana_gd_setup_irqs(), and mana_gd_remove_irqs()
with calls to the new mana_gd_get_gic()/mana_gd_put_gic() helpers
introduced in the preceding patch.
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 5930ab817056..850afdbd9c4e 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1888,30 +1888,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));
[Low]
Before this patch the per-queue interrupts in mana_gd_setup_dyn_irqs()
and mana_gd_setup_irqs() were named as:
"mana_q%d@pci:%s", i - 1, pci_name(pdev)
so they appeared as mana_q0, mana_q1, ... in /proc/interrupts.
After this patch, allocation goes through mana_gd_get_gic(), which
names them using gic->msi (equal to i, not i - 1):
snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_msi%d@pci:%s",
gic->msi, pci_name(dev));
so the same vectors now appear as mana_msi1, mana_msi2, ... The
mana_hwc@pci:%s name is preserved, but every per-queue vector is both
renamed (mana_q -> mana_msi) and renumbered (0-based -> 1-based).
Is this rename intentional? The commit message describes the change
as replacing "the GDMA global interrupt setup code with the new GIC
allocation and release functions", which reads as a pure refactor. If
the rename is intended, could it be mentioned in the changelog so that
operators relying on the mana_q prefix in /proc/interrupts are not
surprised?
> -
> - /* one pci vector is already allocated for HWC */
> - irqs[i - 1] = pci_irq_vector(pdev, i);
> - if (irqs[i - 1] < 0) {
> - err = irqs[i - 1];
> - goto free_current_gic;
> - }
> -
> - err = request_irq(irqs[i - 1], mana_gd_intr, 0, gic->name, gic);
> - if (err)
> - goto free_current_gic;
>
> - xa_store(&gc->irq_contexts, i, gic, GFP_KERNEL);
> + irqs[i - 1] = gic->irq;
> }
>
> /*
> @@ -1933,19 +1916,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);
> }
[Low]
Is the irq_update_affinity_hint(irq, NULL) call here still needed?
Looking at mana_gd_put_gic():
if (!refcount_dec_and_test(&gic->refcount))
goto out;
irq = gic->irq;
irq_update_affinity_hint(irq, NULL);
free_irq(irq, gic);
When the last reference is dropped (which is the case on every caller in
this patch, since the refcount is 1 here), the helper already clears the
affinity hint before calling free_irq(). The explicit call in the caller
duplicates that work.
The duplication is harmless today because irq_update_affinity_hint() is
idempotent, but could the clearing of the affinity hint be left entirely
to mana_gd_put_gic()? Otherwise, if these helpers are ever reused in a
context where the refcount is greater than one, the caller will clear an
affinity hint that the helper would correctly have left intact.
> @@ -1966,34 +1941,13 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev,
> int nvec)
> start_irqs = irqs;
>
> for (i = 0; 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);
> -
> - if (!i)
> - snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_hwc@pci:%s",
> - pci_name(pdev));
> - else
> - snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_q%d@pci:%s",
> - i - 1, pci_name(pdev));
[Low]
The same rename applies here for the non-HWC vectors (mana_q%d with
i - 1 becomes mana_msi%d with i inside mana_gd_get_gic()). Could the
commit message note this user-visible change in /proc/interrupts?
[ ... ]
> @@ -2022,19 +1976,11 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev,
> int nvec)
> kfree(start_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);
> }
[Low]
Same observation as in mana_gd_setup_dyn_irqs(): the explicit
irq_update_affinity_hint(irq, NULL) duplicates what
mana_gd_put_gic() already does when it drops the last reference. Could
this call be dropped from the caller?
> @@ -2109,26 +2055,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);
> }
[Low]
Same pattern in mana_gd_remove_irqs(): the comment says "Need to clear
the hint before free_irq", but the actual free_irq() now happens inside
mana_gd_put_gic(), which itself calls irq_update_affinity_hint(irq, NULL)
right before free_irq(). Could the explicit call (and comment) be
removed so that the affinity-hint clearing lives in one place?
