[PATCH net v4] net: mana: Add support to process throttled EQEs

[PATCH net v4] net: mana: Add support to process throttled EQEs

[Sahil Chandna]

When an event queue nears full, the hardware coalesces the per-CQ
GDMA_EQE_COMPLETION notifications into a single throttle EQE (type 4)
that carries no cq_id. The driver does not handle this event type, so
it falls through to the default case and is dropped. Any CQ whose
completion notification was replaced by the throttle EQE can remain
unserviced, causing its queue to stall.

This was observed internally as a TX stall:

  ens1: NETDEV WATCHDOG: CPU: 45: transmit queue 1 timed out 15133 ms
  mana 7870:00:00.0: HWC: Request timed out: 60000 ms
  mana 7870:00:00.0: Gf stats wk handler: gf stats query timed out.
  ens1: NETDEV WATCHDOG: CPU: 20: transmit queue 0 timed out 15859 ms
  ens1: NETDEV WATCHDOG: CPU: 15: transmit queue 0 timed out 31731 ms
  ens1: NETDEV WATCHDOG: CPU: 21: transmit queue 0 timed out 47603 ms
  ens1: NETDEV WATCHDOG: CPU: 42: transmit queue 0 timed out 62451 ms

Handle the throttle EQE:

- Record up to two child CQs per EQ when a CQ is created with a parent
  EQ. A mana_en EQ serves one TX CQ and one RX CQ, and the HWC EQ
  serves the HWC CQ.
- On a throttle EQE, call the callback of each child CQ that is still
  in cq_table[].
- Before a child CQ is freed, unlink it from its parent EQ and from
  cq_table[] in mana_gd_detach_cq(), then wait for an RCU
  grace period, because mana_gd_intr() reads both under
  rcu_read_lock(). mana_destroy_txq() and mana_destroy_rxq() detach
  the CQ before deleting its NAPI, so the synchronize_net() that NAPI
  deletion already needs also covers the CQ. mana_hwc_destroy_channel()
  detaches the HWC CQ and waits for a grace period before freeing the
  HWC queues.
- Advertise the capability bit for throttled EQE.
- Add a throttle eqe counter per queue.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Cc: stable@vger.kernel.org
Signed-off-by: Sahil Chandna <sahilchandna@linux.microsoft.com>
---
Changes in v4:
- Resolve additional per cq grace period and hwc wait.
- Add throttle count per queue instead of accumulated count.
Link to v3: https://lore.kernel.org/all/20261022145428.323576-1-sahilchandna@linux.microsoft.com/

Changes in v3:
- Resolve race for cq in mana_gd_destroy_cq and irq handler using cq.
- Rebase to net branch
Link to v2: https://lore.kernel.org/all/20261014134304.4177657-1-sahilchandna@linux.microsoft.com/

Changes in v2:
- Use WRITE_ONCE() when clearing max_num_cqs and add RCU
  grace period wait during HWC teardown.
- Use synchronize_net() during CQ destroy only when the CQ was
  attached to its parent EQ.
- Use WRITE_ONCE() and READ_ONCE() for the throttle-event counter.
- Add Fixes tag and net instead of net-next.
- Fix commit message to describe capability bit and counter.
Link to v1: https://lore.kernel.org/all/20261008083315.306076-1-sahilchandna@linux.microsoft.com/
 .../net/ethernet/microsoft/mana/gdma_main.c   | 88 +++++++++++++++++--
 .../net/ethernet/microsoft/mana/hw_channel.c  |  9 +-
 drivers/net/ethernet/microsoft/mana/mana_en.c | 22 ++++-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 19 +++-
 include/net/mana/gdma.h                       | 19 +++-
 include/net/mana/mana.h                       |  1 +
 6 files changed, 143 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 8c915e008784..ab942d5bfe05 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -757,6 +757,27 @@ void mana_gd_free_service_wq(struct gdma_context *gc)
 	gc->service_wq = NULL;
 }

+static void mana_gd_schedule_eq_cqs(struct gdma_queue *eq)
+{
+	struct gdma_context *gc = eq->gdma_dev->gdma_context;
+	struct gdma_queue *cq;
+	u8 i;
+
+	for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) {
+		cq = rcu_dereference(eq->eq.child_cq[i]);
+
+		if (!cq)
+			continue;
+
+		if (cq->id >= READ_ONCE(gc->max_num_cqs) ||
+		    !gc->cq_table || gc->cq_table[cq->id] != cq)
+			continue;
+
+		if (cq->cq.callback)
+			cq->cq.callback(cq->cq.context, cq);
+	}
+}
+
 static void mana_gd_process_eqe(struct gdma_queue *eq)
 {
 	u32 head = eq->head % (eq->queue_size / GDMA_EQE_SIZE);
@@ -787,6 +808,14 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)

 		break;

+	case GDMA_EQE_THROTTLE:
+		u64_stats_update_begin(&eq->eq.stats.syncp);
+		u64_stats_inc(&eq->eq.stats.throttle_count);
+		u64_stats_update_end(&eq->eq.stats.syncp);
+
+		mana_gd_schedule_eq_cqs(eq);
+		break;
+
 	case GDMA_EQE_TEST_EVENT:
 		gc->test_event_eq_id = eq->id;
 		complete(&gc->eq_test_event);
@@ -1032,6 +1061,8 @@ static int mana_gd_create_eq(struct gdma_dev *gd,
 	queue->eq.msix_index = INVALID_PCI_MSIX_INDEX;
 	queue->id = INVALID_QUEUE_ID;

+	u64_stats_init(&queue->eq.stats.syncp);
+
 	log2_num_entries = ilog2(queue->queue_size / GDMA_EQE_SIZE);

 	if (spec->eq.log2_throttle_limit > log2_num_entries) {
@@ -1073,25 +1104,66 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
 			      struct gdma_queue *queue)
 {
 	u32 log2_num_entries = ilog2(spec->queue_size / GDMA_CQE_SIZE);
+	struct gdma_queue *parent;
+	u8 i;

 	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
-	queue->cq.parent = spec->cq.parent_eq;
+	parent = spec->cq.parent_eq;
+	queue->cq.parent = parent;
 	queue->cq.context = spec->cq.context;
 	queue->cq.callback = spec->cq.callback;
+
+	if (!parent)
+		return;
+
+	/* For throttled EQE store the child CQ */
+	for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) {
+		if (!rcu_access_pointer(parent->eq.child_cq[i])) {
+			rcu_assign_pointer(parent->eq.child_cq[i], queue);
+			return;
+		}
+	}
+
+	WARN_ONCE(1, "mana: EQ %u has no free child CQ slot\n", parent->id);
 }

-static void mana_gd_destroy_cq(struct gdma_context *gc,
-			       struct gdma_queue *queue)
+/* Unlink queue from its parent EQ's child_cq[] and from gc->cq_table[].
+ *
+ * Return: true if queue was in the parent EQ's child_cq[]. In that case the
+ * caller must wait for an RCU grace period before freeing queue, because
+ * mana_gd_intr() may still be handling a throttle EQE for it.
+ */
+bool mana_gd_detach_cq(struct gdma_context *gc, struct gdma_queue *queue)
 {
+	struct gdma_queue *eq = queue->cq.parent;
+	bool attached = false;
 	u32 id = queue->id;
+	u8 i;
+
+	if (eq) {
+		for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) {
+			if (rcu_access_pointer(eq->eq.child_cq[i]) == queue) {
+				RCU_INIT_POINTER(eq->eq.child_cq[i], NULL);
+				attached = true;
+			}
+		}
+	}

-	if (id >= gc->max_num_cqs)
-		return;
+	/* Check if CQ might have been detached already, only clear if
+	 * it still points to same queue .
+	 */
+	if (id < gc->max_num_cqs && READ_ONCE(gc->cq_table[id]) == queue)
+		WRITE_ONCE(gc->cq_table[id], NULL);

-	if (!gc->cq_table[id])
-		return;
+	return attached;
+}

-	gc->cq_table[id] = NULL;
+static void mana_gd_destroy_cq(struct gdma_context *gc,
+			       struct gdma_queue *queue)
+{
+	/* Wait for irq reader holding the cq */
+	if (mana_gd_detach_cq(gc, queue))
+		synchronize_net();
 }

 int mana_gd_create_hwc_queue(struct gdma_dev *gd,
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 3bca4b683134..fb9f38f65911 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -364,7 +364,7 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)

 	completions = hwc_cq->comp_buf;
 	comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);
-	WARN_ON_ONCE(comp_read <= 0 || comp_read > hwc_cq->queue_depth);
+	WARN_ON_ONCE(comp_read < 0 || comp_read > hwc_cq->queue_depth);

 	for (i = 0; i < comp_read; ++i) {
 		comp_data = *(struct hwc_rx_oob *)completions[i].cqe_data;
@@ -820,9 +820,14 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	 */
 	if (gc->max_num_cqs > 0) {
 		mana_smc_teardown_hwc(&gc->shm_channel, false);
-		gc->max_num_cqs = 0;
+		WRITE_ONCE(gc->max_num_cqs, 0);
 	}

+	if (hwc->cq)
+		mana_gd_detach_cq(gc, hwc->cq->gdma_cq);
+
+	synchronize_rcu();
+
 	if (hwc->txq)
 		mana_hwc_destroy_wq(hwc, hwc->txq);

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 6b7c027e497e..e6d494df22e4 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2703,6 +2703,7 @@ static void mana_deinit_txq(struct mana_port_context *apc, struct mana_txq *txq)

 static void mana_destroy_txq(struct mana_port_context *apc)
 {
+	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
 	struct napi_struct *napi;
 	int i;

@@ -2721,8 +2722,6 @@ static void mana_destroy_txq(struct mana_port_context *apc)
 			napi_synchronize(napi);
 			napi_disable_locked(napi);
 			cancel_work_sync(&apc->tx_qp[i]->tx_cq.dim.work);
-			netif_napi_del_locked(napi);
-			apc->tx_qp[i]->txq.napi_initialized = false;
 		}

 		/* Forget the SKBs freed without being completed after a drain
@@ -2735,6 +2734,18 @@ static void mana_destroy_txq(struct mana_port_context *apc)
 		if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE)
 			mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i]->tx_object);

+		if (apc->tx_qp[i]->txq.napi_initialized) {
+			/* Detach the CQ only after the WQ object is destroyed,
+			 * so a completion EQE that arrives before then still
+			 * finds the CQ in cq_table[] rather than hitting the
+			 * WARN in mana_gd_process_eqe(). One grace period then
+			 * covers both mana_gd_intr() and NAPI users.
+			 */
+			mana_gd_detach_cq(gc, apc->tx_qp[i]->tx_cq.gdma_cq);
+			netif_napi_del_locked(napi);
+			apc->tx_qp[i]->txq.napi_initialized = false;
+		}
+
 		mana_deinit_cq(apc, &apc->tx_qp[i]->tx_cq);

 		mana_deinit_txq(apc, &apc->tx_qp[i]->txq);
@@ -2941,7 +2952,6 @@ static void mana_destroy_rxq(struct mana_port_context *apc,

 		napi_disable_locked(napi);
 		cancel_work_sync(&rxq->rx_cq.dim.work);
-		netif_napi_del_locked(napi);
 	}

 	if (xdp_rxq_info_is_reg(&rxq->xdp_rxq))
@@ -2950,6 +2960,12 @@ static void mana_destroy_rxq(struct mana_port_context *apc,
 	if (rxq->rxobj != INVALID_MANA_HANDLE)
 		mana_destroy_wq_obj(apc, GDMA_RQ, rxq->rxobj);

+	if (napi_initialized) {
+		/* See the comment in mana_destroy_txq() */
+		mana_gd_detach_cq(gc, rxq->rx_cq.gdma_cq);
+		netif_napi_del_locked(napi);
+	}
+
 	mana_deinit_cq(apc, &rxq->rx_cq);

 	if (rxq->xdp_save_va)
diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index 33db79569d3b..a91eb12badca 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -147,7 +147,8 @@ static int mana_get_sset_count(struct net_device *ndev, int stringset)
 		return ARRAY_SIZE(mana_eth_stats) +
 		       ARRAY_SIZE(mana_phy_stats) +
 		       ARRAY_SIZE(mana_hc_stats)  +
-		       num_queues * (MANA_STATS_RX_COUNT + MANA_STATS_TX_COUNT);
+		       num_queues * (MANA_STATS_RX_COUNT + MANA_STATS_TX_COUNT +
+				       MANA_STATS_EQ_COUNT);

 	case ETH_SS_PRIV_FLAGS:
 		return MANA_PRIV_FLAG_MAX;
@@ -198,6 +199,9 @@ static void mana_get_strings_stats(struct mana_port_context *apc, u8 **data)
 		ethtool_sprintf(data, "tx_%d_csum_partial", i);
 		ethtool_sprintf(data, "tx_%d_mana_map_err", i);
 	}
+
+	for (i = 0; i < num_queues; i++)
+		ethtool_sprintf(data, "eq_%d_throttle_events", i);
 }

 static void mana_get_strings_priv_flags(u8 **data)
@@ -234,6 +238,7 @@ static void mana_get_ethtool_stats(struct net_device *ndev,
 	void *phy_stats = &apc->phy_stats;
 	struct mana_stats_rx *rx_stats;
 	struct mana_stats_tx *tx_stats;
+	struct gdma_eq_stats *eq_stats;
 	unsigned int start;
 	u64 packets, bytes;
 	u64 xdp_redirect;
@@ -251,6 +256,7 @@ static void mana_get_ethtool_stats(struct net_device *ndev,
 	u64 csum_partial;
 	u64 mana_map_err;
 	int q, i = 0, j;
+	u64 throttled;

 	if (!apc->port_is_up)
 		return;
@@ -325,6 +331,17 @@ static void mana_get_ethtool_stats(struct net_device *ndev,
 		data[i++] = csum_partial;
 		data[i++] = mana_map_err;
 	}
+
+	for (q = 0; q < num_queues; q++) {
+		eq_stats = &apc->eqs[q].eq->eq.stats;
+
+		do {
+			start = u64_stats_fetch_begin(&eq_stats->syncp);
+			throttled = u64_stats_read(&eq_stats->throttle_count);
+		} while (u64_stats_fetch_retry(&eq_stats->syncp, start));
+
+		data[i++] = throttled;
+	}
 }

 static u32 mana_get_rx_ring_count(struct net_device *ndev)
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 0b52582118e8..29d152e18596 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -60,6 +60,7 @@ enum gdma_work_request_flags {

 enum gdma_eqe_type {
 	GDMA_EQE_COMPLETION		= 3,
+	GDMA_EQE_THROTTLE		= 4,
 	GDMA_EQE_TEST_EVENT		= 64,
 	GDMA_EQE_HWC_INIT_EQ_ID_DB	= 129,
 	GDMA_EQE_HWC_INIT_DATA		= 130,
@@ -291,6 +292,7 @@ struct gdma_dev {
 #define GDMA_EQE_SIZE 16
 #define GDMA_MAX_SQE_SIZE 512
 #define GDMA_MAX_RQE_SIZE 256
+#define GDMA_EQ_MAX_CHILD_CQ 2

 #define GDMA_COMP_DATA_SIZE 0x3C

@@ -318,6 +320,11 @@ struct gdma_event {

 struct gdma_queue;

+struct gdma_eq_stats {
+	u64_stats_t throttle_count;
+	struct u64_stats_sync syncp;
+};
+
 struct mana_eq {
 	struct gdma_queue	*eq;
 	struct dentry		*mana_eq_debugfs;
@@ -371,6 +378,10 @@ struct gdma_queue {
 			unsigned int irq;

 			u32 log2_throttle_limit;
+
+			struct gdma_eq_stats stats;
+
+			struct gdma_queue __rcu *child_cq[GDMA_EQ_MAX_CHILD_CQ];
 		} eq;

 		struct {
@@ -601,6 +612,8 @@ void mana_gd_free_service_wq(struct gdma_context *gc);
 void mana_gd_ring_dim(struct gdma_queue *cq, u32 mod_usec, bool mod_usec_vld,
 		      u32 mod_comps, bool mod_comps_vld);

+bool mana_gd_detach_cq(struct gdma_context *gc, struct gdma_queue *queue);
+
 struct gdma_wqe {
 	u32 reserved	:24;
 	u32 last_vbytes	:8;
@@ -760,6 +773,9 @@ enum {
 /* Driver supports non-contiguous queue buffers */
 #define GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS BIT(30)

+/* Driver supports handling throttled EQEs */
+#define GDMA_DRV_CAP_FLAG_1_THROTTLED_EVENT_QUEUE BIT_ULL(32)
+
 /* Capabilities in the PCI-only group below rely on dynamic MSI-X allocation
  * and on the servicing and reset paths reached through
  * mana_schedule_serv_work(). Transports that provide neither leave
@@ -784,7 +800,8 @@ enum {
 	 GDMA_DRV_CAP_FLAG_1_HANDLE_STALL_SQ_RECOVERY | \
 	 GDMA_DRV_CAP_FLAG_1_EQ_MSI_UNSHARE_MULTI_VPORT | \
 	 GDMA_DRV_CAP_FLAG_1_DYN_INTERRUPT_MODERATION | \
-	 GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS)
+	 GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS | \
+	 GDMA_DRV_CAP_FLAG_1_THROTTLED_EVENT_QUEUE)

 #define GDMA_DRV_CAP_FLAGS2 0

diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index e1fa9131c490..715e32d45105 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -90,6 +90,7 @@ enum mana_priv_flag_bits {
 /* Update this count whenever the respective structures are changed */
 #define MANA_STATS_RX_COUNT (6 + MANA_CQE_COAL_PKTS_8 - 1)
 #define MANA_STATS_TX_COUNT 11
+#define MANA_STATS_EQ_COUNT 1

 #define MANA_RX_FRAG_ALIGNMENT 64

--
2.53.0

Re: [PATCH net v4] net: mana: Add support to process throttled EQEs

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch tells firmware that every EQ on the function handles
  throttled EQEs.
- [Low] The patch marks only one side of the concurrent accesses.

Pre-existing issues:
- [High] mana_gd_destroy_cq() waits for a grace period
  (synchronize_net()) only when mana_gd_detach_cq() returns true,…

--

net: mana: Add support to process throttled EQEs

This commit handles GDMA_EQE_THROTTLE events. It records up to two child
CQs per EQ and calls their callbacks when a throttle EQE arrives. CQ
teardown now detaches the CQ from its parent EQ and from cq_table[]
before a grace period. The patch also advertises a new capability bit and
adds a per-EQ throttle counter to ethtool stats.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 8c915e0087846..ab942d5bfe058 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -757,6 +757,27 @@ void mana_gd_free_service_wq(struct gdma_context *gc)
>  	gc->service_wq = NULL;
>  }
>  
> +static void mana_gd_schedule_eq_cqs(struct gdma_queue *eq)
> +{
> +	struct gdma_context *gc = eq->gdma_dev->gdma_context;
> +	struct gdma_queue *cq;
> +	u8 i;
> +
> +	for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) {
> +		cq = rcu_dereference(eq->eq.child_cq[i]);
> +
> +		if (!cq)
> +			continue;
> +
> +		if (cq->id >= READ_ONCE(gc->max_num_cqs) ||
> +		    !gc->cq_table || gc->cq_table[cq->id] != cq)
> +			continue;

[Severity: Low]
This patch marks only the write side. mana_gd_detach_cq() clears
gc->cq_table[id] with WRITE_ONCE(), and mana_hwc_destroy_channel() now
sets gc->max_num_cqs with WRITE_ONCE().

The readers still use plain loads. Here that means gc->cq_table and
gc->cq_table[cq->id]. The GDMA_EQE_COMPLETION branch of
mana_gd_process_eqe() also reads both fields without READ_ONCE():

	if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
		break;

	cq = gc->cq_table[cq_id];

Could KCSAN report these as data races? Should the readers use
READ_ONCE() to pair with the marked writers?

This is mostly an annotation issue. The completion-path race was already
there before this patch, and the loads in mana_gd_schedule_eq_cqs() only
compare the pointer value.

> +
> +		if (cq->cq.callback)
> +			cq->cq.callback(cq->cq.context, cq);
> +	}
> +}

[ ... ]

> @@ -1073,25 +1104,66 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
>  			      struct gdma_queue *queue)
>  {
>  	u32 log2_num_entries = ilog2(spec->queue_size / GDMA_CQE_SIZE);
> +	struct gdma_queue *parent;
> +	u8 i;
>  
>  	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
> -	queue->cq.parent = spec->cq.parent_eq;
> +	parent = spec->cq.parent_eq;
> +	queue->cq.parent = parent;
>  	queue->cq.context = spec->cq.context;
>  	queue->cq.callback = spec->cq.callback;
> +
> +	if (!parent)
> +		return;
> +
> +	/* For throttled EQE store the child CQ */
> +	for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) {
> +		if (!rcu_access_pointer(parent->eq.child_cq[i])) {
> +			rcu_assign_pointer(parent->eq.child_cq[i], queue);
> +			return;
> +		}
> +	}
> +
> +	WARN_ONCE(1, "mana: EQ %u has no free child CQ slot\n", parent->id);
>  }

[Severity: High]
Do mana_ib CQs ever end up in child_cq[]?

This is the only place that writes child_cq[], and it returns early when
spec->cq.parent_eq is NULL. mana_ib_create_kernel_queue() uses a zeroed
spec:

drivers/infiniband/hw/mana/main.c:mana_ib_create_kernel_queue() {
	struct gdma_queue_spec spec = {};
	...
	err = mana_gd_create_mana_wq_cq(mdev->gdma_dev, &spec, &queue->kmem);
	...
}

mana_ib_gd_create_cq() then binds the RNIC CQ in hardware to an mana_ib
EQ:

	req.eq_id = mdev->eqs[cq->comp_vector]->id;

mana_ib_install_cq_cb() adds it only to gc->cq_table[].

mana_ib_create_qp_rss() and mana_ib_create_qp_raw() attach raw and RSS
QP CQs to the per-vPort mana_en EQs:

	eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
	cq_spec.attached_eq = eq->eq->id;

These CQs are also registered only through mana_ib_install_cq_cb().

The patch also adds GDMA_DRV_CAP_FLAG_1_THROTTLED_EVENT_QUEUE to
GDMA_DRV_CAP_FLAGS1, which mana_gd_verify_vf_version() sends for the
whole function.

Suppose hardware folds one of these CQs' completions into a
GDMA_EQE_THROTTLE. mana_gd_schedule_eq_cqs() walks child_cq[], which is
empty on mdev->eqs and holds only the mana_en TX and RX CQs on mpc->eqs.
mana_ib_cq_handler() never runs, and nothing polls or re-arms the CQ.

Wouldn't that stall the RDMA CQ the same way the commit message
describes for TX?

A fixed two-entry child_cq[] also cannot represent an EQ shared by many
RDMA CQs on the same comp_vector.

The commit message says:

  "A mana_en EQ serves one TX CQ and one RX CQ, and the HWC EQ serves
  the HWC CQ."

Is that still true once raw QPs attach CQs to mpc->eqs?

If advertising the bit is what turns on throttle EQEs, this patch adds a
new RDMA stall. If it is not, the RDMA case stays unfixed. Could the
commit message cover mana_ib in either case?

> -static void mana_gd_destroy_cq(struct gdma_context *gc,
> -			       struct gdma_queue *queue)
> +/* Unlink queue from its parent EQ's child_cq[] and from gc->cq_table[].
> + *
> + * Return: true if queue was in the parent EQ's child_cq[]. In that case the
> + * caller must wait for an RCU grace period before freeing queue, because
> + * mana_gd_intr() may still be handling a throttle EQE for it.
> + */
> +bool mana_gd_detach_cq(struct gdma_context *gc, struct gdma_queue *queue)
>  {

[ ... ]

> +	/* Check if CQ might have been detached already, only clear if
> +	 * it still points to same queue .
> +	 */
> +	if (id < gc->max_num_cqs && READ_ONCE(gc->cq_table[id]) == queue)
> +		WRITE_ONCE(gc->cq_table[id], NULL);
>  
> -	if (!gc->cq_table[id])
> -		return;
> +	return attached;
> +}
>  
> -	gc->cq_table[id] = NULL;
> +static void mana_gd_destroy_cq(struct gdma_context *gc,
> +			       struct gdma_queue *queue)
> +{
> +	/* Wait for irq reader holding the cq */
> +	if (mana_gd_detach_cq(gc, queue))
> +		synchronize_net();
>  }

[Severity: High]
This isn't a bug introduced by this patch, but mana_gd_detach_cq() also
clears gc->cq_table[id]. The GDMA_EQE_COMPLETION branch of
mana_gd_process_eqe() reads that table and calls the callback under the
rcu_read_lock() in mana_gd_intr():

	cq = gc->cq_table[cq_id];
	...
	if (cq->cq.callback)
		cq->cq.callback(cq->cq.context, cq);

mana_gd_destroy_cq() only waits when the CQ was in child_cq[]. An
mana_ib kernel (kmem) CQ has parent_eq NULL, so detach returns false and
no grace period runs:

mana_ib_destroy_cq()
  mana_ib_remove_cq_cb()       <- returns early for kmem
  mana_ib_gd_destroy_cq()
  mana_ib_destroy_queue()
    mana_gd_destroy_queue(kmem)
      mana_gd_destroy_cq()     <- clears cq_table[id], no wait

Suppose an IRQ on another CPU has already loaded the pointer from
cq_table[]. Can it then call mana_ib_cq_handler() on a freed gdma_queue
and mana_ib_cq?

The old mana_gd_destroy_cq() also cleared the slot without a grace
period, and drivers/infiniband/hw/mana has no synchronize_rcu() or
call_rcu(). However, the new kernel-doc and the "Wait for irq reader
holding the cq" comment now describe a rule that leaves out the
cq_table[] reader.

User CQs look similar. mana_ib_remove_cq_cb() frees the entry before it
clears the slot:

	kfree(gc->cq_table[cq->queue.id]);
	gc->cq_table[cq->queue.id] = NULL;

Should the grace period depend on removal from cq_table[] instead of
child_cq[] membership?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028084020.2051254-1-sahilchandna%40linux.microsoft.com