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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net/mlx5: stale force_fatal_report_work unloads device after
  recovery
--

commit 0df37fc156d59f375d27165a97578e68ca1e9e3f
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
health recovery work item that stops the firmware and forces the device
into MLX5_DEVICE_STATE_INTERNAL_ERROR even when the health sensors do not
report a fault.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/health.c b/drivers/net/ethernet/mellanox/mlx5/core/health.c
> index aeeb136f5ebc7..e76351860b154 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,39 @@ static void mlx5_fw_fatal_reporter_err_work(struct work_struct *work)
>  		mutex_unlock(&dev->intf_state_mutex);
>  		return;
>  	}
> +	if (forced) {
> +		int err;
> +
> +		/*
> +		 * Once the interface is down, the unload already removed the
> +		 * consumers that need the error flow. Once the cmd interface
> +		 * is down, the path that brought it down owns the flush and
> +		 * the recovery.
> +		 */
> +		if (mlx5_cmd_is_down(dev) ||
> +		    !test_bit(MLX5_INTERFACE_STATE_UP, &dev->intf_state)) {
> +			mutex_unlock(&dev->intf_state_mutex);
> +			return;
> +		}
> +		/*
> +		 * The firmware might still be running, so stop it first. Do it
> +		 * under intf_state_mutex, so the cmd interface is valid.
> +		 */
> +		mlx5_stop_health_poll(dev, false);
> +		err = mlx5_cmd_fast_teardown_hca(dev);
> +		if (err)
> +			err = mlx5_cmd_force_teardown_hca(dev);
> +		if (err)
> +			mlx5_core_err(dev, "Failed to stop the device before forcing the error state, err %d\n",
> +				      err);
> +	}

[Severity: Medium]
Can a force_fatal_report_work queued while recovery is already running on
health->wq tear down and unload the device right after it recovers?

Recovery runs synchronously on the single-threaded health->wq via
devlink_health_report() -> ... -> mlx5_recover_device() ->
mlx5_load_one_devl_locked(), which brings the command interface and
MLX5_INTERFACE_STATE_UP back up before the first work item finishes.
Consumers remain attached until mlx5_detach_device() runs during the
unload phase of recovery (for example while crdump collection or software
reset is in progress), and neither mlx5_unload_one_devl_locked() nor
mlx5_load_one_devl_locked() cancels a pending force_fatal_report_work:

health->wq                        consumer
mlx5_fw_fatal_reporter_err()
  enter_error_state(dev, ...)
                                  mlx5_trigger_health_work_force(dev)
                                    queue_work(..., &force_fatal_report_work)
  devlink_health_report(...)
    ...
      mlx5_recover_device(dev)
        mlx5_load_one_devl_locked(dev, true)
          /* cmd and interface are back UP */
mlx5_fw_fatal_reporter_force_err_work()
  mlx5_fw_fatal_reporter_err(dev, true)
    /* mlx5_cmd_is_down() is false and
     * MLX5_INTERFACE_STATE_UP is set */
    mlx5_cmd_fast_teardown_hca(dev)
    enter_error_state(dev, true)
    devlink_health_report(...) -> -ECANCELED
    mlx5_unload_one(dev, false)   <-- unloads recovered device

Because fw_fatal_reporter is still within its graceful period from the
recovery that just completed, devlink_health_report() returns -ECANCELED
and mlx5_fw_fatal_reporter_err() unloads the freshly recovered driver.

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