Re: [PATCH net V2 4/4] net/mlx5: Lag, save and restore vport max_tx_speed across LAG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net/mlx5: broken LAG state check in mlx5_esw_vport_save_max_tx_speed()
- [Medium] net/mlx5: unreset aggregate speed in mlx5_lag_reset_vports_speed()
- [Medium] net/mlx5: single-PF PCIe bandwidth clamp in mlx5_lag_get_restore_speed()
--

commit a16b0bd296f6ec7da26bed226c7219e07a503e4a
Author: Or Har-Toov <ohartoov@nvidia.com>

net/mlx5: Lag, save and restore vport max_tx_speed across LAG

This commit saves each vport's max_tx_speed when enabled so the original
rate limit can be restored on lag teardown even if the link is down. If the
rate was not saved, teardown falls back to a link-derived speed.

Link: https://lore.kernel.org/all/20260915015118.875210-1-kuba@kernel.org/

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index fc197d1dc9dfe..7cf9c15ec65d2 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> @@ -935,6 +935,27 @@ static void esw_vport_cleanup(struct mlx5_eswitch *esw, struct mlx5_vport *vport
>  	esw_vport_cleanup_acl(esw, vport);
>  }
>  
> +static void mlx5_esw_vport_save_max_tx_speed(struct mlx5_eswitch *esw,
> +					     struct mlx5_vport *vport)
> +{
> +	u16 op_mod = MLX5_VPORT_STATE_OP_MOD_ESW_VPORT;
> +	int ret;
> +
> +	if (!MLX5_CAP_ESW(esw->dev, esw_vport_state_max_tx_speed) ||
> +	    vport->vport == MLX5_VPORT_UPLINK || vport->max_tx_speed)
> +		return;
> +
> +	if (mlx5_lag_is_active(esw->dev) && !vport->agg_max_tx_speed)
> +		return;
> +
> +	ret = mlx5_query_vport_max_tx_speed(esw->dev, op_mod, vport->vport,
> +					    true, &vport->max_tx_speed, NULL);

[Severity: High]
Does the check on mlx5_lag_is_active(esw->dev) and !vport->agg_max_tx_speed
in mlx5_esw_vport_save_max_tx_speed() match how vport->agg_max_tx_speed is
maintained across the driver?

In drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c,
mlx5_lag_modify_device_vports_speed() sets vport->agg_max_tx_speed = speed
on every vport and leaves it non-zero after programming firmware:

mlx5_lag_modify_device_vports_speed() {
    ...
		vport->agg_max_tx_speed = speed;

		if (!vport->enabled)
			continue;

		ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod,
						     vport->vport, true, speed);
    ...
}

Similarly, mlx5_esw_vport_set_max_tx_speed() in
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c writes
vport->agg_max_tx_speed to firmware on vport enable without clearing
vport->agg_max_tx_speed. As a result, while lag is active, vports whose
firmware has already been overwritten with the aggregate speed still have a
non-zero vport->agg_max_tx_speed.

Meanwhile, mlx5_lag_is_active() in
drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c returns true when Socket
Direct shared fdb is active, even when no lag bond is active and
vport->agg_max_tx_speed is 0:

drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_lag_is_active() {
    ...
	res  = ldev && (__mlx5_lag_is_active(ldev) ||
			__mlx5_lag_is_sd_active(ldev, dev));
    ...
}

When switching a Socket Direct device to switchdev mode via
mlx5_sd_eswitch_mode_set() -> mlx5_sd_activate_shared_fdb() ->
mlx5_lag_shared_fdb_create() in
drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c,
pf->sd_fdb_active is set to true without calling
mlx5_lag_set_vports_agg_speed():

mlx5_lag_shared_fdb_create() {
    ...
		mlx5_lag_for_each(i, 0, ldev, filter) {
			pf = mlx5_lag_pf(ldev, i);
			pf->sd_fdb_active = true;
		}
    ...
	if (filter == MLX5_LAG_FILTER_PORTS || filter == MLX5_LAG_FILTER_ALL)
		mlx5_lag_set_vports_agg_speed(ldev);
}

The same state (mlx5_lag_is_active(esw->dev) returning true while
vport->agg_max_tx_speed is 0 and firmware still holds the pre-lag speed) also
occurs during lag activation in mlx5_lag_shared_fdb_create() when ldev->lock
is dropped inside mlx5_lag_reload_ib_reps_from_locked() before
mlx5_lag_set_vports_agg_speed() runs, if mlx5_lag_update_agg_speed() fails,
or when creating adjacent vports via mlx5_esw_adj_vport_create().

If a vport is enabled in any of those states (or if the initial
mlx5_query_vport_max_tx_speed() call fails), vport->max_tx_speed remains 0.
Once lag applies the aggregate speed to firmware (setting
vport->agg_max_tx_speed != 0) and that vport is later disabled and
re-enabled while lag is active, won't mlx5_esw_vport_save_max_tx_speed() see
vport->max_tx_speed == 0 and vport->agg_max_tx_speed != 0, query firmware,
and overwrite vport->max_tx_speed with the active lag aggregate rate?

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index afe0b9909f2dc..3e623c96b2a1d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1712,11 +1712,37 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
>  	}
>  }
>  
> +static int mlx5_lag_get_restore_speed(struct mlx5_core_dev *mdev,
> +				      struct mlx5_vport *vport, u32 *speed)
> +{
> +	u32 pci_bw;
> +
> +	if (vport->max_tx_speed) {
> +		*speed = vport->max_tx_speed;
> +		return 0;
> +	}
> +
> +	*speed = 0;
> +	mlx5_port_oper_linkspeed(mdev, speed);
> +	if (!*speed && mlx5_port_max_linkspeed(mdev, speed))
> +		return -ENODATA;
> +
> +	pci_bw = pcie_bandwidth_mbps(mdev->pdev);
> +	if (pci_bw)
> +		*speed = min(*speed, pci_bw);
> +	*speed /= MLX5_MAX_TX_SPEED_UNIT;
> +	return 0;
> +}

[Severity: Medium]
On Socket Direct devices where a single physical port spans multiple pcie
functions in the same group (port->group_id), calling
pcie_bandwidth_mbps(mdev->pdev) in mlx5_lag_get_restore_speed() only checks
a single function's pcie bandwidth.

Earlier in drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c,
mlx5_lag_port_pcie_bw() sums the bandwidth across all member functions in
the Socket Direct group:

drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_lag_port_pcie_bw() {
    ...
	if (!port->group_id)
		return mlx5_lag_dev_pcie_bw(port->dev, bw);

	*bw = 0;
	mlx5_lag_for_each(idx, 0, ldev, port->group_id) {
		member = mlx5_lag_pf(ldev, idx);
		if (!member)
			continue;

		err = mlx5_lag_dev_pcie_bw(member->dev, &member_bw);
    ...
}

Should mlx5_lag_get_restore_speed() use mlx5_lag_port_pcie_bw() instead of
pcie_bandwidth_mbps(mdev->pdev) so that Socket Direct ports are not clamped
to a single function's pcie bandwidth on fallback restore?

> +
>  void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
>  {
> +	u16 op_mod = MLX5_VPORT_STATE_OP_MOD_ESW_VPORT;
>  	struct mlx5_core_dev *mdev;
> +	struct mlx5_eswitch *esw;
> +	struct mlx5_vport *vport;
>  	struct lag_func *pf;
> -	u32 speed;
> +	u32 restore_speed;
> +	unsigned long i;
>  	int pf_idx;
>  	int ret;
>  
> @@ -1728,17 +1754,39 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
>  		mdev = pf->dev;
>  		if (!mdev)
>  			continue;
> -
> -		ret = mlx5_port_oper_linkspeed(mdev, &speed);
> -		if (ret) {
> -			mlx5_core_dbg(mdev,
> -				      "Failed to reset vports speed for device %s. Oper speed is not available (err=%d)\n",
> -				      dev_name(mdev->device), ret);
> +		esw = mdev->priv.eswitch;
> +		if (!esw || !MLX5_CAP_ESW(mdev, esw_vport_state_max_tx_speed))
>  			continue;
> -		}
>  
> -		speed = speed / MLX5_MAX_TX_SPEED_UNIT;
> -		mlx5_lag_modify_device_vports_speed(mdev, speed);
> +		mutex_lock(&esw->state_lock);
> +		mlx5_esw_for_each_vport(esw, i, vport) {
> +			if (!vport || vport->vport == MLX5_VPORT_UPLINK)
> +				continue;
> +
> +			if (!vport->enabled) {
> +				vport->agg_max_tx_speed = vport->max_tx_speed;
> +				continue;
> +			}
> +
> +			if (mlx5_lag_get_restore_speed(mdev, vport,
> +						       &restore_speed)) {
> +				vport->agg_max_tx_speed = 0;
> +				continue;
> +			}
> +
> +			ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod,
> +							     vport->vport,
> +							     true,
> +							     restore_speed);
> +			if (ret) {
> +				mlx5_core_dbg(mdev,
> +					      "Failed to restore vport %d max tx speed, err=%d\n",
> +					      vport->vport, ret);
> +				continue;
> +			}
> +			vport->agg_max_tx_speed = 0;
> +		}
> +		mutex_unlock(&esw->state_lock);
>  	}
>  }
>  #endif

[Severity: Medium]
What happens in mlx5_lag_reset_vports_speed() if a vport was enabled while
lag was active (so firmware was programmed with the aggregate speed), its
baseline vport->max_tx_speed was not saved (remaining 0), and the vport is
then disabled before lag teardown?

When mlx5_lag_reset_vports_speed() runs, the !vport->enabled branch sets
vport->agg_max_tx_speed = vport->max_tx_speed (0) instead of computing a
fallback speed via mlx5_lag_get_restore_speed(). When that vport is later
re-enabled after lag teardown in mlx5_esw_vport_enable():

drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:mlx5_esw_vport_enable() {
    ...
	mlx5_esw_vport_save_max_tx_speed(esw, vport);
	if (vport->agg_max_tx_speed)
		mlx5_esw_vport_set_max_tx_speed(esw, vport);
    ...
}

mlx5_esw_vport_save_max_tx_speed() sees vport->max_tx_speed == 0 and
!mlx5_lag_is_active(esw->dev), queries firmware (which still holds the
unrestored lag aggregate speed), and saves the aggregate speed into
vport->max_tx_speed, while skipping mlx5_esw_vport_set_max_tx_speed()
because vport->agg_max_tx_speed is 0.

Also, for an enabled vport, if mlx5_modify_vport_max_tx_speed() fails in
mlx5_lag_reset_vports_speed(), the error path continues before updating
vport->agg_max_tx_speed. Won't that leave the stale lag aggregate rate in
vport->agg_max_tx_speed, causing a subsequent disable and re-enable of the
vport outside of lag to replay the old lag aggregate speed into firmware?

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