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

[netdev-bot+sashiko@kernel.org]

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

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

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] The patch replaces the unbounded down(&hwc->sema) with
  down_timeout(..., hwc_timeout).
- [Medium] mana_hwc_get_msg_index() replaces the unbounded down() with
  down_timeout().

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  mana_hwc_rx_event_handler() returns without reposting the RX WQE when…

--

net: mana: support concurrent HWC requests

This patch gives each HWC message slot separate sender and response
references, serializes SQ posting, and replaces the unbounded admission
down() with down_timeout(), which returns -EBUSY or -ETIMEDOUT. It also
publishes the channel under a new hwc_lock, drains active senders on
teardown, and moves hwc_timeout updates to atomic helpers.

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

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 03a3a0e9d3f84..9ea67c076ebda 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -6,34 +6,105 @@
>  #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;
> +	u32 wait_ms;
>  	u32 index;
> +	int err;
>  
> -	down(&hwc->sema);
> +	wait_ms = mana_hwc_timeout_read(hwc);
> +	if (down_timeout(&hwc->sema, msecs_to_jiffies(wait_ms))) {

[Severity: High]
Before this patch, admission could only block here. Every command that
reached mana_hwc_send_request() was eventually posted. With
down_timeout(), a channel that is healthy but busy can now return -EBUSY
without ever sending the command.

At HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH of 1, every sender shares one
permit in FIFO order. That includes RDMA verbs that users can reach
through mana_ib. The wait is hwc_timeout, which stays at 1 ms for good
after any mana_hwc_timeout_reduce().

Several teardown callers do not handle a command that was never sent.
mana_gd_destroy_eq() ignores the result:

	if (queue->eq.disable_needed)
		mana_gd_disable_queue(queue);

mana_gd_create_hw_eq() has already set dma_region_handle to
GDMA_INVALID_DMA_REGION, so the later mana_gd_destroy_dma_region() call
in mana_gd_destroy_queue() cannot replace the missed disable.
mana_gd_destroy_queue() then calls mana_gd_free_memory() without
condition. mana_destroy_wq_obj() returns void, so its callers free the
WQ/CQ memory either way.

If GDMA_DISABLE_QUEUE, GDMA_DESTROY_DMA_REGION or MANA_DESTROY_WQ_OBJ is
rejected at admission like this, can the device, which is still healthy,
keep writing EQEs/CQEs into pages the kernel has already freed and
reused?

> +		spin_lock_irqsave(&r->lock, flags);
> +		if (!hwc->channel_up)
> +			err = -ENODEV;
> +		else if (atomic_read(&hwc->abandoned_msgs))
> +			err = -ETIMEDOUT;
> +		else
> +			err = -EBUSY;

[Severity: Medium]
Can this choose -EBUSY when the channel is hung? Only the permit holder
raises abandoned_msgs, in mana_hwc_wait_for_response(), and only after
its own completion wait has expired:

mana_hwc_wait_for_response() {
    ...
	if (err == -EINPROGRESS) {
		ctx->error = -ETIMEDOUT;
		ctx->responded = true;
		atomic_inc(&hwc->abandoned_msgs);
    ...
}

The bootstrap depth gives one permit. Suppose a waiter enters
down_timeout() after holder A takes the permit but before A enters
wait_for_completion_timeout(), or both timers expire in the same jiffy.
The waiter then reads abandoned_msgs == 0 and returns -EBUSY, even
though the device is not responding.

There is a second case after mana_serv_reset() calls
mana_gd_cancel_hwc_timeout(). A request posted before the cancel still
holds the permit with its old wait. The suspend-path commands would then
fail at once in down_timeout(0) with -EBUSY. Before, they blocked in
down() and failed quietly with -ETIMEDOUT.

The existing consumers check for -ETIMEDOUT.

mana_need_log() only suppresses -ETIMEDOUT on a published hwc whose
timeout is 0. Two new errors slip past it:

  - the -EBUSY returned here
  - the -ENODEV that mana_hwc_stop_channel() sets after
    mana_hwc_unpublish_channel() has cleared gc->hwc.driver_data

Both would now print dev_err() messages on reset and teardown paths,
for example from mana_gd_deregister_device().

mana_gd_probe() schedules recovery only for -ETIMEDOUT or -EPROTO, and
mana_serv_reset() rescans after a failed resume only for those two
codes:

	if (err == -ETIMEDOUT || err == -EPROTO) {

If a probe or resume gets -EBUSY from a hung HWC, will it fail with no
recovery scheduled?

> +		spin_unlock_irqrestore(&r->lock, flags);
> +
> +		return err;
> +	}

[ ... ]

> @@ -90,40 +183,53 @@ 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 and was not introduced by this patch. This
rejection path reposts rx_req, but the range check before it in
mana_hwc_rx_event_handler() 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;
	}
    ...
}

Does each response with an out-of-range hwc_msg_id lose one RQ buffer
for good? At depth 1, a single such response from the device or a
hostile CVM host would leave the RQ empty. Every later response could
then not be delivered until a reset.

[ ... ]

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