Re: [net 03/15] net/mlx5: E-switch, Fix duplicate lag creation

[Saeed Mahameed <saeed@kernel.org>]

On 28 Nov 15:23, Jacob Keller wrote:
>
>
>On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
>>From: Chris Mi <cmi@nvidia.com>
>>
>>If creating bond first and then enabling sriov in switchdev mode,
>>will hit the following syndrome:
>>
>>mlx5_core 0000:08:00.0: mlx5_cmd_out_err:778:(pid 25543): CREATE_LAG(0x840) op_mod(0x0) failed, status bad parameter(0x3), syndrome (0x7d49cb), err(-22)
>>
>>The reason is because the offending patch removes eswitch mode
>>none. In vf lag, the checking of eswitch mode none is replaced
>>by checking if sriov is enabled. But when driver enables sriov,
>>it triggers the bond workqueue task first and then setting sriov
>>number in pci_enable_sriov(). So the check fails.
>>
>>Fix it by checking if sriov is enabled using eswitch internal
>>counter that is set before triggering the bond workqueue task.
>>
>>Fixes: f019679ea5f2 ("net/mlx5: E-switch, Remove dependency between sriov and eswitch mode")
>>Signed-off-by: Chris Mi <cmi@nvidia.com>
>>Reviewed-by: Roi Dayan <roid@nvidia.com>
>>Reviewed-by: Mark Bloch <mbloch@nvidia.com>
>>Reviewed-by: Vlad Buslov <vladbu@nvidia.com>
>>Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>>---
>>  drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 8 ++++++++
>>  drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c | 5 +++--
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>

[...]

>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>index be1307a63e6d..4070dc1d17cb 100644
>>--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>@@ -701,8 +701,9 @@ static bool mlx5_lag_check_prereq(struct mlx5_lag *ldev)
>>  #ifdef CONFIG_MLX5_ESWITCH
>>  	dev = ldev->pf[MLX5_LAG_P1].dev;
>>-	if ((mlx5_sriov_is_enabled(dev)) && !is_mdev_switchdev_mode(dev))
>>-		return false;
>>+	for (i = 0; i  < ldev->ports; i++)
>>+		if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && !is_mdev_switchdev_mode(dev))
>>+			return false;
>
>Am I missing something? whats with the for loop iterator here? i isn't 
>used or passed into these functions?
>
>Do you need to check multiple times or do these functions have some 
>side effect? But looking at their implementation neither of them 
>appear to have side effects?
>
>What am I missing?

Great catch! it's a copy/paste bug, here we need to grab each port's
eswitch on every iteration.

something like:
for (i = 0; i  < ldev->ports; i++) {
+     dev = ldev->pf[i].dev;
      if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && !is_mdev_switchdev_mode(dev))
              return false;
}