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

On 09/09/2026 15:15, netdev-bot+sashiko@kernel.org wrote:

External email: Use caution opening links or attachments


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.

[ ... ]

quoted

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index 2285c889c215e..aee5ce471eba7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c

@@ -2589,6 +2589,8 @@ void mlx5_lag_disable_change(struct mlx5_core_dev *dev)
       mpesw = ldev->mode == MLX5_LAG_MODE_MPESW;
       if (mpesw)
               mlx5_mpesw_sd_devcoms_lock(ldev);
+     else if (sd_devcom)
+             mlx5_devcom_comp_lock(sd_devcom);
       mutex_lock(&ldev->lock);

       ldev->mode_changes_in_progress++;

@@ -2599,26 +2601,22 @@ void mlx5_lag_disable_change(struct mlx5_core_dev *dev)
                       mlx5_disable_lag(ldev);
       }

+     if (sd_devcom) {
+             mlx5_lag_for_each(i, 0, ldev, MLX5_LAG_FILTER_ALL) {
+                     pf = mlx5_lag_pf(ldev, i);
+                     if (pf->dev == dev && pf->sd_fdb_active) {
+                             mlx5_lag_shared_fdb_destroy(ldev, pf->group_id);
+                             break;
+                     }
+             }
+     }

[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?

quoted

+
       mutex_unlock(&ldev->lock);
       if (mpesw)
               mlx5_mpesw_sd_devcoms_unlock(ldev);
+     else if (sd_devcom)
+             mlx5_devcom_comp_unlock(sd_devcom);
       mlx5_devcom_comp_unlock(primary->priv.hca_devcom_comp);
-
-     if (!sd_devcom)
-             return;
-
-     /* Teardown SD shared FDB for this device's group if active */
-     mlx5_devcom_comp_lock(sd_devcom);
-     mutex_lock(&ldev->lock);
-     mlx5_lag_for_each(i, 0, ldev, MLX5_LAG_FILTER_ALL) {
-             pf = mlx5_lag_pf(ldev, i);
-             if (pf->dev == dev && pf->sd_fdb_active) {
-                     mlx5_lag_shared_fdb_destroy(ldev, pf->group_id);
-                     break;
-             }
-     }
-     mutex_unlock(&ldev->lock);
-     mlx5_devcom_comp_unlock(sd_devcom);
  }

  void mlx5_lag_enable_change(struct mlx5_core_dev *dev)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
index 4cdc50cd6f038..99cf455a61e1a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c

@@ -345,7 +345,14 @@ static void sd_lag_init(struct mlx5_core_dev *dev)
               return;
       }

+recheck:
       mutex_lock(&ldev->lock);
+     if (ldev->mode_changes_in_progress) {
+             mutex_unlock(&ldev->lock);
+             msleep(100);
+             goto recheck;
+     }
+

[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?

This is a real finding and the fix needs a new approach.
I am dropping this patch.

quoted

       pf = mlx5_lag_pf_by_dev(ldev, primary);
       if (!pf) {
               sd_warn(primary, "%s: primary not registered in ldev, skipping\n",

@@ -388,7 +395,13 @@ static void sd_lag_cleanup(struct mlx5_core_dev *dev)
       if (!ldev)
               return;

+recheck:
       mutex_lock(&ldev->lock);
+     if (ldev->mode_changes_in_progress) {
+             mutex_unlock(&ldev->lock);
+             msleep(100);
+             goto recheck;
+     }
       mlx5_sd_for_each_secondary(i, primary, pos)
               mlx5_ldev_remove_mdev(ldev, pos);

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