Re: [PATCH net-next 1/3] net/mlx5: HWS, Check if device is down while polling for completion

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net/mlx5: HWS, Check if device is down while polling for completion

This commit adds a check to abort polling for hardware completions if the
device state indicates an internal error. This returns -ETIMEDOUT early to
avoid long timeouts when the hardware stops generating completions.

> 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 6dcd9c2a78aa8..eae02bc742218 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c
> @@ -422,6 +422,18 @@ int mlx5hws_bwc_queue_poll(struct mlx5hws_context *ctx,
>  	if (!got_comp && !drain)
>  		return 0;
>  
> +	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.
> +		 * Note that ETIMEDOUT is returned here because the BWC layer
> +		 * already has a special handling for timeouts - it breaks the
> +		 * rehash / resize / shrink loops to avoid chain of timeouts.
> +		 */
> +		mlx5_core_warn_once(ctx->mdev,
> +				    "BWC poll: device is down, polling for completion aborted\n");
> +		return -ETIMEDOUT;
> +	}
> +
>  	queue_full = mlx5hws_send_engine_full(&ctx->send_queue[queue_id]);
>  	while (queue_full || ((got_comp || drain) && *pending_rules)) {

Could this lead to a CPU stall if the device goes down during the polling loop?

The hardware state check is placed outside the while loop. The loop has a
60-second timeout (MLX5HWS_BWC_POLLING_TIMEOUT) and lacks cond_resched(),
meaning it can peg the CPU while spinning.

If the hardware fails and stops generating completions while we are already
inside the loop, the driver's health thread on another CPU might detect the
failure and set ctx->mdev->state to MLX5_DEVICE_STATE_INTERNAL_ERROR.

Since the check is outside the loop, we would fail to detect the state change
and continue to spin uselessly for the remainder of the 60 seconds, which
could trigger a soft lockup panic.

Should the state check be moved inside the polling loop, perhaps using
READ_ONCE() on ctx->mdev->state to ensure the compiler does not hoist the
lockless read?

>  		ret = mlx5hws_send_queue_poll(ctx, queue_id, comp, burst_th);