Re: [PATCH net-next 5/6] net/mlx5: HWS, Handle timeout draining the send queue for FW STEs

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Unpropagated error: on a DRAIN_SYNC timeout the new branch in
  mlx5hws_send_stes_fw() (send.c:1338-1345) jumps to `fail_rule`…

--

net/mlx5: HWS, Handle timeout draining the send queue for FW STEs

The patch makes mlx5hws_send_stes_fw() check the return value of the
DRAIN_SYNC call to mlx5hws_send_queue_action() and jump to the fail_rule
label when the drain times out. Previously the drain result was ignored.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> index 9ff109fc8230a..bddac018b44b8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> @@ -1335,10 +1335,14 @@ void mlx5hws_send_stes_fw(struct mlx5hws_context *ctx,
>  	pdn = ctx->pd_num;
>  
>  	/* Writing through FW can't HW fence, therefore we drain the queue */
> -	if (send_attr->fence)
> -		mlx5hws_send_queue_action(ctx,
> -					  queue_id,
> -					  MLX5HWS_SEND_QUEUE_ACTION_DRAIN_SYNC);
> +	if (send_attr->fence) {
> +		enum mlx5hws_send_queue_actions drain =
> +			MLX5HWS_SEND_QUEUE_ACTION_DRAIN_SYNC;
> +
> +		ret = mlx5hws_send_queue_action(ctx, queue_id, drain);
> +		if (ret)
> +			goto fail_rule;
> +	}

[Severity: High]
Can this new early exit end up reported as success to the caller?

The only site that sets send_attr.fence = 1 is
hws_action_create_dest_match_range_fill_table() in
steering/hws/action.c, and it sets rule = NULL:

	ste_attr.send_attr.user_data = &no_use;
	ste_attr.send_attr.rule = NULL;
	ste_attr.send_attr.fence = 1;

With the new goto, neither of the two hws_send_wqe_fw() calls for
ste_attr->rtc_1 and ste_attr->rtc_0 runs, so no GTA OP_ACTIVATE STE is
written into the range table.

mlx5hws_send_stes_fw() is declared void in steering/hws/send.h, so the
only failure channels are rule->status and the completion entry pushed by
mlx5hws_send_engine_gen_comp(). At fail_rule, rule is NULL:

fail_rule:
	if (likely(rule))
		rule->status = ...;

	mlx5hws_send_engine_gen_comp(queue, send_attr->user_data, MLX5HWS_FLOW_OP_ERROR);

The caller never reads queue->completed. Its only post-send check is its
own drain:

action.c:hws_action_create_dest_match_range_fill_table() {
	/* Send WQEs to FW */
	mlx5hws_send_stes_fw(ctx, queue, &ste_attr);

	/* Poll for completion */
	ret = mlx5hws_send_queue_action(ctx, ctx->queues - 1,
					MLX5HWS_SEND_QUEUE_ACTION_DRAIN_SYNC);
	if (ret) {
		mlx5hws_err(ctx, "Failed to drain control queue");
		goto error;
	}

	mutex_unlock(&ctx->ctrl_lock);

	return 0;
}

Can that second drain succeed after the first one timed out? In
mlx5hws_send_queue_action() the deadline is tested after the poll:

		while (wait_comp && !mlx5hws_send_engine_empty(queue)) {
			hws_send_engine_poll_cq(queue, NULL, &polled, 0);

			if (unlikely(time_after(jiffies, timeout))) {
				...
				return -ETIMEDOUT;
			}
		}

so -ETIMEDOUT can be returned on the very iteration whose poll emptied
the queue, and a transient stall that resolves during the second drain
gives the same result. mlx5hws_send_engine_empty() compares the SQ
cur_post against the CQ poll_wqe only, not queue->completed or
queue->err, so the FLOW_OP_ERROR entry is invisible here.

In that case hws_action_create_dest_match_range_fill_table() returns 0
and mlx5hws_action_create_dest_match_range() continues on to program an
STC pointing at the empty STE table:

	stc_attr.action_type = MLX5_IFC_STC_ACTION_TYPE_JUMP_TO_STE_TABLE;
	stc_attr.ste_table.ste_pool = table_ste->pool;

Does this leave a usable-looking MLX5_FLOW_DEST_RANGE_FIELD_PKT_LEN
action whose RTC contains no activated STE, so traffic steered into it
always misses to miss_ft while the definer, table_ste and hit_ft_action
stay allocated?

Note the behaviour change relative to the pre-patch code: previously the
drain result was ignored but the FW writes still happened (only the fence
ordering was violated), whereas now the writes are skipped while success
can still be returned. Would it make sense for
mlx5hws_send_stes_fw() to return int, or for the caller to consume and
check the generated completion, so the timeout is actually observable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915111912.3926585-1-tariqt%40nvidia.com