Re: [PATCH net-next v7 6/6] RDMA/mana_ib: Allocate interrupt contexts on EQs

[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: RDMA/mana_ib: Allocate interrupt contexts on EQs

This patch converts RDMA/mana_ib EQ creation to use the shared GIC
allocator (mana_gd_get_gic/mana_gd_put_gic), allowing the RDMA EQs
to share interrupt contexts with the Ethernet driver's EQs. It also
adds a new driver capability bit and sets it in the advertised flags.

> diff --git a/drivers/infiniband/hw/mana/main.c b/drivers/infiniband/hw/mana/main.c
> index 8000ab6e8beb..0dc47540fb6f 100644
> --- a/drivers/infiniband/hw/mana/main.c
> +++ b/drivers/infiniband/hw/mana/main.c
> @@ -749,7 +749,8 @@ int mana_ib_create_eqs(struct mana_ib_dev *mdev)
>  {
>  	struct gdma_context *gc = mdev_to_gc(mdev);
>  	struct gdma_queue_spec spec = {};
> -	int err, i;
> +	struct gdma_irq_context *gic;
> +	int err, i, msi;
>  
>  	spec.type = GDMA_EQ;
>  	spec.monitor_avl_buf = false;
> @@ -757,11 +758,18 @@ int mana_ib_create_eqs(struct mana_ib_dev *mdev)
>  	spec.eq.callback = mana_ib_event_handler;
>  	spec.eq.context = mdev;
>  	spec.eq.log2_throttle_limit = LOG2_EQ_THROTTLE;
> -	spec.eq.msix_index = 0;
> +
> +	msi = 0;
> +	gic = mana_gd_get_gic(gc, false, &msi);

[Low]
The commit message states "The driver now supports allocating dedicated
MSI-X for each EQ", and the new capability bit added below is named
GDMA_DRV_CAP_FLAG_1_EQ_MSI_UNSHARE_MULTI_VPORT. Does that description
match what this code does?

The calls here pass use_msi_bitmap=false, which appears to share an
existing gic slot (incrementing its refcount) rather than allocating a
fresh dedicated MSI-X from the bitmap:

	msi = 0;
	gic = mana_gd_get_gic(gc, false, &msi);
	...
	msi = (i + 1) % gc->num_msix_usable;
	gic = mana_gd_get_gic(gc, false, &msi);

The commit body also says "These interrupt contexts may be shared with
Ethernet EQs when MSI-X vectors are limited", which describes
sharing-with-fallback, while the flag name says "UNSHARE". Could the
changelog and flag naming be reconciled so the semantics are clear (for
example, whether the bit advertises "driver can handle unshared EQs" vs
"driver will always be unshared")?

Should the flag also be set only when the driver actually ends up
dedicated rather than advertised unconditionally?

[ ... ]

> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 6c138cc77407..d84e474309a3 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -615,6 +615,7 @@ enum {
>  #define GDMA_DRV_CAP_FLAG_1_HWC_TIMEOUT_RECONFIG BIT(3)
>  #define GDMA_DRV_CAP_FLAG_1_GDMA_PAGES_4MB_1GB_2GB BIT(4)
>  #define GDMA_DRV_CAP_FLAG_1_VARIABLE_INDIRECTION_TABLE_SUPPORT BIT(5)
> +#define GDMA_DRV_CAP_FLAG_1_HW_VPORT_LINK_AWARE BIT(6)
>  
>  /* Driver can handle holes (zeros) in the device list */
>  #define GDMA_DRV_CAP_FLAG_1_DEV_LIST_HOLES_SUP BIT(11)
> @@ -631,7 +632,8 @@ enum {
>  /* Driver detects stalled send queues and recovers them */
>  #define GDMA_DRV_CAP_FLAG_1_HANDLE_STALL_SQ_RECOVERY BIT(18)
>  
> -#define GDMA_DRV_CAP_FLAG_1_HW_VPORT_LINK_AWARE BIT(6)
> +/* Driver supports separate EQ/MSIs for each vPort */
> +#define GDMA_DRV_CAP_FLAG_1_EQ_MSI_UNSHARE_MULTI_VPORT BIT(19)
>  
>  /* Driver supports linearizing the skb when num_sge exceeds hardware limit */
>  #define GDMA_DRV_CAP_FLAG_1_SKB_LINEARIZE BIT(20)

[Low]
The subject line scopes this commit to RDMA/mana_ib, but this hunk
changes the shared header include/net/mana/gdma.h in two ways that
affect the Ethernet driver and firmware interface:

- it introduces the new GDMA_DRV_CAP_FLAG_1_EQ_MSI_UNSHARE_MULTI_VPORT
  BIT(19) advertised to firmware, and
- it silently moves GDMA_DRV_CAP_FLAG_1_HW_VPORT_LINK_AWARE BIT(6) from
  after BIT(18) back to its numeric slot after BIT(5).

Neither change is mentioned in the changelog. Would it be clearer to
split the capability-bit addition into its own patch (or use a subject
not scoped to mana_ib), and to note the BIT(6) relocation as a separate
cleanup so bisecting firmware/Ethernet behavior changes is easier?

> @@ -659,7 +661,8 @@ enum {
>  	 GDMA_DRV_CAP_FLAG_1_SKB_LINEARIZE | \
>  	 GDMA_DRV_CAP_FLAG_1_PROBE_RECOVERY | \
>  	 GDMA_DRV_CAP_FLAG_1_HANDLE_STALL_SQ_RECOVERY | \
> -	 GDMA_DRV_CAP_FLAG_1_HWC_TIMEOUT_RECOVERY)
> +	 GDMA_DRV_CAP_FLAG_1_HWC_TIMEOUT_RECOVERY | \
> +	 GDMA_DRV_CAP_FLAG_1_EQ_MSI_UNSHARE_MULTI_VPORT)
>  
>  #define GDMA_DRV_CAP_FLAGS2 0