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

[netdev-bot+sashiko@kernel.org]

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

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

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

- [High] Once a timed-out request quarantines the HWC's only slot, every
  later request on a dead channel fails with -EBUSY instead of…
- [High] A slot can be freed and given to a new sender while the RX
  handler still holds ctx->lock on it.
- [Low] The title says the patch adds "support concurrent HWC requests",
  and the message says it allows "independent message IDs to make…
- [Low] If mana_hwc_publish_channel() finds the slot occupied (a
  WARN_ON_ONCE case), mana_hwc_create_channel() calls the full…

Pre-existing issues:
- [Medium] After one timeout, mana_hwc_timeout_reduce() permanently
  lowers hwc_timeout to 1 ms.
- [Medium] mana_hwc_rx_event_handler() returns early when msg_id >=
  num_inflight_msg, and on that path it does not repost rx_req, even…

--

net: mana: support concurrent HWC requests

This patch replaces the depth-one HWC timeout latch with per-slot sender
and response reference counting. It also serializes SQ posting, returns
-EBUSY when admission slots are exhausted, and moves hwc_timeout to atomic
helpers. Channel publication and teardown are restructured around a new
gc->hwc_lock and a sender drain.

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

> net: mana: support concurrent HWC requests

[ ... ]

> This preserves response-buffer withdrawal and timeout quarantine while
> allowing independent message IDs to make progress.

[Severity: Low]
Does this describe the commit accurately on its own?

At this commit, mana_hwc_create_bootstrap_channel() always sizes the
channel with HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH, which is 1. That gives
one semaphore permit and a one-bit map, so no requests can run
concurrently yet. The visible change here is the new error and timeout
behaviour.

Larger depths only arrive with the next patch in the series, "net: mana:
add dynamic HWC queue depth with reinit path". Could the message say so?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 89b6e30863dae..9fdd84ccba653 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -6,57 +6,122 @@
>  #include <net/mana/hw_channel.h>
>  #include <linux/vmalloc.h>
>  
> -static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
> +u32 mana_hwc_timeout_read(const struct hw_channel_context *hwc)

[ ... ]

> +static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, void *resp,
> +				  u32 resp_len,
> +				  struct hwc_caller_ctx **caller_ctx)
>  {
>  	struct gdma_resource *r = &hwc->inflight_msg_res;
>  	struct hwc_caller_ctx *ctx;
>  	unsigned long flags;
> +	bool channel_up;
> +	u32 wait_ms;
>  	u32 index;
>  
> -	down(&hwc->sema);
> +	wait_ms = mana_hwc_timeout_read(hwc);
> +	if (down_timeout(&hwc->sema, msecs_to_jiffies(wait_ms))) {
> +		spin_lock_irqsave(&r->lock, flags);
> +		channel_up = hwc->channel_up;
> +		spin_unlock_irqrestore(&r->lock, flags);
>  
> -	spin_lock_irqsave(&r->lock, flags);
> +		/* Slot pressure is not evidence that the HWC stopped responding. */
> +		return channel_up ? -EBUSY : -ENODEV;
> +	}

[Severity: High]
Is -EBUSY the right return value when the slot is held by a request that
timed out?

At this commit the channel has a single slot. On timeout,
mana_hwc_wait_for_response() marks the request abandoned but leaves
resp_pending set:

    if (err == -EINPROGRESS) {
        ctx->responded = true;
        abandoned = true;
    }

So the bitmap bit and the semaphore permit stay held until a late response
arrives or teardown runs. Only that one caller sees -ETIMEDOUT.

Nothing on the timeout path clears channel_up. Every later sender
therefore fails down_timeout() here and gets -EBUSY. Before this series,
and with the latch from the earlier patch in the series, those senders
got -ETIMEDOUT.

The recovery triggers only check for -ETIMEDOUT. For example, in
mana_gf_stats_work_handler():

    err = mana_query_gf_stats(ac);
    if (err == -ETIMEDOUT) {
        ...
        mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);

The recovery path in mana_gd_probe() and the rescan in mana_serv_reset()
check only err == -ETIMEDOUT || err == -EPROTO.

Suppose another command, such as ethtool PHY stats, vport config or an
RDMA command, times out first. Would the gf_stats work then keep getting
-EBUSY every 2 seconds, so the HWC reset is never scheduled and the device
stays wedged?

Logging is affected too. Once mana_gd_cancel_hwc_timeout() sets the
timeout to 0 in mana_serv_reset(), the first command sent during
mana_gd_suspend() is quarantined immediately. Every later command returns
-EBUSY. Since mana_need_log() only suppresses -ETIMEDOUT, each of them is
logged with dev_err.

The commit message describes -EBUSY as admission pressure that "must not
make callers start dead-channel recovery". When abandoned requests hold
the slots, isn't -EBUSY actually a sign of a dead channel?

[ ... ]

> @@ -97,40 +162,54 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  				 struct hwc_work_request *rx_req, u16 msg_id)
>  {
>  	const struct gdma_resp_hdr *resp_msg = rx_req->buf_va;
> +	struct gdma_resource *r = &hwc->inflight_msg_res;
>  	struct hwc_caller_ctx *ctx;
> +	bool release;
>  	int err;
>  
> -	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
> +	spin_lock(&r->lock);
> +	if (!test_bit(msg_id, r->map)) {
> +		spin_unlock(&r->lock);
>  		dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
>  		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
>  		return;
>  	}

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. This path
reposts the receive buffer, but the out-of-range check in
mana_hwc_rx_event_handler() does not:

    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;
    }

With a depth of 1, wouldn't a single out-of-range msg_id from the device
leave the RQ with no posted buffers for the rest of the channel's life?
With the new -EBUSY behaviour above, that starved channel would also never
trigger recovery.

>  
>  	ctx = hwc->caller_ctx + msg_id;
> -
>  	spin_lock(&ctx->lock);
> -	if (!ctx->output_buf) {
> +	spin_unlock(&r->lock);
> +
> +	/* Consume the response-side reference exactly once. This releases a
> +	 * quarantined slot after its late response arrives.
> +	 */
> +	release = ctx->resp_pending;
> +	ctx->resp_pending = false;

[Severity: High]
Can this slot be freed and handed to a new sender while the handler still
holds ctx->lock?

After r->lock is dropped, the handler keeps reading and writing
resp_pending, responded, output_buf and output_buflen, and it calls
memcpy() and complete(). When release is false, it does all of this
without holding a reference.

The last hwc_ctx_put() goes through mana_hwc_put_msg_index(). That
function clears the bit under r->lock only and never takes ctx->lock.
mana_hwc_get_msg_index() then reinitializes the slot, also under r->lock
only:

    refcount_set(&ctx->refcnt, 2);
    ctx->output_buf = resp;
    ctx->output_buflen = resp_len;
    ...
    ctx->responded = false;
    ctx->resp_pending = true;

Take a duplicate response for a msg_id whose first response has already
completed the request. The responded branch suggests duplicates are meant
to be tolerated:

CPU1 (duplicate response)      CPU2 (original sender)
mana_hwc_handle_resp()
  test_bit() succeeds
  spin_lock(&ctx->lock)
  spin_unlock(&r->lock)
                               mana_hwc_wait_for_response()
                                 /* already past its ctx->lock section */
                                 hwc_ctx_put()   /* last reference */
                                   bitmap_clear(r->map, ...)
                                   up(&hwc->sema)

CPU3 (new sender)
mana_hwc_get_msg_index()
  rewrites refcnt, output_buf, output_buflen, resp_pending, responded
  under r->lock only, while CPU1 still holds ctx->lock

Possible outcomes:

The duplicate can overwrite the new resp_pending with false. Or the new
sender's mana_hwc_submit_request() can see responded == true and fail
with -ENODEV. Either way refcnt never reaches zero, so the slot and its
permit leak until teardown, and with depth 1 every later request gets
-EBUSY.

The stale duplicate can be copied into the new caller's output buffer and
complete that request.

On weakly ordered CPUs, the handler could pair a new output_buf with a
stale output_buflen, or with NULL.

The struct hwc_caller_ctx comment says "Once its bitmap bit is set, lock
protects every field below except msg_id and refcnt". The commit message
says the slot is released "only after both sides have finished".

Would it work to initialize the slot while holding ctx->lock nested inside
r->lock, which matches the existing r->lock then ctx->lock order? Or to
clear the bitmap bit while holding ctx->lock?

[ ... ]

> @@ -983,13 +1115,151 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)

[ ... ]

> +	err = mana_hwc_create_bootstrap_channel(hwc);
> +	if (err) {
> +		mana_hwc_release_channel(hwc);
> +		return err;
> +	}
> +
> +	err = mana_hwc_publish_channel(hwc);
> +	if (err) {
> +		mana_hwc_release_channel(hwc);
> +		return err;
> +	}

[Severity: Low]
mana_hwc_publish_channel() fails only when its WARN_ON_ONCE() finds the
slot occupied. In that case this calls the full mana_hwc_release_channel()
on the new, unpublished hwc.

That function also resets gc-wide state that belongs to whichever channel
holds the slot. It frees gc->cq_table, zeroes gc->max_num_cqs, clears
gc->hwc.gdma_context, and resets the doorbell and pdid.

If SMC teardown fails on this path, mana_hwc_retain_channel() finds the
slot occupied and only logs. The hwc and its queues are then leaked with
no owner.

Does this case need a narrower unwind? It can only happen if channel
creation runs concurrently without serialization, which the normal
lifecycle doesn't do.

[ ... ]

> @@ -1004,108 +1274,86 @@ static int mana_hwc_response_status(struct hw_channel_context *hwc,

[ ... ]

> +static int mana_hwc_wait_for_response(struct hw_channel_context *hwc,
> +				      struct hwc_caller_ctx *ctx,
> +				      u32 command)
>  {

[ ... ]

> +	if (wait_ms)
> +		dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
> +			command, wait_ms);
> +
> +	mana_hwc_timeout_reduce(hwc);
> +	hwc_ctx_put(hwc, ctx);
> +
> +	return -ETIMEDOUT;

[Severity: Medium]
This isn't a bug introduced by this patch, but mana_hwc_timeout_reduce()
permanently lowers hwc_timeout to 1 ms after a single timeout.

Once a late response frees the slot, later commands are posted to the
firmware and abandoned after about one jiffy, while the firmware may still
be executing them. Callers like mana_gd_create_mana_wq_cq() then free the
DMA memory those commands refer to.

The code before this series behaved the same way. This patch actually
posts less often, since a slot is reused only after its late response
arrives.

Could the driver and the device still end up disagreeing about state here,
with the device writing by DMA into freed memory?

[ ... ]

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