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.
> 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
[ ... ]
> @@ -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);
[ ... ]
> @@ -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.
>
> 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);
> }
[ ... ]
> @@ -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);
[ ... ]
> @@ -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);
[ ... ]
> 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
