[PATCH net 0/5] net/mlx5: fixes for LAG max_tx_speed update flow

[PATCH net 0/5] net/mlx5: fixes for LAG max_tx_speed update flow

[Tariq Toukan]

Hi,

Commit 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to
vports") added a mechanism that computes the aggregated LAG TX speed and
programs it into each vport's max_tx_speed via MODIFY_VPORT_STATE. This
series by Or fixes bugs found in that flow.

The bugs:

1. Down ports were counted into the effective aggregate speed.
   When a port goes down, PTYS eth_proto_oper is not necessary 0.

2. The aggregate was not limited by the NIC's PCIe bandwidth. It was
   taken from the bond speed, which is calculated from PTYS, and PTYS
   is not bounded by the PCI bandwidth of the NIC. FW initializes
   max_tx_speed to the minimum of the port speed and the PCIe
   bandwidth, so modifying it with a value computed by different rules
   changes the meaning of the field rather than updating it.

3. MPESW teardown never reset the vport speeds, leaving stale values
   after teardown.

4. RoCE LAG never told the RDMA layer about speed changes. The NIC is
   considered to be in a bare metal configuration, without a valid or
   enabled eswitch, so there are no VF, SF or PF rep vports to modify
   and no event of speed change was raised.

Patch #1 splits the LAG aggregate speed into an operative and a max
        helper, so that each can filter on port state on its own.

Patch #2 resets the vport speed on MPESW teardown.

Patch #3 caches the TX-enabled aggregate speed and programs that into
        the vports, instead of reading it back from the bond netdev.

Patch #4 limits the aggregated TX speed by the NIC's PCIe bandwidth.

Patch #5 notifies the RDMA layer of RoCE LAG speed changes via a new
        driver event.

Regards,
Tariq

Or Har-Toov (5):
  net/mlx5: Lag, split aggregate speed into oper and max helpers
  net/mlx5: Lag, reset vport speed on teardown
  {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports
  net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth
  {RDMA,net}/mlx5: notify RoCE LAG speed change via driver event

 drivers/infiniband/hw/mlx5/main.c             |  12 +-
 drivers/net/ethernet/mellanox/mlx5/core/dev.c |  20 +++
 .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 159 +++++++++++++++---
 .../net/ethernet/mellanox/mlx5/core/lag/lag.h |  16 +-
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |   1 +
 include/linux/mlx5/device.h                   |   1 +
 include/linux/mlx5/driver.h                   |   1 +
 7 files changed, 181 insertions(+), 29 deletions(-)


base-commit: 00f9fbc12320253bfc576fb7539d860029c82d0f
-- 
2.44.0

[PATCH net 1/5] net/mlx5: Lag, split aggregate speed into oper and max helpers

[Tariq Toukan]

From: Or Har-Toov <ohartoov@nvidia.com>

Split mlx5_lag_sum_devices_speed into mlx5_lag_get_devices_oper_speed
and mlx5_lag_get_devices_max_speed. The oper function reflects only
the speed currently available and the max function is state-independent
and represents the maximum achievable speed, taking the best single
port for active-backup and summing all ports for XOR, LACP and MPESW.

The oper function skips a port for two reasons:
- the bonding driver set tx_enabled=false - LACP converging or
  active-backup standby slave, where carrier may still be up but the
  port is intentionally not forwarding traffic.
- the link is down.

Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Shay Drori <shayd@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 71 ++++++++++++++-----
 1 file changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index c655f6e32e9b..5aa9d5c98fe1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1412,16 +1412,20 @@ static bool mlx5_lag_should_disable_lag(struct mlx5_lag *ldev, bool do_bond)
 }
 
 #ifdef CONFIG_MLX5_ESWITCH
-static int
-mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
-			   int (*get_speed)(struct mlx5_core_dev *, u32 *))
+static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
+					   u32 *sum_speed)
 {
 	struct mlx5_core_dev *pf_mdev;
 	struct lag_func *pf;
 	int pf_idx;
+	bool mpesw;
 	u32 speed;
+	u8 opmod;
 	int ret;
 
+	mpesw = ldev->mode == MLX5_LAG_MODE_MPESW;
+	opmod = MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT;
+
 	*sum_speed = 0;
 	mlx5_ldev_for_each(pf_idx, 0, ldev) {
 		pf = mlx5_lag_pf(ldev, pf_idx);
@@ -1430,13 +1434,20 @@ mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
 		pf_mdev = pf->dev;
 		if (!pf_mdev)
 			continue;
+		if (mpesw) {
+			if (mlx5_query_vport_state(pf_mdev, opmod, 0) !=
+			    VPORT_STATE_UP)
+				continue;
+		} else if (!ldev->tracker.netdev_state[pf_idx].tx_enabled ||
+			   !ldev->tracker.netdev_state[pf_idx].link_up) {
+			continue;
+		}
 
-		ret = get_speed(pf_mdev, &speed);
+		ret = mlx5_port_oper_linkspeed(pf_mdev, &speed);
 		if (ret) {
 			mlx5_core_dbg(pf_mdev,
-				      "Failed to get device speed using %ps. Device %s speed is not available (err=%d)\n",
-				      get_speed, dev_name(pf_mdev->device),
-				      ret);
+				      "Failed to get device %s oper speed (err=%d)\n",
+				      dev_name(pf_mdev->device), ret);
 			return ret;
 		}
 
@@ -1446,17 +1457,41 @@ mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
 	return 0;
 }
 
-static int mlx5_lag_sum_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
+static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
 {
-	return mlx5_lag_sum_devices_speed(ldev, max_speed,
-					  mlx5_port_max_linkspeed);
-}
+	struct mlx5_core_dev *pf_mdev;
+	struct lag_func *pf;
+	bool take_max;
+	int pf_idx;
+	u32 speed;
+	int ret;
 
-static int mlx5_lag_sum_devices_oper_speed(struct mlx5_lag *ldev,
-					   u32 *oper_speed)
-{
-	return mlx5_lag_sum_devices_speed(ldev, oper_speed,
-					  mlx5_port_oper_linkspeed);
+	take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP;
+	if (ldev->mode == MLX5_LAG_MODE_MPESW)
+		take_max = false;
+
+	*max_speed = 0;
+	mlx5_ldev_for_each(pf_idx, 0, ldev) {
+		pf = mlx5_lag_pf(ldev, pf_idx);
+		if (!pf)
+			continue;
+		pf_mdev = pf->dev;
+		if (!pf_mdev)
+			continue;
+
+		ret = mlx5_port_max_linkspeed(pf_mdev, &speed);
+		if (ret) {
+			mlx5_core_dbg(pf_mdev,
+				      "Failed to get device %s max speed (err=%d)\n",
+				      dev_name(pf_mdev->device), ret);
+			return ret;
+		}
+
+		*max_speed = take_max ?
+			max(*max_speed, speed) : *max_speed + speed;
+	}
+
+	return 0;
 }
 
 static void mlx5_lag_modify_device_vports_speed(struct mlx5_core_dev *mdev,
@@ -1505,7 +1540,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
 	int pf_idx;
 
 	if (ldev->mode == MLX5_LAG_MODE_MPESW) {
-		if (mlx5_lag_sum_devices_oper_speed(ldev, &speed))
+		if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
 			return;
 	} else {
 		speed = ldev->tracker.bond_speed_mbps;
@@ -1514,7 +1549,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
 	}
 
 	/* If speed is not set, use the sum of max speeds of all PFs */
-	if (!speed && mlx5_lag_sum_devices_max_speed(ldev, &speed))
+	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
 		return;
 
 	speed = speed / MLX5_MAX_TX_SPEED_UNIT;
-- 
2.44.0

Re: [PATCH net 1/5] net/mlx5: Lag, split aggregate speed into oper and max helpers

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5: Lag, split aggregate speed into oper and max helpers

This splits the single mlx5_lag_sum_devices_speed() helper into
mlx5_lag_get_devices_oper_speed() and mlx5_lag_get_devices_max_speed().
The oper helper skips ports that are not forwarding traffic, while the max
helper takes the best single port for active-backup and sums the ports for
XOR, LACP and MPESW.

A few questions on the new helpers below.

> net/mlx5: Lag, split aggregate speed into oper and max helpers
>
> The oper function skips a port for two reasons:
> - the bonding driver set tx_enabled=false - LACP converging or
>   active-backup standby slave, where carrier may still be up but the
>   port is intentionally not forwarding traffic.
> - the link is down.

[Severity: Low]
The changelog lists two skip reasons, but the new oper helper also adds a
third, MPESW-only gate that queries the vNIC vport state per PF:

	if (mpesw) {
		if (mlx5_query_vport_state(pf_mdev, opmod, 0) !=
		    VPORT_STATE_UP)
			continue;
	}

Since the value returned by this helper ends up in every vport's
max_tx_speed, would it be worth spelling out the MPESW gate and its effect
on the programmed speed in the changelog too?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index c655f6e32e9b0..5aa9d5c98fe16 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1412,16 +1412,20 @@ static bool mlx5_lag_should_disable_lag(struct mlx5_lag *ldev, bool do_bond)
>  }
>  
>  #ifdef CONFIG_MLX5_ESWITCH
> -static int
> -mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
> -			   int (*get_speed)(struct mlx5_core_dev *, u32 *))
> +static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
> +					   u32 *sum_speed)
>  {
>  	struct mlx5_core_dev *pf_mdev;
>  	struct lag_func *pf;
>  	int pf_idx;
> +	bool mpesw;
>  	u32 speed;
> +	u8 opmod;
>  	int ret;
>  
> +	mpesw = ldev->mode == MLX5_LAG_MODE_MPESW;
> +	opmod = MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT;
> +
>  	*sum_speed = 0;
>  	mlx5_ldev_for_each(pf_idx, 0, ldev) {
>  		pf = mlx5_lag_pf(ldev, pf_idx);
> @@ -1430,13 +1434,20 @@ mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
>  		pf_mdev = pf->dev;
>  		if (!pf_mdev)
>  			continue;
> +		if (mpesw) {
> +			if (mlx5_query_vport_state(pf_mdev, opmod, 0) !=
> +			    VPORT_STATE_UP)
> +				continue;
> +		}

[Severity: Medium]
Can a failed firmware query be mistaken for a down vport here?
mlx5_query_vport_state() in core/vport.c returns 0 on command failure:

	err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
	if (err)
		return 0;

	return MLX5_GET(query_vport_state_out, out, state);

and include/linux/mlx5/device.h has VPORT_STATE_DOWN = 0x0, so a timeout,
-EIO on a device in error state, or -EBUSY is indistinguishable from a real
"vport down" reply.

The PF is then silently dropped from the sum and the helper still returns 0,
unlike the mlx5_port_oper_linkspeed() failure a few lines below which aborts
the whole computation.

The under-computed value reaches the hardware through
mlx5_lag_set_vports_agg_speed() -> mlx5_lag_modify_device_vports_speed(),
which stores it in vport->agg_max_tx_speed and calls
mlx5_modify_vport_max_tx_speed() for every enabled vport, so with one
failing PF in a two-PF MPESW LAG the rate limit is roughly halved until the
next port change event. If every query fails the sum stays 0 and the
capability-max fallback below is used instead.

This path is reachable from ordinary events via
mlx5_lag_mpesw_port_change_event() -> mlx5_mpesw_speed_update_work() ->
mlx5_lag_set_vports_agg_speed(). Should the query failure be distinguished
from VPORT_STATE_DOWN and propagated like the linkspeed error?

> +		} else if (!ldev->tracker.netdev_state[pf_idx].tx_enabled ||
> +			   !ldev->tracker.netdev_state[pf_idx].link_up) {
> +			continue;
> +		}

[Severity: Low]
Is this else-if arm reachable at this point of the series? The only caller
in this commit is mlx5_lag_set_vports_agg_speed(), which calls the helper
exclusively under:

	if (ldev->mode == MLX5_LAG_MODE_MPESW) {
		if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
			return;

so mpesw is always true inside the helper and the tx_enabled/link_up skip
described in the changelog has no effect yet. The later patch
"{RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports"
introduces mlx5_lag_update_agg_speed(), which calls the oper helper for all
LAG modes and makes this branch live, so this is only an intra-series
ordering observation.

[Severity: Low]
This is a pre-existing issue, not introduced by this patch, but these reads
of ldev->tracker.netdev_state[pf_idx].tx_enabled/.link_up (and the
tracker.tx_type read in mlx5_lag_get_devices_max_speed() below) take only
ldev->lock, while the writers use different conventions:

mlx5_lag_netdev_event() republishes the whole struct from RTNL context with
neither ldev->lock nor lag_lock held:

	ldev->tracker = tracker;

	if (changed)
		mlx5_queue_bond_work(ldev, 0);

and mlx5_ldev_add_netdev()/mlx5_ldev_remove_netdev() write the same fields
under lag_lock. The same pattern already exists in the baseline, where
mlx5_do_bond() snapshots ldev->tracker under ldev->lock and
mlx5_infer_tx_enabled() consumes these fields to program the port mapping.

Any mixed snapshot here looks self-correcting because the event path requeues
the bond work, which recomputes the aggregate speed. Is it worth settling on
one lock for the tracker fields while this code is being reworked?

>  
> -		ret = get_speed(pf_mdev, &speed);
> +		ret = mlx5_port_oper_linkspeed(pf_mdev, &speed);
>  		if (ret) {
>  			mlx5_core_dbg(pf_mdev,
> -				      "Failed to get device speed using %ps. Device %s speed is not available (err=%d)\n",
> -				      get_speed, dev_name(pf_mdev->device),
> -				      ret);
> +				      "Failed to get device %s oper speed (err=%d)\n",
> +				      dev_name(pf_mdev->device), ret);
>  			return ret;
>  		}
>  
> @@ -1446,17 +1457,41 @@ mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,
>  	return 0;
>  }
>  
> -static int mlx5_lag_sum_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
> +static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
>  {
> -	return mlx5_lag_sum_devices_speed(ldev, max_speed,
> -					  mlx5_port_max_linkspeed);
> -}
> +	struct mlx5_core_dev *pf_mdev;
> +	struct lag_func *pf;
> +	bool take_max;
> +	int pf_idx;
> +	u32 speed;
> +	int ret;

[ ... ]

> +		ret = mlx5_port_max_linkspeed(pf_mdev, &speed);
> +		if (ret) {
> +			mlx5_core_dbg(pf_mdev,
> +				      "Failed to get device %s max speed (err=%d)\n",
> +				      dev_name(pf_mdev->device), ret);
> +			return ret;
> +		}
> +
> +		*max_speed = take_max ?
> +			max(*max_speed, speed) : *max_speed + speed;
> +	}

[Severity: Low]
Both new helpers sum per-port Ethernet link speeds from
mlx5_port_max_linkspeed()/mlx5_port_oper_linkspeed(), which derive their
value purely from the PTYS protocol capabilities, with no bound from the
device's PCIe link bandwidth. The changelog says the max helper "represents
the maximum achievable speed", yet the summed value can exceed what the host
DMA path can sustain, and that value is programmed as the vport rate limit.

The follow-up patch "net/mlx5: Lag, limit aggregated TX speed by PCIe
bandwidth" adds the missing clamp in both helpers:

	pci_bw = mlx5_pcie_bandwidth(pf_mdev);
	if (pci_bw)
		speed = min(speed, pci_bw);

so this is closed inside the series; was there a reason for keeping the
clamp in a separate patch rather than adding it here?

>  
>  static void mlx5_lag_modify_device_vports_speed(struct mlx5_core_dev *mdev,
> @@ -1505,7 +1540,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
>  	int pf_idx;
>  
>  	if (ldev->mode == MLX5_LAG_MODE_MPESW) {
> -		if (mlx5_lag_sum_devices_oper_speed(ldev, &speed))
> +		if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
>  			return;
>  	} else {
>  		speed = ldev->tracker.bond_speed_mbps;
> @@ -1514,7 +1549,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
>  	}
>  
>  	/* If speed is not set, use the sum of max speeds of all PFs */
> -	if (!speed && mlx5_lag_sum_devices_max_speed(ldev, &speed))
> +	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
>  		return;

[Severity: Low]
Should this comment be updated? mlx5_lag_get_devices_max_speed() no longer
always sums, since:

	take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP;
	...
	*max_speed = take_max ?
		max(*max_speed, speed) : *max_speed + speed;

For an active-backup bond in a non-MPESW mode the fallback returns the best
single port speed, not "the sum of max speeds of all PFs", and that is the
value programmed into vport max_tx_speed.

The comment also survives unchanged in the follow-up patch that moves this
code into mlx5_lag_update_agg_speed(), so nothing later in the series
corrects it.

>  
>  	speed = speed / MLX5_MAX_TX_SPEED_UNIT;

[PATCH net 2/5] net/mlx5: Lag, reset vport speed on teardown

[Tariq Toukan]

From: Or Har-Toov <ohartoov@nvidia.com>

mlx5_lag_disable_mpesw() never resets the cached agg_max_tx_speed on
the vports, unlike regular LAG teardown. This leaves stale speed values
cached on struct mlx5_vport, and pushed to FW, after MPESW is torn
down.

Move the mlx5_lag_reset_vports_speed() call from mlx5_do_bond() into
mlx5_deactivate_lag(), so that every teardown path resets the vport
speeds.

Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Shay Drori <shayd@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index 5aa9d5c98fe1..c2f21eb4ea7c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1075,6 +1075,8 @@ int mlx5_deactivate_lag(struct mlx5_lag *ldev)
 	if (master_idx < 0)
 		return -EINVAL;
 
+	mlx5_lag_reset_vports_speed(ldev);
+
 	dev0 = mlx5_lag_pf(ldev, master_idx)->dev;
 	ldev->mode = MLX5_LAG_MODE_NONE;
 	ldev->mode_flags = 0;
@@ -1676,7 +1678,6 @@ static void mlx5_do_bond(struct mlx5_lag *ldev)
 		mlx5_modify_lag(ldev, &tracker);
 		mlx5_lag_set_vports_agg_speed(ldev);
 	} else if (mlx5_lag_should_disable_lag(ldev, do_bond)) {
-		mlx5_lag_reset_vports_speed(ldev);
 		mlx5_disable_lag(ldev);
 	}
 }
-- 
2.44.0

Re: [PATCH net 2/5] net/mlx5: Lag, reset vport speed on teardown

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5: Lag, reset vport speed on teardown

The cached agg_max_tx_speed was only reset on the regular LAG teardown
path in mlx5_do_bond(), so MPESW teardown left stale values behind. The
reset call is moved into mlx5_deactivate_lag() so all teardown paths run
it.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 5aa9d5c98fe16..c2f21eb4ea7cf 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1075,6 +1075,8 @@ int mlx5_deactivate_lag(struct mlx5_lag *ldev)
>  	if (master_idx < 0)
>  		return -EINVAL;
>  
> +	mlx5_lag_reset_vports_speed(ldev);
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but now that mlx5_deactivate_lag()
is the single place claimed to reset the vport speeds on "every teardown
path", is the helper it calls complete enough for that claim?

mlx5_lag_reset_vports_speed() skips a PF entirely when the oper speed query
fails:

drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_lag_reset_vports_speed() {
	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);
		continue;
	}
}

For that PF, vport->agg_max_tx_speed keeps the LAG aggregate value written
earlier by mlx5_lag_modify_device_vports_speed(), and enabled vports keep
the aggregated max_tx_speed in FW even though the FW LAG object is
destroyed.

The stale value then gets replayed later, in eswitch.c:

mlx5_esw_vport_enable() {
	if (vport->agg_max_tx_speed)
		mlx5_esw_vport_set_max_tx_speed(esw, vport);
}

and is also used as the fallback in vport.c:

mlx5_modify_vport_admin_state() {
	speed = IS_ERR(esw_vport) ? 0 : esw_vport->agg_max_tx_speed;
}

so an inflated reported TX speed can survive with no LAG present. This is
the same best-effort handling that came in with 50f1d188c580, and moving the
call site strictly increases the number of paths that attempt the reset, so
nothing is made worse here.

Both mlx5_lag_reset_vports_speed() and
mlx5_lag_modify_device_vports_speed() return void, so mlx5_deactivate_lag()
cannot observe the skip and still returns 0. Would it make sense to have the
reset path clear the cached agg_max_tx_speed (or fall back to zero) when
mlx5_port_oper_linkspeed() fails, rather than leaving the LAG aggregate
cached?

Also worth noting on the caching order in
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);
	if (ret)
		mlx5_core_dbg(...);

the cache is updated before the FW command and a FW failure is only logged,
so cache and FW can diverge during teardown too.

> @@ -1676,7 +1678,6 @@ static void mlx5_do_bond(struct mlx5_lag *ldev)
>  		mlx5_modify_lag(ldev, &tracker);
>  		mlx5_lag_set_vports_agg_speed(ldev);
>  	} else if (mlx5_lag_should_disable_lag(ldev, do_bond)) {
> -		mlx5_lag_reset_vports_speed(ldev);
>  		mlx5_disable_lag(ldev);
>  	}
>  }

[PATCH net 3/5] {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports

[Tariq Toukan]

From: Or Har-Toov <ohartoov@nvidia.com>

Cache aggregate TX-enabled speed we calculated, and use it instead of
querying the bond speed so we can, in the following patch, limit it by
the PCI bandwidth.

The bond speed takes the calculation from PTYS, and PTYS is not bounded
by the PCI bandwidth of the NIC.

Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Shay Drori <shayd@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/infiniband/hw/mlx5/main.c             |  2 +-
 .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 61 +++++++++++++++----
 .../net/ethernet/mellanox/mlx5/core/lag/lag.h | 15 ++++-
 include/linux/mlx5/driver.h                   |  1 +
 4 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 373ee1f42d4a..6ab7f945e712 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -1654,7 +1654,7 @@ static int mlx5_ib_query_port_speed_from_bond(struct mlx5_ib_dev *dev,
 	u32 bond_speed;
 	int err;
 
-	err = mlx5_lag_query_bond_speed(mdev, &bond_speed);
+	err = mlx5_lag_query_aggregated_speed(mdev, &bond_speed);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index c2f21eb4ea7c..76b574fa0d7a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1413,7 +1413,6 @@ static bool mlx5_lag_should_disable_lag(struct mlx5_lag *ldev, bool do_bond)
 	       ldev->mode != MLX5_LAG_MODE_MPESW;
 }
 
-#ifdef CONFIG_MLX5_ESWITCH
 static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
 					   u32 *sum_speed)
 {
@@ -1496,6 +1495,30 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
 	return 0;
 }
 
+void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
+{
+	u32 speed;
+
+	lockdep_assert_held(&ldev->lock);
+
+	if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
+		return;
+
+	/* If speed is not set, use the sum of max speeds of all PFs */
+	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
+		return;
+
+	ldev->agg_speed_mbps = speed;
+}
+
+void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev)
+{
+	lockdep_assert_held(&ldev->lock);
+
+	ldev->agg_speed_mbps = 0;
+}
+
+#ifdef CONFIG_MLX5_ESWITCH
 static void mlx5_lag_modify_device_vports_speed(struct mlx5_core_dev *mdev,
 						u32 speed)
 {
@@ -1541,17 +1564,10 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
 	u32 speed;
 	int pf_idx;
 
-	if (ldev->mode == MLX5_LAG_MODE_MPESW) {
-		if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
-			return;
-	} else {
-		speed = ldev->tracker.bond_speed_mbps;
-		if (speed == SPEED_UNKNOWN)
-			return;
-	}
+	mlx5_lag_update_agg_speed(ldev);
+	speed = ldev->agg_speed_mbps;
 
-	/* If speed is not set, use the sum of max speeds of all PFs */
-	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
+	if (!speed)
 		return;
 
 	speed = speed / MLX5_MAX_TX_SPEED_UNIT;
@@ -1576,6 +1592,7 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
 	int pf_idx;
 	int ret;
 
+	mlx5_lag_reset_agg_speed(ldev);
 	mlx5_ldev_for_each(pf_idx, 0, ldev) {
 		pf = mlx5_lag_pf(ldev, pf_idx);
 		if (!pf)
@@ -2068,6 +2085,28 @@ static int mlx5_handle_changeinfodata_event(struct mlx5_lag *ldev,
 	return 1;
 }
 
+/* Returns speed in Mbps. */
+int mlx5_lag_query_aggregated_speed(struct mlx5_core_dev *mdev, u32 *speed)
+{
+	struct mlx5_lag *ldev;
+	int ret = 0;
+
+	ldev = mlx5_lag_dev(mdev);
+	if (!ldev)
+		return -ENODEV;
+
+	mutex_lock(&ldev->lock);
+	*speed = ldev->agg_speed_mbps;
+	if (*speed == 0)
+		ret = -EINVAL;
+	mutex_unlock(&ldev->lock);
+
+	if (ret == -EINVAL)
+		mlx5_core_dbg(mdev, "aggregated speed is unknown\n");
+	return ret;
+}
+EXPORT_SYMBOL_GPL(mlx5_lag_query_aggregated_speed);
+
 static void mlx5_lag_update_tracker_speed(struct lag_tracker *tracker,
 					  struct net_device *ndev)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
index e9f0ef83ce1d..744fbf9a0078 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
@@ -99,6 +99,7 @@ struct mlx5_lag {
 	/* Protect lag fields/state changes */
 	struct mutex		  lock;
 	struct lag_mpesw	  lag_mpesw;
+	u32			  agg_speed_mbps;
 };
 
 static inline struct mlx5_lag *
@@ -258,12 +259,22 @@ void mlx5_lag_rescan_dev_locked(struct mlx5_lag *ldev,
 void mlx5_lag_add_devices_filter(struct mlx5_lag *ldev, u32 filter);
 struct mlx5_devcom_comp_dev *mlx5_lag_get_devcom_comp(struct mlx5_lag *ldev);
 
+void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev);
+void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev);
+
 #ifdef CONFIG_MLX5_ESWITCH
 void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev);
 void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev);
 #else
-static inline void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev) {}
-static inline void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev) {}
+static inline void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
+{
+	mlx5_lag_update_agg_speed(ldev);
+}
+
+static inline void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
+{
+	mlx5_lag_reset_agg_speed(ldev);
+}
 #endif
 
 static inline bool mlx5_lag_is_supported(struct mlx5_core_dev *dev)
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 83d0a83bbfbc..a83926689e9a 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -1155,6 +1155,7 @@ bool mlx5_lag_is_roce(struct mlx5_core_dev *dev);
 bool mlx5_lag_is_sriov(struct mlx5_core_dev *dev);
 bool mlx5_lag_is_active(struct mlx5_core_dev *dev);
 int mlx5_lag_query_bond_speed(struct mlx5_core_dev *dev, u32 *speed);
+int mlx5_lag_query_aggregated_speed(struct mlx5_core_dev *dev, u32 *speed);
 bool mlx5_lag_mode_is_hash(struct mlx5_core_dev *dev);
 bool mlx5_lag_is_master(struct mlx5_core_dev *dev);
 bool mlx5_lag_is_shared_fdb(struct mlx5_core_dev *dev);
-- 
2.44.0

Re: [PATCH net 3/5] {RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
{RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports

The patch adds a new ldev->agg_speed_mbps cache in struct mlx5_lag,
populated by the new mlx5_lag_update_agg_speed() and cleared by
mlx5_lag_reset_agg_speed(). mlx5_lag_set_vports_agg_speed() now programs
vports from that cache, and mlx5_ib switches from mlx5_lag_query_bond_speed()
to the new exported mlx5_lag_query_aggregated_speed().

A few questions below.

> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 373ee1f42d4ad..6ab7f945e712c 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -1654,7 +1654,7 @@ static int mlx5_ib_query_port_speed_from_bond(struct mlx5_ib_dev *dev,
>  	u32 bond_speed;
>  	int err;
>  
> -	err = mlx5_lag_query_bond_speed(mdev, &bond_speed);
> +	err = mlx5_lag_query_aggregated_speed(mdev, &bond_speed);
>  	if (err)
>  		return err;

[Severity: Low]
Should the other in-tree consumer of the LAG aggregate speed be converted
too?  After this patch mlx5_ib_query_port_speed_from_bond() uses the new
cached aggregate, but mlx5_esw_qos_max_link_speed_get() in
drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c still uses the bond speed
this commit message describes as unsuitable:

	if (!mlx5_lag_is_active(mdev) ||
	    mlx5_lag_query_bond_speed(mdev, link_speed_max) < 0 ||
	    *link_speed_max == 0)
		goto skip_lag;

That value feeds mlx5_esw_qos_link_speed_verify(), so a devlink rate
request can be accepted against the PTYS-derived bond speed while
mlx5_lag_set_vports_agg_speed() programs the vport with the (possibly
smaller, PCIe-clamped) cached aggregate, and while ib_query_port reports
the cached aggregate for the same LAG.

Two related observations: mlx5_lag_query_bond_speed() stays
EXPORT_SYMBOL_GPL (and declared in include/linux/mlx5/driver.h next to the
new getter) although no caller outside mlx5_core is left after this
conversion, and the two getters use different sentinels for "unknown" -
mlx5_lag_query_bond_speed() returns success with *speed == 0 and errors on
SPEED_UNKNOWN, while mlx5_lag_query_aggregated_speed() errors on 0.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index c2f21eb4ea7cf..76b574fa0d7a6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c

[ ... ]

> @@ -1496,6 +1495,30 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
>  	return 0;
>  }
>  
> +void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
> +{
> +	u32 speed;
> +
> +	lockdep_assert_held(&ldev->lock);
> +
> +	if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
> +		return;
> +
> +	/* If speed is not set, use the sum of max speeds of all PFs */

[Severity: Low]
This isn't a bug, but is the comment still accurate for active-backup?
mlx5_lag_get_devices_max_speed() does not sum in that case:

	take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP;
	if (ldev->mode == MLX5_LAG_MODE_MPESW)
		take_max = false;
	...
		*max_speed = take_max ?
			max(*max_speed, speed) : *max_speed + speed;

The comment was correct before the series, where
mlx5_lag_sum_devices_max_speed() unconditionally did *sum_speed += speed;
the take_max behaviour came in with "net/mlx5: Lag, split aggregate speed
into oper and max helpers" and this patch copies the older comment into the
new helper.

> +	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
> +		return;
> +
> +	ldev->agg_speed_mbps = speed;
> +}

[Severity: Medium]
On the two early returns above, ldev->agg_speed_mbps keeps its previous
value and the void return type gives callers no way to notice.  Is that
intended?

mlx5_lag_get_devices_oper_speed() propagates the error from
mlx5_port_oper_linkspeed() -> mlx5_port_query_eth_proto() ->
ACCESS_REG(PTYS), so an FW command error or a query during a health/reset
window lands here.

> +
> +void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev)
> +{
> +	lockdep_assert_held(&ldev->lock);
> +
> +	ldev->agg_speed_mbps = 0;
> +}
> +
> +#ifdef CONFIG_MLX5_ESWITCH
>  static void mlx5_lag_modify_device_vports_speed(struct mlx5_core_dev *mdev,
>  						u32 speed)
>  {
> @@ -1541,17 +1564,10 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
>  	u32 speed;
>  	int pf_idx;
>  
> -	if (ldev->mode == MLX5_LAG_MODE_MPESW) {
> -		if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
> -			return;
> -	} else {
> -		speed = ldev->tracker.bond_speed_mbps;
> -		if (speed == SPEED_UNKNOWN)
> -			return;
> -	}
> +	mlx5_lag_update_agg_speed(ldev);
> +	speed = ldev->agg_speed_mbps;
>  
> -	/* If speed is not set, use the sum of max speeds of all PFs */
> -	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
> +	if (!speed)
>  		return;

[Severity: Medium]
Can this reprogram hardware with a stale speed?  The !speed test cannot
distinguish a freshly computed value from a leftover non-zero cache, so
after a failed PTYS query mlx5_lag_set_vports_agg_speed() continues into
mlx5_lag_modify_device_vports_speed() and writes the previous aggregate
into vport->agg_max_tx_speed / MODIFY_VPORT_STATE for every non-uplink
vport.

Before this patch the MPESW path aborted on query failure and the
non-MPESW path did not touch PTYS at all (it used
tracker.bond_speed_mbps), so a failing query left the hardware untouched
instead of reprogramming the last known value.  The same stale value is
then handed to mlx5_ib via mlx5_lag_query_aggregated_speed() and reported
as the current port rate.

Would returning an int from mlx5_lag_update_agg_speed(), or invalidating
the cache on failure, be preferable here?

> @@ -2068,6 +2085,28 @@ static int mlx5_handle_changeinfodata_event(struct mlx5_lag *ldev,
>  	return 1;
>  }
>  
> +/* Returns speed in Mbps. */
> +int mlx5_lag_query_aggregated_speed(struct mlx5_core_dev *mdev, u32 *speed)
> +{
> +	struct mlx5_lag *ldev;
> +	int ret = 0;
> +
> +	ldev = mlx5_lag_dev(mdev);
> +	if (!ldev)
> +		return -ENODEV;
> +
> +	mutex_lock(&ldev->lock);
> +	*speed = ldev->agg_speed_mbps;
> +	if (*speed == 0)
> +		ret = -EINVAL;
> +	mutex_unlock(&ldev->lock);

[Severity: Medium]
Does the validity window of this cache match the LAG-active state that
mlx5_ib uses to choose the bond path?  The replaced
mlx5_lag_query_bond_speed() read tracker.bond_speed_mbps, which is set by
the netdev event that triggered bonding and is never cleared on
deactivation, so the failure conditions of the two getters differ.

At activation, mlx5_do_bond() sets the ROCE mode (so mlx5_lag_is_roce()
is already true) and then calls mlx5_lag_rescan_dev_locked(), which by its
own comment drops ldev->lock while the mlx5_ib aux device is probed and
ib_register_device() publishes the device.  Only afterwards is the cache
populated:

	mlx5_lag_rescan_dev_locked(ldev, dev0, true);
	...
	if (!shared_fdb)
		mlx5_lag_set_vports_agg_speed(ldev);

mlx5_lag_shared_fdb_create() has the same ordering, calling
mlx5_lag_rescan_dev_locked() and mlx5_lag_reload_ib_reps_from_locked()
before mlx5_lag_set_vports_agg_speed().

A UVERBS_METHOD_QUERY_PORT_SPEED issued in that window reaches
mlx5_ib_query_port_speed_from_bond(), reads agg_speed_mbps == 0 and gets
-EINVAL back to userspace, where the pre-patch code returned a speed.

On teardown there is a similar ordering in mlx5_deactivate_lag():

	mlx5_lag_reset_vports_speed(ldev);	/* zeroes agg_speed_mbps */
	dev0 = mlx5_lag_pf(ldev, master_idx)->dev;
	ldev->mode = MLX5_LAG_MODE_NONE;

mlx5_ib samples mlx5_lag_is_roce()/mlx5_lag_is_sriov() under lag_lock and
only then takes ldev->lock, so it can observe "LAG active" followed by a
zeroed cache.

Note also that the peer helper mlx5_ib_query_port_speed_from_vport()
treats an unavailable speed as a fallback rather than an error:

	if (vport_state == VPORT_STATE_DOWN || max_tx_speed == 0)
		/* Value 0 indicates field not supported, fallback */
		return mlx5_ib_query_port_speed_from_port(dev, port_num, speed);

Should from_bond() fall back to mlx5_ib_query_port_speed_from_port() as
well instead of propagating -EINVAL?

> +
> +	if (ret == -EINVAL)
> +		mlx5_core_dbg(mdev, "aggregated speed is unknown\n");
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(mlx5_lag_query_aggregated_speed);

[ ... ]

[PATCH net 4/5] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth

[Tariq Toukan]

From: Or Har-Toov <ohartoov@nvidia.com>

FW initializes max_tx_speed to the minimum of the port speed and the
PCI bandwidth. Modifying it with a value computed by different rules
changes the meaning of the field rather than updating it.

Limit each uplink's contribution to the aggregated LAG TX speed by the
NIC's PCIe bandwidth and reset max_tx_speed to the same value when lag
is torn down.

Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Shay Drori <shayd@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/dev.c | 20 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 12 +++++++++++
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |  1 +
 3 files changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dev.c b/drivers/net/ethernet/mellanox/mlx5/core/dev.c
index df2e3ad01819..81f3dc3b8034 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/dev.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/dev.c
@@ -30,6 +30,7 @@
  * SOFTWARE.
  */
 
+#include <linux/bitfield.h>
 #include <linux/mlx5/driver.h>
 #include <linux/mlx5/eswitch.h>
 #include <linux/mlx5/mlx5_ifc_vdpa.h>
@@ -40,6 +41,25 @@
 
 static DEFINE_IDA(mlx5_adev_ida);
 
+#define MLX5_PCIE_MIN_SPEED_MBPS	(2500)
+
+u32 mlx5_pcie_bandwidth(struct mlx5_core_dev *dev)
+{
+	u16 lnksta;
+	int speed;
+	u32 width;
+
+	if (pcie_capability_read_word(dev->pdev, PCI_EXP_LNKSTA, &lnksta))
+		return 0;
+
+	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
+	speed = pcie_link_speed_mbps(dev->pdev);
+	if (speed < MLX5_PCIE_MIN_SPEED_MBPS)
+		return 0;
+
+	return speed * width;
+}
+
 static bool is_eth_rep_supported(struct mlx5_core_dev *dev)
 {
 	if (!IS_ENABLED(CONFIG_MLX5_ESWITCH))
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index 76b574fa0d7a..fd91becd6848 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1418,6 +1418,7 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
 {
 	struct mlx5_core_dev *pf_mdev;
 	struct lag_func *pf;
+	u32 pci_bw;
 	int pf_idx;
 	bool mpesw;
 	u32 speed;
@@ -1452,6 +1453,9 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
 			return ret;
 		}
 
+		pci_bw = mlx5_pcie_bandwidth(pf_mdev);
+		if (pci_bw)
+			speed = min(speed, pci_bw);
 		*sum_speed += speed;
 	}
 
@@ -1463,6 +1467,7 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
 	struct mlx5_core_dev *pf_mdev;
 	struct lag_func *pf;
 	bool take_max;
+	u32 pci_bw;
 	int pf_idx;
 	u32 speed;
 	int ret;
@@ -1488,6 +1493,9 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
 			return ret;
 		}
 
+		pci_bw = mlx5_pcie_bandwidth(pf_mdev);
+		if (pci_bw)
+			speed = min(speed, pci_bw);
 		*max_speed = take_max ?
 			max(*max_speed, speed) : *max_speed + speed;
 	}
@@ -1588,6 +1596,7 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
 {
 	struct mlx5_core_dev *mdev;
 	struct lag_func *pf;
+	u32 pci_bw;
 	u32 speed;
 	int pf_idx;
 	int ret;
@@ -1609,6 +1618,9 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
 			continue;
 		}
 
+		pci_bw = mlx5_pcie_bandwidth(mdev);
+		if (pci_bw)
+			speed = min(speed, pci_bw);
 		speed = speed / MLX5_MAX_TX_SPEED_UNIT;
 		mlx5_lag_modify_device_vports_speed(mdev, speed);
 	}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
index d6713a2ce676..555842085daf 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
@@ -281,6 +281,7 @@ void mlx5_events_cleanup(struct mlx5_core_dev *dev);
 void mlx5_events_start(struct mlx5_core_dev *dev);
 void mlx5_events_stop(struct mlx5_core_dev *dev);
 
+u32 mlx5_pcie_bandwidth(struct mlx5_core_dev *dev);
 int mlx5_adev_idx_alloc(void);
 void mlx5_adev_idx_free(int idx);
 void mlx5_adev_cleanup(struct mlx5_core_dev *dev);
-- 
2.44.0

Re: [PATCH net 4/5] net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth

This adds a new helper mlx5_pcie_bandwidth() that derives a PCIe bandwidth
from PCI_EXP_LNKSTA, and uses it to clamp each uplink's contribution to the
aggregated LAG TX speed in mlx5_lag_get_devices_oper_speed(),
mlx5_lag_get_devices_max_speed() and mlx5_lag_reset_vports_speed().  A few
questions below about how the bandwidth is computed and where the clamp is
applied.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dev.c b/drivers/net/ethernet/mellanox/mlx5/core/dev.c
> index df2e3ad01819f..81f3dc3b8034f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/dev.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/dev.c

[ ... ]

> @@ -40,6 +41,25 @@
>  
>  static DEFINE_IDA(mlx5_adev_ida);
>  
> +#define MLX5_PCIE_MIN_SPEED_MBPS	(2500)
> +
> +u32 mlx5_pcie_bandwidth(struct mlx5_core_dev *dev)
> +{
> +	u16 lnksta;
> +	int speed;
> +	u32 width;
> +
> +	if (pcie_capability_read_word(dev->pdev, PCI_EXP_LNKSTA, &lnksta))
> +		return 0;
> +
> +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
> +	speed = pcie_link_speed_mbps(dev->pdev);
> +	if (speed < MLX5_PCIE_MIN_SPEED_MBPS)
> +		return 0;
> +
> +	return speed * width;
> +}

[Severity: Medium]
Is speed * width the same quantity the PCI core calls PCIe bandwidth?

pcie_link_speed_mbps() returns the raw per-lane transfer rate, while
pcie_bandwidth_available() in drivers/pci/pci.c applies the line encoding
correction and walks upstream taking the minimum:

  drivers/pci/pci.h:
  #define PCIE_SPEED2MBS_ENC(speed) \
	((speed) == PCIE_SPEED_64_0GT ? 64000*1/1 : \
	 (speed) == PCIE_SPEED_32_0GT ? 32000*128/130 : \
	 ...
	 (speed) == PCIE_SPEED_5_0GT  ?  5000*8/10 : \
	 (speed) == PCIE_SPEED_2_5GT  ?  2500*8/10 : \

  drivers/pci/pci.c:pcie_bandwidth_available() {
	...
		next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
		/* Check if current device limits the total bandwidth */
		if (!bw || next_bw <= bw) {
	...
		dev = pci_upstream_bridge(dev);
  }

For a Gen2 x8 link the helper here yields 5000 * 8 = 40000 Mb/s, whereas the
core helper yields 4000 * 8 = 32000 Mb/s.  Does that make
min(speed, pci_bw) a no-op for a 40GbE uplink on a bus that cannot sustain
it?  On Gen3 and later the difference is about 1.5%, which is still larger
than the MLX5_MAX_TX_SPEED_UNIT (100 Mb/s) granularity the value is
programmed at.

The same driver already computes this quantity for the same pdev with the
core helper:

  drivers/net/ethernet/mellanox/mlx5/core/en/params.c:slow_pci_heuristic() {
	...
	pci_bw = pcie_bandwidth_available(mdev->pdev, NULL, NULL, NULL);
	mlx5_core_dbg(mdev, "Max link speed = %d, PCI BW = %d\n", link_speed,
		      pci_bw);
	...
  }

Should mlx5_pcie_bandwidth() use pcie_bandwidth_available() as well, so the
driver does not carry two different definitions of PCI bandwidth for one
device?

[Severity: Low]
Can the width and speed here come from two different link states?  The width
is taken from the local snapshot in lnksta, but the speed comes from a second
independent read of the very same register:

  drivers/pci/pci.c:pcie_link_speed_mbps() {
	err = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
	if (err)
		return err;

	return pcie_dev_speed_mbps(to_pcie_link_speed(lnksta));
  }

PCI_EXP_LNKSTA_NLW and PCI_EXP_LNKSTA_CLS both live in the word already held
in lnksta, and the callers hold only ldev->lock, which does not serialise
link retraining, autonomous width/speed changes, bandwidth control speed
changes or a function reset during health recovery.  If the link changes
between the two reads, speed * width is a bandwidth the link never had.
Would deriving the speed from the existing snapshot with
pcie_dev_speed_mbps(to_pcie_link_speed(lnksta)) be both cheaper and
consistent?

> +
>  static bool is_eth_rep_supported(struct mlx5_core_dev *dev)
>  {
>  	if (!IS_ENABLED(CONFIG_MLX5_ESWITCH))
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 76b574fa0d7a6..fd91becd6848f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c

[ ... ]

> @@ -1452,6 +1453,9 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
>  			return ret;
>  		}
>  
> +		pci_bw = mlx5_pcie_bandwidth(pf_mdev);
> +		if (pci_bw)
> +			speed = min(speed, pci_bw);
>  		*sum_speed += speed;
>  	}

[Severity: Medium]
Does clamping per uplink and then summing actually bound the aggregate for
the two PFs of a dual-port adapter?  Both PFs sit on the same physical PCIe
link and read the same PCI_EXP_LNKSTA, so each contributes up to that single
link's bandwidth.

For a dual-port 200GbE adapter on Gen4 x16, mlx5_pcie_bandwidth() returns
16000 * 16 = 256000 for both PFs, each contributes min(200000, 256000) =
200000, and the aggregate becomes 400000 Mb/s, roughly 1.6x the shared link.

That aggregate is stored in ldev->agg_speed_mbps and then programmed into
every vport:

  mlx5_lag_set_vports_agg_speed() {
	...
	speed = speed / MLX5_MAX_TX_SPEED_UNIT;

	mlx5_ldev_for_each(pf_idx, 0, ldev) {
	...
		mlx5_lag_modify_device_vports_speed(mdev, speed);
  }

Should the PCIe bandwidth instead be applied to the summed value, i.e. after
aggregation, so PFs sharing one link are not counted twice?  Only the
take_max (active-backup) path stays bounded today.

>  
> @@ -1488,6 +1493,9 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
>  			return ret;
>  		}
>  
> +		pci_bw = mlx5_pcie_bandwidth(pf_mdev);
> +		if (pci_bw)
> +			speed = min(speed, pci_bw);
>  		*max_speed = take_max ?
>  			max(*max_speed, speed) : *max_speed + speed;
>  	}

[Severity: Medium]
The same double counting applies in this fallback when take_max is false,
which is the case for LACP and XOR and is forced for MPESW.

[Severity: Low]
Unrelated to the clamp itself, but the caller's comment no longer matches
this helper:

  mlx5_lag_update_agg_speed() {
	...
	/* If speed is not set, use the sum of max speeds of all PFs */
	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
		return;
  }

With take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP,
the fallback returns max(*max_speed, speed) rather than a sum in
active-backup mode.  The comment was accurate in the baseline, where
mlx5_lag_sum_devices_max_speed() unconditionally did *sum_speed += speed;
should it be updated to describe the policy dependent behaviour?

>  
> @@ -1609,6 +1618,9 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
>  			continue;
>  		}
>  
> +		pci_bw = mlx5_pcie_bandwidth(mdev);
> +		if (pci_bw)
> +			speed = min(speed, pci_bw);
>  		speed = speed / MLX5_MAX_TX_SPEED_UNIT;
>  		mlx5_lag_modify_device_vports_speed(mdev, speed);
>  	}

[Severity: Medium]
This is the value restored into vport max_tx_speed on LAG teardown, so if
mlx5_pcie_bandwidth() over-estimates the bus bandwidth (raw rate, no
encoding correction, endpoint link only), the restored value is the
over-estimated one rather than what FW originally programmed.  Does that
still match the commit message claim that FW initializes max_tx_speed to the
minimum of the port speed and the PCI bandwidth?

[PATCH net 5/5] {RDMA,net}/mlx5: notify RoCE LAG speed change via driver event

[Tariq Toukan]

From: Or Har-Toov <ohartoov@nvidia.com>

In RoCE LAG we don't modify any max_tx_speed, so no
IB_EVENT_DEVICE_SPEED_CHANGE is ever fired.

Raise IB_EVENT_DEVICE_SPEED_CHANGE from the LAG layer, in
mlx5_lag_update_agg_speed(), right after the cached aggregate is
updated. That cache is where the IB layer now takes the effective speed
from, so a consumer that queries after receiving the event is
guaranteed to read the new value.

Fixes: c6df9a65cbb0 ("net/mlx5: Skip disabled vports when setting max TX speed")
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Shay Drori <shayd@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/infiniband/hw/mlx5/main.c             | 10 ++++++++++
 .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 20 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/lag/lag.h |  1 +
 include/linux/mlx5/device.h                   |  1 +
 4 files changed, 32 insertions(+)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 6ab7f945e712..bd54af47eda7 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -3693,6 +3693,16 @@ static int lag_event(struct notifier_block *nb, unsigned long event, void *data)
 			rdma_roce_rescan_port(ibdev, portnum + 1);
 		}
 		break;
+	case MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE: {
+		struct ib_event speed_event = {};
+
+		if (!dev->ib_active)
+			break;
+		speed_event.device = ibdev;
+		speed_event.event = IB_EVENT_DEVICE_SPEED_CHANGE;
+		ib_dispatch_event(&speed_event);
+		break;
+	}
 	default:
 		return NOTIFY_DONE;
 	}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index fd91becd6848..5e70ca3b09ba 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1503,8 +1503,24 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
 	return 0;
 }
 
+void mlx5_lag_notify_speed_change(struct mlx5_lag *ldev)
+{
+	struct lag_func *pf;
+	int idx;
+
+	idx = mlx5_lag_get_dev_index_by_seq(ldev, MLX5_LAG_P1);
+	if (idx < 0)
+		return;
+	pf = mlx5_lag_pf(ldev, idx);
+	if (!pf)
+		return;
+	blocking_notifier_call_chain(&pf->dev->priv.lag_nh,
+				     MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE, NULL);
+}
+
 void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
 {
+	u32 old_speed;
 	u32 speed;
 
 	lockdep_assert_held(&ldev->lock);
@@ -1516,7 +1532,11 @@ void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
 	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
 		return;
 
+	old_speed = ldev->agg_speed_mbps;
 	ldev->agg_speed_mbps = speed;
+
+	if (mlx5_lag_is_roce_lag(ldev) && speed != old_speed)
+		mlx5_lag_notify_speed_change(ldev);
 }
 
 void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
index 744fbf9a0078..2b15a0b70fdf 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
@@ -259,6 +259,7 @@ void mlx5_lag_rescan_dev_locked(struct mlx5_lag *ldev,
 void mlx5_lag_add_devices_filter(struct mlx5_lag *ldev, u32 filter);
 struct mlx5_devcom_comp_dev *mlx5_lag_get_devcom_comp(struct mlx5_lag *ldev);
 
+void mlx5_lag_notify_speed_change(struct mlx5_lag *ldev);
 void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev);
 void mlx5_lag_reset_agg_speed(struct mlx5_lag *ldev);
 
diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h
index 8cb321a9fb3d..1ec2331c24ee 100644
--- a/include/linux/mlx5/device.h
+++ b/include/linux/mlx5/device.h
@@ -374,6 +374,7 @@ enum mlx5_driver_event {
 	MLX5_DRIVER_EVENT_AFFILIATION_DONE,
 	MLX5_DRIVER_EVENT_AFFILIATION_REMOVED,
 	MLX5_DRIVER_EVENT_ACTIVE_BACKUP_LAG_CHANGE_LOWERSTATE,
+	MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE,
 };
 
 enum {
-- 
2.44.0

Re: [PATCH net 5/5] {RDMA,net}/mlx5: notify RoCE LAG speed change via driver event

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
{RDMA,net}/mlx5: notify RoCE LAG speed change via driver event

This adds a new MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE driver event that the
LAG layer raises from mlx5_lag_update_agg_speed() when the cached
aggregate speed changes, and the mlx5_ib lag_event() handler turns it
into an IB_EVENT_DEVICE_SPEED_CHANGE for the IB device.  A couple of
questions below about the emit condition and the commit message.

> In RoCE LAG we don't modify any max_tx_speed, so no
> IB_EVENT_DEVICE_SPEED_CHANGE is ever fired.

[Severity: Medium]
Is this description of the existing behaviour accurate?  handle_port_change()
in drivers/infiniband/hw/mlx5/main.c already dispatches this event, and it
does so before the Ethernet link layer early return:

	case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
	case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
	case MLX5_PORT_CHANGE_SUBTYPE_INITIALIZED:
		if (ibdev->ib_active) {
			struct ib_event speed_event = {};

			speed_event.device = &ibdev->ib_dev;
			speed_event.event = IB_EVENT_DEVICE_SPEED_CHANGE;
			ib_dispatch_event(&speed_event);
		}

		/* In RoCE, port up/down events are handled in
		 * mlx5_netdev_event().
		 */
		if (mlx5_ib_port_link_layer(&ibdev->ib_dev, port) ==
					    IB_LINK_LAYER_ETHERNET)
			return -EINVAL;

So for the master device of a RoCE LAG whose own physical port changes
state, the event is fired today.

The gap this patch appears to close is narrower: aggregate changes driven by
a port EQE that only lands on a peer mdev whose IB device was removed by
mlx5_lag_remove_devices(), and bonding state changes (tracker tx_enabled,
v2p remap) that update the cache asynchronously from the port EQE, where a
consumer querying on the existing event can still read the old aggregate.

Since the patch carries a Fixes: tag and is a stable candidate, would it be
worth rewording this to describe the case that is actually uncovered instead
of saying no such event is ever fired?

> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 6ab7f945e712c..bd54af47eda73 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -3693,6 +3693,16 @@ static int lag_event(struct notifier_block *nb, unsigned long event, void *data)
>  			rdma_roce_rescan_port(ibdev, portnum + 1);
>  		}
>  		break;
> +	case MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE: {
> +		struct ib_event speed_event = {};
> +
> +		if (!dev->ib_active)
> +			break;
> +		speed_event.device = ibdev;
> +		speed_event.event = IB_EVENT_DEVICE_SPEED_CHANGE;
> +		ib_dispatch_event(&speed_event);
> +		break;
> +	}
>  	default:
>  		return NOTIFY_DONE;
>  	}

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index fd91becd6848f..5e70ca3b09ba9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1503,8 +1503,24 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)
>  	return 0;
>  }
>  
> +void mlx5_lag_notify_speed_change(struct mlx5_lag *ldev)
> +{
> +	struct lag_func *pf;
> +	int idx;
> +
> +	idx = mlx5_lag_get_dev_index_by_seq(ldev, MLX5_LAG_P1);
> +	if (idx < 0)
> +		return;
> +	pf = mlx5_lag_pf(ldev, idx);
> +	if (!pf)
> +		return;
> +	blocking_notifier_call_chain(&pf->dev->priv.lag_nh,
> +				     MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE, NULL);
> +}
> +
>  void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
>  {
> +	u32 old_speed;
>  	u32 speed;
>  
>  	lockdep_assert_held(&ldev->lock);
> @@ -1516,7 +1532,11 @@ void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
>  	if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
>  		return;
>  
> +	old_speed = ldev->agg_speed_mbps;
>  	ldev->agg_speed_mbps = speed;
> +
> +	if (mlx5_lag_is_roce_lag(ldev) && speed != old_speed)
> +		mlx5_lag_notify_speed_change(ldev);
>  }

[Severity: Medium]
Should the emit be gated on mlx5_lag_is_roce_lag() here?  The same cached
value that was just updated is also what the IB layer reports for the
switchdev uplink representor, in mlx5_ib_query_port_speed_rep():

	if (rep->vport == MLX5_VPORT_UPLINK) {
		if (mlx5_lag_is_sriov(mdev))
			return mlx5_ib_query_port_speed_from_bond(dev,
								  port_num,
								  speed);

mlx5_lag_is_roce_lag() is a "no SR-IOV and legacy eswitch" predicate:

	mlx5_ldev_for_each(i, 0, ldev) {
		pf = mlx5_lag_pf(ldev, i);
		roce_lag = roce_lag && !mlx5_sriov_is_enabled(pf->dev);
	}
	...
		roce_lag = roce_lag && is_mdev_legacy_mode(pf->dev);

so in exactly the SR-IOV/shared-FDB LAG configuration where the uplink
representor reader is used, the gate is always false and no
IB_EVENT_DEVICE_SPEED_CHANGE is delivered even though the value exported
through QUERY_PORT_SPEED changed.

The reasoning given in the commit message applies to that reader too, since
mlx5_lag_modify_device_vports_speed() skips the uplink vport:

		if (vport->vport == MLX5_VPORT_UPLINK)
			continue;

The path I have in mind is: both PFs in switchdev mode and bonded, LAG comes
up in MLX5_LAG_MODE_SRIOV, then the peer PF link goes down and the bond work
runs mlx5_modify_lag() -> mlx5_lag_set_vports_agg_speed() ->
mlx5_lag_update_agg_speed(), which stores a different agg_speed_mbps with no
notification, while the peer's physical port EQE never reaches this IB
device's handle_port_change().

Separately, mlx5_lag_notify_speed_change() only calls the MLX5_LAG_P1 device's
priv.lag_nh, but each IB device registers on its own mdev's chain:

	dev->lag_events.notifier_call = lag_event;
	blocking_notifier_chain_register(&dev->mdev->priv.lag_nh,
					 &dev->lag_events);

In SR-IOV LAG the slave IB devices are not removed, so would the second PF's
IB device be reachable at all here even if the gate were relaxed?

[ ... ]