This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: mana: Allocate interrupt context for each EQ when creating vPort
This commit switches the vPort EQ creation path to acquire a GIC (either
dedicated or shared) for each EQ via mana_gd_get_gic()/mana_gd_put_gic(),
adds a new irq field to struct gdma_queue's EQ union and a debugfs entry
for it, and moves the msix_index invalidation out of mana_gd_deregister_irq()
into the mana_gd_create_eq() error path.
[Low]
The changelog is a single sentence, but the patch makes several coupled
changes beyond acquiring a GIC per EQ:
- it adds a new per-EQ irq field on the shared struct gdma_queue,
- it adds a new debugfs entry for that field,
- it changes an invariant in gdma_main.c (see the comment on
mana_gd_deregister_irq() below),
- it splits the GIC lifecycle across two source files — mana_en.c now
owns get_gic/put_gic around mana_gd_create_mana_eq(), while
mana_gd_register_irq() is still called internally from the create path.
Could the commit message describe the resulting ownership contract
(caller holds a GIC reference across create/destroy, register_irq does
not) and justify the scope of the new field?
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 850afdbd9c4e..54127c385240 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -854,7 +854,6 @@ static void mana_gd_deregister_irq(struct gdma_queue
> *queue)
> }
> spin_unlock_irqrestore(&gic->lock, flags);
>
> - queue->eq.msix_index = INVALID_PCI_MSIX_INDEX;
> synchronize_rcu();
> }
[Low]
Removing the msix_index reset from mana_gd_deregister_irq() and relocating
it to the mana_gd_create_eq() error path is the prerequisite that lets the
new mana_destroy_eq() read eq->eq.msix_index before calling
mana_gd_destroy_queue():
msi = eq->eq.msix_index;
mana_gd_destroy_queue(gc, eq);
mana_gd_put_gic(gc, !gc->msi_sharing, msi);
Could the changelog mention this intentional invariant change and the
ownership contract it implies? Any out-of-tree caller that relied on
mana_gd_deregister_irq() to invalidate msix_index will now silently observe
stale values after a normal destroy.
> @@ -969,6 +968,7 @@ static int mana_gd_create_eq(struct gdma_dev *gd,
> out:
> dev_err(dev, "Failed to create EQ: %d\n", err);
> mana_gd_destroy_eq(gc, false, queue);
> + queue->eq.msix_index = INVALID_PCI_MSIX_INDEX;
> return err;
> }
>
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 2f106d6f5be4..b3684fa3eb4a 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -1664,6 +1668,8 @@ int mana_create_eq(struct mana_port_context *apc)
> struct gdma_queue_spec spec = {};
> int err;
> int i;
> + int msi;
> + struct gdma_irq_context *gic;
>
> WARN_ON(apc->eqs);
> apc->eqs = kzalloc_objs(struct mana_eq, apc->num_queues);
> @@ -1681,12 +1687,24 @@ int mana_create_eq(struct mana_port_context *apc)
> apc->mana_port_debugfs);
>
> for (i = 0; i < apc->num_queues; i++) {
> - spec.eq.msix_index = (i + 1) % gc->num_msix_usable;
> + msi = (i + 1) % gc->num_msix_usable;
> + if (gc->msi_sharing)
> + spec.eq.msix_index = msi;
> +
> + gic = mana_gd_get_gic(gc, !gc->msi_sharing, &msi);
> + if (!gic) {
> + err = -ENOMEM;
> + goto out;
> + }
> + spec.eq.msix_index = msi;
[Low]
Is the conditional write to spec.eq.msix_index dead?
if (gc->msi_sharing)
spec.eq.msix_index = msi;
gic = mana_gd_get_gic(gc, !gc->msi_sharing, &msi);
...
spec.eq.msix_index = msi;
The unconditional assignment after mana_gd_get_gic() always overwrites the
conditional one. In the non-sharing case, mana_gd_get_gic() also reassigns
*msi_requested via find_first_zero_bit(), so the pre-computed
(i + 1) % gc->num_msix_usable is only consumed in the sharing branch.
Would splitting the sharing and non-sharing paths, or dropping the
conditional write entirely, make the intent clearer and avoid the
inverted-boolean fallthrough (!gc->msi_sharing)?
> +
> err = mana_gd_create_mana_eq(gd, &spec, &apc->eqs[i].eq);
> if (err) {
> dev_err(gc->dev, "Failed to create EQ %d : %d\n", i,
> err);
> + mana_gd_put_gic(gc, !gc->msi_sharing, msi);
> goto out;
> }
> + apc->eqs[i].eq->eq.irq = gic->irq;
> mana_create_eq_debugfs(apc, i);
> }
[Low]
The new struct gdma_queue.eq.irq field is only assigned here on the vPort
path:
apc->eqs[i].eq->eq.irq = gic->irq;
The other in-tree caller of mana_gd_create_mana_eq(), mana_ib_create_eqs()
in drivers/infiniband/hw/mana/main.c, creates EQs using the same struct
but never assigns eq.irq, leaving it zero. No debugfs is wired up for ib
EQs today, so the zero is not user-visible, but any future consumer that
reads this field on an ib EQ will see 0 rather than the real IRQ number.
Should mana_gd_create_mana_eq() (or its callers) populate eq.irq
uniformly so the field has one well-defined meaning across all EQs?
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index fbe3c1427b45..6c138cc77407 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -342,6 +342,7 @@ struct gdma_queue {
> void *context;
>
> unsigned int msix_index;
> + unsigned int irq;
>
> u32 log2_throttle_limit;
> } eq;
