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

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

[Wei Hu]

From: Long Li <longli@microsoft.com>

Concurrent requests require the response handler and the waiting sender to
agree on the lifetime and result state of each message slot.

Protect the caller's output buffer, Linux error and device status with a
per-slot lock. Publish the output buffer only while the sender is waiting
and withdraw it before every return. A late response therefore cannot copy
through a pointer to caller storage whose lifetime has ended.

Reinitialize completion and result state when the slot is acquired. If
the response records its result while the timed wait expires, return that
result instead of reporting a false timeout.

HWC responses contain only the slot ID. After a genuine timeout, prevent
the serialized bootstrap channel from reusing that ID until teardown. This
keeps a stale response from completing a later request before the next
patch replaces the depth-one latch with per-slot sender and response
ownership.

Split response-status handling and request cleanup into helpers so the send
path has structured returns rather than phase-jumping gotos.

Link: https://lore.kernel.org/r/20260908035201.402424-3-longli@microsoft.com
Link: https://lore.kernel.org/r/20260908035201.402424-4-longli@microsoft.com
Signed-off-by: Long Li <longli@microsoft.com>
Signed-off-by: Wei Hu <weh@microsoft.com>
---
 .../net/ethernet/microsoft/mana/hw_channel.c  | 134 +++++++++++++-----
 include/net/mana/hw_channel.h                 |   7 +-
 2 files changed, 106 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index b1d972968f0d..89b6e30863da 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -9,6 +9,7 @@
 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;
 
@@ -16,9 +17,22 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 
 	spin_lock_irqsave(&r->lock, flags);
 
+	if (hwc->hwc_timed_out) {
+		spin_unlock_irqrestore(&r->lock, flags);
+		up(&hwc->sema);
+		return -ETIMEDOUT;
+	}
+
 	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);
+	ctx->output_buf = NULL;
+	ctx->output_buflen = 0;
+	ctx->error = -EINPROGRESS;
+	ctx->status_code = 0;
+
 	bitmap_set(hwc->inflight_msg_res.map, index, 1);
 
 	spin_unlock_irqrestore(&r->lock, flags);
@@ -28,12 +42,15 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 	return 0;
 }
 
-static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
+static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id,
+				   bool timed_out)
 {
 	struct gdma_resource *r = &hwc->inflight_msg_res;
 	unsigned long flags;
 
 	spin_lock_irqsave(&r->lock, flags);
+	if (timed_out)
+		hwc->hwc_timed_out = true;
 	bitmap_clear(hwc->inflight_msg_res.map, msg_id, 1);
 	spin_unlock_irqrestore(&r->lock, flags);
 
@@ -90,14 +107,21 @@ 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);
+	if (!ctx->output_buf) {
+		spin_unlock(&ctx->lock);
+		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+		return;
+	}
 
-	memcpy(ctx->output_buf, resp_msg, resp_len);
-out:
+	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->output_buf = NULL;
 	ctx->error = err;
 
 	/* Must post rx wqe before complete(), otherwise the next rx may
@@ -106,6 +130,7 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
 
 	complete(&ctx->comp_event);
+	spin_unlock(&ctx->lock);
 }
 
 static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -694,8 +719,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) {
 		init_completion(&ctx[i].comp_event);
+		spin_lock_init(&ctx[i].lock);
+	}
 
 	hwc->caller_ctx = ctx;
 
@@ -958,6 +985,38 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	vfree(old_cq_table);
 }
 
+static int mana_hwc_response_status(struct hw_channel_context *hwc,
+				    u32 command, int error, u32 status)
+{
+	if (error)
+		return error;
+
+	if (!status || status == GDMA_STATUS_MORE_ENTRIES)
+		return 0;
+
+	if (status == GDMA_STATUS_CMD_UNSUPPORTED)
+		return -EOPNOTSUPP;
+
+	if (command != MANA_QUERY_PHY_STAT)
+		dev_err(hwc->dev, "Command 0x%x failed with status: 0x%x\n",
+			command, status);
+
+	return -EPROTO;
+}
+
+static void mana_hwc_finish_request(struct hw_channel_context *hwc,
+				    struct hwc_caller_ctx *ctx, u16 msg_id,
+				    bool timed_out)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+
+	mana_hwc_put_msg_index(hwc, msg_id, timed_out);
+}
+
 int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 			  const void *req, u32 resp_len, void *resp)
 {
@@ -965,26 +1024,32 @@ 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;
 	u32 dest_vrcq;
 	u32 dest_vrq;
 	u32 command;
+	u32 status;
 	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];
+	ctx = hwc->caller_ctx + msg_id;
 
 	if (req_len > tx_wr->buf_len) {
 		dev_err(hwc->dev, "HWC: req msg size: %d > %d\n", req_len,
 			tx_wr->buf_len);
-		err = -EINVAL;
-		goto out;
+		mana_hwc_finish_request(hwc, ctx, msg_id, false);
+		return -EINVAL;
 	}
 
-	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)
@@ -1006,11 +1071,23 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	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;
+		mana_hwc_finish_request(hwc, ctx, msg_id, false);
+		return err;
 	}
 
 	if (!wait_for_completion_timeout(&ctx->comp_event,
-					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
+					 msecs_to_jiffies(hwc->hwc_timeout))) {
+		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) {
+			mana_hwc_finish_request(hwc, ctx, msg_id, false);
+			return mana_hwc_response_status(hwc, command, err, status);
+		}
+
 		if (hwc->hwc_timeout != 0)
 			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
 				command, hwc->hwc_timeout);
@@ -1019,27 +1096,16 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 		if (hwc->hwc_timeout > 1)
 			hwc->hwc_timeout = 1;
 
-		err = -ETIMEDOUT;
-		goto out;
+		mana_hwc_finish_request(hwc, ctx, msg_id, true);
+		return -ETIMEDOUT;
 	}
 
-	if (ctx->error) {
-		err = ctx->error;
-		goto out;
-	}
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	err = ctx->error;
+	status = ctx->status_code;
+	spin_unlock_irqrestore(&ctx->lock, flags);
 
-	if (ctx->status_code && ctx->status_code != GDMA_STATUS_MORE_ENTRIES) {
-		if (ctx->status_code == GDMA_STATUS_CMD_UNSUPPORTED) {
-			err = -EOPNOTSUPP;
-			goto out;
-		}
-		if (command != MANA_QUERY_PHY_STAT)
-			dev_err(hwc->dev, "Command 0x%x failed with status: 0x%x\n",
-				command, ctx->status_code);
-		err = -EPROTO;
-		goto out;
-	}
-out:
-	mana_hwc_put_msg_index(hwc, msg_id);
-	return err;
+	mana_hwc_finish_request(hwc, ctx, msg_id, false);
+	return mana_hwc_response_status(hwc, command, err, status);
 }
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index eb1c005bc091..e733301f8740 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -168,10 +168,12 @@ struct hwc_wq {
 
 struct hwc_caller_ctx {
 	struct completion comp_event;
+	/* Protects the output buffer and response state from timeout. */
+	spinlock_t lock;
 	void *output_buf;
 	u32 output_buflen;
 
-	u32 error; /* Linux error code */
+	int error; /* Linux error code */
 	u32 status_code;
 };
 
@@ -201,6 +203,9 @@ struct hw_channel_context {
 	u32 dest_vrcq_id;
 	u32 hwc_timeout;
 
+	/* Prevents message ID reuse after a timeout; protected by the map lock. */
+	bool hwc_timed_out;
+
 	/* The PF may own the HWC queues while this is true. */
 	bool setup_active;
 	struct hwc_caller_ctx *caller_ctx;
-- 
2.43.0

Re: [PATCH net-next v7 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 · Medium: 1 · Low: 1

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

- [Medium] At this commit, one genuine HWC timeout sets the channel-wide
  latch hwc->hwc_timed_out, in mana_hwc_put_msg_index(..., true) called…
- [Low] This patch fixes real bugs that exist in the baseline driver,
  including a memory-safety bug, but carries no Fixes: tag and no stable…

--

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

This commit adds a per-slot spinlock to hwc_caller_ctx. The lock protects
output_buf, error and status_code. The commit also reinitializes the
completion and result state when a slot is acquired, and adds a
channel-wide hwc_timed_out latch that rejects new requests after a genuine
timeout. Status handling and request cleanup are split into helpers.

Link: https://lore.kernel.org/r/20260908035201.402424-3-longli@microsoft.com
Link: https://lore.kernel.org/r/20260908035201.402424-4-longli@microsoft.com

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index b1d972968f0d7..89b6e30863dae 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -16,9 +17,22 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
>  
>  	spin_lock_irqsave(&r->lock, flags);
>  
> +	if (hwc->hwc_timed_out) {
> +		spin_unlock_irqrestore(&r->lock, flags);
> +		up(&hwc->sema);
> +		return -ETIMEDOUT;
> +	}

[Severity: Medium]
After one genuine timeout, mana_hwc_send_request() calls
mana_hwc_finish_request(hwc, ctx, msg_id, true). That sets
hwc->hwc_timed_out in mana_hwc_put_msg_index(). Nothing clears the flag
until the hwc is freed and mana_hwc_create_channel() allocates a new one.

After that point, does every mana_hwc_get_msg_index() call return
-ETIMEDOUT without posting anything to the SQ? That would include teardown
commands.

For example, mana_gd_destroy_queue() ignores the result of the destroy
command and frees the host memory anyway:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_queue() {
    ...
	mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
	mana_gd_free_memory(gmi);
	kfree(queue);
}

If the device was only slow and not dead, could it keep a DMA region
mapping that points at freed host pages?

mana_serv_reset() in gdma_pci.c does this:

	/* HWC is not responding in this case, so don't wait */
	hwc->hwc_timeout = 0;

With a zero timeout, the first suspend command now latches the channel,
so the rest of the suspend-time destroy commands are never posted. Before
this patch they were posted.

The firmware HWC_DATA_CFG_HWC_TIMEOUT update in
mana_hwc_init_event_handler() changes hwc_timeout but cannot clear the
latch. Runtime timeout users such as mana_gf_stats_work_handler() will
also see later commands fail without being sent until the reset runs.

The next patch in the series, "net: mana: support concurrent HWC
requests", removes hwc_timed_out. It replaces the latch with per-slot
quarantine, which a late response or teardown releases. So this only
affects this intermediate commit, either during bisection or if this
patch is backported alone.

Could this commit avoid the channel-wide latch so that every commit in the
series is bisect-safe?

> +
>  	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);
> +	ctx->output_buf = NULL;
> +	ctx->output_buflen = 0;
> +	ctx->error = -EINPROGRESS;
> +	ctx->status_code = 0;

[Severity: Low]
This isn't a bug in the new code. These changes, together with the
output_buf handling under ctx->lock, fix problems that exist in the
baseline driver, but the commit has no Fixes: tag and no Cc: stable.

In the baseline, the bootstrap channel has depth one, so every request
uses msg_id 0. Three problems follow from that.

First, mana_hwc_get_msg_index() never called reinit_completion(). A late
complete() for a request that had already timed out left comp_event done.
The next wait_for_completion_timeout() then returned at once, with stale
error and status_code values and a response buffer that was never filled.

Second, mana_hwc_handle_resp() only checked test_bit(msg_id) before
memcpy(ctx->output_buf, ...). There is a window between bitmap_set() in
mana_hwc_get_msg_index() and this assignment in mana_hwc_send_request():

	ctx->output_buf = resp;

A stale response arriving in that window could pass the check and copy
into the previous caller's output_buf. That buffer is often on the stack,
such as the resp in mana_gd_query_hwc_timeout(), and that function has
already returned.

Third, a late response arriving after the next request had set its bit
and output_buf would be accepted as the answer to that request.

The commit message does say "A late response therefore cannot copy through
a pointer to caller storage whose lifetime has ended". Without a Fixes:
tag, though, stable trees won't pick this up. A plain backport of this
patch would also carry the hwc_timed_out latch that the next patch
removes.

Would it make sense to send a self-contained fix for these, with a Fixes:
tag, to net separately?

> +
>  	bitmap_set(hwc->inflight_msg_res.map, index, 1);
>  
>  	spin_unlock_irqrestore(&r->lock, flags);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791561995.git.weh%40linux.microsoft.com