Re: [PATCH rdma-next 01/23] net/mlx5: Add mlx5_trigger_health_work_force()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/mlx5: missing internal error check before mlx5_cmd_fast_teardown_hca()
- [High] net/mlx5: forced health work recovers device when interface is already down
--

commit 2777ddd787832a64204ac170752c47964f6bf998
Author: Yishai Hadas <yishaih@nvidia.com>

net/mlx5: Add mlx5_trigger_health_work_force()

This commit adds mlx5_trigger_health_work_force() to queue a dedicated work
item that stops the health poller, runs fast HCA teardown, and forces the
device into MLX5_DEVICE_STATE_INTERNAL_ERROR before running health recovery,
even when the fatal health sensors report no error.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/health.c b/drivers/net/ethernet/mellanox/mlx5/core/health.c
> index aeeb136f5ebc7..d000ac335fa7a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/health.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/health.c
> @@ -627,18 +627,11 @@ mlx5_fw_fatal_reporter_dump(struct devlink_health_reporter *reporter,
>  	return err;
>  }
>  
> -static void mlx5_fw_fatal_reporter_err_work(struct work_struct *work)
> +static void mlx5_fw_fatal_reporter_err(struct mlx5_core_dev *dev, bool forced)
>  {
>  	struct mlx5_fw_reporter_ctx fw_reporter_ctx;
> -	struct mlx5_core_health *health;
> -	struct mlx5_core_dev *dev;
> -	struct devlink *devlink;
> -	struct mlx5_priv *priv;
> -
> -	health = container_of(work, struct mlx5_core_health, fatal_report_work);
> -	priv = container_of(health, struct mlx5_priv, health);
> -	dev = container_of(priv, struct mlx5_core_dev, priv);
> -	devlink = priv_to_devlink(dev);
> +	struct mlx5_core_health *health = &dev->priv.health;
> +	struct devlink *devlink = priv_to_devlink(dev);
>  
>  	mutex_lock(&dev->intf_state_mutex);
>  	if (test_bit(MLX5_DROP_HEALTH_WORK, &health->flags)) {
> @@ -646,8 +639,26 @@ static void mlx5_fw_fatal_reporter_err_work(struct work_struct *work)
>  		mutex_unlock(&dev->intf_state_mutex);
>  		return;
>  	}
> +	/*
> +	 * The firmware might still be running, so stop it first. Do it under
> +	 * intf_state_mutex and only while UP, so the cmd interface is valid.
> +	 */
> +	if (forced && test_bit(MLX5_INTERFACE_STATE_UP, &dev->intf_state)) {
> +		int err;
> +
> +		mlx5_stop_health_poll(dev, false);
> +		err = mlx5_cmd_fast_teardown_hca(dev);
> +		if (err)
> +			mlx5_core_err(dev, "Failed to stop the device before forcing the error state, err %d\n",
> +				      err);
> +	}

[Severity: High]
Should mlx5_fw_fatal_reporter_err() also verify that dev->state is not
already MLX5_DEVICE_STATE_INTERNAL_ERROR before calling
mlx5_cmd_fast_teardown_hca(dev), similar to mlx5_try_fast_unload()?

drivers/net/ethernet/mellanox/mlx5/core/main.c:mlx5_try_fast_unload() {
    ...
	if (dev->state == MLX5_DEVICE_STATE_INTERNAL_ERROR) {
		mlx5_core_dbg(dev, "Device in internal error state, giving up\n");
		return -EAGAIN;
	}
    ...
}

MLX5_INTERFACE_STATE_UP remains set when poll_health() or
mlx5_enter_error_state() transitions dev->state to
MLX5_DEVICE_STATE_INTERNAL_ERROR. When dev->state is already
MLX5_DEVICE_STATE_INTERNAL_ERROR, mlx5_cmd_is_down(dev) is true, so
mlx5_cmd_exec_inout() -> cmd_exec() aborts MLX5_CMD_OP_TEARDOWN_HCA with
-ENXIO without sending it to firmware, and mlx5_cmd_check() ->
mlx5_internal_err_ret_value() emulates MLX5_CMD_STAT_OK (0) with a zeroed
output buffer.

In mlx5_cmd_fast_teardown_hca(), this emulated success causes the driver to
write MLX5_INITIAL_SEG_NIC_INTERFACE_DISABLED to MMIO via
mlx5_set_nic_state() without firmware preparation and busy-wait in a loop
until the teardown timeout expires while holding dev->intf_state_mutex:

drivers/net/ethernet/mellanox/mlx5/core/fw.c:mlx5_cmd_fast_teardown_hca() {
    ...
	ret = mlx5_cmd_exec_inout(dev, teardown_hca, in, out);
	if (ret)
		return ret;

	state = MLX5_GET(teardown_hca_out, out, state);
	if (state == MLX5_TEARDOWN_HCA_OUT_FORCE_STATE_FAIL) {
		mlx5_core_warn(dev, "teardown with fast mode failed\n");
		return -EIO;
	}

	mlx5_set_nic_state(dev, MLX5_INITIAL_SEG_NIC_INTERFACE_DISABLED);
    ...
}

In addition, because mlx5_health_check_fatal_sensors() checks for
MLX5_INITIAL_SEG_NIC_INTERFACE_DISABLED before sensor_fw_synd_rfr(dev),
overwriting the NIC state register masks MLX5_SENSOR_FW_SYND_RFR and
prevents mlx5_error_sw_reset() -> reset_fw_if_needed() from performing a
firmware SW reset.

> +
> +	/*
> +	 * Enter the error state before unlocking, or an unload could send
> +	 * TEARDOWN_HCA to the stopped NIC, fail and skip function disable.
> +	 */
> +	enter_error_state(dev, forced);
>  	mutex_unlock(&dev->intf_state_mutex);

[Severity: High]
What happens in mlx5_fw_fatal_reporter_err() when forced is true, but
MLX5_INTERFACE_STATE_UP is already cleared?

During a non-remove unload such as mlx5_suspend(), mlx5_devlink_reload_down(),
or mlx5_pci_err_detected(), mlx5_unload_one_devl_locked() clears
MLX5_INTERFACE_STATE_UP and calls mlx5_function_teardown(dev, false) ->
mlx5_function_disable(dev, false) -> mlx5_stop_health_poll(dev, false),
which stops the timer without setting MLX5_DROP_HEALTH_WORK:

drivers/net/ethernet/mellanox/mlx5/core/main.c:mlx5_unload_one_devl_locked() {
    ...
	clear_bit(MLX5_INTERFACE_STATE_UP, &dev->intf_state);
	mlx5_unload(dev);
	mlx5_function_teardown(dev, false);
out:
	mutex_unlock(&dev->intf_state_mutex);
}

If mlx5_trigger_health_work_force() was queued just before the unload completed,
mlx5_fw_fatal_reporter_err(dev, true) sees MLX5_DROP_HEALTH_WORK == 0 and
MLX5_INTERFACE_STATE_UP == 0. It skips mlx5_cmd_fast_teardown_hca(), but still
calls enter_error_state(dev, true) and proceeds to devlink_health_report() /
mlx5_health_try_recover(), which will reset and reload a device that was
just suspended or unloaded (or tear down a device that just finished
devlink reload).

Should mlx5_fw_fatal_reporter_err() unlock dev->intf_state_mutex and return
early if !test_bit(MLX5_INTERFACE_STATE_UP, &dev->intf_state) when forced
is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103163949.1554-1-yishaih@nvidia.com?part=1