Re: [PATCH net-next 10/15] net/mlx5: LAG, do not add a device to an already-active LAG

[Shay Drori <shayd@nvidia.com>]

On 03/11/2026 19:10, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
> 
> mlx5_ldev_add_mdev() would add a newly probed device to the LAG even
> when the LAG is already active, mutating membership while getters may be
> iterating it and while the active mapping assumes a fixed member set.
> 
> Reject the join when the group's LAG is active:
> __mlx5_lag_dev_add_mdev() returns -EOPNOTSUPP, and mlx5_lag_add_mdev()
> treats that as terminal - unregister the HCA devcom component and leave
> the device standalone, rather than the -EAGAIN retry used for the
> bounded mode-change window.
> The device stays out of the LAG until it is reprobed.
> 
> This keeps LAG membership stable while active. It is a no-op for
> existing configurations, where no device is added to a LAG once it is
> active.
> 

answers to net-dev sashiko

Sashiko (Low):
This isn't a bug, but the commit message describes a broader rule than
the code implements. The subject says "do not add a device to an
already-active LAG", and the body says:
   "Reject the join when the group's LAG is active"
   "This keeps LAG membership stable while active."
The check here only fires when mlx5_virt_lag_is_supported(dev) is true,
which means multi_uplink_eswitch and lag_master are set and
num_lag_ports < 2. A device that is not virt LAG capable can still join
an active ldev through this path.
sd_lag_init() in lib/sd.c, the other caller of mlx5_ldev_add_mdev(),
also adds SD secondaries to the primary's ldev without checking
__mlx5_lag_is_active():
     err = mlx5_ldev_add_mdev(ldev, pos, sd->group_id);
Could the commit message say that the rejection applies only to virt
LAG capable functions, or explain why the other join paths are safe?

[SD] The other join paths are safe. A LAG of PFs is activated only
once all the PFs of the card are members, so there is no further PF that
could join it, and SD secondaries join at load, before MPESW can be
enabled. A LAG of VFs/SFs is the only one whose members come and go
independently of the activation, which is why the check is for virt LAG
capable functions only.

In case of v2, The commit message will be fixed.


> In addition, do not register the eswitch devcom component of such a
> device, so it does not pair with the eswitches of the LAG members.
> 
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>   .../net/ethernet/mellanox/mlx5/core/eswitch_offloads.c   | 4 ++++
>   drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c        | 9 +++++++++
>   drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h        | 5 +++++
>   3 files changed, 18 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index c712848b202d..70e6dd9561a6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -3573,6 +3573,10 @@ void mlx5_esw_offloads_devcom_init(struct mlx5_eswitch *esw,
>   	xa_init(&esw->offloads.peer_flows);
>   	mutex_init(&esw->offloads.peer_mutex);
>   
> +	/* do not pair a virt LAG capable VF/SF that is not part of a LAG */
> +	if (!mlx5_core_is_pf(esw->dev) && mlx5_virt_lag_nonmember(esw->dev))
> +		return;
> +
>   	if (!MLX5_CAP_ESW(esw->dev, merged_eswitch))
>   		return;
>   
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 9bc4a929cef3..d73bac8441f7 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -2449,6 +2449,11 @@ static int __mlx5_lag_dev_add_mdev(struct mlx5_core_dev *dev)
>   		mutex_unlock(&ldev->lock);
>   		return -EAGAIN;
>   	}
> +
> +	if (mlx5_virt_lag_is_supported(dev) && __mlx5_lag_is_active(ldev)) {
> +		mutex_unlock(&ldev->lock);
> +		return -EOPNOTSUPP;
> +	}
>   	mlx5_ldev_get(ldev);
>   	err = mlx5_ldev_add_mdev(ldev, dev, 0);
>   	if (err) {
> @@ -2537,6 +2542,10 @@ void mlx5_lag_add_mdev(struct mlx5_core_dev *dev)
>   	err = __mlx5_lag_dev_add_mdev(dev);
>   	mlx5_devcom_comp_unlock(dev->priv.hca_devcom_comp);
>   
> +	if (err == -EOPNOTSUPP) {
> +		mlx5_lag_unregister_hca_devcom_comp(dev);
> +		return;
> +	}
>   	if (err) {
>   		msleep(100);
>   		goto recheck;


Sashiko (Medium):
After this change only -EOPNOTSUPP is final. Every other error from
__mlx5_lag_dev_add_mdev() is retried with no limit. Can this loop
forever on an error that waiting will not clear?
__mlx5_lag_dev_add_mdev() returns the mlx5_ldev_add_mdev() error
unchanged, and mlx5_ldev_add_mdev() does:
     err = xa_alloc(&ldev->fns, &idx, fn, XA_LIMIT(0, U8_MAX - 1),
                GFP_KERNEL);
     if (err) {
         kfree(fn);
         return err;
     }
xa_alloc() returns -EBUSY once all 255 slots are in use. Two earlier
commits in this series make that reachable:
   "net/mlx5: LAG, decouple iteration end-of-walk from MLX5_MAX_PORTS"
   raised the limit from MLX5_MAX_PORTS - 1.
   "net/mlx5: LAG, enable LAG over functions that do not represent a
   physical port" lets virt LAG capable VFs and SFs join.
mlx5_sw_guid_sf() and mlx5_sw_guid_vf() return a constant marker. So
all such SFs (and likewise all such VFs) on the same HW share one
devcom key, and they end up in the same ldev when they share a netns.
With 255 members already present and the LAG inactive, the next probe
would see:
   mlx5_load()
     mlx5_lag_add_mdev()
       __mlx5_lag_dev_add_mdev()
         mlx5_ldev_add_mdev()
           xa_alloc() returns -EBUSY
       msleep(100); goto recheck;
Membership does not change while it waits, so mlx5_load() would never
return. Because the thread sleeps in 100ms steps, the hung task detector
would not report it, and unbinding that device would block behind the
probe thread. The final state of the series still has this loop.
It is not clear whether firmware actually exposes that many virt LAG
capable functions in one group. Would it make sense to retry only on
-EAGAIN and give up on other errors?

[SD] This is pre-existing: mlx5_lag_add_mdev() retried every error
before this series as well. for example future HW can return -EBUSY.
The fix will be sent as a separate patch.


> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
> index cbe380ae8bf9..385c767e24ae 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
> @@ -280,6 +280,11 @@ static inline bool mlx5_virt_lag_is_supported(const struct mlx5_core_dev *dev)
>   	return true;
>   }
>   
> +static inline bool mlx5_virt_lag_nonmember(struct mlx5_core_dev *dev)
> +{
> +	return mlx5_virt_lag_is_supported(dev) && !mlx5_lag_dev(dev);
> +}
> +
>   static inline bool mlx5_lag_is_supported(struct mlx5_core_dev *dev)
>   {
>   	if (!MLX5_CAP_GEN(dev, vport_group_manager) ||