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

[Shay Drori <shayd@nvidia.com>]

On 09/09/2026 15:15, netdev-bot+sashiko@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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?


good finding, will spell it out in next version


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