Re: [PATCH net V2 3/4] net/mlx5: SD, unload reps on shared FDB create error path

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Commit message of "net/mlx5: SD, unload reps on shared FDB create
  error path" describes the change only as symmetry ("The teardown path…

--

net/mlx5: SD, unload reps on shared FDB create error path

The SD branch of mlx5_lag_shared_fdb_create() now unloads the
representors on the error path after destroying the single FDB, matching
what mlx5_lag_shared_fdb_destroy() does.  The patch carries a Fixes: tag
for 68c2dd59a6c7 ("net/mlx5: E-Switch, Tie rep load/unload to SD LAG
state").

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
> index 6b4ad3c53f2f3..424040918fa37 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
> @@ -270,6 +270,7 @@ int mlx5_lag_shared_fdb_create(struct mlx5_lag *ldev,
>  			pf->sd_fdb_active = false;
>  		}
>  		mlx5_lag_destroy_single_fdb_filter(ldev, group_id);
> +		mlx5_lag_unload_reps_from_locked(ldev, filter);

[Severity: Low]
This isn't a bug in the code, but could the changelog say what state was
actually left behind before this call was added?

The commit message only describes symmetry:

    The teardown path unloads the representors; align the create error
    path to do the same.

In the SD branch, mlx5_lag_shared_fdb_create() sets sd_fdb_active for
every group member before the rescan and the reps reload:

	mlx5_lag_for_each(i, 0, ldev, filter) {
		pf = mlx5_lag_pf(ldev, i);
		pf->sd_fdb_active = true;
	}
	...
	mlx5_lag_rescan_dev_locked(ldev, dev0, true);
	err = mlx5_lag_reload_ib_reps_from_locked(ldev, 0, filter, false);
	if (err) {
		mlx5_core_err(dev0, "Failed to enable lag\n");
		goto err_rescan_drivers;
	}

Non-uplink reps are only skipped while SD LAG is inactive, per
mlx5_esw_offloads_rep_load() in eswitch_offloads.c:

	if (vport_num != MLX5_VPORT_UPLINK &&
	    mlx5_get_sd(esw->dev) && !mlx5_lag_is_active(esw->dev))
		return 0;

so reps do get loaded in that window.  If
mlx5_lag_reload_ib_reps_from_locked() then fails, err_rescan_drivers
clears pf->sd_fdb_active and calls
mlx5_lag_destroy_single_fdb_filter(), which left the VF/SF reps loaded
while SD LAG is no longer active, i.e. exactly the state that
68c2dd59a6c7 was written to avoid.

Since this is Fixes-tagged and therefore stable material, would it help
to spell that resulting state out in the changelog?

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