Re: [PATCH net-next v7 5/6] net: mana: Allocate interrupt context for each EQ when creating vPort

[Jakub Kicinski <kuba@kernel.org>]

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;