Re: [PATCH net V2 1/4] net/mlx5: SD, serialize SD LAG init/cleanup against LAG mode changes

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

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

- [High] New unbounded `msleep(100)` wait loops in `sd_lag_init()` and
  `sd_lag_cleanup()` (drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c)…

Pre-existing issues:
- [Medium] `sd_lag_cleanup()`
  (drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c:386-412) dismantles…

--

net/mlx5: SD, serialize SD LAG init/cleanup against LAG mode changes

This makes sd_lag_init() and sd_lag_cleanup() wait for
ldev->mode_changes_in_progress to reach zero before touching ldev, folds
the SD shared-FDB teardown in mlx5_lag_disable_change() into the main
locked section, and moves mlx5_sd_eswitch_mode_set() in
mlx5_eswitch_disable() after mlx5_lag_enable_change(). A couple of
questions below about the new wait loop and about the group_id handling in
the relocated teardown loop.

[ ... ]

[Severity: Medium]

This is a pre-existing issue and not introduced by this patch, but since
the loop is being relocated here: pf->group_id is passed to
mlx5_lag_shared_fdb_destroy() without checking that it is non-zero, and
mlx5_lag_shared_fdb_destroy() in lag/shared_fdb.c treats zero as
"everything":

	u32 filter = group_id ? group_id : MLX5_LAG_FILTER_ALL;
	...
	if (filter == MLX5_LAG_FILTER_PORTS || filter == MLX5_LAG_FILTER_ALL) {
		err = mlx5_deactivate_lag(ldev);

while lag.h documents group_id 0 as "not SD":

	u32 group_id;        /* SD group ID, 0 = not SD */
	bool sd_fdb_active;  /* set on all SD group members */

sd_lag_cleanup() zeroes group_id but never looks at sd_fdb_active, and
mlx5_lag_shared_fdb_create() sets sd_fdb_active on every member of the
group, so the primary can be left with sd_fdb_active == true and
group_id == 0. sd_lag_init()'s error path produces the same combination.

Do the normal teardown paths hide this because
mlx5_unload_one_devl_locked() calls mlx5_eswitch_disable() before
mlx5_detach_device(), so sd_fdb_active is already false by the time
sd_lag_cleanup() runs?

What happens on a manual auxiliary driver unbind, where mlx5e_remove() ->
mlx5_sd_cleanup() -> sd_lag_cleanup() runs with the eswitch still in
switchdev and the SD shared FDB armed? From there it looks like the
per-group single-FDB association, the peer-miss rules and the secondary
FDB-root alias are left installed, sd_cmd_unset_secondary() goes on to
silence the secondary TX root and destroy the alias flow table object, and
a later mlx5_lag_disable_change() reaching the loop above with
group_id == 0 would take the MLX5_LAG_FILTER_ALL / mlx5_deactivate_lag()
path rather than the SD group path.

Would it make sense for sd_lag_cleanup() to call
mlx5_lag_shared_fdb_destroy(ldev, group_id) (or at least warn when
sd_fdb_active is set) before erasing the group's lag_func entries?

[Severity: High]

Can this loop sleep while holding the locks that the mode-change thread
needs in order to make progress?

sd_lag_init() is called from mlx5_sd_init() with sd->devcom held, and
mlx5_sd_init() itself runs from adrv->probe/resume, i.e. inside the
hca_devcom_comp write semaphore taken by mlx5_attach_device() /
mlx5_register_device(). The same is true of the copy of this loop in
sd_lag_cleanup(), reached via mlx5_detach_device() -> mlx5e_suspend() /
mlx5e_remove() -> mlx5_sd_cleanup(). So the msleep() poll runs with two
devcom write semaphores held.

On the other side, mlx5_lag_disable_change() raises
mode_changes_in_progress and then drops every lock, and code running
inside that window re-takes hca_devcom_comp:

mlx5_devlink_eswitch_mode_set()
  mlx5_lag_disable_change(esw->dev)   /* mode_changes_in_progress++, locks dropped */
  esw_offloads_start()
    esw_mode_change()
      mlx5_devcom_comp_lock(esw->dev->priv.hca_devcom_comp)  /* blocks */
  mlx5_lag_enable_change(esw->dev)    /* never reached, counter stays > 0 */

and mlx5_devcom_comp_lock() is a write semaphore shared by all devices
matching the same sw system image GUID:

drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c:
void mlx5_devcom_comp_lock(struct mlx5_devcom_comp_dev *devcom)
{
	if (!devcom)
		return;
	down_write(&devcom->comp->sem);
}

The two threads can be on different devlink instances (mode change on one
PF, aux probe/remove on another PF of the same ldev), so the devlink lock
does not serialize them. msleep() is uninterruptible and the loop has no
timeout or bail-out, and since the wait is a poll on a plain int it is
also invisible to lockdep.

The commit message says the new loop is "mirroring mlx5_lag_remove_mdev()"
- mlx5_lag_remove_mdev() polls the same counter but holds no devcom
semaphore. Is the mirror still valid for callers that do hold sd->devcom
and hca_devcom_comp?

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