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

From
netdev-bot+sashiko@kernel.org
Date
2026-09-11 06:53:27
Also in
linux-hyperv, linux-rdma, lkml
Reply to
Long Li, 2026-09-08
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

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

- [Medium] The commit message presents this patch as pure preparation
  ("Add per-slot locking, sender/response references and a responded…
- [Low] The new field comments added to struct hwc_caller_ctx in
  include/net/mana/hw_channel.h describe a narrower contract than…

Pre-existing issues:
- [High] In this patch, mana_hwc_send_request()'s out: label latches
  ctx->responded, clears output_buf and drops both the sender and the…
- [Medium] hwc->hwc_timeout is read and read-modify-written by senders in
  process context (`wait_ms = hwc->hwc_timeout;` and `if…

--

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

This adds a per-slot spinlock, a sender/response reference pair and a
responded flag to struct hwc_caller_ctx, makes ctx->error signed, and
withdraws the caller's response buffer once the sender stops waiting.
It also ignores a zero timeout value in a GDMA_QUERY_HWC_TIMEOUT reply.

No Link: tags in the commit header.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 88e92e94e2e90..6605e7a9c481b 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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: Medium]
Should this hunk carry a Fixes: tag rather than being described as
preparation?

The commit message says:

    Add per-slot locking, sender/response references and a responded flag;
    make ctx->error signed. Stop copying after buffer withdrawal or
    completion.

"Stop copying after buffer withdrawal or completion" reads like a refactor
item, but at the baseline mana_hwc_send_request() released the message slot
on timeout while leaving ctx->output_buf and ctx->output_buflen pointing at
the caller's response buffer, which is usually on the caller's stack:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_query_hwc_timeout() {
	struct gdma_query_hwc_timeout_resp resp = {};
	...
	err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
}

The baseline mana_hwc_handle_resp() then copied a late response into that
pointer as soon as the bit was set again by the next sender:

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

With HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH == 1 the same index is always
reused, so the write lands in a stack frame that has already returned, and
the length check runs against the stale output_buflen.

Is the new output_buf test here, together with the ctx->output_buf = NULL
withdrawal in mana_hwc_send_request(), what closes that window? If so, would
a Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network
Adapter (MANA)") and a mention of the corruption in the changelog make this
visible to stable? The changelog notes the work was re-targeted from a "net
fixes-only posting" to net-next, which makes the fix easy to miss.
quoted hunk ↗ jump to hunk
 
 	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);
 }
[ ... ]
quoted hunk ↗ jump to hunk
@@ -878,8 +925,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);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -895,43 +945,108 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
+	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);
[ ... ]
quoted hunk ↗ jump to hunk
 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 befa09674ce56..b377e221aa5c8 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;
 };
[Severity: Low]
These three comments look narrower than what hw_channel.c actually relies
on. Could they be extended?

The lock comment mentions output_buf only, but the out: block in
mana_hwc_send_request() also uses it to serialize ->responded and to decide
who owns the response-side reference:

	spin_lock_irqsave(&ctx->lock, flags);
	ctx->output_buf = NULL;
	drop_resp_ref = !ctx->responded;
	ctx->responded = true;
	spin_unlock_irqrestore(&ctx->lock, flags);

and ctx->error / ctx->status_code are read and written under the same lock
in both mana_hwc_handle_resp() and mana_hwc_send_request().

The responded comment says it is set "by the first handle_resp(), or by the
sender's timeout path", but the out: block above is also reached from the
req_len > tx_wr->buf_len rejection, from mana_hwc_post_tx_wqe() failure, and
from the no-wait teardown path where hwc_timeout is 0.

The error comment says "Linux error code (negative errno or 0)", while
mana_hwc_get_msg_index() stores -EINPROGRESS as an in-band "no response yet"
state that mana_hwc_send_request() special-cases with
if (err != -EINPROGRESS). Should that sentinel be documented next to the
field, since every reader of ->error has to know about it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com
Thread (10 messages) 10 messages, 3 authors, last reply 2026-09-15
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help