[PATCH net-next 0/6] net/mlx5: more HWS misc enhancements

[PATCH net-next 0/6] net/mlx5: more HWS misc enhancements

[Tariq Toukan]

Hi,

This series by Yevgeny brings more misc improvements to the mlx5 HWS
driver.

Regards,
Tariq

Yevgeny Kliteynik (6):
  net/mlx5: HWS, Return meaningful error code in hws_send_wqe_fw
  net/mlx5: HWS, Fix error message in mlx5hws_cmd_generate_wqe
  net/mlx5: HWS, Replace kzalloc with kzalloc_obj
  net/mlx5: HWS, Add timeout mechanism to draining send queues
  net/mlx5: HWS, Handle timeout draining the send queue for FW STEs
  net/mlx5: HWS, Remove unneeded WRITE_ONCE in post send

 .../mellanox/mlx5/core/steering/hws/bwc.c     |  4 ++-
 .../mellanox/mlx5/core/steering/hws/send.c    | 34 +++++++++++++------
 2 files changed, 26 insertions(+), 12 deletions(-)


base-commit: 272a65db243bfa34b9277632830e0e06d7e3518e
-- 
2.44.0

[PATCH net-next 1/6] net/mlx5: HWS, Return meaningful error code in hws_send_wqe_fw

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

Returning '-1' is non-idiomatic in kernel code.
Use a meaningful error code instead.

Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Erez Shitrit <erezsh@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 df3b93499eeb..ae3ff232c63b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
@@ -1305,7 +1305,7 @@ hws_send_wqe_fw(struct mlx5_core_dev *mdev,
 		goto send_wqe;
 	}
 
-	return -1;
+	return -EIO;
 }
 
 void mlx5hws_send_stes_fw(struct mlx5hws_context *ctx,
-- 
2.44.0

[PATCH net-next 2/6] net/mlx5: HWS, Fix error message in mlx5hws_cmd_generate_wqe

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

Kernel log messages should end with '\n' for proper formatting.

Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Erez Shitrit <erezsh@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 ae3ff232c63b..80cfd0ddba08 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
@@ -1287,7 +1287,7 @@ hws_send_wqe_fw(struct mlx5_core_dev *mdev,
 send_wqe:
 	ret = mlx5hws_cmd_generate_wqe(mdev, &attr, &cqe);
 	if (ret) {
-		mlx5_core_err(mdev, "Failed to write WQE using command");
+		mlx5_core_err(mdev, "Failed to write WQE using command\n");
 		return ret;
 	}
 
-- 
2.44.0

Re: [PATCH net-next 2/6] net/mlx5: HWS, Fix error message in mlx5hws_cmd_generate_wqe

[netdev-bot+sashiko]

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

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

- [Low] Commit subject/message misidentifies the location of the fix.

--

net/mlx5: HWS, Fix error message in mlx5hws_cmd_generate_wqe

This adds the missing trailing newline to the error message printed when
the generate WQE command fails in the HWS send path.

> net/mlx5: HWS, Fix error message in mlx5hws_cmd_generate_wqe

[Severity: Low]
This isn't a code bug, but does the subject point at the right function?
The only change is in hws_send_wqe_fw() in
drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c, which is the
caller of mlx5hws_cmd_generate_wqe():

send_wqe:
	ret = mlx5hws_cmd_generate_wqe(mdev, &attr, &cqe);
	if (ret) {
		mlx5_core_err(mdev, "Failed to write WQE using command\n");
		return ret;
	}

mlx5hws_cmd_generate_wqe() itself lives in hws/cmd.c and both of its error
prints already end with '\n':

	ret = mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
	if (ret) {
		mlx5_core_err(mdev, "Failed to write GTA WQE using FW\n");
		return ret;
	}

	status = MLX5_GET(generate_wqe_out, out, status);
	if (status) {
		mlx5_core_err(mdev, "Invalid FW CQE status %d\n", status);
		return -EINVAL;
	}

Would something like "net/mlx5: HWS, Add missing newline to error message in
hws_send_wqe_fw" be more accurate, so that someone grepping cmd.c for the
quoted string is not sent to the wrong place?

> 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 ae3ff232c63bc..80cfd0ddba08b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> @@ -1287,7 +1287,7 @@ hws_send_wqe_fw(struct mlx5_core_dev *mdev,
>  send_wqe:
>  	ret = mlx5hws_cmd_generate_wqe(mdev, &attr, &cqe);
>  	if (ret) {
> -		mlx5_core_err(mdev, "Failed to write WQE using command");
> +		mlx5_core_err(mdev, "Failed to write WQE using command\n");
>  		return ret;
>  	}
>

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

[PATCH net-next 3/6] net/mlx5: HWS, Replace kzalloc with kzalloc_obj

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

Replace kzalloc with the newer kzalloc_obj allocation function.

Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Erez Shitrit <erezsh@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 80cfd0ddba08..49dee44d0bef 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
@@ -732,7 +732,7 @@ static int hws_send_ring_alloc_sq(struct mlx5_core_dev *mdev,
 		goto destroy_wq_cyc;
 	}
 
-	sq->wr_priv = kzalloc(sizeof(*sq->wr_priv) * buf_sz, GFP_KERNEL);
+	sq->wr_priv = kzalloc_objs(*sq->wr_priv, buf_sz);
 	if (!sq->wr_priv) {
 		err = -ENOMEM;
 		goto free_dep_wqe;
-- 
2.44.0

Re: [PATCH net-next 3/6] net/mlx5: HWS, Replace kzalloc with kzalloc_obj

[netdev-bot+sashiko]

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

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

- [Low] Commit message names the wrong helper and does not describe the
  actual (partial) scope of the change.

--

net/mlx5: HWS, Replace kzalloc with kzalloc_obj

This converts one allocation in hws_send_ring_alloc_sq() from an open-coded
kzalloc(sizeof(*p) * n, GFP_KERNEL) to the newer object-array allocation
macro.

>  net/mlx5: HWS, Replace kzalloc with kzalloc_obj
>
>  Replace kzalloc with the newer kzalloc_obj allocation function.

[Severity: Low]
This isn't a bug in the code, but does the changelog name the right helper?

The subject and body both say kzalloc_obj, while the only converted site
uses the array variant kzalloc_objs(). include/linux/slab.h defines these
as two separate macros:

  #define kzalloc_obj(P, ...) \
	__alloc_objs(kzalloc, default_gfp(__VA_ARGS__), typeof(P), 1)

  #define kzalloc_objs(P, COUNT, ...) \
	__alloc_objs(kzalloc, default_gfp(__VA_ARGS__), typeof(P), COUNT)

Grepping this patch for kzalloc_obj as written in the message finds nothing.

Also, the message reads as a general replacement sweep, but only this one
array allocation is converted.  The single-object kzalloc(sizeof(*x),
GFP_KERNEL) sites that kzalloc_obj would actually apply to are still present
in the same driver, for example in hws/bwc_complex.c:

	subm->bwc_matcher = kzalloc(sizeof(*subm->bwc_matcher), GFP_KERNEL);

as well as the cdata and sr_data allocations in the same file, and they
remain unconverted at the end of the series.  Could the message mention that
the conversion is limited to this call site?

> 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 80cfd0ddba08b..49dee44d0bef5 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> @@ -732,7 +732,7 @@ static int hws_send_ring_alloc_sq(struct mlx5_core_dev *mdev,
>  		goto destroy_wq_cyc;
>  	}
>  
> -	sq->wr_priv = kzalloc(sizeof(*sq->wr_priv) * buf_sz, GFP_KERNEL);
> +	sq->wr_priv = kzalloc_objs(*sq->wr_priv, buf_sz);
>  	if (!sq->wr_priv) {
>  		err = -ENOMEM;
>  		goto free_dep_wqe;

The expansion here looks equivalent to the old code, with default_gfp()
yielding GFP_KERNEL and __alloc_objs() using size_mul() for the product, so
no functional change is expected.

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

[PATCH net-next 4/6] net/mlx5: HWS, Add timeout mechanism to draining send queues

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

HWS has a timeout mechanism for usual polling for completion,
but it was missing such a mechanism for draining the send queues.

Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Erez Shitrit <erezsh@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/steering/hws/bwc.c  |  4 +++-
 .../ethernet/mellanox/mlx5/core/steering/hws/send.c | 13 ++++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c
index 3bcf412a08c4..14a2bb9167a4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c
@@ -460,7 +460,9 @@ int mlx5hws_bwc_queue_poll(struct mlx5hws_context *ctx,
 		got_comp = !!ret;
 
 		if (unlikely(!got_comp && time_after(jiffies, timeout))) {
-			mlx5hws_err(ctx, "BWC poll error: polling queue %d - TIMEOUT\n", queue_id);
+			mlx5hws_err(ctx,
+				    "BWC poll error: polling queue %d - TIMEOUT (%d sec)\n",
+				    queue_id, MLX5HWS_BWC_POLLING_TIMEOUT);
 			return -ETIMEDOUT;
 		}
 	}
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 49dee44d0bef..9ff109fc8230 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
@@ -1192,6 +1192,8 @@ int mlx5hws_send_queue_action(struct mlx5hws_context *ctx,
 			      u16 queue_id,
 			      u32 actions)
 {
+	unsigned long timeout = jiffies +
+				secs_to_jiffies(MLX5HWS_BWC_POLLING_TIMEOUT);
 	struct mlx5hws_send_ring_sq *send_sq;
 	struct mlx5hws_send_engine *queue;
 	bool wait_comp = false;
@@ -1213,9 +1215,18 @@ int mlx5hws_send_queue_action(struct mlx5hws_context *ctx,
 			mlx5hws_send_engine_flush_queue(queue);
 
 		/* Poll queue until empty */
-		while (wait_comp && !mlx5hws_send_engine_empty(queue))
+		while (wait_comp && !mlx5hws_send_engine_empty(queue)) {
 			hws_send_engine_poll_cq(queue, NULL, &polled, 0);
 
+			if (unlikely(time_after(jiffies, timeout))) {
+				mlx5hws_err(ctx,
+					    "Error draining send queue %d - TIMEOUT (%d sec)\n",
+					    queue_id,
+					    MLX5HWS_BWC_POLLING_TIMEOUT);
+				return -ETIMEDOUT;
+			}
+		}
+
 		break;
 	default:
 		return -EINVAL;
-- 
2.44.0

Re: [PATCH net-next 4/6] net/mlx5: HWS, Add timeout mechanism to draining send queues

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

- [Medium] The new drain timeout in mlx5hws_send_queue_action()
  (send.c:1218-1228) does not mirror the…
- [Medium] The new -ETIMEDOUT return from mlx5hws_send_queue_action()
  (send.c:1226) abandons the drain while WQEs are still owned by the HW…
- [Low] RESOLVED LATER IN THIS SERIES (retained only per the validation-
  experiment directive): at this commit, mlx5hws_send_stes_fw()…
- [Low] This change converts a previously unbounded (infinite)
  synchronous drain loop into one that fails with -ETIMEDOUT, i.e. it…
- [Low] In mlx5hws_send_queue_action() (send.c:1218-1228) the new
  deadline test is evaluated unconditionally after…

--

net/mlx5: HWS, Add timeout mechanism to draining send queues

This adds a 60 second deadline (MLX5HWS_BWC_POLLING_TIMEOUT) to the
synchronous drain loop in mlx5hws_send_queue_action(), which previously
polled until the send engine became empty with no exit condition. It
also extends the existing BWC poll timeout message with the timeout
value.

A few questions below.

> net/mlx5: HWS, Add timeout mechanism to draining send queues
>
> HWS has a timeout mechanism for usual polling for completion,
> but it was missing such a mechanism for draining the send queues.

[Severity: Low]
Should this carry a Fixes: tag?

The DRAIN_SYNC arm of mlx5hws_send_queue_action() had no exit other than the
queue emptying, so callers such as mlx5hws_arg_write_inline_arg_data() and
hws_action_create_dest_match_range_fill_table() could spin indefinitely with
ctx->ctrl_lock held when the device stops generating completions.

git blame points at 2ca62599aa0b ("net/mlx5: HWS, added send engine and
context handling") for that loop.

The commit message describes the change as adding a missing mechanism rather
than fixing an indefinite hang, which makes it harder for stable maintainers
to pick this up.

> 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 49dee44d0bef5..9ff109fc8230a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> @@ -1192,6 +1192,8 @@ int mlx5hws_send_queue_action(struct mlx5hws_context *ctx,
>  			      u16 queue_id,
>  			      u32 actions)
>  {
> +	unsigned long timeout = jiffies +
> +				secs_to_jiffies(MLX5HWS_BWC_POLLING_TIMEOUT);
>  	struct mlx5hws_send_ring_sq *send_sq;
>  	struct mlx5hws_send_engine *queue;
>  	bool wait_comp = false;
> @@ -1213,9 +1215,18 @@ int mlx5hws_send_queue_action(struct mlx5hws_context *ctx,
>  			mlx5hws_send_engine_flush_queue(queue);
>  
>  		/* Poll queue until empty */
> -		while (wait_comp && !mlx5hws_send_engine_empty(queue))
> +		while (wait_comp && !mlx5hws_send_engine_empty(queue)) {
>  			hws_send_engine_poll_cq(queue, NULL, &polled, 0);
>  
> +			if (unlikely(time_after(jiffies, timeout))) {

[Severity: Medium]
Should this loop also short-circuit on MLX5_DEVICE_STATE_INTERNAL_ERROR the
way its sibling in bwc.c does?

mlx5hws_bwc_queue_poll() aborts immediately when the device is down:

	if (unlikely(ctx->mdev->state == MLX5_DEVICE_STATE_INTERNAL_ERROR)) {
		/* If the device is down for any reason (e.g. FLR), the HW will
		 * no longer generate completions.
		 ...
		return -ETIMEDOUT;
	}

The drain loop has no equivalent. hws_send_engine_poll_cq() returns before
reaching any device-state check when the CQ is empty:

	cqe = mlx5_cqwq_get_cqe(&cq->wq);
	if (!cqe)
		return;

The device-state test lives in mlx5hws_cq_poll_one(), which is only reached
after a CQE has already been found, so it never fires on a dead device.

So in exactly the FLR / health-failure case where the hardware is documented
to stop producing completions, doesn't every DRAIN_SYNC call now spin on CQ
memory for the full MLX5HWS_BWC_POLLING_TIMEOUT of 60 seconds?

There is also no cond_resched() or cpu_relax() in the loop body, and both
callers hold ctx->ctrl_lock across the drain:

  mlx5hws_arg_write_inline_arg_data() -> mlx5hws_send_queue_action()
  hws_action_create_dest_match_range_fill_table() -> mlx5hws_send_queue_action()

On CONFIG_PREEMPT_NONE or PREEMPT_VOLUNTARY, can a 60 second spin here trip
the soft lockup detector (2 * watchdog_thresh, 20 seconds) and the RCU CPU
stall timeout, while every other task waiting on ctrl_lock is blocked?

[Severity: Low]
Can this report a timeout for a drain that actually completed?

The deadline test runs unconditionally after the poll and returns before the
while condition is re-evaluated. hws_send_engine_poll_cq() advances
cq->poll_wqe:

	cq->poll_wqe = (wqe_cnt + priv->num_wqebbs) & sq->buf_mask;

and mlx5hws_send_engine_empty() compares exactly that field against
send_sq->cur_post:

	return ((send_sq->cur_post & send_sq->buf_mask) == send_cq->poll_wqe);

So if the poll that empties the queue lands on the same iteration in which
jiffies crosses the deadline, -ETIMEDOUT is returned for a drained queue.

The sibling loop in mlx5hws_bwc_queue_poll() gates its check on lack of
progress:

	if (unlikely(!got_comp && time_after(jiffies, timeout))) {

Would re-testing mlx5hws_send_engine_empty() (or adding a progress
qualifier) before returning be worth doing here?

> +				mlx5hws_err(ctx,
> +					    "Error draining send queue %d - TIMEOUT (%d sec)\n",
> +					    queue_id,
> +					    MLX5HWS_BWC_POLLING_TIMEOUT);
> +				return -ETIMEDOUT;

[Severity: Medium]
This return abandons the drain while cur_post != cq->poll_wqe, that is while
WQEs are still owned by the hardware, without setting queue->err and without
reaping the matching sq->wr_priv entries.

The bad-CQE path in hws_send_engine_poll_cq() does fence the engine off:

	if (unlikely(cqe_opcode != MLX5_CQE_REQ))
		queue->err = true;

Since queue->err stays clear on the timeout path, mlx5hws_send_engine_err()
keeps reporting the queue as healthy and it continues to be used with a
skewed cur_post / poll_wqe pair.

Callers treat the new error as an ordinary failure and free the objects the
outstanding WQEs still reference. In mlx5hws_arg_create():

	ret = mlx5hws_arg_write_inline_arg_data(ctx, id, data, data_sz);
	if (ret) {
		mlx5hws_err(ctx, "Failed writing arg data\n");
		mlx5hws_cmd_arg_destroy(ctx->mdev, id);
		return ret;
	}

while the MOD_ARG WQEs posted by mlx5hws_arg_write() still target that id:

	send_attr.id = arg_idx++;
	mlx5hws_send_engine_post_end(&ctrl, &send_attr);

Similarly, hws_action_create_dest_match_range_fill_table() propagates the
drain failure and mlx5hws_action_create_dest_match_range() unwinds:

	destroy_hit_ft_action:
		mlx5hws_action_destroy(hit_ft_action);
	destroy_table_ste:
		hws_action_destroy_dest_match_range_table(ctx, table_ste);

freeing the STC offset and destroying the RTCs and STE pool that
un-completed control queue work still refers to.

If the timeout is caused by a stalled but still live device that later
resumes executing those WQEs, can the argument id / STC offset / RTC id have
been reallocated to an unrelated object by then, so the late execution writes
into the new owner? Before this patch the DRAIN_SYNC arm could only return 0,
so these caller error paths were not reachable from this call.

Would setting queue->err on the timeout path, or otherwise quiescing the
queue before letting callers destroy objects, address this?

[Severity: Low]
One caller discards the new return value. In mlx5hws_send_stes_fw():

	/* 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);

The drain is the software substitute for a hardware fence here, and the path
is live: hws_action_create_dest_match_range_fill_table() sets
ste_attr.send_attr.fence = 1. With -ETIMEDOUT ignored, the firmware STE/RTC
write proceeds and MLX5HWS_FLOW_OP_SUCCESS is generated even though the
fence never completed.

This is addressed by the following patch in the series, "net/mlx5: HWS,
Handle timeout draining the send queue for FW STEs", which checks the return
value and goes to fail_rule, so it is only an issue at this commit.

> +			}
> +		}
> +
>  		break;
>  	default:
>  		return -EINVAL;

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

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

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

Handle a case in mlx5hws_send_stes_fw where
draining a queue fails with timeout.

Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Erez Shitrit <erezsh@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/steering/hws/send.c  | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

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 9ff109fc8230..bddac018b44b 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;
+	}
 
 	if (ste_attr->rtc_1) {
 		send_attr->id = ste_attr->rtc_1;
-- 
2.44.0

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

[netdev-bot+sashiko]

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

[PATCH net-next 6/6] net/mlx5: HWS, Remove unneeded WRITE_ONCE in post send

[Tariq Toukan]

From: Yevgeny Kliteynik <kliteyn@nvidia.com>

Remove the unneeded WRITE_ONCE, for two reasons:
a) This write is erroneous - it just writes to a local variable
and doesn't have any effect.
b) This code originated from another place (copy-paste bug),
where there was actually a CQ polling right after this line,
and the WRITE_ONCE was writing to another place.

Signed-off-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
Reviewed-by: Erez Shitrit <erezsh@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c | 3 ---
 1 file changed, 3 deletions(-)

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 bddac018b44b..8b70040eaca0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
@@ -111,9 +111,6 @@ static void hws_send_engine_post_ring(struct mlx5hws_send_ring_sq *sq,
 	wmb();
 
 	mlx5_write64((__be32 *)doorbell_cseg, sq->uar_map);
-
-	/* Ensure doorbell is written on uar_page before poll_cq */
-	WRITE_ONCE(doorbell_cseg, NULL);
 }
 
 static void
-- 
2.44.0

Re: [PATCH net-next 6/6] net/mlx5: HWS, Remove unneeded WRITE_ONCE in post send

[netdev-bot+sashiko]

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

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

- [Low] Incomplete cleanup plus an incorrect commit-message premise.

--

net/mlx5: HWS, Remove unneeded WRITE_ONCE in post send

This drops the WRITE_ONCE(doorbell_cseg, NULL) statement and its comment
from hws_send_engine_post_ring() in the HWS send path, on the grounds that
the store targets a by-value parameter and therefore has no effect.

> 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 bddac018b44b8..8b70040eaca03 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> @@ -111,9 +111,6 @@ static void hws_send_engine_post_ring(struct mlx5hws_send_ring_sq *sq,
>  	wmb();
>  
>  	mlx5_write64((__be32 *)doorbell_cseg, sq->uar_map);
> -
> -	/* Ensure doorbell is written on uar_page before poll_cq */
> -	WRITE_ONCE(doorbell_cseg, NULL);
>  }

[Severity: Low]
The changelog says:

    b) This code originated from another place (copy-paste bug),
    where there was actually a CQ polling right after this line,
    and the WRITE_ONCE was writing to another place.

Does that match the in-tree origin?  The only other place carrying this
exact comment and idiom is mlx5_aso_post_wqe() in
drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c:

	mlx5_write64((__be32 *)doorbell_cseg, aso->uar_map);

	/* Ensure doorbell is written on uar_page before poll_cq */
	WRITE_ONCE(doorbell_cseg, NULL);

and there doorbell_cseg is also a by-value parameter:

	void mlx5_aso_post_wqe(struct mlx5_aso *aso, bool with_data,
			       struct mlx5_wqe_ctrl_seg *doorbell_cseg)

so the store is dead at that site too, rather than "writing to another
place".  That line predates the HWS send path (it came in with commit
c491ded04325b9 in 2022).

Should aso.c get the same deletion in this patch?  Unlike the HWS site, the
aso.c one really is followed by CQ polling in every caller, for example
mlx5e_ipsec_aso_query():

	mlx5_aso_post_wqe(aso->aso, false, &wqe->ctrl);
	read_poll_timeout_atomic(mlx5_aso_poll_cq, ret, !ret, 10,
				 10 * USEC_PER_MSEC, false, aso->aso, false);

with the same pattern in mlx5e_tc_meter (en/tc/meter.c) and the macsec
paths, so the comment there documents an ordering guarantee the code never
implemented.  No runtime failure follows from it, since all of those
callers poll inside a read_poll_timeout*() retry loop and simply retry.

After this change, the two functionally identical doorbell-ring helpers in
the same driver no longer agree, and the construct the changelog calls
erroneous is still present at the site the changelog points to as its
origin.  Would it make sense to either remove it there as well, or, if some
real ordering requirement exists at that site, add an actual barrier
instead of a store to a local?

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