[PATCH net-next v4 0/4] net: mana: concurrent HWC requests and dynamic queue depth

[PATCH net-next v4 0/4] net: mana: concurrent HWC requests and dynamic queue depth

[Long Li]

The MANA Hardware Channel (HWC) is the control path the driver uses to
talk to the device.  It is created at a fixed depth of one outstanding
request, so every management command serialises behind the previous one.
This series lets several commands be in flight and raises the queue depth
to what the device reports.

Patches 1-2 are preparation and change no behaviour on their own.  Patch 1
records whether mana_smc_setup_hwc() got as far as handing the queue
addresses to the PF, which the reinit path in patch 4 needs in order to
know whether the device may still be using the queues.  Patch 2 gives each
message slot its own lock, refcount and completion state, which is what
makes more than one slot usable at a time.

Patch 3 makes the channel actually concurrent: a per-queue lock around
mana_gd_post_and_ring(), a bounded wait for a free slot, and a teardown
that drains in-flight senders before freeing the HWC.

Patch 4 bootstraps the HWC at depth 1, queries the device maximum and, if
it is larger, tears the queues down and rebuilds them at that depth.  The
device-reported dimensions are validated before they size DMA allocations,
and the dynamic-depth capability is advertised so firmware enables it only
for drivers that support it.

None of the four carries a Fixes: tag; the bug fixes that used to be part
of this series are being handled separately (see below).

Changes since v2:

 - Split the series.  v2 mixed five Fixes:-tagged bug fixes with the two
   feature patches and targeted net-next.  Per review feedback [1] the
   fixes now go via the net tree on their own, and this series carries
   only the feature work.  It no longer depends on that series: it
   applies to net-next as it stands today.

 - Because the fixes are no longer in front of them, the two feature
   patches were reworked rather than rebased.  The groundwork they used
   to inherit is now provided by the two preparation patches 1-2, which
   are deliberately not fixes and carry no Fixes: tag.

 - patch 3: the slot admission was reworked.  Acquisition uses
   down_timeout() so a caller expires rather than blocking on a slot
   that a timed-out request is still holding, and channel_up is
   re-checked under the bitmap lock once a permit is held so teardown
   cannot be missed.

 - patch 3: a slot whose request timed out is no longer released back to
   the pool -- only the matching response, or teardown, reclaims it.
   Releasing it early let a late response land on whichever command had
   since reused the slot.  mlx5 gates its command slots the same way
   ("only real completion can free the cmd slot", drivers/net/ethernet/
   mellanox/mlx5/core/cmd.c).

 - patch 4: validate the device-reported dimensions before they size DMA
   allocations -- require the negotiated message sizes to match the
   bootstrap ones, bound the depth by HW_CHANNEL_MAX_QUEUE_DEPTH, check
   that q_depth * max_msg_size plus alignment fits in u32, and carry the
   depth as u32 since the device field is 24-bit and truncating to u16
   could wrap a large value to a small depth.

 - patch 4: refuse the channel if the initialisation handshake did not
   supply a doorbell, rather than letting INVALID_DOORBELL reach
   mana_gd_ring_doorbell().

 - patch 4: a device that refuses the larger-depth establish now falls
   back to a working bootstrap channel instead of failing probe, but
   only once a retried DESTROY_HWC has confirmed the queue mappings are
   gone.

Testing on Azure hardware, with the series applied on net-next:

 - 17518 HWC commands, all completing successfully, with up to 2 in
   flight at once (the bootstrap channel can only ever have 1).
 - PCI remove/rescan x3: the reinit path runs every time, and the
   channel comes back at the larger depth.
 - iperf3 -P16: 1.24 TBytes at 182 Gbit/s, no taint and no splats.
 - Fault injection: with every HWC response dropped so that all slots
   end up held by timed-out requests, callers return -ETIMEDOUT in
   sub-millisecond time instead of blocking, no hung tasks, and the
   driver recovers once responses resume.

[1] https://lore.kernel.org/all/7108c005-6b8d-4dda-82cc-e665cbe3b6a4@redhat.com/

 - v2: https://lore.kernel.org/all/20260721234339.1476932-1-longli@microsoft.com/
 - v1: https://lore.kernel.org/netdev/20260715032942.3945317-1-longli@microsoft.com/

Long Li (4):
  net: mana: track when the HWC has been handed to the PF
  net: mana: give each HWC message slot its own completion state
  net: mana: support concurrent HWC requests
  net: mana: add dynamic HWC queue depth with reinit path

 .../net/ethernet/microsoft/mana/gdma_main.c   |  63 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 763 ++++++++++++++++--
 .../net/ethernet/microsoft/mana/shm_channel.c |  11 +-
 include/net/mana/gdma.h                       |  19 +
 include/net/mana/hw_channel.h                 |  64 +-
 include/net/mana/shm_channel.h                |   2 +-
 6 files changed, 862 insertions(+), 60 deletions(-)


base-commit: 1bb784eb6e38fd73143f021608e4ef3095d0c0d7
-- 
2.43.0

[PATCH net-next v4 1/4] net: mana: track when the HWC has been handed to the PF

[Long Li]

mana_hwc_destroy_channel() decides whether to tear the channel down by
looking at gc->max_num_cqs, which is only set when the device delivers an
HWC_INIT_DATA_MAX_NUM_CQS bootstrap event.  That is a proxy for "the
device is using the queues", and it is a poor one: the event arrives well
after mana_smc_setup_hwc() has already handed the queue addresses to the
PF, so a setup that fails in between leaves no reliable signal.

Record the handover directly instead.  mana_smc_setup_hwc() clears the
new setup_active flag on entry and sets it immediately before it writes
the ESTABLISH_HWC message, so the flag is exact in both directions: the
three exits before that write leave the device untouched and clear the
flag, while everything after it -- including a failed response -- leaves
it set, because the PF has the queue addresses either way.

No functional change is intended for the current single-establish flow.
This is preparation for the reinit path added later in this series, which
tears the channel down and rebuilds it at a larger queue depth and needs
to know whether a given attempt left the device holding the queues.

Signed-off-by: Long Li <longli@microsoft.com>
---
 .../net/ethernet/microsoft/mana/hw_channel.c  | 19 +++++++++++++------
 .../net/ethernet/microsoft/mana/shm_channel.c | 11 ++++++++++-
 include/net/mana/hw_channel.h                 | 10 ++++++++++
 include/net/mana/shm_channel.h                |  2 +-
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 263e7c4e2934186af037be4c80350a6e322b6771..75fdccdc8c48201b011a243fd0bd41b84c477076 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -683,7 +683,7 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 				 cq->mem_info.dma_handle,
 				 rq->mem_info.dma_handle,
 				 sq->mem_info.dma_handle,
-				 eq->eq.msix_index);
+				 eq->eq.msix_index, &hwc->setup_active);
 	if (err)
 		return err;
 
@@ -815,13 +815,20 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	if (!hwc)
 		return;
 
-	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
-	 * non-zero, the HWC worked and we should tear down the HWC here.
+	/* Tear down only if setup_hwc() handed the queues to the PF.  Until
+	 * then the device never saw them, so there is nothing to undo.
 	 */
-	if (gc->max_num_cqs > 0) {
-		mana_smc_teardown_hwc(&gc->shm_channel, false);
-		gc->max_num_cqs = 0;
+	if (hwc->setup_active) {
+		/* Only a successful teardown invalidates the MST entries.  If
+		 * it fails the device may still be using the queues, so leave
+		 * the flag set rather than record a clean teardown.
+		 */
+		if (!mana_smc_teardown_hwc(&gc->shm_channel, false))
+			hwc->setup_active = false;
+		else
+			dev_err(hwc->dev, "Failed to tear down HWC\n");
 	}
+	gc->max_num_cqs = 0;
 
 	if (hwc->txq)
 		mana_hwc_destroy_wq(hwc, hwc->txq);
diff --git a/drivers/net/ethernet/microsoft/mana/shm_channel.c b/drivers/net/ethernet/microsoft/mana/shm_channel.c
index d21b5db06e5092d82249fb1053d07f65aa38490c..1ec8e3661721dc8663975477b7ed09fcda72e2ab 100644
--- a/drivers/net/ethernet/microsoft/mana/shm_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/shm_channel.c
@@ -129,9 +129,15 @@ void mana_smc_init(struct shm_channel *sc, struct device *dev,
 	sc->base = base;
 }
 
+/* Clear *submitted on entry and set it once the ESTABLISH_HWC message has
+ * been handed to the PF, i.e. once the device may start using the queues.
+ * The caller uses it to decide whether a failure still needs
+ * mana_smc_teardown_hwc(): the exits below leave the device untouched, so
+ * they must not require one.
+ */
 int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
 		       u64 cq_addr, u64 rq_addr, u64 sq_addr,
-		       u32 eq_msix_index)
+		       u32 eq_msix_index, bool *submitted)
 {
 	union smc_proto_hdr *hdr;
 	u16 all_addr_h4bits = 0;
@@ -144,6 +150,8 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
 	int err;
 	int i;
 
+	*submitted = false;
+
 	/* Ensure VF already has possession of shared memory */
 	err = mana_smc_poll_register(sc->base, false);
 	if (err) {
@@ -229,6 +237,7 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
 	/* Write 256-message buffer to shared memory (final 32-bit write
 	 * triggers HW to set possession bit to PF).
 	 */
+	*submitted = true;
 	dword = (u32 *)shm_buf;
 	for (i = 0; i < SMC_APERTURE_DWORDS; i++)
 		writel(*dword++, sc->base + i * SMC_BASIC_UNIT);
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 16feb39616c1bead1a043b3fadc2e18a90651516..237608b488e5ee773d7889d3c1d291ca92ebbeb9 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -199,6 +199,16 @@ struct hw_channel_context {
 	u32 pf_dest_vrcq_id;
 	u32 hwc_timeout;
 
+	/* True once mana_smc_setup_hwc() has handed the ESTABLISH_HWC message
+	 * to the PF, so the device may DMA into the HWC buffers.  That
+	 * function clears it on entry and sets it at the handover, so only a
+	 * failure before the handover leaves it false; a failure after it --
+	 * including one reported by mana_hwc_establish_channel() -- leaves it
+	 * set, which is what makes teardown attempt DESTROY_HWC.  Cleared
+	 * again once that teardown succeeds.
+	 */
+	bool setup_active;
+
 	struct hwc_caller_ctx *caller_ctx;
 };
 
diff --git a/include/net/mana/shm_channel.h b/include/net/mana/shm_channel.h
index dbabcfb95daf3e87b39a657e3a5f23a1508d4f31..e96387d795259d52aeec7b3fe0981825d90a3822 100644
--- a/include/net/mana/shm_channel.h
+++ b/include/net/mana/shm_channel.h
@@ -20,7 +20,7 @@ void mana_smc_init(struct shm_channel *sc, struct device *dev,
 
 int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
 		       u64 cq_addr, u64 rq_addr, u64 sq_addr,
-		       u32 eq_msix_index);
+		       u32 eq_msix_index, bool *submitted);
 
 int mana_smc_teardown_hwc(struct shm_channel *sc, bool reset_vf);
 
-- 
2.43.0

[PATCH net-next v4 2/4] net: mana: give each HWC message slot its own completion state

[Long Li]

An HWC message slot is currently owned jointly by the sender and the
response handler with nothing arbitrating between them: the sender
publishes ctx->output_buf, waits, and on timeout releases the slot, while
mana_hwc_handle_resp() writes through that pointer from the CQ interrupt.
At the bootstrap queue depth of one this is survivable because there is
never more than one command outstanding.

Give each slot the state it needs to be owned independently:

  - a per-slot spinlock, so the sender's timeout path and the response
    handler serialise on the slot rather than on the channel;
  - a refcount holding one reference for the sender and one for the
    response handler, with the last put releasing the bitmap slot, so
    neither side can retire a slot the other is still using;
  - a responded flag, set by whichever side gets there first, so a
    second response for the same request is dropped rather than applied
    twice.

ctx->output_buf becomes the sender's ownership marker: the handler
honours a response only while it is published and the slot has not
already been answered.  ctx->error also changes from u32 to int, since it
carries a negative errno.

A response that arrives after its slot has already been released and
reused is still applied to the new owner; the slot index is the only
thing correlating a response to a request, so nothing here can tell the
two apart.  The next patch stops the slot being released at all while a
response may still arrive.

No functional change is intended at the current queue depth of one.  This
is preparation for allowing several commands to be in flight at once,
which is what makes independent per-slot ownership necessary.

Signed-off-by: Long Li <longli@microsoft.com>
---
 .../net/ethernet/microsoft/mana/gdma_main.c   |   8 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 167 +++++++++++++++---
 include/net/mana/hw_channel.h                 |  18 +-
 3 files changed, 165 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index f92b2d0bf926e1b715ff665d37f8173a2103e6fe..a023d3e1a95deeea3d15860f6fe7cab24f0b64e5 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -331,7 +331,13 @@ static int mana_gd_query_hwc_timeout(struct pci_dev *pdev, u32 *timeout_val)
 	if (err || resp.hdr.status)
 		return err ? err : -EPROTO;
 
-	*timeout_val = resp.timeout_ms;
+	/* Zero is the driver's own "do not wait, do not log" sentinel, set by
+	 * mana_serv_reset() when the HWC has stopped responding.  A zero from
+	 * the device would enter that state instead: ignore it and keep the
+	 * caller's positive value.
+	 */
+	if (resp.timeout_ms)
+		*timeout_val = resp.timeout_ms;
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 75fdccdc8c48201b011a243fd0bd41b84c477076..0056bdd8c53f5bf6b6b5f318f21faf7d9e14de53 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -6,9 +6,11 @@
 #include <net/mana/hw_channel.h>
 #include <linux/vmalloc.h>
 
+/* Acquire a free inflight message slot, waiting for one if all are in use. */
 static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 {
 	struct gdma_resource *r = &hwc->inflight_msg_res;
+	struct hwc_caller_ctx *ctx;
 	unsigned long flags;
 	u32 index;
 
@@ -19,6 +21,17 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 	index = find_first_zero_bit(hwc->inflight_msg_res.map,
 				    hwc->inflight_msg_res.size);
 
+	ctx = &hwc->caller_ctx[index];
+	reinit_completion(&ctx->comp_event);
+	/* Take both references (sender + handle_resp) before publishing the
+	 * slot, so an early response cannot free it under the sender.
+	 */
+	refcount_set(&ctx->refcnt, 2);
+	ctx->responded = false;
+	ctx->msg_id = index;
+	ctx->error = -EINPROGRESS;
+
+	/* Publish the slot last, after it is fully initialised. */
 	bitmap_set(hwc->inflight_msg_res.map, index, 1);
 
 	spin_unlock_irqrestore(&r->lock, flags);
@@ -40,6 +53,13 @@ static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
 	up(&hwc->sema);
 }
 
+static void hwc_ctx_put(struct hw_channel_context *hwc,
+			struct hwc_caller_ctx *ctx)
+{
+	if (refcount_dec_and_test(&ctx->refcnt))
+		mana_hwc_put_msg_index(hwc, ctx->msg_id);
+}
+
 static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx *caller_ctx,
 				    const struct gdma_resp_hdr *resp_msg,
 				    u32 resp_len)
@@ -90,22 +110,35 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	}
 
 	ctx = hwc->caller_ctx + msg_id;
-	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
-	if (err)
-		goto out;
 
-	ctx->status_code = resp_msg->status;
+	spin_lock(&ctx->lock);
 
-	memcpy(ctx->output_buf, resp_msg, resp_len);
-out:
+	/* Honour a response only while the sender owns the slot (output_buf
+	 * published) and has not already been answered; otherwise drop it as
+	 * premature, stale or duplicate without touching the refcount.
+	 */
+	if (!ctx->output_buf || ctx->responded) {
+		spin_unlock(&ctx->lock);
+		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+		return;
+	}
+	ctx->responded = true;
+
+	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
+	if (!err) {
+		ctx->status_code = resp_msg->status;
+		memcpy(ctx->output_buf, resp_msg, resp_len);
+	}
 	ctx->error = err;
 
-	/* Must post rx wqe before complete(), otherwise the next rx may
-	 * hit no_wqe error.
+	/* Post RX WQE before completing — the next response may arrive
+	 * immediately and needs a posted buffer.
 	 */
 	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
-
 	complete(&ctx->comp_event);
+	spin_unlock(&ctx->lock);
+
+	hwc_ctx_put(hwc, ctx);
 }
 
 static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -657,8 +690,10 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
 	if (!ctx)
 		return -ENOMEM;
 
-	for (i = 0; i < q_depth; ++i)
+	for (i = 0; i < q_depth; ++i) {
+		spin_lock_init(&ctx[i].lock);
 		init_completion(&ctx[i].comp_event);
+	}
 
 	hwc->caller_ctx = ctx;
 
@@ -669,6 +704,12 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 				      u32 *max_req_msg_size,
 				      u32 *max_resp_msg_size)
 {
+	/* mana_hwc_init_event_handler() fills the bootstrap fields from hard
+	 * IRQ on GDMA_EQE_HWC_INIT_DATA and then signals hwc_init_eqe_comp on
+	 * GDMA_EQE_HWC_INIT_DONE.  The wait_for_completion() below pairs with
+	 * that complete(), so every value stored before INIT_DONE is ordered
+	 * against the reads that follow it here.
+	 */
 	struct hw_channel_context *hwc = gc->hwc.driver_data;
 	struct gdma_queue *rq = hwc->rxq->gdma_wq;
 	struct gdma_queue *sq = hwc->txq->gdma_wq;
@@ -867,13 +908,19 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	struct hwc_wq *txq = hwc->txq;
 	struct gdma_req_hdr *req_msg;
 	struct hwc_caller_ctx *ctx;
+	unsigned long flags;
+	bool drop_resp_ref;
 	u32 dest_vrcq = 0;
 	u32 dest_vrq = 0;
 	u32 command;
+	u32 status;
+	u32 wait_ms;
 	u16 msg_id;
 	int err;
 
-	mana_hwc_get_msg_index(hwc, &msg_id);
+	err = mana_hwc_get_msg_index(hwc, &msg_id);
+	if (err)
+		return err;
 
 	tx_wr = &txq->msg_buf->reqs[msg_id];
 
@@ -885,8 +932,11 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	}
 
 	ctx = hwc->caller_ctx + msg_id;
+
+	spin_lock_irqsave(&ctx->lock, flags);
 	ctx->output_buf = resp;
 	ctx->output_buflen = resp_len;
+	spin_unlock_irqrestore(&ctx->lock, flags);
 
 	req_msg = (struct gdma_req_hdr *)tx_wr->buf_va;
 	if (req)
@@ -902,43 +952,108 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 		dest_vrcq = hwc->pf_dest_vrcq_id;
 	}
 
+	/* The response-side reference (from get_msg_index) keeps the slot
+	 * alive if hardware responds right after the doorbell.
+	 */
 	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
 	if (err) {
 		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
 		goto out;
 	}
 
+	wait_ms = hwc->hwc_timeout;
 	if (!wait_for_completion_timeout(&ctx->comp_event,
-					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
-		if (hwc->hwc_timeout != 0)
+					 msecs_to_jiffies(wait_ms))) {
+		/* Clear output_buf so a late response cannot write the caller's
+		 * buffer, then check whether one already arrived
+		 * (error != -EINPROGRESS).
+		 */
+		spin_lock_irqsave(&ctx->lock, flags);
+		ctx->output_buf = NULL;
+		err = ctx->error;
+		status = ctx->status_code;
+		spin_unlock_irqrestore(&ctx->lock, flags);
+
+		if (err != -EINPROGRESS) {
+			/* A response raced in just after the timeout, so the
+			 * hardware is alive: keep the channel and report what
+			 * that response said rather than a timeout.  It may
+			 * itself be an error -- a malformed response leaves
+			 * -EPROTO here -- which is still the answer to this
+			 * command.
+			 */
+			hwc_ctx_put(hwc, ctx);
+			goto check_status;
+		}
+
+		if (wait_ms != 0)
 			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
-				command, hwc->hwc_timeout);
+				command, wait_ms);
+
+		err = -ETIMEDOUT;
+
+		/* No-wait teardown (hwc_timeout == 0) is expected to expire;
+		 * just release the slot so the next teardown command can reuse
+		 * it.
+		 */
+		if (wait_ms == 0)
+			goto out;
 
-		/* Reduce further waiting if HWC no response */
+		/* Genuine timeout: shorten later waits so subsequent commands
+		 * fail fast instead of each draining the full timeout.
+		 */
 		if (hwc->hwc_timeout > 1)
 			hwc->hwc_timeout = 1;
 
-		err = -ETIMEDOUT;
+		/* Release the slot via out:; a late response no longer touches
+		 * it, so the sender must drop the reference here.
+		 */
 		goto out;
 	}
 
-	if (ctx->error) {
-		err = ctx->error;
-		goto out;
-	}
+	/* Clear output_buf and read the result under the lock; the slot may
+	 * be reused after hwc_ctx_put().
+	 */
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	err = ctx->error;
+	status = ctx->status_code;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	hwc_ctx_put(hwc, ctx);
+
+check_status:
+	if (err)
+		goto done;
 
-	if (ctx->status_code && ctx->status_code != GDMA_STATUS_MORE_ENTRIES) {
-		if (ctx->status_code == GDMA_STATUS_CMD_UNSUPPORTED) {
+	if (status && status != GDMA_STATUS_MORE_ENTRIES) {
+		if (status == GDMA_STATUS_CMD_UNSUPPORTED) {
 			err = -EOPNOTSUPP;
-			goto out;
+			goto done;
 		}
+
 		if (command != MANA_QUERY_PHY_STAT)
 			dev_err(hwc->dev, "Command 0x%x failed with status: 0x%x\n",
-				command, ctx->status_code);
+				command, status);
 		err = -EPROTO;
-		goto out;
+		goto done;
 	}
+
+	err = 0;
+	goto done;
 out:
-	mana_hwc_put_msg_index(hwc, msg_id);
+	/* Error, no-wait teardown, or timeout: drop the sender's and the
+	 * response-side references.  Latch ->responded so a racing response
+	 * is a no-op, and only drop the response-side ref if it has not.
+	 */
+	ctx = hwc->caller_ctx + msg_id;
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	drop_resp_ref = !ctx->responded;
+	ctx->responded = true;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	if (drop_resp_ref)
+		refcount_dec(&ctx->refcnt);
+	hwc_ctx_put(hwc, ctx);
+done:
 	return err;
 }
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 237608b488e5ee773d7889d3c1d291ca92ebbeb9..daff051472a0dd9f57ee0b9fb3d71029fca909ad 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -171,8 +171,24 @@ struct hwc_caller_ctx {
 	void *output_buf;
 	u32 output_buflen;
 
-	u32 error; /* Linux error code */
+	int error; /* Linux error code (negative errno or 0) */
 	u32 status_code;
+
+	/* Protects output_buf against concurrent access from
+	 * handle_resp() (CQ interrupt) and the sender timeout path.
+	 */
+	spinlock_t lock;
+
+	/* Tracks sender + handle_resp ownership.  The last put
+	 * (refcount reaches 0) releases the bitmap slot.
+	 */
+	refcount_t refcnt;
+	u16 msg_id;
+
+	/* Set by the first handle_resp(), or by the sender's timeout path,
+	 * so a later or duplicate response is dropped.
+	 */
+	bool responded;
 };
 
 struct hw_channel_context {
-- 
2.43.0

[PATCH net-next v4 3/4] net: mana: support concurrent HWC requests

[Long Li]

The HWC now manages message slots with a refcounted, per-slot-locked
bitmap (see the previous patch) but still runs at the bootstrap queue
depth of 1.  Prepare the channel for multiple in-flight requests and
make teardown safe against them.

  - Add a per-queue lock to hwc_wq; mana_gd_post_and_ring() is not safe
    to call concurrently on the same queue.

  - Bound the wait for a message slot.  A sender blocked in down() could
    only be woken by up(), which teardown never issued, so it slept with
    no way to observe that the channel was going away.  Keep the
    counting semaphore -- its count is exactly the number of free slots
    -- but acquire with down_timeout(), so a caller expires instead of
    blocking for ever, and re-check channel_up under the bitmap lock
    once a permit is held, returning the permit if the channel is on its
    way down.  A slot whose request timed out is never posted back (see
    below), so the count falls permanently until the response that owns
    it arrives or teardown reclaims it; that is what makes the bounded
    wait terminate rather than spin.  mlx5 gates its command slots the
    same way (down_timeout() on cmd->vars.sem, -EBUSY on expiry).

  - Add a channel_up flag, set once the channel is established and
    cleared under the bitmap lock in destroy_channel(), so
    mana_hwc_get_msg_index() rejects new senders during teardown.  It is
    read under the same lock that publishes a slot, so a sender that
    already holds a permit cannot miss the clear: it either publishes
    before teardown observes the bitmap, or finds the flag clear and
    posts the permit back.  Each released waiter posting its permit back
    releases the next, so the force-completion in destroy_channel() --
    which returns every in-flight slot, including the ones a timed-out
    request was holding -- drains the whole queue.

  - destroy_channel() must not free the HWC while senders are still in
    flight.  Count active senders in a gc->hwc_lock-protected counter;
    destroy_channel() force-completes the in-flight slots (-ENODEV), then
    drains the counter to zero with wait_event_lock_irq() before freeing.
    The counter changes only under hwc_lock and the last sender's
    wake_up() runs under that lock, so evaluating the drain condition
    under hwc_lock guarantees the waking sender has already dropped the
    lock -- finished touching gc -- before the drain returns; it cannot
    race the later free of gc.  The waitqueue itself lives on
    gdma_context, not hwc, so the wake never dereferences freed hwc.

  - The sender looks up the channel via gc->hwc.driver_data, which
    destroy_channel() clears and then frees.  Without serialization the
    lookup and the reference can straddle the free:

      CPU A (mana_gd_send_request)      CPU B (destroy_channel)
      ------------------------------    ------------------------------
      hwc = gc->hwc.driver_data;  // ok
                                        driver_data = NULL;
                                        wait active_senders == 0;  // 0!
                                        kfree(hwc);
      hwc->active_senders++;            // use-after-free

    Guard driver_data with a new gc->hwc_lock spinlock, taken by the
    readers (mana_gd_send_request, mana_need_log, mana_serv_reset) and by
    the publish/clear, so "load the pointer + take a sender reference" is
    atomic against the clear.  After the clear a sender either already
    holds a reference (and is waited for) or observes NULL and returns
    -ENODEV.  These are all control-plane paths (HWC commands sleep,
    reset runs on a workqueue), so a plain spinlock -- not RCU -- is
    sufficient.

With more than one slot in use, a timed-out command also stops being
harmless to retire.  The previous patch releases its slot while the
device may still answer, so the next request can take that slot and be
completed with the previous request's response -- responses are
correlated only by the slot index.  So do not release it: a slot whose
request reached the hardware stays taken until the response arrives, and
only that response frees it.  A request that never reached the hardware
cannot be answered, so its slot is still released immediately.

Slots held this way are counted, so a channel that is merely busy can be
told from one where nothing will ever free a slot again.  Once every slot
is held by an unanswered command, mana_hwc_get_msg_index() fails with
-ETIMEDOUT rather than sleeping on a queue no one can wake, which would
otherwise deadlock teardown and reset against a device that has stopped
responding.

The next patch raises the depth to the device-reported maximum.

Signed-off-by: Long Li <longli@microsoft.com>
---
 .../net/ethernet/microsoft/mana/gdma_main.c   |  55 ++-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 330 +++++++++++++++---
 include/net/mana/gdma.h                       |  15 +
 include/net/mana/hw_channel.h                 |  27 ++
 4 files changed, 384 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index a023d3e1a95deeea3d15860f6fe7cab24f0b64e5..9e7efce669ae0686e352cf953c051de160d74a0d 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -162,6 +162,8 @@ static int mana_gd_init_registers(struct pci_dev *pdev)
 bool mana_need_log(struct gdma_context *gc, int err)
 {
 	struct hw_channel_context *hwc;
+	bool need_log = true;
+	unsigned long flags;
 
 	if (err != -ETIMEDOUT)
 		return true;
@@ -169,11 +171,13 @@ bool mana_need_log(struct gdma_context *gc, int err)
 	if (!gc)
 		return true;
 
+	spin_lock_irqsave(&gc->hwc_lock, flags);
 	hwc = gc->hwc.driver_data;
 	if (hwc && hwc->hwc_timeout == 0)
-		return false;
+		need_log = false;
+	spin_unlock_irqrestore(&gc->hwc_lock, flags);
 
-	return true;
+	return need_log;
 }
 
 static int mana_gd_query_max_resources(struct pci_dev *pdev)
@@ -393,9 +397,27 @@ static int mana_gd_detect_devices(struct pci_dev *pdev)
 int mana_gd_send_request(struct gdma_context *gc, u32 req_len, const void *req,
 			 u32 resp_len, void *resp)
 {
-	struct hw_channel_context *hwc = gc->hwc.driver_data;
+	struct hw_channel_context *hwc;
+	unsigned long flags;
+	int err;
+
+	spin_lock_irqsave(&gc->hwc_lock, flags);
+	hwc = gc->hwc.driver_data;
+	if (!hwc) {
+		spin_unlock_irqrestore(&gc->hwc_lock, flags);
+		return -ENODEV;
+	}
+	hwc->active_senders++;
+	spin_unlock_irqrestore(&gc->hwc_lock, flags);
+
+	err = mana_hwc_send_request(hwc, req_len, req, resp_len, resp);
 
-	return mana_hwc_send_request(hwc, req_len, req, resp_len, resp);
+	spin_lock_irqsave(&gc->hwc_lock, flags);
+	if (--hwc->active_senders == 0)
+		wake_up(&gc->hwc_drain_waitq);
+	spin_unlock_irqrestore(&gc->hwc_lock, flags);
+
+	return err;
 }
 EXPORT_SYMBOL_NS(mana_gd_send_request, "NET_MANA");
 
@@ -716,6 +738,7 @@ static void mana_serv_reset(struct pci_dev *pdev)
 {
 	struct gdma_context *gc = pci_get_drvdata(pdev);
 	struct hw_channel_context *hwc;
+	unsigned long flags;
 	int ret;
 
 	if (!gc) {
@@ -725,14 +748,17 @@ static void mana_serv_reset(struct pci_dev *pdev)
 		return;
 	}
 
+	spin_lock_irqsave(&gc->hwc_lock, flags);
 	hwc = gc->hwc.driver_data;
 	if (!hwc) {
+		spin_unlock_irqrestore(&gc->hwc_lock, flags);
 		dev_err(&pdev->dev, "MANA service: no HWC\n");
 		goto out;
 	}
 
 	/* HWC is not responding in this case, so don't wait */
 	hwc->hwc_timeout = 0;
+	spin_unlock_irqrestore(&gc->hwc_lock, flags);
 
 	dev_info(&pdev->dev, "MANA reset cycle start\n");
 
@@ -1339,6 +1365,16 @@ static int mana_gd_create_dma_region(struct gdma_dev *gd,
 	if (gmi->nr_pages == 0 && !MANA_PAGE_ALIGNED(gmi->virt_addr))
 		return -EINVAL;
 
+	/* No RCU needed: this runs only on the data-path queue-creation
+	 * path (mana_gd_create_mana_eq/mana_gd_create_mana_wq_cq, called
+	 * by mana_en under RTNL and by mana_ib RDMA verbs, or during
+	 * init).  Every teardown path — mana_gd_remove, mana_gd_suspend,
+	 * and the HWC reset/service path (which goes through
+	 * mana_gd_suspend) — drains those consumers via mana_rdma_remove()
+	 * + mana_remove() before mana_hwc_destroy_channel() clears
+	 * gc->hwc.driver_data, so no concurrent destroy can race with
+	 * this dereference.
+	 */
 	hwc = gc->hwc.driver_data;
 	req_msg_size = struct_size(req, page_addr_list, num_page);
 	if (req_msg_size > hwc->max_req_msg_size)
@@ -1544,7 +1580,17 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
 	struct hw_channel_context *hwc;
 	int err;
 
+	/* No RCU needed: this runs only inside mana_gd_setup, on the
+	 * probe and resume paths.  The PCI/PM core holds device_lock
+	 * across .probe/.resume and .remove/.suspend, so setup cannot
+	 * overlap teardown of the same device.  The HWC reset/service
+	 * path is additionally serialized by GC_IN_SERVICE and runs
+	 * suspend (destroy) then resume (this) sequentially in one work
+	 * item.  driver_data was just set by mana_hwc_create_channel
+	 * earlier in this same setup call, so it is live here.
+	 */
 	hwc = gc->hwc.driver_data;
+
 	mana_gd_init_req_hdr(&req.hdr, GDMA_VERIFY_VF_DRIVER_VERSION,
 			     sizeof(req), sizeof(resp));
 
@@ -2538,6 +2584,7 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	mutex_init(&gc->eq_test_event_mutex);
 	mutex_init(&gc->gic_mutex);
+	spin_lock_init(&gc->hwc_lock);
 	pci_set_drvdata(pdev, gc);
 	gc->bar0_pa = pci_resource_start(pdev, 0);
 	gc->bar0_size = pci_resource_len(pdev, 0);
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 0056bdd8c53f5bf6b6b5f318f21faf7d9e14de53..91fcf7c092113133d392f0e43f2eda62ca5cb919 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -6,7 +6,11 @@
 #include <net/mana/hw_channel.h>
 #include <linux/vmalloc.h>
 
-/* Acquire a free inflight message slot, waiting for one if all are in use. */
+/* Acquire a free message slot from the inflight bitmap, waiting for one if
+ * all are in use.  Returns -ENODEV if the channel is being torn down, or
+ * -ETIMEDOUT if a prior HWC command has timed out (preserving the error
+ * code callers expect).
+ */
 static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 {
 	struct gdma_resource *r = &hwc->inflight_msg_res;
@@ -14,12 +18,32 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 	unsigned long flags;
 	u32 index;
 
-	down(&hwc->sema);
+	/* Bounded wait for a slot.  A timed-out request keeps its slot until
+	 * the device answers for it, so the semaphore is never posted back
+	 * for that slot and a caller expires here rather than blocking on a
+	 * release that is not coming.  Teardown reclaims those slots, which
+	 * posts the semaphore and releases anyone waiting below.
+	 */
+	if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
+		return -ETIMEDOUT;
 
 	spin_lock_irqsave(&r->lock, flags);
 
-	index = find_first_zero_bit(hwc->inflight_msg_res.map,
-				    hwc->inflight_msg_res.size);
+	if (!hwc->channel_up) {
+		spin_unlock_irqrestore(&r->lock, flags);
+		up(&hwc->sema);
+		return -ENODEV;
+	}
+
+	/* The semaphore admits at most r->size holders at a time, so a slot
+	 * acquired above always has a free bit waiting for it here.
+	 */
+	index = find_first_zero_bit(r->map, r->size);
+	if (WARN_ON_ONCE(index >= r->size)) {
+		spin_unlock_irqrestore(&r->lock, flags);
+		up(&hwc->sema);
+		return -EIO;
+	}
 
 	ctx = &hwc->caller_ctx[index];
 	reinit_completion(&ctx->comp_event);
@@ -28,11 +52,12 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 	 */
 	refcount_set(&ctx->refcnt, 2);
 	ctx->responded = false;
+	ctx->resp_pending = true;
 	ctx->msg_id = index;
 	ctx->error = -EINPROGRESS;
 
 	/* Publish the slot last, after it is fully initialised. */
-	bitmap_set(hwc->inflight_msg_res.map, index, 1);
+	bitmap_set(r->map, index, 1);
 
 	spin_unlock_irqrestore(&r->lock, flags);
 
@@ -101,6 +126,7 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 {
 	const struct gdma_resp_hdr *resp_msg = rx_req->buf_va;
 	struct hwc_caller_ctx *ctx;
+	bool release;
 	int err;
 
 	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
@@ -113,15 +139,32 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 
 	spin_lock(&ctx->lock);
 
-	/* Honour a response only while the sender owns the slot (output_buf
-	 * published) and has not already been answered; otherwise drop it as
-	 * premature, stale or duplicate without touching the refcount.
+	/* The sender has not published its buffer yet, so nothing asked for
+	 * this response.  Keep the slot reserved and drop the message.
 	 */
-	if (!ctx->output_buf || ctx->responded) {
+	if (!ctx->output_buf && !ctx->responded) {
 		spin_unlock(&ctx->lock);
 		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
 		return;
 	}
+
+	/* Take the response-side reference away exactly once: releasing it
+	 * is what frees a slot whose sender has already given up.
+	 */
+	release = ctx->resp_pending;
+	ctx->resp_pending = false;
+
+	if (ctx->responded) {
+		/* The sender timed out and abandoned the slot, or a response
+		 * was already applied.  Consume this one without writing
+		 * anything, then release the slot it was holding.
+		 */
+		spin_unlock(&ctx->lock);
+		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+		if (release)
+			hwc_ctx_put(hwc, ctx);
+		return;
+	}
 	ctx->responded = true;
 
 	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
@@ -138,7 +181,8 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	complete(&ctx->comp_event);
 	spin_unlock(&ctx->lock);
 
-	hwc_ctx_put(hwc, ctx);
+	if (release)
+		hwc_ctx_put(hwc, ctx);
 }
 
 static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -593,6 +637,7 @@ static int mana_hwc_create_wq(struct hw_channel_context *hwc,
 	hwc_wq->gdma_wq = queue;
 	hwc_wq->queue_depth = q_depth;
 	hwc_wq->hwc_cq = hwc_cq;
+	spin_lock_init(&hwc_wq->lock);
 
 	err = mana_hwc_alloc_dma_buf(hwc, q_depth, max_msg_size,
 				     &hwc_wq->msg_buf);
@@ -610,7 +655,7 @@ static int mana_hwc_create_wq(struct hw_channel_context *hwc,
 	return err;
 }
 
-static int mana_hwc_post_tx_wqe(const struct hwc_wq *hwc_txq,
+static int mana_hwc_post_tx_wqe(struct hwc_wq *hwc_txq,
 				struct hwc_work_request *req,
 				u32 dest_virt_rq_id, u32 dest_virt_rcq_id,
 				bool dest_pf)
@@ -649,7 +694,11 @@ static int mana_hwc_post_tx_wqe(const struct hwc_wq *hwc_txq,
 	req->wqe_req.inline_oob_data = tx_oob;
 	req->wqe_req.client_data_unit = 0;
 
+	/* Serialize WQE posting — multiple senders may call concurrently. */
+	spin_lock(&hwc_txq->lock);
 	err = mana_gd_post_and_ring(hwc_txq->gdma_wq, &req->wqe_req, NULL);
+	spin_unlock(&hwc_txq->lock);
+
 	if (err)
 		dev_err(dev, "Failed to post WQE on HWC SQ: %d\n", err);
 	return err;
@@ -660,6 +709,9 @@ static int mana_hwc_init_inflight_msg(struct hw_channel_context *hwc,
 {
 	int err;
 
+	/* One permit per slot; a permit is returned only when the slot is
+	 * released, so the count always mirrors the free slots.
+	 */
 	sema_init(&hwc->sema, num_msg);
 
 	err = mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);
@@ -675,6 +727,7 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
 	struct hwc_wq *hwc_rxq = hwc->rxq;
 	struct hwc_work_request *req;
 	struct hwc_caller_ctx *ctx;
+	unsigned long flags;
 	int err;
 	int i;
 
@@ -697,7 +750,34 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
 
 	hwc->caller_ctx = ctx;
 
-	return mana_gd_test_eq(gc, hwc->cq->gdma_eq);
+	/* channel_up must be set before the test EQ request, because
+	 * the request goes through mana_hwc_get_msg_index() which
+	 * checks channel_up.  caller_ctx is allocated above, so
+	 * concurrent access to a NULL caller_ctx is not possible.
+	 *
+	 * Publish it under the bitmap lock, the same one the waiters and
+	 * mana_hwc_destroy_channel() use, so the flag is never stored
+	 * concurrently with the teardown that clears it.
+	 */
+	spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+	hwc->channel_up = true;
+	spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+
+	err = mana_gd_test_eq(gc, hwc->cq->gdma_eq);
+	if (err) {
+		/* Clear channel_up under the bitmap lock, mirroring
+		 * mana_hwc_destroy_channel().  Any sender already waiting on
+		 * the semaphore is released by the slot holder that posts it,
+		 * or by the teardown the caller runs on this error; each
+		 * released waiter finds the flag clear and posts the permit
+		 * straight back, so one permit walks the whole queue.
+		 */
+		spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+		hwc->channel_up = false;
+		spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+	}
+
+	return err;
 }
 
 static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
@@ -797,6 +877,7 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 	u32 max_req_msg_size, max_resp_msg_size;
 	struct gdma_dev *gd = &gc->hwc;
 	struct hw_channel_context *hwc;
+	unsigned long flags;
 	u16 q_depth_max;
 	int err;
 
@@ -805,10 +886,11 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 		return -ENOMEM;
 
 	gd->gdma_context = gc;
-	gd->driver_data = hwc;
 	hwc->gdma_dev = gd;
 	hwc->dev = gc->dev;
 	hwc->hwc_timeout = HW_CHANNEL_WAIT_RESOURCE_TIMEOUT_MS;
+	hwc->active_senders = 0;
+	init_waitqueue_head(&gc->hwc_drain_waitq);
 
 	/* HWC's instance number is always 0. */
 	gd->dev_id.as_uint32 = 0;
@@ -817,6 +899,15 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 	gd->pdid = INVALID_PDID;
 	gd->doorbell = INVALID_DOORBELL;
 
+	/* Publish driver_data last, under hwc_lock: the lock orders the hwc
+	 * initialisation above before the pointer becomes visible and
+	 * serialises the publish against the control-plane readers in
+	 * mana_gd_send_request(), mana_need_log() and mana_serv_reset().
+	 */
+	spin_lock_irqsave(&gc->hwc_lock, flags);
+	gc->hwc.driver_data = hwc;
+	spin_unlock_irqrestore(&gc->hwc_lock, flags);
+
 	/* mana_hwc_init_queues() only creates the required data structures,
 	 * and doesn't touch the HWC device.
 	 */
@@ -851,11 +942,120 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 
 void mana_hwc_destroy_channel(struct gdma_context *gc)
 {
+	/* This is the only destroy entry point.  driver_data is read
+	 * plainly here (teardown is serialised against other teardown);
+	 * it is cleared under hwc_lock below before hwc is freed.
+	 */
 	struct hw_channel_context *hwc = gc->hwc.driver_data;
+	unsigned long flags;
 
 	if (!hwc)
 		return;
 
+	/* Prevent new requests from starting.  Clear channel_up under the
+	 * bitmap lock so get_msg_index() cannot acquire a slot and increment
+	 * active_senders after this point.  Senders already waiting on the
+	 * semaphore are released by the force-completion loop below, which
+	 * returns every in-flight slot -- including the ones a timed-out
+	 * request was holding; each released waiter sees the flag clear and
+	 * posts its permit straight back, so they drain in turn.
+	 *
+	 * Gate on the bitmap rather than on channel_up: reading the flag
+	 * unlocked and only then taking the lock would let a concurrent
+	 * setup publish it in between and leave the channel up.  A zero
+	 * num_inflight_msg means mana_gd_alloc_res_map() never ran, so the
+	 * lock is not initialised yet -- and no sender can exist either.
+	 */
+	if (hwc->num_inflight_msg) {
+		spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+		hwc->channel_up = false;
+		spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+	}
+
+	/* Clear the pointer under hwc_lock so new callers in
+	 * mana_gd_send_request() see NULL and return -ENODEV.  The lock
+	 * makes the readers' "load driver_data + active_senders++"
+	 * atomic against this store, so once it returns no new sender can
+	 * take a reference; the active_senders drain below waits out those
+	 * that already did, before their hwc is freed.
+	 */
+	spin_lock_irqsave(&gc->hwc_lock, flags);
+	gc->hwc.driver_data = NULL;
+	spin_unlock_irqrestore(&gc->hwc_lock, flags);
+
+	/* Force-complete any in-flight senders so they observe -ENODEV,
+	 * return, and drop their references.  This runs before the HWC
+	 * hardware teardown below, so a live interrupt may still deliver
+	 * a real response via handle_resp() concurrently — that is safe
+	 * because the per-slot refcount model tolerates a concurrent
+	 * complete() and both paths (handle_resp and this loop) drop
+	 * their refs without double-releasing the slot.
+	 */
+	if (hwc->caller_ctx) {
+		struct hwc_caller_ctx *ctx;
+		bool drop_resp_ref;
+		int i;
+
+		for (i = 0; i < hwc->num_inflight_msg; i++) {
+			if (!test_bit(i, hwc->inflight_msg_res.map))
+				continue;
+
+			ctx = &hwc->caller_ctx[i];
+
+			/* Wake senders blocked on wait_for_completion.
+			 * Set error under lock to avoid racing with
+			 * handle_resp() which writes error under the
+			 * same lock.  The sender NULLs output_buf
+			 * after waking — doing it here would race
+			 * with a sender that hasn't set output_buf yet.
+			 *
+			 * Latch ->responded so that a response still in
+			 * flight cannot overwrite -ENODEV and report
+			 * success for a request the channel is abandoning.
+			 * handle_resp() then drops that response without
+			 * touching the refcount, so release the
+			 * response-side reference here instead.
+			 */
+			spin_lock_irqsave(&ctx->lock, flags);
+			/* Do not clobber a response mana_hwc_handle_resp() has
+			 * already delivered: its payload is in the caller's
+			 * buffer and the command really did complete, so
+			 * reporting -ENODEV would make the caller treat a
+			 * hardware object it now owns as never created.
+			 */
+			if (!ctx->responded)
+				ctx->error = -ENODEV;
+			drop_resp_ref = ctx->resp_pending;
+			ctx->resp_pending = false;
+			ctx->responded = true;
+			complete(&ctx->comp_event);
+			spin_unlock_irqrestore(&ctx->lock, flags);
+
+			if (drop_resp_ref)
+				hwc_ctx_put(hwc, ctx);
+		}
+	}
+
+	/* Wait for all sender threads to finish and drop their refs
+	 * before touching the hardware or freeing anything, so no
+	 * in-flight sender is still running when this function returns;
+	 * otherwise a stranded sender would dereference gc->hwc_lock /
+	 * gc->hwc_drain_waitq after the caller frees gc.
+	 * After this, only slots held by timed-out senders whose
+	 * handle_resp() never ran remain in the bitmap.
+	 *
+	 * active_senders is only ever modified under hwc_lock, and the
+	 * last sender's wake_up() runs under that lock before it is
+	 * released.  Evaluating the condition under hwc_lock therefore
+	 * guarantees that once we observe 0 the waking sender has
+	 * already dropped the lock -- i.e. finished touching gc -- so it
+	 * cannot race the caller freeing gc after this returns.
+	 */
+	spin_lock_irq(&gc->hwc_lock);
+	wait_event_lock_irq(gc->hwc_drain_waitq,
+			    hwc->active_senders == 0, gc->hwc_lock);
+	spin_unlock_irq(&gc->hwc_lock);
+
 	/* Tear down only if setup_hwc() handed the queues to the PF.  Until
 	 * then the device never saw them, so there is nothing to undo.
 	 */
@@ -871,14 +1071,36 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	}
 	gc->max_num_cqs = 0;
 
+	/* Destroy the HWC CQ object before the TXQ and RQ.  The
+	 * active_senders drain above already guarantees no sender is
+	 * still reaching the CQ through txq->hwc_cq.
+	 */
+	if (hwc->cq)
+		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
+
 	if (hwc->txq)
 		mana_hwc_destroy_wq(hwc, hwc->txq);
 
 	if (hwc->rxq)
 		mana_hwc_destroy_wq(hwc, hwc->rxq);
 
-	if (hwc->cq)
-		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
+	/* Safety net: the force-complete loop above dropped the
+	 * response-side reference of every occupied slot and the sender
+	 * drain released the matching sender references, so nothing should
+	 * still be set here.  Release anything that is, rather than leak it.
+	 */
+	if (hwc->caller_ctx) {
+		struct hwc_caller_ctx *ctx;
+		int i;
+
+		for (i = 0; i < hwc->num_inflight_msg; i++) {
+			if (!test_bit(i, hwc->inflight_msg_res.map))
+				continue;
+
+			ctx = &hwc->caller_ctx[i];
+			hwc_ctx_put(hwc, ctx);
+		}
+	}
 
 	kfree(hwc->caller_ctx);
 	hwc->caller_ctx = NULL;
@@ -893,7 +1115,6 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	hwc->hwc_timeout = 0;
 
 	kfree(hwc);
-	gc->hwc.driver_data = NULL;
 	gc->hwc.gdma_context = NULL;
 
 	vfree(gc->cq_table);
@@ -910,6 +1131,8 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	struct hwc_caller_ctx *ctx;
 	unsigned long flags;
 	bool drop_resp_ref;
+	bool abandoned = false;
+	bool cancelled;
 	u32 dest_vrcq = 0;
 	u32 dest_vrq = 0;
 	u32 command;
@@ -955,7 +1178,25 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	/* The response-side reference (from get_msg_index) keeps the slot
 	 * alive if hardware responds right after the doorbell.
 	 */
-	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
+	/* Submit under the slot lock, so mana_hwc_destroy_channel() cannot
+	 * cancel this request between the check and the doorbell: it takes
+	 * the same lock, so it either cancels before this runs -- and the
+	 * request is never handed to the device -- or after, when the
+	 * request is genuinely in flight.  Posting is a WQE write plus a
+	 * doorbell, so it does not sleep.
+	 */
+	spin_lock_irqsave(&ctx->lock, flags);
+	cancelled = ctx->responded;
+	if (cancelled)
+		err = ctx->error;
+	else
+		err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq,
+					   false);
+	spin_unlock_irqrestore(&ctx->lock, flags);
+
+	if (cancelled)
+		goto out;
+
 	if (err) {
 		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
 		goto out;
@@ -972,9 +1213,23 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 		ctx->output_buf = NULL;
 		err = ctx->error;
 		status = ctx->status_code;
+		if (err == -EINPROGRESS) {
+			/* Give up on this request in the same critical section
+			 * that clears output_buf, so a response can never
+			 * observe the slot as "sender has not published yet"
+			 * and be discarded as premature -- that would strand
+			 * the slot, because only a response frees it.
+			 *
+			 * Keep the response-side reference: the device may
+			 * still answer, so the slot stays taken until it does
+			 * and must not be handed to another request.
+			 */
+			ctx->responded = true;
+			abandoned = true;
+		}
 		spin_unlock_irqrestore(&ctx->lock, flags);
 
-		if (err != -EINPROGRESS) {
+		if (!abandoned) {
 			/* A response raced in just after the timeout, so the
 			 * hardware is alive: keep the channel and report what
 			 * that response said rather than a timeout.  It may
@@ -986,29 +1241,25 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 			goto check_status;
 		}
 
-		if (wait_ms != 0)
+		if (wait_ms != 0) {
 			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
 				command, wait_ms);
 
-		err = -ETIMEDOUT;
-
-		/* No-wait teardown (hwc_timeout == 0) is expected to expire;
-		 * just release the slot so the next teardown command can reuse
-		 * it.
-		 */
-		if (wait_ms == 0)
-			goto out;
+			/* Genuine timeout: shorten later waits so subsequent
+			 * commands fail fast instead of each draining the
+			 * full timeout.
+			 */
+			if (hwc->hwc_timeout > 1)
+				hwc->hwc_timeout = 1;
+		}
 
-		/* Genuine timeout: shorten later waits so subsequent commands
-		 * fail fast instead of each draining the full timeout.
-		 */
-		if (hwc->hwc_timeout > 1)
-			hwc->hwc_timeout = 1;
+		err = -ETIMEDOUT;
 
-		/* Release the slot via out:; a late response no longer touches
-		 * it, so the sender must drop the reference here.
+		/* Drop only the sender's reference; the response-side one is
+		 * what keeps the slot reserved.
 		 */
-		goto out;
+		hwc_ctx_put(hwc, ctx);
+		goto done;
 	}
 
 	/* Clear output_buf and read the result under the lock; the slot may
@@ -1041,14 +1292,15 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	err = 0;
 	goto done;
 out:
-	/* Error, no-wait teardown, or timeout: drop the sender's and the
-	 * response-side references.  Latch ->responded so a racing response
-	 * is a no-op, and only drop the response-side ref if it has not.
+	/* Only reached before the request reached the hardware, so no
+	 * response can ever arrive for it: latch ->responded and drop both
+	 * the response-side and the sender's reference, freeing the slot.
 	 */
 	ctx = hwc->caller_ctx + msg_id;
 	spin_lock_irqsave(&ctx->lock, flags);
 	ctx->output_buf = NULL;
-	drop_resp_ref = !ctx->responded;
+	drop_resp_ref = ctx->resp_pending;
+	ctx->resp_pending = false;
 	ctx->responded = true;
 	spin_unlock_irqrestore(&ctx->lock, flags);
 	if (drop_resp_ref)
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 308950f9b54b0485bac66b80d63e257eaf5f787e..f97f63f8fee418f4ca36f88019690c096e6f2e6a 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -468,6 +468,21 @@ struct gdma_context {
 	/* Hardware communication channel (HWC) */
 	struct gdma_dev		hwc;
 
+	/* destroy_channel() waits here for all HWC senders to exit.
+	 * Lives on gc (not hwc) so wake_up() after the last sender's
+	 * atomic_dec doesn't dereference freed hwc memory.
+	 */
+	wait_queue_head_t	hwc_drain_waitq;
+
+	/* Serializes hwc.driver_data (the hw_channel_context pointer)
+	 * between the control-plane readers in mana_gd_send_request(),
+	 * mana_need_log() and mana_serv_reset() and the publish/clear in
+	 * mana_hwc_create_channel()/mana_hwc_destroy_channel().  All users
+	 * are control-plane (HWC commands sleep; reset runs on a workqueue),
+	 * so a plain spinlock -- not RCU -- is sufficient.
+	 */
+	spinlock_t		hwc_lock;
+
 	/* Azure network adapter */
 	struct gdma_dev		mana;
 
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index daff051472a0dd9f57ee0b9fb3d71029fca909ad..5bddc87e8e78900097974968cefb6fb2d9415132 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -164,6 +164,9 @@ struct hwc_wq {
 	u16 queue_depth;
 
 	struct hwc_cq *hwc_cq;
+
+	/* Serializes concurrent mana_gd_post_and_ring() calls. */
+	spinlock_t lock;
 };
 
 struct hwc_caller_ctx {
@@ -189,6 +192,12 @@ struct hwc_caller_ctx {
 	 * so a later or duplicate response is dropped.
 	 */
 	bool responded;
+
+	/* True while the response-side reference is still held, i.e. while a
+	 * response for this acquisition may still arrive.  Dropped exactly
+	 * once, by whoever establishes that no further response is coming.
+	 */
+	bool resp_pending;
 };
 
 struct hw_channel_context {
@@ -196,6 +205,7 @@ struct hw_channel_context {
 	struct device *dev;
 
 	u16 num_inflight_msg;
+
 	u32 max_req_msg_size;
 
 	u16 hwc_init_q_depth_max;
@@ -208,6 +218,12 @@ struct hw_channel_context {
 	struct hwc_wq *txq;
 	struct hwc_cq *cq;
 
+	/* Counts the message slots that are free to acquire.  A slot held by
+	 * a timed-out request is never posted back, so the count falls
+	 * permanently until the response that owns it arrives or teardown
+	 * reclaims it; a sender then expires in down_timeout() instead of
+	 * blocking on a slot nothing will release.
+	 */
 	struct semaphore sema;
 	struct gdma_resource inflight_msg_res;
 
@@ -215,6 +231,11 @@ struct hw_channel_context {
 	u32 pf_dest_vrcq_id;
 	u32 hwc_timeout;
 
+	/* Set after channel is fully established; cleared on teardown to
+	 * abort waiters in mana_hwc_get_msg_index() and reject new sends.
+	 */
+	bool channel_up;
+
 	/* True once mana_smc_setup_hwc() has handed the ESTABLISH_HWC message
 	 * to the PF, so the device may DMA into the HWC buffers.  That
 	 * function clears it on entry and sets it at the handover, so only a
@@ -225,6 +246,12 @@ struct hw_channel_context {
 	 */
 	bool setup_active;
 
+	/* Count of in-flight mana_gd_send_request() callers.  Protected
+	 * by gc->hwc_lock; the last sender to drop it to zero wakes
+	 * gc->hwc_drain_waitq for the mana_hwc_destroy_channel() drain.
+	 */
+	unsigned int active_senders;
+
 	struct hwc_caller_ctx *caller_ctx;
 };
 
-- 
2.43.0

[PATCH net-next v4 4/4] net: mana: add dynamic HWC queue depth with reinit path

[Long Li]

The HWC is first established at a bootstrap queue depth of 1.  Query the
device's maximum supported depth and, if larger, tear down and rebuild
the HWC queues at that depth before re-establishing the channel, so more
management commands can be in flight.  Advertise
GDMA_DRV_CAP_FLAG_1_DYN_HWC_QUEUE_DEPTH so the firmware knows the driver
supports a non-bootstrap depth.  That capability is sent by
mana_gd_verify_vf_version(), which is itself an HWC command and so
necessarily runs after the channel exists; the depth chosen here comes
from the device's own reported maximum.

mana_hwc_destroy_queues() tears down the CQ first, which deregisters the
EQ IRQ (mana_gd_deregister_irq() + synchronize_rcu()) so no interrupt
handler can touch the queues, then the TXQ, RXQ and inflight resources.

Validate the device-reported dimensions before they size DMA
allocations: take only the depth from the device and require the
negotiated message sizes to match the bootstrap ones, since mandatory
commands such as GDMA_VERIFY_VF_DRIVER_VERSION are much larger than the
protocol header minimum and mana_hwc_send_request() bounds a request
only against the slot it lands in; bound the depth by
HW_CHANNEL_MAX_QUEUE_DEPTH so the two coherent message buffers stay a
sane size; ensure
q_depth * max_msg_size plus alignment fits in u32, and cap CQ depth to
U16_MAX/2.  Round the message-buffer length up to a power of two, which
mana_gd_alloc_memory() requires and which the EQ and CQ rings already
did, so a device reporting dimensions whose product is not a power of
two still gets the larger depth instead of falling back.  Carry the
depth as u32 -- the device field is 24-bit, so
truncating to u16 on receipt could wrap a large value to a small depth
and silently pass these checks.

If the bootstrap channel cannot be torn down, keep using it at depth 1.
If the rebuild then fails before the new queue addresses were handed to
the PF, restore the bootstrap-depth channel: the device never saw them,
so the memory can be reused.  If they were already handed over the
device may still DMA into those queues, so tear the channel down once
more before reusing them -- only a successful DESTROY_HWC invalidates
the MST entries.  That matters because mana_smc_setup_hwc() marks the
handover before it reads the response, so a device that refuses the
larger depth still leaves the queues marked as handed over; retrying the
teardown lets such a device fall back to a working bootstrap channel
instead of failing probe outright.  If that teardown also fails, nothing
has established that the device is finished with the queues, so give up
rather than rebuild over them.

Reject a device-reported message size above the driver maximum, and
reset the negotiated init values before each establish so a firmware
that omits an HWC_INIT_DATA_* item cannot reuse stale dimensions or
drive an oversized DMA allocation.  The routing identities -- doorbell,
PDID and the PF destination queues -- are reset with them, since the
rebuilt queues are not the ones those described.  Because that reset
runs on every establish, a firmware that supplied the doorbell on the
first one but omits it on the second would leave INVALID_DOORBELL
behind, so refuse the channel in that case rather than let
mana_gd_ring_doorbell() turn it into an unchecked write far outside the
BAR.  The same write is already reachable without this patch when the
very first establish omits the doorbell; that is pre-existing and not
addressed here.  Anything else left unset either fails the dimension
checks above or leaves the queues unable to complete, which
mana_hwc_test_channel() already catches.  Finally, refuse a
rebuilt channel whose report contradicts the one its queues were built
from -- a shallower queue or smaller message limits -- rather than
oversubscribe what the device now admits to.

Signed-off-by: Long Li <longli@microsoft.com>
---
 .../net/ethernet/microsoft/mana/hw_channel.c  | 297 +++++++++++++++++-
 include/net/mana/gdma.h                       |   4 +
 include/net/mana/hw_channel.h                 |   9 +-
 3 files changed, 304 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 91fcf7c092113133d392f0e43f2eda62ca5cb919..83df4f194ee3bb41affb2110553d502322057b98 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -223,7 +223,12 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
 			break;
 
 		case HWC_INIT_DATA_QUEUE_DEPTH:
-			hwc->hwc_init_q_depth_max = (u16)val;
+			/* HWC_INIT_DATA_QUEUE_DEPTH is a 24-bit field.  Keep
+			 * the full device-reported value here; it is clamped
+			 * and validated in mana_hwc_create_channel() rather
+			 * than silently truncated to u16.
+			 */
+			hwc->hwc_init_q_depth_max = val;
 			break;
 
 		case HWC_INIT_DATA_MAX_REQUEST:
@@ -553,7 +558,11 @@ static int mana_hwc_alloc_dma_buf(struct hw_channel_context *hwc, u16 q_depth,
 
 	dma_buf->num_reqs = q_depth;
 
-	buf_size = MANA_PAGE_ALIGN(q_depth * max_msg_size);
+	/* mana_gd_alloc_memory() only accepts a power-of-two length, as
+	 * already assumed for the EQ and CQ rings above.  The slots are
+	 * carved from the head of the buffer, so any tail is unused.
+	 */
+	buf_size = roundup_pow_of_two(MANA_PAGE_ALIGN(q_depth * max_msg_size));
 
 	gmi = &dma_buf->mem_info;
 	err = mana_gd_alloc_memory(gc, buf_size, gmi, false);
@@ -780,7 +789,7 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
 	return err;
 }
 
-static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
+static int mana_hwc_establish_channel(struct gdma_context *gc, u32 *q_depth,
 				      u32 *max_req_msg_size,
 				      u32 *max_resp_msg_size)
 {
@@ -797,6 +806,21 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	struct gdma_queue *cq = hwc->cq->gdma_cq;
 	int err;
 
+	/* Clear the values a previous establish left behind so a firmware
+	 * that omits an HWC_INIT_DATA_* item on this cycle cannot silently
+	 * reuse stale dimensions from the last one.  The same applies to the
+	 * routing identities: the queues are rebuilt from scratch, so a
+	 * doorbell, PDID or PF destination left over from the previous
+	 * channel does not describe them.
+	 */
+	hwc->hwc_init_q_depth_max = 0;
+	hwc->hwc_init_max_req_msg_size = 0;
+	hwc->hwc_init_max_resp_msg_size = 0;
+	gc->hwc.doorbell = INVALID_DOORBELL;
+	gc->hwc.pdid = INVALID_PDID;
+	hwc->pf_dest_vrq_id = 0;
+	hwc->pf_dest_vrcq_id = 0;
+
 	init_completion(&hwc->hwc_init_eqe_comp);
 
 	err = mana_smc_setup_hwc(&gc->shm_channel, false,
@@ -815,6 +839,20 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
 	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
 
+	/* The doorbell was cleared before the handshake, so a firmware that
+	 * signals INIT_DONE without sending GDMA_EQE_HWC_INIT_EQ_ID_DB
+	 * leaves INVALID_DOORBELL behind.  mana_gd_ring_doorbell() turns
+	 * that into gc->db_page_base + gc->db_page_size * 0xffffffff, an
+	 * unchecked MMIO write far outside the mapped BAR, and the channel
+	 * test below rings it.  Everything else the device reports either
+	 * fails the dimension checks in mana_hwc_create_channel() or leaves
+	 * the queues unable to complete, which that test already catches.
+	 */
+	if (gc->hwc.doorbell == INVALID_DOORBELL) {
+		dev_err(hwc->dev, "HWC: no doorbell in init data\n");
+		return -EPROTO;
+	}
+
 	/* Both were set in mana_hwc_init_event_handler(). */
 	if (WARN_ON(cq->id >= gc->max_num_cqs))
 		return -EPROTO;
@@ -833,6 +871,12 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 {
 	int err;
 
+	/* CQ depth is q_depth * 2 (SQ + RQ) passed as u16 to create_cq.
+	 * Cap to prevent u16 truncation.
+	 */
+	if (q_depth > U16_MAX / 2)
+		q_depth = U16_MAX / 2;
+
 	err = mana_hwc_init_inflight_msg(hwc, q_depth);
 	if (err)
 		return err;
@@ -872,13 +916,64 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 	return err;
 }
 
+/* Tear down all HWC queues and free associated resources.  Used on
+ * the reinit-with-higher-queue-depth path and reinit fallback.
+ *
+ * PRECONDITION: must be called only during channel bring-up in
+ * mana_hwc_create_channel(), before the channel carries traffic:
+ * channel_up is still false, caller_ctx is not yet allocated, the
+ * data path is not probed yet, and active_senders is 0 — so no
+ * request or response user can reach these queues.  That is why this
+ * skips the hwc_lock-protected driver_data clear + active_senders
+ * drain that mana_hwc_destroy_channel() needs for the runtime
+ * teardown race; only the CQ-first ordering below (to fence off a
+ * pending interrupt) is required.  Bring-up itself runs under the
+ * PCI/PM device_lock, or under GC_IN_SERVICE on the service path;
+ * those two do not exclude each other, so a service reset racing a PM
+ * transition is not serialized — but that is pre-existing and applies
+ * equally to mana_hwc_destroy_channel(), which frees the same
+ * objects.  Calling this on a live, published channel would be a
+ * use-after-free.
+ */
+static void mana_hwc_destroy_queues(struct hw_channel_context *hwc)
+{
+	struct gdma_context *gc = hwc->gdma_dev->gdma_context;
+
+	/* Destroy CQ first to deregister the EQ from the interrupt
+	 * handler list before freeing caller_ctx, TXQ, or RXQ memory.
+	 * A pending interrupt handler could still reach handle_resp()
+	 * which dereferences caller_ctx.
+	 */
+	if (hwc->cq) {
+		mana_hwc_destroy_cq(gc, hwc->cq);
+		hwc->cq = NULL;
+	}
+
+	kfree(hwc->caller_ctx);
+	hwc->caller_ctx = NULL;
+
+	if (hwc->txq) {
+		mana_hwc_destroy_wq(hwc, hwc->txq);
+		hwc->txq = NULL;
+	}
+
+	if (hwc->rxq) {
+		mana_hwc_destroy_wq(hwc, hwc->rxq);
+		hwc->rxq = NULL;
+	}
+
+	mana_gd_free_res_map(&hwc->inflight_msg_res);
+	hwc->num_inflight_msg = 0;
+}
+
 int mana_hwc_create_channel(struct gdma_context *gc)
 {
 	u32 max_req_msg_size, max_resp_msg_size;
 	struct gdma_dev *gd = &gc->hwc;
 	struct hw_channel_context *hwc;
+	struct gdma_queue **old_cq_table;
 	unsigned long flags;
-	u16 q_depth_max;
+	u32 q_depth_max;
 	int err;
 
 	hwc = kzalloc_obj(*hwc);
@@ -926,8 +1021,200 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 		goto out;
 	}
 
+	/* The channel was bootstrapped at a minimal queue depth.  If the
+	 * device reports a higher maximum, tear down and rebuild with
+	 * the larger depth so more HWC commands can be in flight.
+	 */
+	if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {
+		/* q_depth_max now carries the full device-reported value
+		 * (HWC_INIT_DATA_QUEUE_DEPTH is 24-bit).  Clamp it before
+		 * the overflow check below, so an over-large but otherwise
+		 * valid depth is reduced instead of wrapping or being
+		 * rejected.  The bound also keeps the two coherent DMA
+		 * buffers, which scale with the depth, to a sane size.
+		 */
+		if (q_depth_max > HW_CHANNEL_MAX_QUEUE_DEPTH)
+			q_depth_max = HW_CHANNEL_MAX_QUEUE_DEPTH;
+
+		/* Sanity-check device-reported values before using them to
+		 * size DMA allocations.  Only the depth is taken from the
+		 * device: the rebuilt queues must keep the bootstrap
+		 * message sizes, because the rest of the driver already
+		 * assumes it can send any request up to
+		 * HW_CHANNEL_MAX_REQUEST_SIZE -- mandatory commands such as
+		 * GDMA_VERIFY_VF_DRIVER_VERSION are far larger than the
+		 * protocol header minimum, and mana_hwc_send_request() only
+		 * bounds a request against the slot it lands in.  Also check
+		 * that q_depth * max_msg_size plus alignment headroom fits
+		 * in u32 (for mana_hwc_alloc_dma_buf's MANA_PAGE_ALIGN).
+		 */
+		if (max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
+		    max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE ||
+		    (u64)q_depth_max * max_req_msg_size >
+			U32_MAX - MANA_PAGE_SIZE ||
+		    (u64)q_depth_max * max_resp_msg_size >
+			U32_MAX - MANA_PAGE_SIZE) {
+			dev_err(hwc->dev,
+				"HWC: invalid dims q=%u req=%u resp=%u\n",
+				q_depth_max, max_req_msg_size,
+				max_resp_msg_size);
+			q_depth_max = HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH;
+			goto skip_reinit;
+		}
+
+		err = mana_smc_teardown_hwc(&gc->shm_channel, false);
+		if (err) {
+			/* Keep using the bootstrap-depth channel.  The
+			 * destroy request may already have been written to
+			 * the PF before the response failed, so the PF may
+			 * have invalidated the MST entries; nothing is freed
+			 * here, and mana_hwc_test_channel() below fails the
+			 * channel creation if the queues are no longer
+			 * usable.
+			 */
+			dev_err(hwc->dev,
+				"Failed to teardown HWC for reinit: %d\n",
+				err);
+			q_depth_max = HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH;
+			goto skip_reinit;
+		}
+
+		hwc->setup_active = false;
+
+		/* Destroy the queues before the CQ table they refer to:
+		 * mana_hwc_destroy_queues() releases the HWC CQ, so it must
+		 * run while cq_table is still valid.
+		 */
+		mana_hwc_destroy_queues(hwc);
+
+		old_cq_table = gc->cq_table;
+		gc->cq_table = NULL;
+		/* Clear the bound with the table: mana_gd_destroy_cq() gates
+		 * on max_num_cqs before indexing cq_table, so leaving a stale
+		 * bound behind would let it dereference the NULL table.
+		 */
+		gc->max_num_cqs = 0;
+		synchronize_rcu();
+		vfree(old_cq_table);
+
+		err = mana_hwc_init_queues(hwc, q_depth_max,
+					   max_req_msg_size,
+					   max_resp_msg_size);
+		if (err) {
+			dev_err(hwc->dev, "Failed to reinit HWC: %d\n", err);
+			goto reinit_fallback;
+		}
+
+		err = mana_hwc_establish_channel(gc, &q_depth_max,
+						 &max_req_msg_size,
+						 &max_resp_msg_size);
+		if (!err &&
+		    (q_depth_max < hwc->num_inflight_msg ||
+		     max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
+		     max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE)) {
+			/* The rebuilt channel contradicts the report its own
+			 * queues were built from: a shallower queue would be
+			 * oversubscribed by the slots already allocated, and
+			 * smaller message limits would be exceeded by every
+			 * command sized for the buffers already allocated.
+			 * The queues are handed to the PF by now, so give up
+			 * rather than run past what the device admits to.
+			 */
+			dev_err(hwc->dev,
+				"HWC: rebuilt q=%u req=%u resp=%u, built for %u/%u/%u\n",
+				q_depth_max, max_req_msg_size,
+				max_resp_msg_size, hwc->num_inflight_msg,
+				HW_CHANNEL_MAX_REQUEST_SIZE,
+				HW_CHANNEL_MAX_RESPONSE_SIZE);
+			err = -EPROTO;
+		}
+		if (err) {
+			dev_err(hwc->dev, "Failed to re-establish HWC: %d\n",
+				err);
+			/* setup_active tells us whether this attempt got
+			 * as far as handing the queue addresses to the PF.
+			 * If it did not, the device never saw them and the
+			 * bootstrap fallback can safely reuse the memory.
+			 *
+			 * If it did, the mappings are live and rebuilding
+			 * over them would let the device DMA into queues
+			 * this path is about to free.  Tear the channel down
+			 * once more first: only a successful DESTROY_HWC
+			 * invalidates the MST entries, and that is what makes
+			 * the fallback safe again -- so a device that refuses
+			 * the larger depth still ends up on a working
+			 * bootstrap channel rather than failing probe
+			 * outright.  If that teardown also fails, nothing has
+			 * established that the device is finished with the
+			 * queues, so give up rather than reuse them.
+			 */
+			if (hwc->setup_active) {
+				if (mana_smc_teardown_hwc(&gc->shm_channel,
+							  false)) {
+					dev_err(hwc->dev,
+						"Failed to tear down HWC after failed reinit\n");
+					goto out;
+				}
+				hwc->setup_active = false;
+			}
+			goto reinit_fallback;
+		}
+	}
+
+	goto skip_reinit;
+
+reinit_fallback:
+	/* Restore bootstrap-depth channel so the device remains functional.
+	 * Free cq_table if it was allocated by a partially successful
+	 * establish attempt.
+	 */
+	dev_warn(hwc->dev, "HWC reinit failed, falling back to bootstrap depth\n");
+
+	mana_hwc_destroy_queues(hwc);
+
+	old_cq_table = gc->cq_table;
+	gc->cq_table = NULL;
+	/* Clear the bound with the table, as above. */
+	gc->max_num_cqs = 0;
+	synchronize_rcu();
+	vfree(old_cq_table);
+
+	err = mana_hwc_init_queues(hwc, HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
+				   HW_CHANNEL_MAX_REQUEST_SIZE,
+				   HW_CHANNEL_MAX_RESPONSE_SIZE);
+	if (err) {
+		dev_err(hwc->dev, "Failed to restore bootstrap HWC: %d\n", err);
+		goto out;
+	}
+
+	err = mana_hwc_establish_channel(gc, &q_depth_max, &max_req_msg_size,
+					 &max_resp_msg_size);
+	if (!err &&
+	    (max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
+	     max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE)) {
+		/* The queues above were rebuilt with the bootstrap sizes, so
+		 * a handshake that now reports different limits describes
+		 * queues that do not exist.  Commands are only bounded by the
+		 * slots they land in, so they would be sized for the
+		 * bootstrap limits and could exceed what the device accepts.
+		 */
+		dev_err(hwc->dev, "HWC: bootstrap reports req=%u resp=%u\n",
+			max_req_msg_size, max_resp_msg_size);
+		err = -EPROTO;
+	}
+	if (err) {
+		dev_err(hwc->dev, "Failed to re-establish bootstrap HWC: %d\n",
+			err);
+		goto out;
+	}
+
+skip_reinit:
+
+	/* No RCU needed: still in mana_hwc_create_channel, the
+	 * pointer has not been published to concurrent senders yet.
+	 */
 	err = mana_hwc_test_channel(gc->hwc.driver_data,
-				    HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
+				    hwc->num_inflight_msg,
 				    max_req_msg_size, max_resp_msg_size);
 	if (err) {
 		dev_err(hwc->dev, "Failed to test HWC: %d\n", err);
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index f97f63f8fee418f4ca36f88019690c096e6f2e6a..b435b2039c1f9d469f0f054ed37625cdfd1e1212 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -698,6 +698,9 @@ enum {
 /* Driver supports dynamic interrupt moderation - DIM */
 #define GDMA_DRV_CAP_FLAG_1_DYN_INTERRUPT_MODERATION BIT(28)
 
+/* Driver supports dynamic queue depth for HWC */
+#define GDMA_DRV_CAP_FLAG_1_DYN_HWC_QUEUE_DEPTH BIT(29)
+
 /* Driver supports non-contiguous queue buffers */
 #define GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS BIT(30)
 
@@ -716,6 +719,7 @@ enum {
 	 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_DYN_HWC_QUEUE_DEPTH | \
 	 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)
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 5bddc87e8e78900097974968cefb6fb2d9415132..c2954f1f613736e7107c54d316bb12a804e83817 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -11,6 +11,13 @@
 
 #define HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH 1
 
+/* Upper bound on the device-reported HWC queue depth.  The HWC is a
+ * control channel, so a modest depth already keeps enough management
+ * commands in flight, while the depth directly scales two coherent DMA
+ * buffers of q_depth * max_msg_size bytes each.
+ */
+#define HW_CHANNEL_MAX_QUEUE_DEPTH 128
+
 #define HWC_INIT_DATA_CQID		1
 #define HWC_INIT_DATA_RQID		2
 #define HWC_INIT_DATA_SQID		3
@@ -208,7 +215,7 @@ struct hw_channel_context {
 
 	u32 max_req_msg_size;
 
-	u16 hwc_init_q_depth_max;
+	u32 hwc_init_q_depth_max;
 	u32 hwc_init_max_req_msg_size;
 	u32 hwc_init_max_resp_msg_size;
 
-- 
2.43.0

Re: [PATCH net-next v4 1/4] net: mana: track when the HWC has been handed to the PF

[netdev-bot+sashiko]

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

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

- [Medium] Commit message mischaracterizes the change: "No functional
  change is intended for the current single-establish flow" is not…
- [Low] The teardown-failure branch of mana_hwc_destroy_channel() is an
  inert store, and the documentation added by the patch describes…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is now
  explicitly acknowledged in code and left unfixed: when DESTROY_HWC…

--

net: mana: track when the HWC has been handed to the PF

The patch replaces the gc->max_num_cqs test in mana_hwc_destroy_channel()
with a new hw_channel_context::setup_active flag.  mana_smc_setup_hwc()
clears the flag on entry and sets it just before the ESTABLISH_HWC message
is written to shared memory, so teardown can tell whether the PF was ever
given the queue addresses.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 263e7c4e29341..75fdccdc8c482 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -683,7 +683,7 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  				 cq->mem_info.dma_handle,
>  				 rq->mem_info.dma_handle,
>  				 sq->mem_info.dma_handle,
> -				 eq->eq.msix_index);
> +				 eq->eq.msix_index, &hwc->setup_active);
>  	if (err)
>  		return err;
>  
> @@ -815,13 +815,20 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	if (!hwc)
>  		return;
>  
> -	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
> -	 * non-zero, the HWC worked and we should tear down the HWC here.
> +	/* Tear down only if setup_hwc() handed the queues to the PF.  Until
> +	 * then the device never saw them, so there is nothing to undo.
>  	 */
> -	if (gc->max_num_cqs > 0) {
> -		mana_smc_teardown_hwc(&gc->shm_channel, false);
> -		gc->max_num_cqs = 0;
> +	if (hwc->setup_active) {

[Severity: Medium]
The commit message says:

    No functional change is intended for the current single-establish flow.

Is that accurate for the failure paths?  gc->max_num_cqs is written in
exactly one place, mana_hwc_init_event_handler():

	case HWC_INIT_DATA_MAX_NUM_CQS:
		gd->gdma_context->max_num_cqs = val;

so it stays zero until the PF has processed ESTABLISH_HWC and started
delivering bootstrap EQEs.  hwc->setup_active is set in
mana_smc_setup_hwc() immediately before the writel() loop, so the new
condition is a strict superset of the old one.

Two paths reach mana_hwc_destroy_channel() with gc->max_num_cqs still 0 and
now send DESTROY_HWC where they previously sent nothing:

mana_hwc_establish_channel()
	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
		return -ETIMEDOUT;

mana_smc_setup_hwc()
	err = mana_smc_read_response(sc, SMC_MSG_TYPE_ESTABLISH_HWC,
				     SMC_MSG_TYPE_ESTABLISH_HWC_VERSION,
				     reset_vf);
	if (err) {
		dev_err(sc->dev, "Error when setting up HWC: %d\n", err);
		return err;
	}

On those paths mana_smc_teardown_hwc() polls mana_smc_poll_register() for up
to 20 * 1000 iterations of usleep_range(1000, 2000) and then polls again
inside mana_smc_read_response(), so a failing probe or resume against a
wedged PF can now take tens of seconds longer than before.

Should the changelog describe the error-path change and the extra polling
delay instead of stating no functional change?  Since the newly attempted
teardown is also what closes the existing window where the queue memory is
freed while the PF still holds its addresses on the establish-timeout path,
would a Fixes: tag be appropriate here?

> +		/* Only a successful teardown invalidates the MST entries.  If
> +		 * it fails the device may still be using the queues, so leave
> +		 * the flag set rather than record a clean teardown.
> +		 */
> +		if (!mana_smc_teardown_hwc(&gc->shm_channel, false))
> +			hwc->setup_active = false;
> +		else
> +			dev_err(hwc->dev, "Failed to tear down HWC\n");
>  	}
> +	gc->max_num_cqs = 0;

[Severity: Low]
The comment explains that the flag is deliberately left set for a later
reader, but is there a reader left?  mana_hwc_destroy_channel() frees hwc a
few lines further down in the same function:

	kfree(hwc);
	gc->hwc.driver_data = NULL;

and mana_hwc_destroy_channel() is the only place that reads setup_active;
the only other references are the *submitted stores in mana_smc_setup_hwc()
and the &hwc->setup_active argument passed by
mana_hwc_establish_channel().  mana_hwc_create_channel() allocates a fresh
zeroed hw_channel_context, so a new attempt starts with setup_active ==
false regardless.

Does the else branch have any observable effect today, or is the retained
value discarded by the kfree(hwc) below?

>  
>  	if (hwc->txq)
>  		mana_hwc_destroy_wq(hwc, hwc->txq);

[Severity: High]
This isn't a bug introduced by this patch, but the new comment now states
the hazard explicitly while the code still proceeds: when
mana_smc_teardown_hwc() fails, "the device may still be using the queues",
yet the queue memory is freed unconditionally right here.

mana_hwc_destroy_wq() releases both the message buffer and the ring:

	mana_hwc_dealloc_dma_buf(hwc, hwc_wq->msg_buf);

	if (hwc_wq->gdma_wq)
		mana_gd_destroy_queue(hwc->gdma_dev->gdma_context,
				      hwc_wq->gdma_wq);

and both paths end in mana_gd_free_memory():

	dma_free_coherent(gmi->dev, PAGE_SIZE, gmi->pages_va[i],
			  gmi->pages_dma[i]);

mana_smc_teardown_hwc() documents why the response matters:

	/* Read shmem response (polling for VF possession) and validate.
	 * For teardown, waiting for response is required to ensure hardware
	 * invalidates MST entries before software frees memory.
	 */

A non-zero return therefore means the driver has no confirmation that the
MST entries were invalidated (mana_smc_poll_register() timed out, or the
response header did not confirm DESTROY_HWC).  Can the PF keep DMAing
completions and EQEs into pages that dma_free_coherent() has already
returned to the allocator in that case?  Would it be better to leak or
quarantine the HWC buffers when teardown is not confirmed rather than free
them?  The same log-only-and-continue sequence is still present at the tip
of the series (mana_hwc_destroy_channel() in c850f2d5).

> diff --git a/drivers/net/ethernet/microsoft/mana/shm_channel.c b/drivers/net/ethernet/microsoft/mana/shm_channel.c
> index d21b5db06e509..1ec8e3661721d 100644
> --- a/drivers/net/ethernet/microsoft/mana/shm_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/shm_channel.c
> @@ -144,6 +150,8 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
>  	int err;
>  	int i;
>  
> +	*submitted = false;
> +
>  	/* Ensure VF already has possession of shared memory */
>  	err = mana_smc_poll_register(sc->base, false);
>  	if (err) {

[Severity: Low]
The clear here is unconditional and happens before the three pre-handover
exits (poll timeout, non-page-aligned addresses, msix index outside
VECTOR_MASK).  If a future caller ever enters mana_smc_setup_hwc() with a
handover still outstanding, would one of those exits silently downgrade the
flag to "device untouched" and make mana_hwc_destroy_channel() skip
DESTROY_HWC?  I checked that every mana_smc_setup_hwc() call in
mana_hwc_create_channel() at the series tip is preceded by a successful
mana_smc_teardown_hwc() plus hwc->setup_active = false, so this is not
reachable today, only a trap for the next caller.

[ ... ]

> diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
> index 16feb39616c1b..237608b488e5e 100644
> --- a/include/net/mana/hw_channel.h
> +++ b/include/net/mana/hw_channel.h
> @@ -199,6 +199,16 @@ struct hw_channel_context {
>  	u32 pf_dest_vrcq_id;
>  	u32 hwc_timeout;
>  
> +	/* True once mana_smc_setup_hwc() has handed the ESTABLISH_HWC message
> +	 * to the PF, so the device may DMA into the HWC buffers.  That
> +	 * function clears it on entry and sets it at the handover, so only a
> +	 * failure before the handover leaves it false; a failure after it --
> +	 * including one reported by mana_hwc_establish_channel() -- leaves it
> +	 * set, which is what makes teardown attempt DESTROY_HWC.  Cleared
> +	 * again once that teardown succeeds.
> +	 */
> +	bool setup_active;
> +
>  	struct hwc_caller_ctx *caller_ctx;
>  };

[Severity: Low]
"Cleared again once that teardown succeeds" reads as state that survives an
attempt, but the field lives in the object that mana_hwc_destroy_channel()
kfree()s and that mana_hwc_create_channel() re-allocates zeroed.  Could the
comment say that the flag only lives for the lifetime of one
hw_channel_context?

[ ... ]

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

Re: [PATCH net-next v4 2/4] net: mana: give each HWC message slot its own completion state

[netdev-bot+sashiko]

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

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

- [Medium] Undisclosed behavioural changes bundled into a patch whose
  changelog says "No functional change is intended" and never mentions…
- [Medium] The patch adds a guard in mana_gd_query_hwc_timeout()
  (gdma_main.c:343) plus an in-tree comment declaring that a device-…

Pre-existing issues:
- [High] mana_hwc_rx_event_handler()
  (drivers/net/ethernet/microsoft/mana/hw_channel.c:315-318) returns…
- [High] mana_gd_query_hwc_timeout() sanitises only the zero case of the
  device-reported timeout and still accepts any other u32 (`struct…
- [Medium] DISCARDED (fixed later in this series).
- [Medium] DISCARDED (fixed later in this series).

--

net: mana: give each HWC message slot its own completion state

Each HWC message slot gains a per-slot spinlock, a refcount held by the
sender and the response handler, and a responded flag, so the sender's
timeout path and mana_hwc_handle_resp() can own the slot independently.
ctx->error also changes from u32 to int, and mana_gd_query_hwc_timeout()
now ignores a device-reported timeout of zero.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index f92b2d0bf926e..a023d3e1a95de 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -331,7 +331,13 @@ static int mana_gd_query_hwc_timeout(struct pci_dev *pdev, u32 *timeout_val)
>  	if (err || resp.hdr.status)
>  		return err ? err : -EPROTO;
>  
> -	*timeout_val = resp.timeout_ms;
> +	/* Zero is the driver's own "do not wait, do not log" sentinel, set by
> +	 * mana_serv_reset() when the HWC has stopped responding.  A zero from
> +	 * the device would enter that state instead: ignore it and keep the
> +	 * caller's positive value.
> +	 */
> +	if (resp.timeout_ms)
> +		*timeout_val = resp.timeout_ms;
>  
>  	return 0;
>  }

[Severity: Medium]
The changelog says "No functional change is intended at the current queue
depth of one" and never mentions gdma_main.c, but this hunk changes
behaviour on its own.  The caller is:

  mana_gd_query_hwc_timeout(pdev, &hwc->hwc_timeout);

and hwc_timeout == 0 is the driver's own sentinel:

  mana_need_log():
	if (hwc && hwc->hwc_timeout == 0)
		return false;

So a device answering GDMA_QUERY_HWC_TIMEOUT with 0 previously disabled
all HWC waiting and all HWC logging.  Is this hunk a fix that wants its
own patch and a Fixes: tag rather than being folded into a preparation
patch?

Two further semantic changes in mana_hwc_send_request() are not described
either.  The timeout path now reports a response that raced in after the
wait expired:

		if (err != -EINPROGRESS) {
			hwc_ctx_put(hwc, ctx);
			goto check_status;
		}

which returns that response's status (possibly -EPROTO) instead of
-ETIMEDOUT, and skips the hwc->hwc_timeout = 1 clamp.  And the new
"!ctx->output_buf || ctx->responded" gate in mana_hwc_handle_resp() stops
a response arriving after its sender timed out from being memcpy'd through
a stale ctx->output_buf that usually pointed into a dead stack frame.

Could the changelog spell these out, or could they be split off?

[Severity: Medium]
The new comment states that a device-supplied value of 0 must be rejected
because 0 is reserved for internal driver state.  The other writer of the
same field, fed from the same device-controlled source, is not changed:

  mana_hwc_init_event_handler():
		case HWC_DATA_CFG_HWC_TIMEOUT:
			hwc->hwc_timeout = val;
			break;

Can a GDMA_EQE_HWC_SOC_RECONFIG_DATA event carrying HWC_DATA_CFG_HWC_TIMEOUT
with val == 0 install exactly the state this guard is meant to prevent?
msecs_to_jiffies(0) would make every subsequent command expire at once,
mana_hwc_send_request() would take the new "No-wait teardown
(hwc_timeout == 0) is expected to expire" branch for ordinary commands
(skipping both the error log and the clamp), and mana_need_log() would
suppress the diagnostics.

Should the zero check live where hwc_timeout is assigned, so both ingest
points agree?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 75fdccdc8c482..0056bdd8c53f5 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -19,6 +21,17 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
>  	index = find_first_zero_bit(hwc->inflight_msg_res.map,
>  				    hwc->inflight_msg_res.size);
>  
> +	ctx = &hwc->caller_ctx[index];
> +	reinit_completion(&ctx->comp_event);
> +	/* Take both references (sender + handle_resp) before publishing the
> +	 * slot, so an early response cannot free it under the sender.
> +	 */
> +	refcount_set(&ctx->refcnt, 2);
> +	ctx->responded = false;
> +	ctx->msg_id = index;
> +	ctx->error = -EINPROGRESS;
> +
> +	/* Publish the slot last, after it is fully initialised. */
>  	bitmap_set(hwc->inflight_msg_res.map, index, 1);
>  
>  	spin_unlock_irqrestore(&r->lock, flags);
> @@ -40,6 +53,13 @@ static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
>  	up(&hwc->sema);
>  }
>  
> +static void hwc_ctx_put(struct hw_channel_context *hwc,
> +			struct hwc_caller_ctx *ctx)
> +{
> +	if (refcount_dec_and_test(&ctx->refcnt))
> +		mana_hwc_put_msg_index(hwc, ctx->msg_id);
> +}
> +

[ ... ]

> @@ -90,22 +110,35 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	}
>  
>  	ctx = hwc->caller_ctx + msg_id;
> -	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> -	if (err)
> -		goto out;
>  
> -	ctx->status_code = resp_msg->status;
> +	spin_lock(&ctx->lock);
>  
> -	memcpy(ctx->output_buf, resp_msg, resp_len);
> -out:
> +	/* Honour a response only while the sender owns the slot (output_buf
> +	 * published) and has not already been answered; otherwise drop it as
> +	 * premature, stale or duplicate without touching the refcount.
> +	 */
> +	if (!ctx->output_buf || ctx->responded) {
> +		spin_unlock(&ctx->lock);
> +		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> +		return;
> +	}
> +	ctx->responded = true;

[Severity: High]
This isn't a bug introduced by this patch, but every drop path in
mana_hwc_handle_resp() now carefully reposts the RX WQE, including the new
premature/stale/duplicate path above and the invalid-bitmap-bit path, while
the caller still has one path that does not:

  mana_hwc_rx_event_handler():
	msg_id = READ_ONCE(resp->response.hwc_msg_id);
	if (msg_id >= hwc->num_inflight_msg) {
		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
		return;
	}

rx_req has already been resolved at that point, and RX WQEs are armed only
once at bring-up:

  mana_hwc_test_channel():
	for (i = 0; i < q_depth; i++) {
		req = &hwc_rxq->msg_buf->reqs[i];
		err = mana_hwc_post_rx_wqe(hwc_rxq, req);

afterwards they are only re-armed from mana_hwc_handle_resp().  Since
msg_id comes from the shared DMA response buffer, can a host writing a
value >= hwc->num_inflight_msg drop the only RQ buffer at the bootstrap
depth of one, leaving every later mana_hwc_send_request() to time out
forever?  The same early return is still there at the end of the series.

>  
> +	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> +	if (!err) {
> +		ctx->status_code = resp_msg->status;
> +		memcpy(ctx->output_buf, resp_msg, resp_len);
> +	}
>  	ctx->error = err;
>  
> -	/* Must post rx wqe before complete(), otherwise the next rx may
> -	 * hit no_wqe error.
> +	/* Post RX WQE before completing — the next response may arrive
> +	 * immediately and needs a posted buffer.
>  	 */
>  	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> -
>  	complete(&ctx->comp_event);
> +	spin_unlock(&ctx->lock);
> +
> +	hwc_ctx_put(hwc, ctx);
>  }

[Severity: Medium]
This is a pre-existing ordering issue rather than something this patch
introduces, but the patch adds more accesses after complete().
mana_hwc_destroy_channel() frees the work queues before the CQ/EQ:

	if (hwc->txq)
		mana_hwc_destroy_wq(hwc, hwc->txq);

	if (hwc->rxq)
		mana_hwc_destroy_wq(hwc, hwc->rxq);

	if (hwc->cq)
		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);

and only mana_hwc_destroy_cq() reaches mana_gd_deregiser_irq(), whose
list_del_rcu() plus synchronize_rcu() fences off an in-flight EQ handler.
hwc->rxq and hwc->txq are not set to NULL either.

If mana_smc_teardown_hwc() failed and the device still delivers a response
in that window, can mana_hwc_post_rx_wqe(hwc->rxq, rx_req) here touch the
freed hwc_wq and msg_buf?  The hwc_ctx_put(hwc, ctx) added after complete()
also touches ctx->refcnt, ctx->msg_id, hwc->inflight_msg_res and hwc->sema.

For the record, the destroy order is corrected later in this series, where
the CQ is destroyed before the TXQ and RQ after channel_up is cleared and
active_senders drained, so this comment only applies to the intermediate
state at this commit.

[ ... ]

> @@ -902,43 +952,108 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  		dest_vrcq = hwc->pf_dest_vrcq_id;
>  	}
>  
> +	/* The response-side reference (from get_msg_index) keeps the slot
> +	 * alive if hardware responds right after the doorbell.
> +	 */
>  	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
>  	if (err) {
>  		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
>  		goto out;
>  	}
>  
> +	wait_ms = hwc->hwc_timeout;
>  	if (!wait_for_completion_timeout(&ctx->comp_event,
> -					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
> +					 msecs_to_jiffies(wait_ms))) {

[Severity: High]
This problem wasn't introduced by this patch, but the patch hardens the
same assignment against the benign value while leaving the damaging one
unchecked.  resp.timeout_ms is a plain u32 and lands directly in
hwc->hwc_timeout, so wait_ms here is fully device-controlled.

__msecs_to_jiffies() returns MAX_JIFFY_OFFSET for any value whose signed
interpretation is negative, i.e. >= 0x80000000, and
wait_for_completion_timeout() sleeps in TASK_UNINTERRUPTIBLE.

Can a device that reports timeout_ms = 0xffffffff and then stops answering
wedge the calling thread indefinitely, with no way to kill it?  Even
0x7fffffff gives roughly a 24 day wait, and HWC commands are issued from
paths holding rtnl_lock.  The event path in mana_hwc_init_event_handler()
stores up to 0xffffff ms unbounded as well.  Would a clamp against a sane
maximum at both writers be appropriate?  The gap is still present at the
end of the series.

[ ... ]

>  out:
> -	mana_hwc_put_msg_index(hwc, msg_id);
> +	/* Error, no-wait teardown, or timeout: drop the sender's and the
> +	 * response-side references.  Latch ->responded so a racing response
> +	 * is a no-op, and only drop the response-side ref if it has not.
> +	 */
> +	ctx = hwc->caller_ctx + msg_id;
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	ctx->output_buf = NULL;
> +	drop_resp_ref = !ctx->responded;
> +	ctx->responded = true;
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +	if (drop_resp_ref)
> +		refcount_dec(&ctx->refcnt);
> +	hwc_ctx_put(hwc, ctx);
> +done:
>  	return err;
>  }

[Severity: Medium]
This is pre-existing behaviour rather than something the patch introduces,
and the changelog already acknowledges it, but it may be worth spelling out
what happens in this intermediate state.

On a genuine timeout this path drops both references, so
mana_hwc_put_msg_index() clears the bitmap bit and posts the semaphore
while a response for that msg_id may still be in flight.

mana_hwc_get_msg_index() then hands the same index (always 0 at depth one)
to the next command and resets the slot:

	refcount_set(&ctx->refcnt, 2);
	ctx->responded = false;

so the old response passes the gate in mana_hwc_handle_resp():

	if (!ctx->output_buf || ctx->responded) {

Does that mean the stale payload is memcpy'd into the new caller's buffer,
ctx->error set to 0, and complete() waking the new sender, which returns
success carrying another command's response?

The next patch in the series keeps the response-side reference across a
timeout so the slot is never handed out again until the device answers,
which removes this window; the note is only about the state at this commit.

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

Re: [PATCH net-next v4 3/4] net: mana: support concurrent HWC requests

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 7 · Low: 5

- [Medium] mana_hwc_send_request() no longer releases the message slot
  when the wait budget is zero (the deleted `if (wait_ms == 0) goto…
- [Medium] mana_hwc_destroy_channel() is silently reordered so that
  mana_hwc_destroy_cq() runs before mana_hwc_destroy_wq(txq)/(rxq), and…
- [Medium] The new gc->hwc_lock contract documented in
  include/net/mana/gdma.h enumerates the readers of gc->hwc.driver_data…
- [Medium] mana_hwc_get_msg_index() returns -ETIMEDOUT whenever
  down_timeout() on hwc->sema expires, i.e. also for plain slot…
- [Medium] mana_hwc_create_channel() publishes gc->hwc.driver_data (now
  under gc->hwc_lock, with a comment claiming 'the lock orders the hwc…
- [Medium] The new `cancelled` short-circuit in mana_hwc_send_request()
  returns ctx->error directly via `goto out`, bypassing the…
- [Medium] The patch newly reads/writes hwc->hwc_timeout under
  gc->hwc_lock in mana_need_log() and mana_serv_reset(), implying the…
- [Low] Commit message / code mismatch on two points.
- [Low] The commit message presents late-response slot reuse as becoming
  unsafe only once more than one slot is in use ('With more than one…
- [Low] The patch fixes a user-visible defect — a timed-out HWC command
  released its slot while the device could still answer, so a later…
- [Low] The lifetime scheme for hw_channel_context is hand-rolled from
  three cooperating pieces spread across two files and two structs:…
- [Low] Three comments added by this patch contradict the code.

Pre-existing issues:
- [High] mana_hwc_destroy_cq() frees callback-reachable state before the
  EQ that drives the callback is deregistered: it does…
- [High] mana_hwc_destroy_channel() frees the HWC queue and message DMA
  buffers even when DESTROY_HWC failed.

--

net: mana: support concurrent HWC requests

This patch prepares the MANA hardware channel for more than one in-flight
request: a per-queue lock around WQE posting, a bounded wait for a message
slot, a channel_up flag plus an active-sender count so teardown can drain
senders before freeing the channel, and retention of a slot whose request
timed out while the device may still answer.

Two notes on the changelog itself before the diff.

[Severity: Low]

> With more than one slot in use, a timed-out command also stops being
> harmless to retire.

Is this accurate at the current depth?  With HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH
every command uses slot 0, and before this patch a timed-out command released
slot 0 while the device could still answer.

mana_hwc_handle_resp() correlates a response purely by msg_id:

	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) { ... }
	ctx = hwc->caller_ctx + msg_id;
	...
	memcpy(ctx->output_buf, resp_msg, resp_len);

So command A's late response could be copied into command B's buffer and
complete B, already at depth 1.

Does the change that keeps the response-side reference (ctx->responded = true;
abandoned = true; in mana_hwc_send_request()) therefore fix an already
reachable wrong-command completion, rather than only a hazard created by the
later depth increase?

[Severity: Low]

Related to the above: the retained response-side reference corrects a
user-visible defect (a later command reusing a slot id could be completed with
the previous command's payload and status), but the trailers carry no Fixes:
tag.  Would

  Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

be appropriate here?

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index a023d3e1a95de..9e7efce669ae0 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -162,6 +162,8 @@ static int mana_gd_init_registers(struct pci_dev *pdev)
>  bool mana_need_log(struct gdma_context *gc, int err)
>  {
>  	struct hw_channel_context *hwc;
> +	bool need_log = true;
> +	unsigned long flags;
>  
>  	if (err != -ETIMEDOUT)
>  		return true;
> @@ -169,11 +171,13 @@ bool mana_need_log(struct gdma_context *gc, int err)
>  	if (!gc)
>  		return true;
>  
> +	spin_lock_irqsave(&gc->hwc_lock, flags);
>  	hwc = gc->hwc.driver_data;
>  	if (hwc && hwc->hwc_timeout == 0)
> -		return false;
> +		need_log = false;
> +	spin_unlock_irqrestore(&gc->hwc_lock, flags);
>  
> -	return true;
> +	return need_log;
>  }

[ ... ]

> @@ -393,9 +397,27 @@ static int mana_gd_detect_devices(struct pci_dev *pdev)
>  int mana_gd_send_request(struct gdma_context *gc, u32 req_len, const void *req,
>  			 u32 resp_len, void *resp)
>  {
> -	struct hw_channel_context *hwc = gc->hwc.driver_data;
> +	struct hw_channel_context *hwc;
> +	unsigned long flags;
> +	int err;
> +
> +	spin_lock_irqsave(&gc->hwc_lock, flags);
> +	hwc = gc->hwc.driver_data;
> +	if (!hwc) {
> +		spin_unlock_irqrestore(&gc->hwc_lock, flags);
> +		return -ENODEV;
> +	}
> +	hwc->active_senders++;
> +	spin_unlock_irqrestore(&gc->hwc_lock, flags);
> +
> +	err = mana_hwc_send_request(hwc, req_len, req, resp_len, resp);
>  
> -	return mana_hwc_send_request(hwc, req_len, req, resp_len, resp);
> +	spin_lock_irqsave(&gc->hwc_lock, flags);
> +	if (--hwc->active_senders == 0)
> +		wake_up(&gc->hwc_drain_waitq);
> +	spin_unlock_irqrestore(&gc->hwc_lock, flags);
> +
> +	return err;
>  }
>  EXPORT_SYMBOL_NS(mana_gd_send_request, "NET_MANA");

[Severity: Low]

This isn't a bug, but the hw_channel_context lifetime is now hand-rolled
from three pieces spread over two files and two structs:

  - gc->hwc_lock, initialised in mana_gd_probe()
  - hwc->active_senders, a field of the hw_channel.c-owned object but
    incremented and decremented here in gdma_main.c
  - gc->hwc_drain_waitq, a gdma_context-lifetime field that
    mana_hwc_create_channel() re-initialises on every channel creation

Would a kref on the channel, or an rwsem/percpu_ref taken by senders and
write-acquired by mana_hwc_destroy_channel(), express the same intent with one
primitive and keep the accounting inside hw_channel.c?

> @@ -716,6 +738,7 @@ static void mana_serv_reset(struct pci_dev *pdev)
>  {
>  	struct gdma_context *gc = pci_get_drvdata(pdev);
>  	struct hw_channel_context *hwc;
> +	unsigned long flags;
>  	int ret;
>  
>  	if (!gc) {
> @@ -725,14 +748,17 @@ static void mana_serv_reset(struct pci_dev *pdev)
>  		return;
>  	}
>  
> +	spin_lock_irqsave(&gc->hwc_lock, flags);
>  	hwc = gc->hwc.driver_data;
>  	if (!hwc) {
> +		spin_unlock_irqrestore(&gc->hwc_lock, flags);
>  		dev_err(&pdev->dev, "MANA service: no HWC\n");
>  		goto out;
>  	}
>  
>  	/* HWC is not responding in this case, so don't wait */
>  	hwc->hwc_timeout = 0;
> +	spin_unlock_irqrestore(&gc->hwc_lock, flags);

[Severity: Medium]

What does gc->hwc_lock protect for hwc->hwc_timeout here and in
mana_need_log()?  Every other access to that field is unlocked:

  mana_hwc_send_request()      if (hwc->hwc_timeout > 1)
                                       hwc->hwc_timeout = 1;
  mana_hwc_get_msg_index()     down_timeout(&hwc->sema,
                                       msecs_to_jiffies(hwc->hwc_timeout))
  mana_hwc_init_event_handler() case HWC_DATA_CFG_HWC_TIMEOUT:
                                       hwc->hwc_timeout = val;

The last one runs from hard IRQ.  So the lockset intersection over all
accesses is empty.

Can this interleaving lose the zero sentinel?

CPU A (sender, just timed out)        CPU B (mana_serv_reset)
------------------------------        ------------------------------
reads hwc->hwc_timeout (30000)
                                      spin_lock_irqsave(&gc->hwc_lock)
                                      hwc->hwc_timeout = 0;
                                      spin_unlock_irqrestore(...)
hwc->hwc_timeout = 1;

Every following teardown command then waits 1 ms instead of 0 and
mana_need_log() starts returning true again, which is the log spam the zero
sentinel exists to suppress.  Should the writers take the same lock, or should
the field simply use READ_ONCE()/WRITE_ONCE()?

> @@ -1339,6 +1365,16 @@ static int mana_gd_create_dma_region(struct gdma_dev *gd,
>  	if (gmi->nr_pages == 0 && !MANA_PAGE_ALIGNED(gmi->virt_addr))
>  		return -EINVAL;
>  
> +	/* No RCU needed: this runs only on the data-path queue-creation
> +	 * path (mana_gd_create_mana_eq/mana_gd_create_mana_wq_cq, called
> +	 * by mana_en under RTNL and by mana_ib RDMA verbs, or during
> +	 * init).  Every teardown path — mana_gd_remove, mana_gd_suspend,
> +	 * and the HWC reset/service path (which goes through
> +	 * mana_gd_suspend) — drains those consumers via mana_rdma_remove()
> +	 * + mana_remove() before mana_hwc_destroy_channel() clears
> +	 * gc->hwc.driver_data, so no concurrent destroy can race with
> +	 * this dereference.
> +	 */
>  	hwc = gc->hwc.driver_data;
>  	req_msg_size = struct_size(req, page_addr_list, num_page);
>  	if (req_msg_size > hwc->max_req_msg_size)
> @@ -1544,7 +1580,17 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
>  	struct hw_channel_context *hwc;
>  	int err;
>  
> +	/* No RCU needed: this runs only inside mana_gd_setup, on the
> +	 * probe and resume paths.  The PCI/PM core holds device_lock
> +	 * across .probe/.resume and .remove/.suspend, so setup cannot
> +	 * overlap teardown of the same device.  The HWC reset/service
> +	 * path is additionally serialized by GC_IN_SERVICE and runs
> +	 * suspend (destroy) then resume (this) sequentially in one work
> +	 * item.  driver_data was just set by mana_hwc_create_channel
> +	 * earlier in this same setup call, so it is live here.
> +	 */
>  	hwc = gc->hwc.driver_data;
> +

[Severity: Medium]

The new contract on gc->hwc_lock in include/net/mana/gdma.h enumerates the
readers of gc->hwc.driver_data as mana_gd_send_request(), mana_need_log() and
mana_serv_reset().  Are there not three more readers that dereference the same
pointer with neither the lock nor an active_senders reference, and with no NULL
check?

	mana_gd_create_dma_region()
		hwc = gc->hwc.driver_data;
		if (req_msg_size > hwc->max_req_msg_size)

	mana_gd_verify_vf_version()
		hwc = gc->hwc.driver_data;
		err = mana_gd_query_hwc_timeout(pdev, &hwc->hwc_timeout);

	mana_ib_gd_create_dma_region() in drivers/infiniband/hw/mana/main.c
		hwc = gc->hwc.driver_data;
		request_buf = kzalloc(hwc->max_req_msg_size, GFP_KERNEL);

mana_hwc_destroy_channel() now also clears driver_data at the very start of
teardown instead of after kfree(hwc), so for these readers the stale-pointer
window becomes a NULL pointer for the whole teardown, including the
mana_smc_teardown_hwc() polling.  The mana_ib reader is not mentioned in the
new contract at all.

Should the enumerated list be corrected, or the invariant actually enforced at
these sites?  The driver's own comment in mana_hwc_destroy_queues() notes that
the PM device_lock and the GC_IN_SERVICE service path "do not exclude each
other", which makes the "no concurrent destroy can race with this
dereference" claim above hard to verify.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 0056bdd8c53f5..91fcf7c092113 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -6,7 +6,11 @@
>  #include <net/mana/hw_channel.h>
>  #include <linux/vmalloc.h>
>  
> -/* Acquire a free inflight message slot, waiting for one if all are in use. */
> +/* Acquire a free message slot from the inflight bitmap, waiting for one if
> + * all are in use.  Returns -ENODEV if the channel is being torn down, or
> + * -ETIMEDOUT if a prior HWC command has timed out (preserving the error
> + * code callers expect).
> + */
>  static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
>  {
>  	struct gdma_resource *r = &hwc->inflight_msg_res;
> @@ -14,12 +18,32 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
>  	unsigned long flags;
>  	u32 index;
>  
> -	down(&hwc->sema);
> +	/* Bounded wait for a slot.  A timed-out request keeps its slot until
> +	 * the device answers for it, so the semaphore is never posted back
> +	 * for that slot and a caller expires here rather than blocking on a
> +	 * release that is not coming.  Teardown reclaims those slots, which
> +	 * posts the semaphore and releases anyone waiting below.
> +	 */
> +	if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
> +		return -ETIMEDOUT;

[Severity: Medium]

This returns -ETIMEDOUT for any expiry of the semaphore wait, including plain
slot contention on a healthy channel.  The changelog says:

> Slots held this way are counted, so a channel that is merely busy can be
> told from one where nothing will ever free a slot again.

Where is that counted?  struct hw_channel_context gains channel_up,
active_senders and the per-slot resp_pending, and this function inspects no
request state before choosing the errno, so busy and dead produce the same
return value.

In-tree consumers treat -ETIMEDOUT as a faulty device:

	mana_gf_stats_work_handler()
		if (err == -ETIMEDOUT) {
			ac->hwc_timeout_occurred = true;
			memset(&ac->hc_stats, 0, sizeof(ac->hc_stats));
			...
			mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);

	mana_gd_probe()
		if (err == -ETIMEDOUT || err == -EPROTO) { ... schedule
		    delayed recovery work ... }

Can contention on a live channel therefore trigger a full reset cycle or a PCI
rescan?  The changelog cites mlx5 as precedent, and mlx5 returns -EBUSY for
this case rather than a timeout errno.

[ ... ]

> @@ -101,6 +126,7 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  {
>  	const struct gdma_resp_hdr *resp_msg = rx_req->buf_va;
>  	struct hwc_caller_ctx *ctx;
> +	bool release;
>  	int err;
>  
>  	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
> @@ -113,15 +139,32 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  
>  	spin_lock(&ctx->lock);
>  
> -	/* Honour a response only while the sender owns the slot (output_buf
> -	 * published) and has not already been answered; otherwise drop it as
> -	 * premature, stale or duplicate without touching the refcount.
> +	/* The sender has not published its buffer yet, so nothing asked for
> +	 * this response.  Keep the slot reserved and drop the message.
>  	 */
> -	if (!ctx->output_buf || ctx->responded) {
> +	if (!ctx->output_buf && !ctx->responded) {
>  		spin_unlock(&ctx->lock);
>  		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
>  		return;
>  	}

[ ... ]

> @@ -593,6 +637,7 @@ static int mana_hwc_create_wq(struct hw_channel_context *hwc,
>  	hwc_wq->gdma_wq = queue;
>  	hwc_wq->queue_depth = q_depth;
>  	hwc_wq->hwc_cq = hwc_cq;
> +	spin_lock_init(&hwc_wq->lock);
>  
>  	err = mana_hwc_alloc_dma_buf(hwc, q_depth, max_msg_size,
>  				     &hwc_wq->msg_buf);
> @@ -610,7 +655,7 @@ static int mana_hwc_create_wq(struct hw_channel_context *hwc,
>  	return err;
>  }
>  
> -static int mana_hwc_post_tx_wqe(const struct hwc_wq *hwc_txq,
> +static int mana_hwc_post_tx_wqe(struct hwc_wq *hwc_txq,
>  				struct hwc_work_request *req,
>  				u32 dest_virt_rq_id, u32 dest_virt_rcq_id,
>  				bool dest_pf)
> @@ -649,7 +694,11 @@ static int mana_hwc_post_tx_wqe(const struct hwc_wq *hwc_txq,
>  	req->wqe_req.inline_oob_data = tx_oob;
>  	req->wqe_req.client_data_unit = 0;
>  
> +	/* Serialize WQE posting — multiple senders may call concurrently. */
> +	spin_lock(&hwc_txq->lock);
>  	err = mana_gd_post_and_ring(hwc_txq->gdma_wq, &req->wqe_req, NULL);
> +	spin_unlock(&hwc_txq->lock);
> +
>  	if (err)
>  		dev_err(dev, "Failed to post WQE on HWC SQ: %d\n", err);
>  	return err;

[Severity: Low]

The changelog says:

> - Add a per-queue lock to hwc_wq; mana_gd_post_and_ring() is not safe
>   to call concurrently on the same queue.

mana_hwc_create_wq() runs spin_lock_init(&hwc_wq->lock) for both the RQ and
the SQ, but only mana_hwc_post_tx_wqe() takes it.  mana_hwc_post_rx_wqe()
still takes a const struct hwc_wq * and posts unlocked:

	static int mana_hwc_post_rx_wqe(const struct hwc_wq *hwc_rxq, ...)
		err = mana_gd_post_and_ring(hwc_rxq->gdma_wq, &req->wqe_req, NULL);

So hwc->rxq->lock is initialised and never used.  Is the RQ meant to be
covered, or should the description say the SQ only?

Also, the comment added on gdma_context::hwc_drain_waitq speaks of "the last
sender's atomic_dec", but active_senders is a plain unsigned int updated under
gc->hwc_lock (if (--hwc->active_senders == 0) wake_up(...)), and
hw_channel.h documents the same field as "Protected by gc->hwc_lock".

[ ... ]

> @@ -817,6 +899,15 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  	gd->pdid = INVALID_PDID;
>  	gd->doorbell = INVALID_DOORBELL;
>  
> +	/* Publish driver_data last, under hwc_lock: the lock orders the hwc
> +	 * initialisation above before the pointer becomes visible and
> +	 * serialises the publish against the control-plane readers in
> +	 * mana_gd_send_request(), mana_need_log() and mana_serv_reset().
> +	 */
> +	spin_lock_irqsave(&gc->hwc_lock, flags);
> +	gc->hwc.driver_data = hwc;
> +	spin_unlock_irqrestore(&gc->hwc_lock, flags);
> +
>  	/* mana_hwc_init_queues() only creates the required data structures,
>  	 * and doesn't touch the HWC device.
>  	 */

[Severity: Medium]

Is "publish driver_data last" accurate here?  The state a sender touches first
is initialised after this store:

	spin_lock_irqsave(&gc->hwc_lock, flags);
	gc->hwc.driver_data = hwc;		/* visible now */
	spin_unlock_irqrestore(&gc->hwc_lock, flags);
	...
	err = mana_hwc_init_queues(hwc, HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH, ...)
		-> mana_hwc_init_inflight_msg()
			sema_init(&hwc->sema, num_msg);
			mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);

hwc comes from kzalloc, so in that window hwc->sema has count 0 with
wait_list = {NULL, NULL} and inflight_msg_res.{map,size,lock} are all zero.

Can the new channel_up gate reject a sender in that window?  It is only
evaluated after the blocking wait:

	if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
		return -ETIMEDOUT;

	spin_lock_irqsave(&r->lock, flags);

	if (!hwc->channel_up) {

With count 0, down_timeout() enters ___down_common() and does
list_add_tail(&waiter.list, &sem->wait_list) on a NULL-linked list head, and
mana_gd_send_request() has already taken a sender reference on the strength of
a non-NULL pointer alone.

The publish-before-init predates the patch (the baseline had gd->driver_data =
hwc at the top of the same function), so this is not newly introduced, but
should the publish move after mana_hwc_init_queues(), given the comment now
claims the ordering?

> @@ -851,11 +942,120 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  
>  void mana_hwc_destroy_channel(struct gdma_context *gc)
>  {
> +	/* This is the only destroy entry point.  driver_data is read
> +	 * plainly here (teardown is serialised against other teardown);
> +	 * it is cleared under hwc_lock below before hwc is freed.
> +	 */
>  	struct hw_channel_context *hwc = gc->hwc.driver_data;
> +	unsigned long flags;
>  
>  	if (!hwc)
>  		return;
>  
> +	/* Prevent new requests from starting.  Clear channel_up under the
> +	 * bitmap lock so get_msg_index() cannot acquire a slot and increment
> +	 * active_senders after this point.  Senders already waiting on the
> +	 * semaphore are released by the force-completion loop below, which
> +	 * returns every in-flight slot -- including the ones a timed-out
> +	 * request was holding; each released waiter sees the flag clear and
> +	 * posts its permit straight back, so they drain in turn.
> +	 *
> +	 * Gate on the bitmap rather than on channel_up: reading the flag
> +	 * unlocked and only then taking the lock would let a concurrent
> +	 * setup publish it in between and leave the channel up.  A zero
> +	 * num_inflight_msg means mana_gd_alloc_res_map() never ran, so the
> +	 * lock is not initialised yet -- and no sender can exist either.
> +	 */
> +	if (hwc->num_inflight_msg) {
> +		spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
> +		hwc->channel_up = false;
> +		spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
> +	}
> +
> +	/* Clear the pointer under hwc_lock so new callers in
> +	 * mana_gd_send_request() see NULL and return -ENODEV.  The lock
> +	 * makes the readers' "load driver_data + active_senders++"
> +	 * atomic against this store, so once it returns no new sender can
> +	 * take a reference; the active_senders drain below waits out those
> +	 * that already did, before their hwc is freed.
> +	 */
> +	spin_lock_irqsave(&gc->hwc_lock, flags);
> +	gc->hwc.driver_data = NULL;
> +	spin_unlock_irqrestore(&gc->hwc_lock, flags);

[ ... ]

> @@ -871,14 +1071,36 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	}
>  	gc->max_num_cqs = 0;
>  
> +	/* Destroy the HWC CQ object before the TXQ and RQ.  The
> +	 * active_senders drain above already guarantees no sender is
> +	 * still reaching the CQ through txq->hwc_cq.
> +	 */
> +	if (hwc->cq)
> +		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> +
>  	if (hwc->txq)
>  		mana_hwc_destroy_wq(hwc, hwc->txq);
>  
>  	if (hwc->rxq)
>  		mana_hwc_destroy_wq(hwc, hwc->rxq);
>  
> -	if (hwc->cq)
> -		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> +	/* Safety net: the force-complete loop above dropped the
> +	 * response-side reference of every occupied slot and the sender
> +	 * drain released the matching sender references, so nothing should
> +	 * still be set here.  Release anything that is, rather than leak it.
> +	 */
> +	if (hwc->caller_ctx) {
> +		struct hwc_caller_ctx *ctx;
> +		int i;
> +
> +		for (i = 0; i < hwc->num_inflight_msg; i++) {
> +			if (!test_bit(i, hwc->inflight_msg_res.map))
> +				continue;
> +
> +			ctx = &hwc->caller_ctx[i];
> +			hwc_ctx_put(hwc, ctx);
> +		}
> +	}

[Severity: Medium]

The CQ destruction is hoisted above the TXQ and RXQ destruction, and the
changelog does not mention it.  Is this a fix for a pre-existing
use-after-free?  Destroying the CQ is what deregisters the HWC EQ:

  mana_hwc_destroy_cq() -> mana_gd_destroy_queue(eq) -> mana_gd_destroy_eq()
    -> mana_gd_deregister_irq() -> list_del_rcu() + synchronize_rcu()

In the old order a live interrupt could reach
mana_hwc_handle_resp() -> mana_hwc_post_rx_wqe(hwc->rxq, ...) and dereference
hwc->caller_ctx after both had been freed by mana_hwc_destroy_wq() and
kfree(hwc->caller_ctx).  Should this carry its own patch and a Fixes: tag so
it can be backported on its own?

Separately, the appended "safety net" loop says by its own comment that
nothing should still be set.  If a bit is still set there, the refcount model
has been violated; would a WARN_ON be better than silently putting the
reference?

[Severity: High]

This isn't a bug introduced by this patch, but the helper called here frees
callback-reachable state before the EQ that drives the callback is
deregistered:

	static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
	{
		kfree(hwc_cq->comp_buf);

		if (hwc_cq->gdma_cq)
			mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);

		if (hwc_cq->gdma_eq)
			mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);

Only the last call reaches mana_gd_deregister_irq() with its list_del_rcu() +
synchronize_rcu(), and the HWC IRQ is released later still, in
mana_gd_remove_irqs() after mana_hwc_destroy_channel() returns.  Can a
concurrent interrupt therefore use freed memory?

CPU A (destroy)                       CPU B (mana_gd_intr)
------------------------------        ------------------------------
kfree(hwc_cq->comp_buf)
mana_gd_destroy_queue(gdma_cq)
  (frees DMA ring + struct)
                                      mana_hwc_comp_event(ctx, q_self)
                                        completions = hwc_cq->comp_buf;
                                        mana_gd_poll_cq(q_self, completions,
                                                        queue_depth);
mana_gd_destroy_queue(gdma_eq)
  synchronize_rcu()                     /* writes into freed comp_buf */

The comment added above states the teardown deliberately tolerates a live
interrupt still delivering a response, so does this ordering need fixing for
that reasoning to hold?

[Severity: High]

This is also a pre-existing issue rather than something this patch
introduces, but the queue and message DMA buffers are freed here even when
DESTROY_HWC failed just above:

	if (hwc->setup_active) {
		if (!mana_smc_teardown_hwc(&gc->shm_channel, false))
			hwc->setup_active = false;
		else
			dev_err(hwc->dev, "Failed to tear down HWC\n");
	}
	gc->max_num_cqs = 0;

	if (hwc->cq)
		mana_hwc_destroy_cq(...);
	if (hwc->txq)
		mana_hwc_destroy_wq(hwc, hwc->txq);
	if (hwc->rxq)
		mana_hwc_destroy_wq(hwc, hwc->rxq);

mana_smc_teardown_hwc() explains why waiting matters:

	/* Read shmem response (polling for VF possession) and validate.
	 * For teardown, waiting for response is required to ensure hardware
	 * invalidates MST entries before software frees memory.
	 */

On failure setup_active is deliberately left set, meaning the PF may still be
using the queues, yet the coherent EQ/CQ/SQ/RQ rings and message buffers go
back to the DMA allocator.  Can the device keep DMAing into memory that has
been reassigned?  Since this series is what introduced the setup_active
bookkeeping, would it make sense to skip the frees (and leak) when that
teardown fails?

[ ... ]

> @@ -955,7 +1178,25 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  	/* The response-side reference (from get_msg_index) keeps the slot
>  	 * alive if hardware responds right after the doorbell.
>  	 */
> -	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
> +	/* Submit under the slot lock, so mana_hwc_destroy_channel() cannot
> +	 * cancel this request between the check and the doorbell: it takes
> +	 * the same lock, so it either cancels before this runs -- and the
> +	 * request is never handed to the device -- or after, when the
> +	 * request is genuinely in flight.  Posting is a WQE write plus a
> +	 * doorbell, so it does not sleep.
> +	 */
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	cancelled = ctx->responded;
> +	if (cancelled)
> +		err = ctx->error;
> +	else
> +		err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq,
> +					   false);
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +
> +	if (cancelled)
> +		goto out;
> +
>  	if (err) {
>  		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
>  		goto out;

[Severity: Medium]

Can this path return success for a command that was never posted?

mana_hwc_handle_resp() accepts a response as soon as output_buf is published,
which happens before the doorbell:

	if (!ctx->output_buf && !ctx->responded) {
		...drop...
	}
	...
	ctx->responded = true;
	...
	ctx->status_code = resp_msg->status;
	memcpy(ctx->output_buf, resp_msg, resp_len);
	ctx->error = err;			/* 0 on a well-formed message */

So a response arriving in that window (a device duplicate for a previously
reused slot, or, in a confidential VM, a host-supplied hwc_msg_id the driver
already treats as untrusted) leaves ctx->responded == true and
ctx->error == 0.  The new block then takes cancelled = true, err = 0, skips
mana_hwc_post_tx_wqe() and does goto out, which falls into done: return err.

The check_status: block is the only place the device status is validated:

	check_status:
		if (err)
			goto done;

		if (status && status != GDMA_STATUS_MORE_ENTRIES) { ... err = -EPROTO; }

Before this patch the same injected response still reached check_status:.
Should the cancelled path fall through to check_status:, or refuse to report
success when the request was never handed to the device?

The out: comment also states "Only reached before the request reached the
hardware, so no response can ever arrive for it", which the cancelled path
reaches after a response has already been applied.

> @@ -972,9 +1213,23 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  		ctx->output_buf = NULL;
>  		err = ctx->error;
>  		status = ctx->status_code;
> +		if (err == -EINPROGRESS) {
> +			/* Give up on this request in the same critical section
> +			 * that clears output_buf, so a response can never
> +			 * observe the slot as "sender has not published yet"
> +			 * and be discarded as premature -- that would strand
> +			 * the slot, because only a response frees it.
> +			 *
> +			 * Keep the response-side reference: the device may
> +			 * still answer, so the slot stays taken until it does
> +			 * and must not be handed to another request.
> +			 */
> +			ctx->responded = true;
> +			abandoned = true;
> +		}
>  		spin_unlock_irqrestore(&ctx->lock, flags);
>  
> -		if (err != -EINPROGRESS) {
> +		if (!abandoned) {
>  			/* A response raced in just after the timeout, so the
>  			 * hardware is alive: keep the channel and report what
>  			 * that response said rather than a timeout.  It may
> @@ -986,29 +1241,25 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  			goto check_status;
>  		}
>  
> -		if (wait_ms != 0)
> +		if (wait_ms != 0) {
>  			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
>  				command, wait_ms);
>  
> -		err = -ETIMEDOUT;
> -
> -		/* No-wait teardown (hwc_timeout == 0) is expected to expire;
> -		 * just release the slot so the next teardown command can reuse
> -		 * it.
> -		 */
> -		if (wait_ms == 0)
> -			goto out;
> +			/* Genuine timeout: shorten later waits so subsequent
> +			 * commands fail fast instead of each draining the
> +			 * full timeout.
> +			 */
> +			if (hwc->hwc_timeout > 1)
> +				hwc->hwc_timeout = 1;
> +		}
>  
> -		/* Genuine timeout: shorten later waits so subsequent commands
> -		 * fail fast instead of each draining the full timeout.
> -		 */
> -		if (hwc->hwc_timeout > 1)
> -			hwc->hwc_timeout = 1;
> +		err = -ETIMEDOUT;
>  
> -		/* Release the slot via out:; a late response no longer touches
> -		 * it, so the sender must drop the reference here.
> +		/* Drop only the sender's reference; the response-side one is
> +		 * what keeps the slot reserved.
>  		 */
> -		goto out;
> +		hwc_ctx_put(hwc, ctx);
> +		goto done;
>  	}

[Severity: Medium]

The wait_ms == 0 fast path that released the slot is dropped here.  Does that
strand the channel on the no-wait teardown path?

mana_serv_reset() sets hwc->hwc_timeout = 0 before the suspend sequence, so
the first command expires immediately in
wait_for_completion_timeout(&ctx->comp_event, 0), takes the abandoned branch,
and keeps its bitmap bit and its semaphore permit for good.

Every later command then fails before a WQE is built, because
mana_hwc_get_msg_index() does:

	if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
		return -ETIMEDOUT;

and ___down_common() has:

	if (unlikely(timeout <= 0))
		goto timed_out;

At the bootstrap depth used by this patch that is exactly one teardown command
reaching the device; the remaining ones (disable vport, destroy WQ/CQ objects,
destroy DMA regions) are dropped, and mana_need_log() returns false while
hwc_timeout == 0, so nothing is logged either.  The changelog only justifies
retaining slots for requests that "reached the hardware" and does not discuss
removing this deliberate no-wait release.

> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 308950f9b54b0..f97f63f8fee41 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -468,6 +468,21 @@ struct gdma_context {
>  	/* Hardware communication channel (HWC) */
>  	struct gdma_dev		hwc;
>  
> +	/* destroy_channel() waits here for all HWC senders to exit.
> +	 * Lives on gc (not hwc) so wake_up() after the last sender's
> +	 * atomic_dec doesn't dereference freed hwc memory.
> +	 */
> +	wait_queue_head_t	hwc_drain_waitq;

[Severity: Low]

Three comments added by this patch look inconsistent with the code.

There is no atomic_dec: active_senders is a plain unsigned int and
mana_gd_send_request() does

	if (--hwc->active_senders == 0)
		wake_up(&gc->hwc_drain_waitq);

under gc->hwc_lock, which is also what hw_channel.h documents.

In mana_hwc_destroy_channel(), "A zero num_inflight_msg means
mana_gd_alloc_res_map() never ran, so the lock is not initialised yet" reads
inverted.  mana_hwc_init_queues() calls mana_hwc_init_inflight_msg() first,
which runs sema_init() and mana_gd_alloc_res_map() (bitmap_zalloc() +
spin_lock_init()), and only assigns hwc->num_inflight_msg = q_depth after the
CQ and WQ creations succeed.  So a failure in mana_hwc_create_cq() or
mana_hwc_create_wq() leaves num_inflight_msg == 0 with the lock initialised
and the bitmap allocated.

Finally, the new kerneldoc on mana_hwc_get_msg_index() lists only -ENODEV and
-ETIMEDOUT, while the WARN_ON_ONCE(index >= r->size) branch added in the same
hunk returns -EIO.

[ ... ]

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

Re: [PATCH net-next v4 4/4] net: mana: add dynamic HWC queue depth with reinit path

[netdev-bot+sashiko]

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

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

- [High] The HWC doorbell page index is taken straight from the device
  (`gd->doorbell = eq_db.doorbell` in mana_hwc_init_event_handler(),…
- [High] NULL pointer dereference on the new reinit_fallback path.
- [High] The new INVALID_DOORBELL guard is placed after the write it is
  supposed to prevent.
- [Medium] Negotiated-dimension validation is not applied on every
  establish, contradicting the commit message.
- [Low] Commit message claims the pre-existing 'first establish omits the
  doorbell' unchecked-MMIO path is not addressed, while the new…
- [Low] Contradictory comments inside mana_hwc_create_channel(): the
  early publish stores gc->hwc.driver_data under gc->hwc_lock with a…

Pre-existing issues:
- [High] mana_hwc_destroy_cq() frees the CQ completion state before the
  EQ interrupt is fenced: it does kfree(hwc_cq->comp_buf), then…
- [High] The service-reset path and the PCI PM lifecycle can enter HWC
  teardown concurrently.
- [High] Service work is queued on the system workqueue and is never
  cancelled or flushed during driver teardown, so it can run against a…

--

net: mana: add dynamic HWC queue depth with reinit path

The HWC is bootstrapped at queue depth 1, then torn down and rebuilt at the
device-reported maximum depth so more management commands can be in flight,
with a fallback back to the bootstrap depth when the rebuild fails.  A new
capability flag advertises the support and the negotiated dimensions are
validated before they size DMA allocations.

A few questions on the doorbell handling, the dimension validation and the
fallback ordering below.

>From the commit message:

> Because that reset
> runs on every establish, a firmware that supplied the doorbell on the
> first one but omits it on the second would leave INVALID_DOORBELL
> behind, so refuse the channel in that case rather than let
> mana_gd_ring_doorbell() turn it into an unchecked write far outside the
> BAR.  The same write is already reachable without this patch when the
> very first establish omits the doorbell; that is pre-existing and not
> addressed here.

[Severity: Low]
Is this description of the scope accurate?  The new check sits in the
common body of mana_hwc_establish_channel(), so it also runs on the first
establish, not just the second one.

What remains unaddressed is something different: the doorbell is rung from
hard IRQ in mana_gd_process_eq_events() before the check can run at all, on
either establish (see the separate comment on the reset hunk).  Could the
wording say which part is actually still reachable?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 91fcf7c092113..83df4f194ee3b 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -797,6 +806,21 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	struct gdma_queue *cq = hwc->cq->gdma_cq;
>  	int err;
>  
> +	/* Clear the values a previous establish left behind so a firmware
> +	 * that omits an HWC_INIT_DATA_* item on this cycle cannot silently
> +	 * reuse stale dimensions from the last one.  The same applies to the
> +	 * routing identities: the queues are rebuilt from scratch, so a
> +	 * doorbell, PDID or PF destination left over from the previous
> +	 * channel does not describe them.
> +	 */
> +	hwc->hwc_init_q_depth_max = 0;
> +	hwc->hwc_init_max_req_msg_size = 0;
> +	hwc->hwc_init_max_resp_msg_size = 0;
> +	gc->hwc.doorbell = INVALID_DOORBELL;

[Severity: High]
Can this write happen before the guard added below can catch it?

The HWC EQ is armed from hard IRQ while the handshake is still running:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_process_eq_events() {
	...
	mana_gd_ring_doorbell(gc, eq->gdma_dev->doorbell, eq->type, eq->id,
			      head, SET_ARM_BIT);
	...
}

So the call chain is:

mana_gd_intr() -> mana_gd_process_eq_events() -> mana_gd_ring_doorbell()

If an EQE batch (INIT_DATA, INIT_DONE) is processed before
GDMA_EQE_HWC_INIT_EQ_ID_DB is consumed, gd->doorbell is still the sentinel
this hunk just stored, and mana_gd_ring_doorbell() does writeq() at
gc->db_page_base + gc->db_page_size * 0xffffffff, which is far outside the
ioremap'ed BAR0 window.  The guard below only runs after
wait_for_completion_timeout() returns.

On the second establish this also discards a doorbell index that was
already known good.  Would keeping (and range-checking) the index be
preferable to clearing it and checking afterwards?

> +	gc->hwc.pdid = INVALID_PDID;
> +	hwc->pf_dest_vrq_id = 0;
> +	hwc->pf_dest_vrcq_id = 0;
> +
>  	init_completion(&hwc->hwc_init_eqe_comp);
>  
>  	err = mana_smc_setup_hwc(&gc->shm_channel, false,
> @@ -815,6 +839,20 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
>  	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
>  
> +	/* The doorbell was cleared before the handshake, so a firmware that
> +	 * signals INIT_DONE without sending GDMA_EQE_HWC_INIT_EQ_ID_DB
> +	 * leaves INVALID_DOORBELL behind.  mana_gd_ring_doorbell() turns
> +	 * that into gc->db_page_base + gc->db_page_size * 0xffffffff, an
> +	 * unchecked MMIO write far outside the mapped BAR, and the channel
> +	 * test below rings it.  Everything else the device reports either
> +	 * fails the dimension checks in mana_hwc_create_channel() or leaves
> +	 * the queues unable to complete, which that test already catches.
> +	 */
> +	if (gc->hwc.doorbell == INVALID_DOORBELL) {
> +		dev_err(hwc->dev, "HWC: no doorbell in init data\n");
> +		return -EPROTO;
> +	}

[Severity: High]
Does this catch anything other than the omission of
GDMA_EQE_HWC_INIT_EQ_ID_DB?

The doorbell index comes straight from the device in
mana_hwc_init_event_handler():

	case GDMA_EQE_HWC_INIT_EQ_ID_DB:
		eq_db.as_uint32 = event->details[0];
		hwc->cq->gdma_eq->id = eq_db.eq_id;
		gd->doorbell = eq_db.doorbell;

union hwc_init_eq_id_db declares doorbell as a 16-bit field, so the device
can never report UINT_MAX; only an omitted EQ_ID_DB produces the sentinel.
An in-field but out-of-BAR value such as 0xffff still reaches:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_ring_doorbell() {
	void __iomem *addr = gc->db_page_base + gc->db_page_size * db_index;
	...
	writeq(e.as_uint64, addr);
}

with a 4 KB doorbell page that is roughly 256 MB past the mapping.  The
driver already has the corresponding check on the peer path:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_register_device() {
	if (gc->db_page_off + gc->db_page_size * ((u64)resp.db_id + 1) > gc->bar0_size) {
		dev_err(gc->dev, "Doorbell ID %u out of range\n", resp.db_id);
		return -EPROTO;
	}
}

Would the same range test here work better, given it also subsumes the
sentinel case?

>  	/* Both were set in mana_hwc_init_event_handler(). */
>  	if (WARN_ON(cq->id >= gc->max_num_cqs))
>  		return -EPROTO;

[ ... ]

> @@ -872,13 +916,64 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  	return err;
>  }
>  
> +/* Tear down all HWC queues and free associated resources.  Used on
> + * the reinit-with-higher-queue-depth path and reinit fallback.
> + *
> + * PRECONDITION: must be called only during channel bring-up in
> + * mana_hwc_create_channel(), before the channel carries traffic:
> + * channel_up is still false, caller_ctx is not yet allocated, the
> + * data path is not probed yet, and active_senders is 0 — so no
> + * request or response user can reach these queues.  That is why this
> + * skips the hwc_lock-protected driver_data clear + active_senders
> + * drain that mana_hwc_destroy_channel() needs for the runtime
> + * teardown race; only the CQ-first ordering below (to fence off a
> + * pending interrupt) is required.  Bring-up itself runs under the
> + * PCI/PM device_lock, or under GC_IN_SERVICE on the service path;
> + * those two do not exclude each other, so a service reset racing a PM
> + * transition is not serialized — but that is pre-existing and applies
> + * equally to mana_hwc_destroy_channel(), which frees the same
> + * objects.  Calling this on a live, published channel would be a
> + * use-after-free.

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but since
the comment documents the gap: can the two teardown contexts free the same
hwc twice?

mana_serv_reset() drives the suspend/resume cycle from a workqueue with no
device_lock held:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
	spin_unlock_irqrestore(&gc->hwc_lock, flags);

	dev_info(&pdev->dev, "MANA reset cycle start\n");

	mana_gd_suspend(pdev, PMSG_SUSPEND);
	...
}

while .suspend and .remove run under device_lock, and GC_IN_SERVICE only
serializes service jobs against each other.  mana_hwc_destroy_channel()
then reads the pointer outside hwc_lock:

	struct hw_channel_context *hwc = gc->hwc.driver_data;

	if (!hwc)
		return;

so both callers can observe the same non-NULL hwc and both proceed to
destroy the CQ/TXQ/RXQ and kfree(hwc).  Is something else preventing that?

[Severity: High]
This is also pre-existing and not introduced by this patch, but related to
the service path named in the comment: is the service work ever cancelled
before gc is freed?

mana_schedule_serv_work() puts the item on the system workqueue:

	INIT_WORK(&mns_wk->serv_work, mana_serv_func);
	schedule_work(&mns_wk->serv_work);

while cleanup only drains the unrelated ordered queue:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_cleanup_device() {
	if (gc->service_wq) {
		destroy_workqueue(gc->service_wq);
		gc->service_wq = NULL;
	}
}

and mana_gd_remove() then does vfree(gc).  mana_serv_func() ->
mana_do_service() -> mana_serv_reset() takes gc from
pci_get_drvdata(pdev) and dereferences gc->hwc_lock and
gc->hwc.driver_data, so a worker that already read a valid pointer would
operate on freed memory.  Would a cancel_work_sync() for the pending item
in the remove path close that?

> + */
> +static void mana_hwc_destroy_queues(struct hw_channel_context *hwc)
> +{
> +	struct gdma_context *gc = hwc->gdma_dev->gdma_context;
> +
> +	/* Destroy CQ first to deregister the EQ from the interrupt
> +	 * handler list before freeing caller_ctx, TXQ, or RXQ memory.
> +	 * A pending interrupt handler could still reach handle_resp()
> +	 * which dereferences caller_ctx.
> +	 */
> +	if (hwc->cq) {
> +		mana_hwc_destroy_cq(gc, hwc->cq);
> +		hwc->cq = NULL;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does mana_hwc_destroy_cq()
actually fence the interrupt before it frees its own state?

mana_hwc_destroy_cq() frees comp_buf and the GDMA CQ first, and only then
the EQ:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_cq() {
	kfree(hwc_cq->comp_buf);

	if (hwc_cq->gdma_cq)
		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);

	if (hwc_cq->gdma_eq)
		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
	...
}

but the IRQ removal and the RCU fence only happen in the EQ teardown:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_deregister_irq() {
	list_del_rcu(&eq->entry);
	...
	synchronize_rcu();
}

So a handler already inside its read section via mana_gd_intr() ->
mana_gd_process_eq_events() -> mana_gd_process_eqe() ->
mana_hwc_comp_event() can still touch the freed gdma_cq and the freed
comp_buf.  The comment here relies on the CQ-first ordering fencing off a
pending interrupt, which only holds for what is freed after
mana_hwc_destroy_cq() returns.  Would destroying/deregistering the EQ
before freeing comp_buf and the CQ be the right order?

> +
> +	kfree(hwc->caller_ctx);
> +	hwc->caller_ctx = NULL;

[ ... ]

> @@ -926,8 +1021,200 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  		goto out;
>  	}
>  
> +	/* The channel was bootstrapped at a minimal queue depth.  If the
> +	 * device reports a higher maximum, tear down and rebuild with
> +	 * the larger depth so more HWC commands can be in flight.
> +	 */
> +	if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {

[ ... ]

> +		if (max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
> +		    max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE ||
> +		    (u64)q_depth_max * max_req_msg_size >
> +			U32_MAX - MANA_PAGE_SIZE ||
> +		    (u64)q_depth_max * max_resp_msg_size >
> +			U32_MAX - MANA_PAGE_SIZE) {
> +			dev_err(hwc->dev,
> +				"HWC: invalid dims q=%u req=%u resp=%u\n",
> +				q_depth_max, max_req_msg_size,
> +				max_resp_msg_size);
> +			q_depth_max = HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH;
> +			goto skip_reinit;
> +		}

[Severity: Medium]
The commit message says "Reject a device-reported message size above the
driver maximum", but is that what happens here?

This is the only message-size validation added, and it is nested under
q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH.  A device that reports
depth 0 or 1 together with arbitrary or zero HWC_INIT_DATA_MAX_REQUEST /
HWC_INIT_DATA_MAX_RESPONSE values never reaches this check;
mana_hwc_init_event_handler() stores them verbatim and
mana_hwc_establish_channel() hands them back unchecked.

When the depth is larger and the sizes do mismatch, the mismatch is not
rejected either - q_depth_max is reset and the code jumps to skip_reinit,
continuing on the already-established channel.

The backstop the commit message names also does not look like one:

	static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
					 u32 max_req_msg_size, u32 max_resp_msg_size)

neither max_req_msg_size nor max_resp_msg_size is read in the body, so it
cannot catch a contradictory or omitted size report.

Was returning an error intended for the mismatch case, and should the check
move out of the depth > bootstrap branch?

[ ... ]

> +	}
> +
> +	goto skip_reinit;
> +
> +reinit_fallback:
> +	/* Restore bootstrap-depth channel so the device remains functional.
> +	 * Free cq_table if it was allocated by a partially successful
> +	 * establish attempt.
> +	 */
> +	dev_warn(hwc->dev, "HWC reinit failed, falling back to bootstrap depth\n");
> +
> +	mana_hwc_destroy_queues(hwc);
> +
> +	old_cq_table = gc->cq_table;
> +	gc->cq_table = NULL;
> +	/* Clear the bound with the table, as above. */
> +	gc->max_num_cqs = 0;

[Severity: High]
Can this order oops on a NULL gc->cq_table?

mana_hwc_destroy_queues() runs before gc->max_num_cqs is cleared here, and
on this path gc->cq_table was already set to NULL by the reinit branch
above.  If the re-establish then failed after
mana_hwc_init_event_handler() stored HWC_INIT_DATA_MAX_NUM_CQS and
HWC_INIT_DATA_CQID from hard IRQ, but before gc->cq_table was allocated,
max_num_cqs is non-zero with a NULL table.  The new -EPROTO return for the
missing doorbell, the wait_for_completion_timeout() -ETIMEDOUT return and
the vcalloc() -ENOMEM return all leave that state.

mana_hwc_destroy_queues() -> mana_hwc_destroy_cq() ->
mana_gd_destroy_queue() then reaches:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_cq() {
	u32 id = queue->id;

	if (id >= gc->max_num_cqs)
		return;

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

which passes the bound and dereferences the NULL table.  The runtime
teardown avoids this by setting gc->max_num_cqs = 0 before
mana_hwc_destroy_cq().  Would zeroing gc->max_num_cqs and gc->cq_table
before the mana_hwc_destroy_queues() call here (and resetting
gc->max_num_cqs alongside the other stale init fields in
mana_hwc_establish_channel()) be enough?

> +	synchronize_rcu();
> +	vfree(old_cq_table);
> +

[ ... ]

> +skip_reinit:
> +
> +	/* No RCU needed: still in mana_hwc_create_channel, the
> +	 * pointer has not been published to concurrent senders yet.
> +	 */

[Severity: Low]
This comment and the publish earlier in the same function seem to disagree.
Earlier in mana_hwc_create_channel():

	/* Publish driver_data last, under hwc_lock: the lock orders the hwc
	 * initialisation above before the pointer becomes visible and
	 * serialises the publish against the control-plane readers in
	 * mana_gd_send_request(), mana_need_log() and mana_serv_reset().
	 */
	spin_lock_irqsave(&gc->hwc_lock, flags);
	gc->hwc.driver_data = hwc;
	spin_unlock_irqrestore(&gc->hwc_lock, flags);

so the pointer has been published by the time skip_reinit is reached.  What
keeps senders out is hwc->channel_up, which mana_hwc_get_msg_index()
tests.  Could the comment say that instead?

>  	err = mana_hwc_test_channel(gc->hwc.driver_data,
> -				    HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
> +				    hwc->num_inflight_msg,
>  				    max_req_msg_size, max_resp_msg_size);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to test HWC: %d\n", err);

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