Skip to contentSkip to conversation

net: mana: Per-vPort EQ and MSI-X interrupt management

PATCH Superseded Long Li 2 participants 16 messages May 7 – May 13

Subsystems (5)

v7 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Long Li

author
#0 lore raw

v7 cover net: mana: Per-vPort EQ and MSI-X interrupt management

This series adds per-vPort Event Queue (EQ) allocation and MSI-X interrupt management for the MANA driver. Previously, all vPorts shared a single set of EQs. This change enables dedicated EQs per vPort with support for both dedicated and shared MSI-X vector allocation modes.

Patch 1 moves EQ ownership from mana_context to per-vPort mana_port_context and exports create/destroy functions for the RDMA driver. Also adds EQ create/destroy calls to mana_ib_cfg_vport/uncfg_vport so RDMA vPorts get their own EQs.

Patch 2 adds device capability queries to determine whether MSI-X vectors should be dedicated per-vPort or shared. When the number of available MSI-X vectors is insufficient for dedicated allocation, the driver enables sharing mode with bitmap-based vector assignment.

Patch 3 introduces the GIC (GDMA IRQ Context) abstraction with reference counting, allowing multiple EQs to safely share a single MSI-X vector.

Patch 4 converts the global EQ allocation in probe/resume to use the new GIC functions.

Patch 5 adds per-vPort GIC lifecycle management, calling get/put on each EQ creation and destruction during vPort open/close.

Patch 6 extends the same GIC lifecycle management to the RDMA driver's EQ allocation path.

Changes in v7:
- Rebased on net-next/main
- Patch 1: Guard ibdev_dbg() in mana_ib_cfg_vport() with error check so the vport handle is not logged on the failure path
- Patch 1: Fix checkpatch line length warning in debugfs_create_dir() call
- Patch 2: Use rounddown_pow_of_two() instead of roundup_pow_of_two() when computing per-vPort queue count to avoid unnecessarily forcing shared MSI-X mode in borderline configurations
- Patch 2: Call mana_gd_setup_remaining_irqs() unconditionally to ensure irq_contexts are populated in both dedicated and shared MSI-X modes, fixing bisectability between patches 2 and 5
- Patch 2: Fix checkpatch line length warning in debugfs_create_u16() call
- Patch 3: Use cached gic->irq instead of pci_irq_vector() lookup in mana_gd_put_gic() for consistency with the allocation path
- Patch 3: Fix checkpatch line length warning in mana_gd_get_gic() declaration
- Patch 5: Fix unsigned int* to int* pointer type mismatch when calling mana_gd_get_gic() by using a local int variable for the MSI index
- Patch 6: Fix same unsigned int* to int* pointer type mismatch in RDMA EQ creation path

Changes in v6:
- Rebased on net-next/main (v7.1-rc1)

Changes in v5:
- Rebased on net-next/main

Changes in v4:
- Rebased on net-next/main 7.0-rc4
- Patch 2: Use MANA_DEF_NUM_QUEUES instead of hardcoded 16 for max_num_queues clamping
- Patch 3: Track dyn_msix in GIC context instead of re-checking pci_msix_can_alloc_dyn() on each call; improved remove_irqs iteration to skip unallocated entries

Changes in v3:
- Rebased on net-next/main
- Patch 1: Added NULL check for mpc->eqs in mana_ib_create_qp_rss() to prevent NULL pointer dereference when RSS QP is created before a raw QP has configured the vport and allocated EQs

Changes in v2:
- Rebased on net-next/main (adapted to kzalloc_objs/kzalloc_obj macros, new GDMA_DRV_CAP_FLAG definitions)
- Patch 2: Fixed misleading comment for max_num_queues vs max_num_queues_vport in gdma.h
- Patch 3: Fixed spelling typo in gdma_main.c ("difference" -> "different")

Long Li (6):
net: mana: Create separate EQs for each vPort
net: mana: Query device capabilities and configure MSI-X sharing for
EQs
net: mana: Introduce GIC context with refcounting for interrupt
management
net: mana: Use GIC functions to allocate global EQs
net: mana: Allocate interrupt context for each EQ when creating vPort RDMA/mana_ib: Allocate interrupt contexts on EQs

 drivers/infiniband/hw/mana/main.c             |  60 +++-
 drivers/infiniband/hw/mana/qp.c               |  16 +-
 .../net/ethernet/microsoft/mana/gdma_main.c   | 297 +++++++++++++-----
 drivers/net/ethernet/microsoft/mana/mana_en.c | 168 ++++++----
 include/net/mana/gdma.h                       |  33 +-
 include/net/mana/mana.h                       |   7 +-
 6 files changed, 425 insertions(+), 156 deletions(-)
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH v7 1/6 net: mana: Create separate EQs for each vPort

To prepare for assigning vPorts to dedicated MSI-X vectors, remove EQ sharing among the vPorts and create dedicated EQs for each vPort.

Move the EQ definition from struct mana_context to struct mana_port_context and update related support functions. Export mana_create_eq() and mana_destroy_eq() for use by the MANA RDMA driver.

---
 drivers/infiniband/hw/mana/main.c             |  19 ++-
 drivers/infiniband/hw/mana/qp.c               |  16 ++-
 drivers/net/ethernet/microsoft/mana/mana_en.c | 111 ++++++++++--------
 include/net/mana/mana.h                       |   7 +-
 4 files changed, 98 insertions(+), 55 deletions(-)
drivers/infiniband/hw/mana/main.c +14 -5
index ac5e75dd3494..8000ab6e8beb 100644
@@ -20,8 +20,10 @@ void mana_ib_uncfg_vport(struct mana_ib_dev *dev, struct mana_ib_pd *pd,
 	pd->vport_use_count--;
 	WARN_ON(pd->vport_use_count < 0);
 
-	if (!pd->vport_use_count)
+	if (!pd->vport_use_count) {
+		mana_destroy_eq(mpc);
 		mana_uncfg_vport(mpc);
+	}
 
 	mutex_unlock(&pd->vport_mutex);
 }
@@ -55,15 +57,22 @@ int mana_ib_cfg_vport(struct mana_ib_dev *dev, u32 port, struct mana_ib_pd *pd,
 		return err;
 	}
 
-	mutex_unlock(&pd->vport_mutex);
 
 	pd->tx_shortform_allowed = mpc->tx_shortform_allowed;
 	pd->tx_vp_offset = mpc->tx_vp_offset;
+	err = mana_create_eq(mpc);
+	if (err) {
+		mana_uncfg_vport(mpc);
+		pd->vport_use_count--;
+	}
 
-	ibdev_dbg(&dev->ib_dev, "vport handle %llx pdid %x doorbell_id %x\n",
-		  mpc->port_handle, pd->pdn, doorbell_id);
+	mutex_unlock(&pd->vport_mutex);
 
-	return 0;
+	if (!err)
+		ibdev_dbg(&dev->ib_dev, "vport handle %llx pdid %x doorbell_id %x\n",
+			  mpc->port_handle, pd->pdn, doorbell_id);
+
+	return err;
 }
 
 int mana_ib_alloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
drivers/infiniband/hw/mana/qp.c +14 -2
index 645581359cee..6f1043383e8c 100644
@@ -168,7 +168,15 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
 		cq_spec.gdma_region = cq->queue.gdma_region;
 		cq_spec.queue_size = cq->cqe * COMP_ENTRY_SIZE;
 		cq_spec.modr_ctx_id = 0;
-		eq = &mpc->ac->eqs[cq->comp_vector];
+		/* EQs are created when a raw QP configures the vport.
+		 * A raw QP must be created before creating rwq_ind_tbl.
+		 */
+		if (!mpc->eqs) {
+			ret = -EINVAL;
+			i--;
+			goto fail;
+		}
+		eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
 		cq_spec.attached_eq = eq->eq->id;
 
 		ret = mana_create_wq_obj(mpc, mpc->port_handle, GDMA_RQ,
@@ -317,7 +325,11 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd *ibpd,
 	cq_spec.queue_size = send_cq->cqe * COMP_ENTRY_SIZE;
 	cq_spec.modr_ctx_id = 0;
 	eq_vec = send_cq->comp_vector;
-	eq = &mpc->ac->eqs[eq_vec];
+	if (!mpc->eqs) {
+		err = -EINVAL;
+		goto err_destroy_queue;
+	}
+	eq = &mpc->eqs[eq_vec % mpc->num_queues];
 	cq_spec.attached_eq = eq->eq->id;
 
 	err = mana_create_wq_obj(mpc, mpc->port_handle, GDMA_SQ, &wq_spec,
drivers/net/ethernet/microsoft/mana/mana_en.c +65 -46

264 lines not on this page · raw message

include/net/mana/mana.h +5 -2
index aa90a858c8e3..c8e7d16f6685 100644
@@ -480,8 +480,6 @@ struct mana_context {
 	u8 bm_hostmode;
 
 	struct mana_ethtool_hc_stats hc_stats;
-	struct mana_eq *eqs;
-	struct dentry *mana_eqs_debugfs;
 	struct workqueue_struct *per_port_queue_reset_wq;
 	/* Workqueue for querying hardware stats */
 	struct delayed_work gf_stats_work;
@@ -501,6 +499,9 @@ struct mana_port_context {
 
 	u8 mac_addr[ETH_ALEN];
 
+	struct mana_eq *eqs;
+	struct dentry *mana_eqs_debugfs;
+
 	enum TRI_STATE rss_state;
 
 	mana_handle_t default_rxobj;
@@ -1034,6 +1035,8 @@ void mana_destroy_wq_obj(struct mana_port_context *apc, u32 wq_type,
 int mana_cfg_vport(struct mana_port_context *apc, u32 protection_dom_id,
 		   u32 doorbell_pg_id);
 void mana_uncfg_vport(struct mana_port_context *apc);
+int mana_create_eq(struct mana_port_context *apc);
+void mana_destroy_eq(struct mana_port_context *apc);
 
 struct net_device *mana_get_primary_netdev(struct mana_context *ac,
 					   u32 port_index,
Signature · 1 line
2.43.0
maintainer
#2 lore raw

Re: [PATCH net-next v7 1/6] net: mana: Create separate EQs for each vPort

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: Create separate EQs for each vPort

This change removes EQ sharing across vPorts and moves EQ ownership from struct mana_context to struct mana_port_context. It exports mana_create_eq() and mana_destroy_eq() so the MANA RDMA driver can create EQs when a raw QP configures the vport, and also rearranges mana_alloc_queues()/mana_dealloc_queues() to own the netdev-side EQ lifecycle.

A few questions on the resulting ownership model below.

Quoted 6 lines
drivers/infiniband/hw/mana/main.c +0 -0
index ac5e75dd3494..8000ab6e8beb 100644
@@ -20,8 +20,10 @@ void mana_ib_uncfg_vport(struct mana_ib_dev *dev, struct mana_ib_pd *pd,
 	pd->vport_use_count--;

drivers/infiniband/hw/mana/main.c void mana_ib_uncfg_vport(struct mana_ib_dev *dev, struct mana_ib_pd *pd, · line 29

Quoted 10 lines
drivers/infiniband/hw/mana/main.c +3 -1
 	WARN_ON(pd->vport_use_count < 0);
 
-	if (!pd->vport_use_count)
+	if (!pd->vport_use_count) {
+		mana_destroy_eq(mpc);
 		mana_uncfg_vport(mpc);
+	}
 
 	mutex_unlock(&pd->vport_mutex);
 }

[High]
After this change, mpc->eqs has two distinct owners: the RDMA path frees it here under pd->vport_mutex, and the netdev path frees it under rtnl_lock via mana_dealloc_queues() -> mana_destroy_eq(). Neither lock is held by the readers in qp.c. See the comment on the RSS QP path below.

Quoted 9 lines
drivers/infiniband/hw/mana/qp.c +0 -1
index 645581359cee..6f1043383e8c 100644
@@ -168,7 +168,15 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
 		cq_spec.gdma_region = cq->queue.gdma_region;
 		cq_spec.queue_size = cq->cqe * COMP_ENTRY_SIZE;
 		cq_spec.modr_ctx_id = 0;
-		eq = &mpc->ac->eqs[cq->comp_vector];

drivers/infiniband/hw/mana/qp.c static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd, · line 180

Quoted 10 lines
drivers/infiniband/hw/mana/qp.c +9 -0
+		/* EQs are created when a raw QP configures the vport.
+		 * A raw QP must be created before creating rwq_ind_tbl.
+		 */
+		if (!mpc->eqs) {
+			ret = -EINVAL;
+			i--;
+			goto fail;
+		}
+		eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
 		cq_spec.attached_eq = eq->eq->id;

[High]
Is the NULL check on mpc->eqs safe here without any synchronization?

Before this patch, ac->eqs was allocated in mana_probe() and freed in mana_remove(), so its lifetime spanned the whole driver. With this patch, apc->eqs can be freed concurrently by either owner:

    mana_ib_uncfg_vport()                 (pd->vport_mutex)
      mana_destroy_eq(mpc)
        kfree(apc->eqs);
        apc->eqs = NULL;

    mana_dealloc_queues()                 (rtnl_lock, ndo_close)
      mana_destroy_eq(apc)
        kfree(apc->eqs);
        apc->eqs = NULL;

This path holds neither lock. Can the 'if (!mpc->eqs)' observation survive through the subsequent '&mpc->eqs[...]' and 'eq->eq->id' dereference, or can a concurrent destroyer free apc->eqs between the check and the deref and turn this into a read of freed memory?

There is also an initialization window in the writer:

    apc->eqs = kzalloc_objs(struct mana_eq, apc->num_queues);
    ...
    for (i = 0; i < apc->num_queues; i++) {
        ...
        err = mana_gd_create_mana_eq(gd, &spec, &apc->eqs[i].eq);

apc->eqs is published before apc->eqs[i].eq is filled in. Can a concurrent reader here see apc->eqs non-NULL but apc->eqs[i].eq still NULL, and fault on eq->eq->id?

[Medium]
mana_ib advertises ib_dev.num_comp_vectors as gc->max_num_queues in mana_ib_probe(), but this patch sizes the per-port pool as apc->num_queues, which defaults to min(gc->max_num_queues, MANA_DEF_NUM_QUEUES) and can be reduced further via ethtool -L.

    eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];

When gc->max_num_queues > apc->num_queues, does this modulo silently collapse distinct advertised completion vectors onto shared EQs, so CQs created by userspace with different comp_vector values end up on the same notification stream? Would it be better to cap ib_dev.num_comp_vectors to apc->num_queues, or reject comp_vector >= apc->num_queues, instead of wrapping?

[Medium]
The added comment codifies a new ordering constraint:

    /* EQs are created when a raw QP configures the vport.
     * A raw QP must be created before creating rwq_ind_tbl.
     */

Before this patch, ac->eqs existed from mana_probe() onward, so mana_ib_create_qp_rss() succeeded whether or not the netdev was up and whether or not any raw QP had been created on the port. After this patch, if the netdev is administratively down and no raw QP has configured the vport, mpc->eqs is NULL and this path returns -EINVAL.

Is that ABI-visible change to RSS QP creation intentional, and worth calling out in the commit message?

The comment is also slightly incomplete: EQs are created on netdev open via mana_alloc_queues() -> mana_create_eq() as well, not only when a raw QP configures the vport. Could the wording be adjusted?

Quoted 4 lines
drivers/infiniband/hw/mana/qp.c +0 -0
 		ret = mana_create_wq_obj(mpc, mpc->port_handle, GDMA_RQ,
 
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0

[ ... ]

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +3 -0
@@ -3343,6 +3367,9 @@ static int mana_dealloc_queues(struct net_device *ndev)
 	mana_fence_rqs(apc);
 
 	/* Even in err case, still need to cleanup the vPort */
+	mana_destroy_rxqs(apc);
+	mana_destroy_txq(apc);
+	mana_destroy_eq(apc);
 	mana_destroy_vport(apc);
 
 	return 0;
Signature · 1 line
pw-bot: cr
maintainer
#3 lore raw

Re: [PATCH net-next v7 1/6] net: mana: Create separate EQs for each vPort

In reply to Long Li

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: Create separate EQs for each vPort

This commit prepares for assigning vPorts to dedicated MSI-X vectors by removing EQ sharing among the vPorts and creating dedicated EQs for each vPort. It moves the EQ definition from struct mana_context to struct mana_port_context and exports related functions for the RDMA driver.

Quoted 11 lines
drivers/infiniband/hw/mana/main.c +0 -1
index ac5e75dd3494d..8000ab6e8beb9 100644
@@ -55,15 +57,22 @@ int mana_ib_cfg_vport(struct mana_ib_dev *dev, u32 port, struct mana_ib_pd *pd,
 		return err;
 	}
 
-	mutex_unlock(&pd->vport_mutex);
 
 	pd->tx_shortform_allowed = mpc->tx_shortform_allowed;

drivers/infiniband/hw/mana/main.c int mana_ib_cfg_vport(struct mana_ib_dev *dev, u32 port, struct mana_ib_pd *pd, · line 69

Quoted 10 lines
drivers/infiniband/hw/mana/main.c +6 -2
 	pd->tx_vp_offset = mpc->tx_vp_offset;
+	err = mana_create_eq(mpc);
+	if (err) {
+		mana_uncfg_vport(mpc);
+		pd->vport_use_count--;
+	}
 
-	ibdev_dbg(&dev->ib_dev, "vport handle %llx pdid %x doorbell_id %x\n",
-		  mpc->port_handle, pd->pdn, doorbell_id);
+	mutex_unlock(&pd->vport_mutex);

How does this handle a Protection Domain shared across multiple physical ports?

Looking at the earlier part of mana_ib_cfg_vport(), if a PD is already in use, the code increments pd->vport_use_count and returns early:

	pd->vport_use_count++;
	if (pd->vport_use_count > 1) {
		ibdev_dbg(&dev->ib_dev,
			  "Skip as this PD is already configured vport\n");
		mutex_unlock(&pd->vport_mutex);
		return 0;
	}

If a second port is configured with the same PD, it skips calling mana_create_eq() for the new port's mpc. Will this cause mpc->eqs to remain NULL for the second port and fail subsequent queue creations?

[ ... ]

Quoted 7 lines
drivers/infiniband/hw/mana/qp.c +0 -0
index 0fbcf449c134b..6167742df9d29 100644
@@ -171,7 +171,15 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
 		cq_spec.gdma_region = cq->queue.gdma_region;
 		cq_spec.queue_size = cq->cqe * COMP_ENTRY_SIZE;

drivers/infiniband/hw/mana/qp.c static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd, · line 181

Quoted 10 lines
drivers/infiniband/hw/mana/qp.c +8 -1
 		cq_spec.modr_ctx_id = 0;
-		eq = &mpc->ac->eqs[cq->comp_vector];
+		/* EQs are created when a raw QP configures the vport.
+		 * A raw QP must be created before creating rwq_ind_tbl.
+		 */
+		if (!mpc->eqs) {
+			ret = -EINVAL;
+			i--;
+			goto fail;
+		}

Does this explicit i-- cause a resource leak in the error path?

The fail block cleans up previously created WQ objects using a post-decrement while loop:

fail:

	while (i-- > 0) {
		ibwq = ind_tbl->ind_tbl[i];
		ibcq = ibwq->cq;
		...
		mana_destroy_wq_obj(mpc, GDMA_RQ, wq->rx_object);
	}

If we manually decrement i before jumping to the fail block, the while loop will decrement it again prior to evaluation, completely skipping the cleanup of the WQ object that was just successfully created at the original index i.

drivers/infiniband/hw/mana/qp.c static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd, · line 179

Quoted 1 line
drivers/infiniband/hw/mana/qp.c +1 -0
+		eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];

What guarantees the lifetime of the mpc->eqs array for active RSS QPs?

Since mana_ib_create_qp_rss() doesn't take a reference count or hold a lock that prevents mana_destroy_eq() from running, if the Ethernet interface is brought down or the RAW QP is destroyed, could mana_destroy_eq() free the array while RSS QPs are still actively using it?

Additionally, can this out-of-bounds read if an ethtool channel update changes mpc->num_queues while the interface is down?

If a RAW QP is created while the interface is down, mpc->eqs is sized to the old channel count. If the channel count is then increased, mpc->num_queues updates but the array isn't reallocated because the interface is down and queue deallocation was skipped. This could lead to accessing past the end of the allocation both here and in mana_destroy_eq().

[ ... ]

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index b2faa7cf398f3..cef86c9310543 100644

[ ... ]

drivers/net/ethernet/microsoft/mana/mana_en.c void mana_destroy_wq_obj(struct mana_port_context *apc, u32 wq_type, · line 1654

Quoted 11 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +3 -3
-static void mana_create_eq_debugfs(struct mana_context *ac, int i)
+static void mana_create_eq_debugfs(struct mana_port_context *apc, int i)
 {
-	struct mana_eq eq = ac->eqs[i];
+	struct mana_eq eq = apc->eqs[i];
 	char eqnum[32];
 
 	sprintf(eqnum, "eq%d", i);
-	eq.mana_eq_debugfs = debugfs_create_dir(eqnum, ac->mana_eqs_debugfs);
+	eq.mana_eq_debugfs = debugfs_create_dir(eqnum, apc->mana_eqs_debugfs);
 }

Does this update the actual debugfs pointer in the array?

By using struct mana_eq eq = apc->eqs[i];, it creates a local by-value copy on the stack. The debugfs directory is created, but the pointer in the actual apc->eqs array remains NULL. Would it be better to use a pointer reference like struct mana_eq *eq = &apc->eqs[i]; here?

[ ... ]

Quoted 24 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +6 -5
-static int mana_create_eq(struct mana_context *ac)
+int mana_create_eq(struct mana_port_context *apc)
 {
-	struct gdma_dev *gd = ac->gdma_dev;
+	struct gdma_dev *gd = apc->ac->gdma_dev;
 	struct gdma_context *gc = gd->gdma_context;
 	struct gdma_queue_spec spec = {};
 	int err;
 	int i;
 
-	ac->eqs = kzalloc_objs(struct mana_eq, gc->max_num_queues);
-	if (!ac->eqs)
+	WARN_ON(apc->eqs);
+	apc->eqs = kzalloc_objs(struct mana_eq, apc->num_queues);
+	if (!apc->eqs)
 		return -ENOMEM;
 
 	spec.type = GDMA_EQ;
 	spec.monitor_avl_buf = false;
 	spec.queue_size = EQ_SIZE;
 	spec.eq.callback = NULL;
-	spec.eq.context = ac->eqs;
+	spec.eq.context = apc->eqs;
 	spec.eq.log2_throttle_limit = LOG2_EQ_THROTTLE;

drivers/net/ethernet/microsoft/mana/mana_en.c void mana_destroy_wq_obj(struct mana_port_context *apc, u32 wq_type, · line 1681

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +4 -3
 
-	ac->mana_eqs_debugfs = debugfs_create_dir("EQs", gc->mana_pci_debugfs);
+	apc->mana_eqs_debugfs = debugfs_create_dir("EQs",
+						    apc->mana_port_debugfs);
 
-	for (i = 0; i < gc->max_num_queues; i++) {
+	for (i = 0; i < apc->num_queues; i++) {
 		spec.eq.msix_index = (i + 1) % gc->num_msix_usable;
-		err = mana_gd_create_mana_eq(gd, &spec, &ac->eqs[i].eq);
+		err = mana_gd_create_mana_eq(gd, &spec, &apc->eqs[i].eq);

Could there be a race condition between mana_create_eq() and mana_ib_create_qp_rss() here?

apc->eqs is assigned immediately after allocation, but before the individual eq structures are populated by mana_gd_create_mana_eq() in this loop.

If an RDMA application concurrently calls mana_ib_create_qp_rss(), it checks if (!mpc->eqs). That check will pass because the array is allocated, but accessing eq->eq->id could result in a NULL pointer dereference because the element has not been fully initialized yet.

author
#4 lore raw

PATCH v7 2/6 net: mana: Query device capabilities and configure MSI-X sharing for EQs

In reply to Long Li

When querying the device, adjust the max number of queues to allow dedicated MSI-X vectors for each vPort. The number of queues per vPort is clamped to no less than MANA_DEF_NUM_QUEUES. MSI-X sharing among vPorts is disabled by default and is only enabled when there are not enough MSI-X vectors for dedicated allocation.

Rename mana_query_device_cfg() to mana_gd_query_device_cfg() as it is used at GDMA device probe time for querying device capabilities.

---
 .../net/ethernet/microsoft/mana/gdma_main.c   | 56 ++++++++++++++++++-
 drivers/net/ethernet/microsoft/mana/mana_en.c | 37 +++++++-----
 include/net/mana/gdma.h                       | 13 ++++-
 3 files changed, 87 insertions(+), 19 deletions(-)
drivers/net/ethernet/microsoft/mana/gdma_main.c +53 -3
index f3316e929175..3aa96329f359 100644
@@ -149,6 +149,9 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
 	struct gdma_context *gc = pci_get_drvdata(pdev);
 	struct gdma_query_max_resources_resp resp = {};
 	struct gdma_general_req req = {};
+	unsigned int max_num_queues;
+	u8 bm_hostmode;
+	u16 num_ports;
 	int err;
 
 	mana_gd_init_req_hdr(&req.hdr, GDMA_QUERY_MAX_RESOURCES,
@@ -197,6 +200,40 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
 	if (gc->max_num_queues == 0)
 		return -ENOSPC;
 
+	err = mana_gd_query_device_cfg(gc, MANA_MAJOR_VERSION, MANA_MINOR_VERSION,
+				       MANA_MICRO_VERSION, &num_ports, &bm_hostmode);
+	if (err)
+		return err;
+
+	if (!num_ports)
+		return -EINVAL;
+
+	/*
+	 * Adjust gc->max_num_queues returned from the SOC to allow dedicated
+	 * MSIx for each vPort. Clamp to no less than MANA_DEF_NUM_QUEUES.
+	 */
+	max_num_queues = (gc->num_msix_usable - 1) / num_ports;
+	max_num_queues = rounddown_pow_of_two(max(max_num_queues, 1U));
+	if (max_num_queues < MANA_DEF_NUM_QUEUES)
+		max_num_queues = MANA_DEF_NUM_QUEUES;
+
+	/*
+	 * Use dedicated MSIx for EQs whenever possible, use MSIx sharing for
+	 * Ethernet EQs when (max_num_queues * num_ports > num_msix_usable - 1)
+	 */
+	max_num_queues = min(gc->max_num_queues, max_num_queues);
+	if (max_num_queues * num_ports > gc->num_msix_usable - 1)
+		gc->msi_sharing = true;
+
+	/* If MSI is shared, use max allowed value */
+	if (gc->msi_sharing)
+		gc->max_num_queues_vport = min(gc->num_msix_usable - 1, gc->max_num_queues);
+	else
+		gc->max_num_queues_vport = max_num_queues;
+
+	dev_info(gc->dev, "MSI sharing mode %d max queues %d\n",
+		 gc->msi_sharing, gc->max_num_queues);
+
 	return 0;
 }
 
@@ -1859,6 +1896,7 @@ static int mana_gd_setup_hwc_irqs(struct pci_dev *pdev)
 		/* Need 1 interrupt for HWC */
 		max_irqs = min(num_online_cpus(), MANA_MAX_NUM_QUEUES) + 1;
 		min_irqs = 2;
+		gc->msi_sharing = true;
 	}
 
 	nvec = pci_alloc_irq_vectors(pdev, min_irqs, max_irqs, PCI_IRQ_MSIX);
@@ -1937,6 +1975,8 @@ static void mana_gd_remove_irqs(struct pci_dev *pdev)
 
 	pci_free_irq_vectors(pdev);
 
+	bitmap_free(gc->msi_bitmap);
+	gc->msi_bitmap = NULL;
 	gc->max_num_msix = 0;
 	gc->num_msix_usable = 0;
 }
@@ -1971,6 +2011,10 @@ static int mana_gd_setup(struct pci_dev *pdev)
 	if (err)
 		goto destroy_hwc;
 
+	err = mana_gd_detect_devices(pdev);
+	if (err)
+		goto destroy_hwc;
+
 	err = mana_gd_query_max_resources(pdev);
 	if (err)
 		goto destroy_hwc;
@@ -1981,9 +2025,15 @@ static int mana_gd_setup(struct pci_dev *pdev)
 		goto destroy_hwc;
 	}
 
-	err = mana_gd_detect_devices(pdev);
-	if (err)
-		goto destroy_hwc;
+	if (!gc->msi_sharing) {
+		gc->msi_bitmap = bitmap_zalloc(gc->num_msix_usable, GFP_KERNEL);
+		if (!gc->msi_bitmap) {
+			err = -ENOMEM;
+			goto destroy_hwc;
+		}
+		/* Set bit for HWC */
+		set_bit(0, gc->msi_bitmap);
+	}
 
 	dev_dbg(&pdev->dev, "mana gdma setup successful\n");
 	return 0;
drivers/net/ethernet/microsoft/mana/mana_en.c +22 -15
index a13204b3ee79..2f106d6f5be4 100644
@@ -1007,10 +1007,9 @@ static int mana_init_port_context(struct mana_port_context *apc)
 	return !apc->rxqs ? -ENOMEM : 0;
 }
 
-static int mana_send_request(struct mana_context *ac, void *in_buf,
-			     u32 in_len, void *out_buf, u32 out_len)
+static int gdma_mana_send_request(struct gdma_context *gc, void *in_buf,
+				  u32 in_len, void *out_buf, u32 out_len)
 {
-	struct gdma_context *gc = ac->gdma_dev->gdma_context;
 	struct gdma_resp_hdr *resp = out_buf;
 	struct gdma_req_hdr *req = in_buf;
 	struct device *dev = gc->dev;
@@ -1044,6 +1043,14 @@ static int mana_send_request(struct mana_context *ac, void *in_buf,
 	return 0;
 }
 
+static int mana_send_request(struct mana_context *ac, void *in_buf,
+			     u32 in_len, void *out_buf, u32 out_len)
+{
+	struct gdma_context *gc = ac->gdma_dev->gdma_context;
+
+	return gdma_mana_send_request(gc, in_buf, in_len, out_buf, out_len);
+}
+
 static int mana_verify_resp_hdr(const struct gdma_resp_hdr *resp_hdr,
 				const enum mana_command_code expected_code,
 				const u32 min_size)
@@ -1177,11 +1184,10 @@ static void mana_pf_deregister_filter(struct mana_port_context *apc)
 			   err, resp.hdr.status);
 }
 
-static int mana_query_device_cfg(struct mana_context *ac, u32 proto_major_ver,
-				 u32 proto_minor_ver, u32 proto_micro_ver,
-				 u16 *max_num_vports, u8 *bm_hostmode)
+int mana_gd_query_device_cfg(struct gdma_context *gc, u32 proto_major_ver,
+			     u32 proto_minor_ver, u32 proto_micro_ver,
+			     u16 *max_num_vports, u8 *bm_hostmode)
 {
-	struct gdma_context *gc = ac->gdma_dev->gdma_context;
 	struct mana_query_device_cfg_resp resp = {};
 	struct mana_query_device_cfg_req req = {};
 	struct device *dev = gc->dev;
@@ -1196,7 +1202,7 @@ static int mana_query_device_cfg(struct mana_context *ac, u32 proto_major_ver,
 	req.proto_minor_ver = proto_minor_ver;
 	req.proto_micro_ver = proto_micro_ver;
 
-	err = mana_send_request(ac, &req, sizeof(req), &resp, sizeof(resp));
+	err = gdma_mana_send_request(gc, &req, sizeof(req), &resp, sizeof(resp));
 	if (err) {
 		dev_err(dev, "Failed to query config: %d", err);
 		return err;
@@ -1230,8 +1236,6 @@ static int mana_query_device_cfg(struct mana_context *ac, u32 proto_major_ver,
 	else
 		*bm_hostmode = 0;
 
-	debugfs_create_u16("adapter-MTU", 0400, gc->mana_pci_debugfs, &gc->adapter_mtu);
-
 	return 0;
 }
 
@@ -3415,7 +3419,7 @@ static int mana_probe_port(struct mana_context *ac, int port_idx,
 	int err;
 
 	ndev = alloc_etherdev_mq(sizeof(struct mana_port_context),
-				 gc->max_num_queues);
+				 gc->max_num_queues_vport);
 	if (!ndev)
 		return -ENOMEM;
 
@@ -3424,9 +3428,9 @@ static int mana_probe_port(struct mana_context *ac, int port_idx,
 	apc = netdev_priv(ndev);
 	apc->ac = ac;
 	apc->ndev = ndev;
-	apc->max_queues = gc->max_num_queues;
+	apc->max_queues = gc->max_num_queues_vport;
 	/* Use MANA_DEF_NUM_QUEUES as default, still honoring the HW limit */
-	apc->num_queues = min(gc->max_num_queues, MANA_DEF_NUM_QUEUES);
+	apc->num_queues = min(gc->max_num_queues_vport, MANA_DEF_NUM_QUEUES);
 	apc->tx_queue_size = DEF_TX_BUFFERS_PER_QUEUE;
 	apc->rx_queue_size = DEF_RX_BUFFERS_PER_QUEUE;
 	apc->port_handle = INVALID_MANA_HANDLE;
@@ -3690,13 +3694,16 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
 
 	INIT_DELAYED_WORK(&ac->gf_stats_work, mana_gf_stats_work_handler);
 
-	err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION, MANA_MINOR_VERSION,
-				    MANA_MICRO_VERSION, &num_ports, &bm_hostmode);
+	err = mana_gd_query_device_cfg(gc, MANA_MAJOR_VERSION, MANA_MINOR_VERSION,
+				       MANA_MICRO_VERSION, &num_ports, &bm_hostmode);
 	if (err)
 		goto out;
 
 	ac->bm_hostmode = bm_hostmode;
 
+	debugfs_create_u16("adapter-MTU", 0400,
+			   gc->mana_pci_debugfs, &gc->adapter_mtu);
+
 	if (!resuming) {
 		ac->num_ports = num_ports;
 	} else {
include/net/mana/gdma.h +12 -1
index 6d836060976a..9c05b1e15c3e 100644
@@ -399,8 +399,10 @@ struct gdma_context {
 	struct device		*dev;
 	struct dentry		*mana_pci_debugfs;
 
-	/* Per-vPort max number of queues */
+	/* Hardware max number of queues */
 	unsigned int		max_num_queues;
+	/* Per-vPort max number of queues */
+	unsigned int		max_num_queues_vport;
 	unsigned int		max_num_msix;
 	unsigned int		num_msix_usable;
 	struct xarray		irq_contexts;
@@ -446,6 +448,12 @@ struct gdma_context {
 	struct workqueue_struct *service_wq;
 
 	unsigned long		flags;
+
+	/* Indicate if this device is sharing MSI for EQs on MANA */
+	bool msi_sharing;
+
+	/* Bitmap tracks where MSI is allocated when it is not shared for EQs */
+	unsigned long *msi_bitmap;
 };
 
 static inline bool mana_gd_is_mana(struct gdma_dev *gd)
@@ -1018,4 +1026,7 @@ int mana_gd_resume(struct pci_dev *pdev);
 
 bool mana_need_log(struct gdma_context *gc, int err);
 
+int mana_gd_query_device_cfg(struct gdma_context *gc, u32 proto_major_ver,
+			     u32 proto_minor_ver, u32 proto_micro_ver,
+			     u16 *max_num_vports, u8 *bm_hostmode);
 #endif /* _GDMA_H */
Signature · 1 line
2.43.0
maintainer
#5 lore raw

Re: [PATCH net-next v7 2/6] net: mana: Query device capabilities and configure MSI-X sharing for EQs

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: Query device capabilities and configure MSI-X sharing for EQs

This change queries device capabilities earlier during GDMA setup, renames mana_query_device_cfg() to mana_gd_query_device_cfg(), and adds a new gc->msi_sharing flag plus a gc->msi_bitmap to manage dedicated vs. shared MSI-X vectors for EQs across vPorts.

A few questions below, mostly about msi_sharing state across resume and about some wording in the commit message and in the code.

Quoted 15 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +3 -0
index f3316e929175..3aa96329f359 100644
@@ -149,6 +149,9 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
 	struct gdma_context *gc = pci_get_drvdata(pdev);
 	struct gdma_query_max_resources_resp resp = {};
 	struct gdma_general_req req = {};
+	unsigned int max_num_queues;
+	u8 bm_hostmode;
+	u16 num_ports;
 	int err;
 
 	mana_gd_init_req_hdr(&req.hdr, GDMA_QUERY_MAX_RESOURCES,

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_query_max_resources(struct pci_dev *pdev) · line 209

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +7 -0
@@ -197,6 +200,40 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
 	if (gc->max_num_queues == 0)
 		return -ENOSPC;
 
+	err = mana_gd_query_device_cfg(gc, MANA_MAJOR_VERSION, MANA_MINOR_VERSION,
+				       MANA_MICRO_VERSION, &num_ports, &bm_hostmode);
+	if (err)
+		return err;
+
+	if (!num_ports)
+		return -EINVAL;

[Low]
This is the first of two back-to-back mana_gd_query_device_cfg() calls per probe cycle. The second one happens in mana_probe() (further down in this same patch) and also fetches num_ports and bm_hostmode from firmware. The first caller here discards bm_hostmode; the second one uses both.

Would it be reasonable to cache num_ports and bm_hostmode on struct gdma_context during the first call and have mana_probe() read them from there, instead of doing a second HWC round-trip that must return the same values for the per-vPort math in mana_gd_query_max_resources() to match ac->num_ports in mana_probe()?

Quoted 5 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +5 -0
+
+	/*
+	 * Adjust gc->max_num_queues returned from the SOC to allow dedicated
+	 * MSIx for each vPort. Clamp to no less than MANA_DEF_NUM_QUEUES.
+	 */

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_query_max_resources(struct pci_dev *pdev) · line 224

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +10 -0
+	max_num_queues = (gc->num_msix_usable - 1) / num_ports;
+	max_num_queues = rounddown_pow_of_two(max(max_num_queues, 1U));
+	if (max_num_queues < MANA_DEF_NUM_QUEUES)
+		max_num_queues = MANA_DEF_NUM_QUEUES;
+
+	/*
+	 * Use dedicated MSIx for EQs whenever possible, use MSIx sharing for
+	 * Ethernet EQs when (max_num_queues * num_ports > num_msix_usable - 1)
+	 */
+	max_num_queues = min(gc->max_num_queues, max_num_queues);

[Low]
The commit message says "The number of queues per vPort is clamped to no less than MANA_DEF_NUM_QUEUES". After the clamp-up above, this min() with gc->max_num_queues can bring the result back below MANA_DEF_NUM_QUEUES whenever gc->max_num_queues is below MANA_DEF_NUM_QUEUES (which is 16).

gc->max_num_queues was previously clamped by num_online_cpus(), resp.max_eq/cq/sq/rq, and gc->num_msix_usable - 1, so a VM with fewer than 16 online CPUs will end up with gc->max_num_queues_vport below MANA_DEF_NUM_QUEUES in the non-sharing branch.

Could the commit message be reworded to describe the actual behaviour (the per-vPort count is clamped towards MANA_DEF_NUM_QUEUES but never exceeds the hardware maximum)?

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_query_max_resources(struct pci_dev *pdev) · line 226

Quoted 2 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +2 -0
+	if (max_num_queues * num_ports > gc->num_msix_usable - 1)
+		gc->msi_sharing = true;

[Medium]
Is gc->msi_sharing ever cleared?

Walking the write sites for this flag in the series: it is set to true unconditionally in the non-dyn branch of mana_gd_setup_hwc_irqs() (see the hunk below), and conditionally true here. I could not find any path that writes false, and neither mana_gd_remove_irqs() nor mana_gd_cleanup() reset it.

The same gdma_context allocated in mana_gd_probe() survives a suspend / resume cycle via mana_gd_suspend() -> mana_gd_cleanup() -> mana_gd_resume() -> mana_gd_setup(). In the dynamic MSI-X path (pci_msix_can_alloc_dyn() true, i.e. the PF), gc->num_msix_usable is recomputed as min(resp.max_msix, num_online_cpus() + 1), so it can legitimately grow across suspend/resume if CPUs came online while the guest was suspended.

If the first probe set msi_sharing=true because MSI-X was tight, but on resume there are enough vectors for dedicated allocation, does the flag stay stuck at true? If so:

- gc->max_num_queues_vport is computed via the sharing branch

    min(gc->num_msix_usable - 1, gc->max_num_queues) rather than the
    dedicated max_num_queues value;
  - the if (!gc->msi_sharing) branch at the end of mana_gd_setup()
    is skipped and gc->msi_bitmap stays NULL;

- later patches in this series that consult !gc->msi_sharing as the "use bitmap" predicate keep taking the shared path.

Would it be appropriate to reset gc->msi_sharing = false at the top of mana_gd_query_max_resources() (and/or in mana_gd_setup_hwc_irqs()'s other branch) before the conditions that may set it to true are evaluated? This would also line up with the commit message's claim that "MSI-X sharing among vPorts is disabled by default and is only enabled when there are not enough MSI-X vectors for dedicated allocation."

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_query_max_resources(struct pci_dev *pdev) · line 235

Quoted 9 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +9 -0
+
+	/* If MSI is shared, use max allowed value */
+	if (gc->msi_sharing)
+		gc->max_num_queues_vport = min(gc->num_msix_usable - 1, gc->max_num_queues);
+	else
+		gc->max_num_queues_vport = max_num_queues;
+
+	dev_info(gc->dev, "MSI sharing mode %d max queues %d\n",
+		 gc->msi_sharing, gc->max_num_queues);

[Low, Low]
Two small wording questions on this block:

The comment above reads "Adjust gc->max_num_queues returned from the SOC to allow dedicated MSIx for each vPort", but the code never modifies gc->max_num_queues; it updates a local and assigns to gc->max_num_queues_vport. Should the comment say gc->max_num_queues_vport instead?

For the dev_info, should the printed field be gc->max_num_queues_vport rather than gc->max_num_queues? As written, the log always shows the hardware maximum that was already decided earlier in the function, not the per-vPort value that the preceding logic just chose, so the number never reflects the effect of toggling msi_sharing.

Quoted 11 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +2 -0
+
 	return 0;
 }
 
@@ -1859,6 +1896,7 @@ static int mana_gd_setup_hwc_irqs(struct pci_dev *pdev)
 		/* Need 1 interrupt for HWC */
 		max_irqs = min(num_online_cpus(), MANA_MAX_NUM_QUEUES) + 1;
 		min_irqs = 2;
+		gc->msi_sharing = true;
 	}
 

[ ... ]

author
#6 lore raw

PATCH v7 3/6 net: mana: Introduce GIC context with refcounting for interrupt management

In reply to Long Li

To allow Ethernet EQs to use dedicated or shared MSI-X vectors and RDMA EQs to share the same MSI-X, introduce a GIC (GDMA IRQ Context) with reference counting. This allows the driver to create an interrupt context on an assigned or unassigned MSI-X vector and share it across multiple EQ consumers.

---
 .../net/ethernet/microsoft/mana/gdma_main.c   | 159 ++++++++++++++++++
 include/net/mana/gdma.h                       |  12 ++
 2 files changed, 171 insertions(+)
drivers/net/ethernet/microsoft/mana/gdma_main.c +159 -0
index 3aa96329f359..5930ab817056 100644
@@ -1615,6 +1615,164 @@ static irqreturn_t mana_gd_intr(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+void mana_gd_put_gic(struct gdma_context *gc, bool use_msi_bitmap, int msi)
+{
+	struct pci_dev *dev = to_pci_dev(gc->dev);
+	struct msi_map irq_map;
+	struct gdma_irq_context *gic;
+	int irq;
+
+	mutex_lock(&gc->gic_mutex);
+
+	gic = xa_load(&gc->irq_contexts, msi);
+	if (WARN_ON(!gic)) {
+		mutex_unlock(&gc->gic_mutex);
+		return;
+	}
+
+	if (use_msi_bitmap)
+		gic->bitmap_refs--;
+
+	if (use_msi_bitmap && gic->bitmap_refs == 0)
+		clear_bit(msi, gc->msi_bitmap);
+
+	if (!refcount_dec_and_test(&gic->refcount))
+		goto out;
+
+	irq = gic->irq;
+
+	irq_update_affinity_hint(irq, NULL);
+	free_irq(irq, gic);
+
+	if (gic->dyn_msix) {
+		irq_map.virq = irq;
+		irq_map.index = msi;
+		pci_msix_free_irq(dev, irq_map);
+	}
+
+	xa_erase(&gc->irq_contexts, msi);
+	kfree(gic);
+
+out:
+	mutex_unlock(&gc->gic_mutex);
+}
+EXPORT_SYMBOL_NS(mana_gd_put_gic, "NET_MANA");
+
+/*
+ * Get a GIC (GDMA IRQ Context) on a MSI vector
+ * a MSI can be shared between different EQs, this function supports setting
+ * up separate MSIs using a bitmap, or directly using the MSI index
+ *
+ * @use_msi_bitmap:
+ * True if MSI is assigned by this function on available slots from bitmap.
+ * False if MSI is passed from *msi_requested
+ */
+struct gdma_irq_context *mana_gd_get_gic(struct gdma_context *gc,
+					 bool use_msi_bitmap,
+					 int *msi_requested)
+{
+	struct gdma_irq_context *gic;
+	struct pci_dev *dev = to_pci_dev(gc->dev);
+	struct msi_map irq_map = { };
+	int irq;
+	int msi;
+	int err;
+
+	mutex_lock(&gc->gic_mutex);
+
+	if (use_msi_bitmap) {
+		msi = find_first_zero_bit(gc->msi_bitmap, gc->num_msix_usable);
+		if (msi >= gc->num_msix_usable) {
+			dev_err(gc->dev, "No free MSI vectors available\n");
+			gic = NULL;
+			goto out;
+		}
+		*msi_requested = msi;
+	} else {
+		msi = *msi_requested;
+	}
+
+	gic = xa_load(&gc->irq_contexts, msi);
+	if (gic) {
+		refcount_inc(&gic->refcount);
+		if (use_msi_bitmap) {
+			gic->bitmap_refs++;
+			set_bit(msi, gc->msi_bitmap);
+		}
+		goto out;
+	}
+
+	irq = pci_irq_vector(dev, msi);
+	if (irq == -EINVAL) {
+		irq_map = pci_msix_alloc_irq_at(dev, msi, NULL);
+		if (!irq_map.virq) {
+			err = irq_map.index;
+			dev_err(gc->dev,
+				"Failed to alloc irq_map msi %d err %d\n",
+				msi, err);
+			gic = NULL;
+			goto out;
+		}
+		irq = irq_map.virq;
+		msi = irq_map.index;
+	}
+
+	gic = kzalloc(sizeof(*gic), GFP_KERNEL);
+	if (!gic) {
+		if (irq_map.virq)
+			pci_msix_free_irq(dev, irq_map);
+		goto out;
+	}
+
+	gic->handler = mana_gd_process_eq_events;
+	gic->msi = msi;
+	gic->irq = irq;
+	INIT_LIST_HEAD(&gic->eq_list);
+	spin_lock_init(&gic->lock);
+
+	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));
+
+	err = request_irq(irq, mana_gd_intr, 0, gic->name, gic);
+	if (err) {
+		dev_err(gc->dev, "Failed to request irq %d %s\n",
+			irq, gic->name);
+		kfree(gic);
+		gic = NULL;
+		if (irq_map.virq)
+			pci_msix_free_irq(dev, irq_map);
+		goto out;
+	}
+
+	gic->dyn_msix = !!irq_map.virq;
+	refcount_set(&gic->refcount, 1);
+	gic->bitmap_refs = use_msi_bitmap ? 1 : 0;
+
+	err = xa_err(xa_store(&gc->irq_contexts, msi, gic, GFP_KERNEL));
+	if (err) {
+		dev_err(gc->dev, "Failed to store irq context for msi %d: %d\n",
+			msi, err);
+		free_irq(irq, gic);
+		kfree(gic);
+		gic = NULL;
+		if (irq_map.virq)
+			pci_msix_free_irq(dev, irq_map);
+		goto out;
+	}
+
+	if (use_msi_bitmap)
+		set_bit(msi, gc->msi_bitmap);
+
+out:
+	mutex_unlock(&gc->gic_mutex);
+	return gic;
+}
+EXPORT_SYMBOL_NS(mana_gd_get_gic, "NET_MANA");
+
 int mana_gd_alloc_res_map(u32 res_avail, struct gdma_resource *r)
 {
 	r->map = bitmap_zalloc(res_avail, GFP_KERNEL);
@@ -2104,6 +2262,7 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto release_region;
 
 	mutex_init(&gc->eq_test_event_mutex);
+	mutex_init(&gc->gic_mutex);
 	pci_set_drvdata(pdev, gc);
 	gc->bar0_pa = pci_resource_start(pdev, 0);
 	gc->bar0_size = pci_resource_len(pdev, 0);
include/net/mana/gdma.h +12 -0
index 9c05b1e15c3e..fbe3c1427b45 100644
@@ -388,6 +388,11 @@ struct gdma_irq_context {
 	spinlock_t lock;
 	struct list_head eq_list;
 	char name[MANA_IRQ_NAME_SZ];
+	unsigned int msi;
+	unsigned int irq;
+	refcount_t refcount;
+	unsigned int bitmap_refs;
+	bool dyn_msix;
 };
 
 enum gdma_context_flags {
@@ -449,6 +454,9 @@ struct gdma_context {
 
 	unsigned long		flags;
 
+	/* Protect access to GIC context */
+	struct mutex		gic_mutex;
+
 	/* Indicate if this device is sharing MSI for EQs on MANA */
 	bool msi_sharing;
 
@@ -1026,6 +1034,10 @@ int mana_gd_resume(struct pci_dev *pdev);
 
 bool mana_need_log(struct gdma_context *gc, int err);
 
+struct gdma_irq_context *mana_gd_get_gic(struct gdma_context *gc,
+					 bool use_msi_bitmap,
+					 int *msi_requested);
+void mana_gd_put_gic(struct gdma_context *gc, bool use_msi_bitmap, int msi);
 int mana_gd_query_device_cfg(struct gdma_context *gc, u32 proto_major_ver,
 			     u32 proto_minor_ver, u32 proto_micro_ver,
 			     u16 *max_num_vports, u8 *bm_hostmode);
Signature · 1 line
2.43.0
author
#7 lore raw

PATCH v7 4/6 net: mana: Use GIC functions to allocate global EQs

In reply to Long Li

Replace the GDMA global interrupt setup code with the new GIC allocation and release functions for managing interrupt contexts.

---
 .../net/ethernet/microsoft/mana/gdma_main.c   | 80 +++----------------
 1 file changed, 10 insertions(+), 70 deletions(-)
drivers/net/ethernet/microsoft/mana/gdma_main.c +10 -70
index 5930ab817056..850afdbd9c4e 100644
@@ -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));
-
-		/* 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);
 	}
 	kfree(irqs);
 	return err;
@@ -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));
-
-		irqs[i] = pci_irq_vector(pdev, i);
-		if (irqs[i] < 0) {
-			err = irqs[i];
-			goto free_current_gic;
-		}
-
-		err = request_irq(irqs[i], mana_gd_intr, 0, gic->name, gic);
-		if (err)
-			goto free_current_gic;
-
-		xa_store(&gc->irq_contexts, i, gic, GFP_KERNEL);
+		irqs[i] = gic->irq;
 	}
 
 	/* If number of IRQ is one extra than number of online CPUs,
@@ -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);
 	}
 
 	kfree(start_irqs);
@@ -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);
 	}
 
 	pci_free_irq_vectors(pdev);
Signature · 1 line
2.43.0
maintainer
#8 lore raw

Re: [PATCH net-next v7 4/6] net: mana: Use GIC functions to allocate global EQs

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.

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -1
index 5930ab817056..850afdbd9c4e 100644
@@ -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);

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_setup_dyn_irqs(struct pci_dev *pdev, int nvec) · line 1901

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +0 -6
 		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?

Quoted 26 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -14
-
-		/* 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);

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_setup_dyn_irqs(struct pci_dev *pdev, int nvec) · line 1924

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -7
-		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.

Quoted 11 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -1
@@ -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;
 		}
 

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_setup_irqs(struct pci_dev *pdev, int nvec) · line 1984

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +0 -10
-		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?

[ ... ]

Quoted 9 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +0 -2
@@ -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);

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_setup_irqs(struct pci_dev *pdev, int nvec) · line 1984

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -7
-		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?

Quoted 19 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -7
@@ -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;

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_setup_remaining_irqs(struct pci_dev *pdev) · line 2072

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +3 -3
 
 		/* 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?

maintainer
#9 lore raw

Re: [PATCH net-next v7 4/6] net: mana: Use GIC functions to allocate global EQs

In reply to Long Li

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.

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -1
index 81bf379283663..72a67b0b2b5d4 100644
@@ -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);

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_setup_dyn_irqs(struct pci_dev *pdev, int nvec) · line 1931

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +0 -6
 		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.

[ ... ]

Quoted 21 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -9
@@ -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;

[ ... ]

Quoted 19 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -7
@@ -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;

drivers/net/ethernet/microsoft/mana/gdma_main.c static int mana_gd_setup_remaining_irqs(struct pci_dev *pdev) · line 2102

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +3 -3
 
 		/* 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.

author
#10 lore raw

PATCH v7 5/6 net: mana: Allocate interrupt context for each EQ when creating vPort

In reply to Long Li

Use GIC functions to create a dedicated interrupt context or acquire a shared interrupt context for each EQ when setting up a vPort.

---
 .../net/ethernet/microsoft/mana/gdma_main.c   |  2 +-
 drivers/net/ethernet/microsoft/mana/mana_en.c | 20 ++++++++++++++++++-
 include/net/mana/gdma.h                       |  1 +
 3 files changed, 21 insertions(+), 2 deletions(-)
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -1
index 850afdbd9c4e..54127c385240 100644
@@ -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();
 }
 
@@ -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;
 }
 
drivers/net/ethernet/microsoft/mana/mana_en.c +19 -1
index 2f106d6f5be4..b3684fa3eb4a 100644
@@ -1625,6 +1625,7 @@ void mana_destroy_eq(struct mana_port_context *apc)
 	struct gdma_context *gc = ac->gdma_dev->gdma_context;
 	struct gdma_queue *eq;
 	int i;
+	unsigned int msi;
 
 	if (!apc->eqs)
 		return;
@@ -1637,7 +1638,9 @@ void mana_destroy_eq(struct mana_port_context *apc)
 		if (!eq)
 			continue;
 
+		msi = eq->eq.msix_index;
 		mana_gd_destroy_queue(gc, eq);
+		mana_gd_put_gic(gc, !gc->msi_sharing, msi);
 	}
 
 	kfree(apc->eqs);
@@ -1654,6 +1657,7 @@ static void mana_create_eq_debugfs(struct mana_port_context *apc, int i)
 	eq.mana_eq_debugfs = debugfs_create_dir(eqnum, apc->mana_eqs_debugfs);
 	debugfs_create_u32("head", 0400, eq.mana_eq_debugfs, &eq.eq->head);
 	debugfs_create_u32("tail", 0400, eq.mana_eq_debugfs, &eq.eq->tail);
+	debugfs_create_u32("irq", 0400, eq.mana_eq_debugfs, &eq.eq->eq.irq);
 	debugfs_create_file("eq_dump", 0400, eq.mana_eq_debugfs, eq.eq, &mana_dbg_q_fops);
 }
 
@@ -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;
+
 		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);
 	}
 
include/net/mana/gdma.h +1 -0
index fbe3c1427b45..6c138cc77407 100644
@@ -342,6 +342,7 @@ struct gdma_queue {
 			void *context;
 
 			unsigned int msix_index;
+			unsigned int irq;
 
 			u32 log2_throttle_limit;
 		} eq;
Signature · 1 line
2.43.0
maintainer
#11 lore raw

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

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?

drivers/net/ethernet/microsoft/mana/gdma_main.c static void mana_gd_deregister_irq(struct gdma_queue *queue) · line 858

Quoted 11 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +0 -1
index 850afdbd9c4e..54127c385240 100644
@@ -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.

Quoted 12 lines
drivers/net/ethernet/microsoft/mana/gdma_main.c +1 -0
@@ -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;
 }
 
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index 2f106d6f5be4..b3684fa3eb4a 100644

[ ... ]

Quoted 14 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +2 -1
@@ -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;

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_create_eq(struct mana_port_context *apc) · line 1699

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +10 -0
+		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)?

drivers/net/ethernet/microsoft/mana/mana_en.c int mana_create_eq(struct mana_port_context *apc) · line 1709

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +3 -0
+
 		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?

Quoted 12 lines
include/net/mana/gdma.h +1 -0
index fbe3c1427b45..6c138cc77407 100644
@@ -342,6 +342,7 @@ struct gdma_queue {
 			void *context;
 
 			unsigned int msix_index;
+			unsigned int irq;
 
 			u32 log2_throttle_limit;
 		} eq;
author
#12 lore raw

PATCH v7 6/6 RDMA/mana_ib: Allocate interrupt contexts on EQs

In reply to Long Li

Use the GIC functions to allocate interrupt contexts for RDMA EQs. These interrupt contexts may be shared with Ethernet EQs when MSI-X vectors are limited.

The driver now supports allocating dedicated MSI-X for each EQ. Indicate this capability through driver capability bits.

---
 drivers/infiniband/hw/mana/main.c | 41 +++++++++++++++++++++++++------
 include/net/mana/gdma.h           |  7 ++++--
 2 files changed, 38 insertions(+), 10 deletions(-)
drivers/infiniband/hw/mana/main.c +33 -8
index 8000ab6e8beb..0dc47540fb6f 100644
@@ -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);
+	if (!gic)
+		return -ENOMEM;
+	spec.eq.msix_index = msi;
 
 	err = mana_gd_create_mana_eq(mdev->gdma_dev, &spec, &mdev->fatal_err_eq);
-	if (err)
+	if (err) {
+		mana_gd_put_gic(gc, false, 0);
 		return err;
+	}
 
 	mdev->eqs = kzalloc_objs(struct gdma_queue *,
 				 mdev->ib_dev.num_comp_vectors);
@@ -771,32 +779,49 @@ int mana_ib_create_eqs(struct mana_ib_dev *mdev)
 	}
 	spec.eq.callback = NULL;
 	for (i = 0; i < mdev->ib_dev.num_comp_vectors; i++) {
-		spec.eq.msix_index = (i + 1) % gc->num_msix_usable;
+		msi = (i + 1) % gc->num_msix_usable;
+
+		gic = mana_gd_get_gic(gc, false, &msi);
+		if (!gic) {
+			err = -ENOMEM;
+			goto destroy_eqs;
+		}
+		spec.eq.msix_index = msi;
+
 		err = mana_gd_create_mana_eq(mdev->gdma_dev, &spec, &mdev->eqs[i]);
-		if (err)
+		if (err) {
+			mana_gd_put_gic(gc, false, msi);
 			goto destroy_eqs;
+		}
 	}
 
 	return 0;
 
 destroy_eqs:
-	while (i-- > 0)
+	while (i-- > 0) {
 		mana_gd_destroy_queue(gc, mdev->eqs[i]);
+		mana_gd_put_gic(gc, false, (i + 1) % gc->num_msix_usable);
+	}
 	kfree(mdev->eqs);
 destroy_fatal_eq:
 	mana_gd_destroy_queue(gc, mdev->fatal_err_eq);
+	mana_gd_put_gic(gc, false, 0);
 	return err;
 }
 
 void mana_ib_destroy_eqs(struct mana_ib_dev *mdev)
 {
 	struct gdma_context *gc = mdev_to_gc(mdev);
-	int i;
+	int i, msi;
 
 	mana_gd_destroy_queue(gc, mdev->fatal_err_eq);
+	mana_gd_put_gic(gc, false, 0);
 
-	for (i = 0; i < mdev->ib_dev.num_comp_vectors; i++)
+	for (i = 0; i < mdev->ib_dev.num_comp_vectors; i++) {
 		mana_gd_destroy_queue(gc, mdev->eqs[i]);
+		msi = (i + 1) % gc->num_msix_usable;
+		mana_gd_put_gic(gc, false, msi);
+	}
 
 	kfree(mdev->eqs);
 }
include/net/mana/gdma.h +5 -2
index 6c138cc77407..d84e474309a3 100644
@@ -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)
@@ -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
 
Signature · 1 line
2.43.0
maintainer
#13 lore raw

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

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.

Quoted 15 lines
drivers/infiniband/hw/mana/main.c +2 -1
index 8000ab6e8beb..0dc47540fb6f 100644
@@ -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;

drivers/infiniband/hw/mana/main.c int mana_ib_create_eqs(struct mana_ib_dev *mdev) · line 763

Quoted 7 lines
drivers/infiniband/hw/mana/main.c +3 -1
@@ -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?

[ ... ]

Quoted 13 lines
include/net/mana/gdma.h +1 -0
index 6c138cc77407..d84e474309a3 100644
@@ -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)

include/net/mana/gdma.h enum { · line 639

Quoted 9 lines
include/net/mana/gdma.h +2 -1
@@ -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?

Quoted 9 lines
include/net/mana/gdma.h +2 -1
@@ -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
maintainer
#14 lore raw
In reply to Long Li
Quoted 4 lines · On Thu, 7 May 2026 12:12:31 -0700 Long Li wrote:

This series adds per-vPort Event Queue (EQ) allocation and MSI-X interrupt management for the MANA driver. Previously, all vPorts shared a single set of EQs. This change enables dedicated EQs per vPort with support for both dedicated and shared MSI-X vector allocation modes.

Once all the AI review comments are address / only false positives remain - could you pop these patches on a branch and add PR info to the cover letter so that both RDMA and netdev can pull this?

author
#15 lore raw
Quoted 10 lines · On Thu, 7 May 2026 12:12:31 -0700 Long Li wrote:
Quoted 4 lines

This series adds per-vPort Event Queue (EQ) allocation and MSI-X interrupt management for the MANA driver. Previously, all vPorts shared a single set of EQs. This change enables dedicated EQs per vPort with support for both dedicated and shared MSI-X vector allocation

modes.

Once all the AI review comments are address / only false positives remain - could you pop these patches on a branch and add PR info to the cover letter so that both RDMA and netdev can pull this?

I have sent v9 with PR info in the cover letter.

Thanks,
Long