Re: [PATCH net-next 05/15] net/mlx5: LAG, decouple iteration end-of-walk from MLX5_MAX_PORTS

[Paolo Abeni <pabeni@redhat.com>]

Hi,

there are a couple of unanswered comments here...

On 11/3/26 18:09, Tariq Toukan wrote:
> @@ -2095,7 +2096,7 @@ static int mlx5_handle_changeupper_event(struct mlx5_lag *ldev,
>   				break;
>   			}
>   		}
> -		if (i < MLX5_MAX_PORTS) {
> +		if (i < ldev->ports) {

[medium]
Should this compare against ldev->max_funcs, like the other converted call
sites?
The iterator's end-of-walk value is now ldev->max_funcs. That holds both in
mlx5_get_next_lag_func() and in the mlx5_lag_for_each() condition.
Indices are never compacted and max_funcs never shrinks. So a PORT-marked
member can sit at an index in [ldev->ports, max_funcs) while the ldev still
has ldev->ports members. One way to get there: more PFs than num_lag_ports
join the ldev, and a lower-index one is removed later.
In that case the inner loop finds the matching pf and breaks with
i >= ldev->ports. idx has already been incremented, but this block is
skipped, so the bond_status |= (1 << idx) and has_inactive updates are
lost. The later check would then fail:
	is_in_lag = num_slaves == ldev->ports &&
		bond_status == GENMASK(ldev->ports - 1, 0);
Wouldn't that leave this bond silently not offloaded?
The old i < MLX5_MAX_PORTS check covered the whole index range that the old
xa_alloc() limit allowed. This code is unchanged at the end of the series.

This looks valid to me.

>   			if (netif_is_bond_master(upper)) {
>   				slave = bond_slave_get_rcu(ndev_tmp);
>   				if (slave)
> @@ -2360,13 +2361,16 @@ int mlx5_ldev_add_mdev(struct mlx5_lag *ldev,
>   	if (!pf)
>   		return -ENOMEM;
>   
> -	err = xa_alloc(&ldev->pfs, &idx, pf, XA_LIMIT(0, MLX5_MAX_PORTS - 1),
> +	err = xa_alloc(&ldev->pfs, &idx, pf, XA_LIMIT(0, U8_MAX - 1),
>   		       GFP_KERNEL);

[high]
Can this let member indices reach 8 or more while several arrays are still
sized MLX5_MAX_PORTS?

this is solved at the end of the series, so not really a problem.

Note that there are more unanswered comments on patch 15/15.

I think a v2 is need. For the future I suggest using smaller series:
15 non trivial patches will have increasingly hard time with AI
reviews.

/P