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

[Yishai Hadas <yishaih@nvidia.com>]

On 07/11/2026 22:24, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 · Medium: 2 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The forced path in mlx5_fw_fatal_reporter_err()
>    (drivers/net/ethernet/mellanox/mlx5/core/health.c) tries only…
> - [High] net/mlx5: forced health work recovers device when interface is
>    already down
> - [Medium] Nothing deduplicates force_fatal_report_work against
>    fatal_report_work or against the device state.
> - [Medium] A forced request is dropped with no record or retry if
>    force_fatal_report_work runs while MLX5_DROP_HEALTH_WORK is set only…
> - [Low] The patch changes the existing sensor-triggered recovery path
>    (fatal_report_work, forced=false) in mlx5_fw_fatal_reporter_err().
> - [Low] A forced, consumer-detected fault goes through the same devlink
>    fw_fatal reporter path as a real firmware-fatal error.
> 
> --
> 
> net/mlx5: Add mlx5_trigger_health_work_force()
> 
> This adds mlx5_trigger_health_work_force(). It queues a new
> force_fatal_report_work that runs the fatal health recovery with the error
> state forced, whatever the health sensors report. Before forcing the error
> state, the forced path stops the health poller and issues a fast teardown
> of the HCA under intf_state_mutex. The function is exported for mlx5_ib,
> which uses it later in the series.
> 
>> 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)
>>   {
> 
> [ ... ]
> 
>>   	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;
>>   	}
> 
> [Severity: Medium]
> Can a forced request get lost here? If force_fatal_report_work runs while
> MLX5_DROP_HEALTH_WORK is set only for a while, mlx5_fw_fatal_reporter_err()
> logs "health works are not permitted at this stage" and returns. The
> request is not recorded and not retried.
> 
> The FW sync reset flow opens such a window.
> mlx5_sync_reset_set_reset_requested() sets the bit without holding
> intf_state_mutex:
> 
> 	mlx5_stop_health_poll(dev, true);
> 
> On abort or timeout, mlx5_sync_reset_clear_reset_requested() clears it
> again:
> 
> 	if (poll_health)
> 		mlx5_start_health_poll(dev);
> 
> A dropped sensor fault is caught again by the next poll_health(). A forced
> fault cannot be, because the sensors never see it. So dev->state stays
> MLX5_DEVICE_STATE_UP. The kernel-doc of mlx5_trigger_health_work_force()
> says this API exists to prevent exactly that.
> 
> In the mlx5_ib consumer later in this series, umrc->state is already
> MLX5_UMR_STATE_ERR at this point, and cq->poll_one is already
> mlx5_ib_poll_one_stopped. The UMR waiters in mlx5r_umr_post_send_wait()
> depend on the internal error flush to wake them, and they wait with no
> timeout:
> 
> 	wait_for_completion(&umr_context.done);
> 
> If the forced request is dropped this way, would those waiters hang
> forever?

The same rare hang can already happen today: a UMR waiter waits for its 
completion or a flush, and a fatal error while unbind runs, after the 
health work is dropped, leaves its WQE unflushed. A forged completion in 
this window reaches the same state, so it is a pre-existing hang, i.e. a 
DoS, which is out of scope of this series.

> 
>> +	/*
>> +	 * 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]
> What happens here when MLX5_INTERFACE_STATE_UP is already clear? 
[...]

> Should the forced path unlock and return when the interface is not UP,
> instead of only skipping the teardown? Or should the non-remove unload
> paths cancel force_fatal_report_work?

Yes, v1 will return early. An unload clears UP only after it detached
the consumers, under intf_state_mutex, so once the forced work sees the
interface down there is nothing left to force.

> 
> [Severity: High]
> What happens here when mlx5_cmd_fast_teardown_hca() fails? 
[...]

> Does this path need a mlx5_cmd_force_teardown_hca() fallback, or bus
> mastering cleared before the error state is forced? 
> 

v1 will fall back to mlx5_cmd_force_teardown_hca(), as 
mlx5_try_fast_unload() does.

> [Severity: Medium]
> Does anything deduplicate this against fatal_report_work, or check
> dev->state? 

[...]

In v1 the forced work will do nothing once mlx5_cmd_is_down() is true, 
which covers a device already in internal error. The other order needs
a forged completion close to a real fatal error, and at worst ends in a 
second recovery or an unload, i.e. a DoS, which is out of scope of this 
series.

pw-bot: cr

Yishai