Re: [PATCH net-next 11/15] net/mlx5: E-switch, add match-metadata allocator for nested VF/SF eswitches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/mlx5: broken virt-LAG check in mlx5_esw_match_metadata_alloc()
--

commit 9baf575a43e87029cfdb18972493f93999d3c0c8
Author: Shay Drory <shayd@nvidia.com>

net/mlx5: E-switch, add match-metadata allocator for nested VF/SF eswitches

This commit introduces a match-metadata allocator for nested VF and SF
eswitches that encodes the function's LAG member index and an 8-bit vport
ID into the 16-bit source-port metadata field.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 70e6dd9561a66..ef1043517e9e2 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -3651,6 +3651,48 @@ static u32 mlx5_esw_match_metadata_reserved(struct mlx5_eswitch *esw)
>  	return MLX5_ESW_METADATA_RSVD_UPLINK;
>  }
>  
> +/* Nested (virt LAG) source-port metadata layout: the upper ESW_NESTED_IDX_BITS
> + * hold the VF/SF's slot in the LAG and the lower ESW_NESTED_VPORT_BITS hold a
> + * vport id unique within its eswitch. The two fields still sum to
> + * ESW_SOURCE_PORT_METADATA_BITS. The LAG slot (0-254, per the xa_alloc() limit
> + * in mlx5_ldev_add_mdev()) fits in ESW_NESTED_IDX_BITS; slots 0xf0 and up would
> + * drive the PFNUM bits to the 0xf reserved by internal port offload, so only
> + * 0-239 are used.
> + */
> +#define ESW_NESTED_VPORT_BITS 8
> +#define ESW_NESTED_IDX_BITS (ESW_SOURCE_PORT_METADATA_BITS - ESW_NESTED_VPORT_BITS)
> +
> +static u32 mlx5_nested_esw_match_metadata_alloc(struct mlx5_eswitch *esw)
> +{
> +	u32 vport_end_ida = (1 << ESW_NESTED_VPORT_BITS) - 1;
> +	u32 max_idx = (1 << ESW_NESTED_IDX_BITS) -
> +		      (1 << (ESW_NESTED_IDX_BITS - ESW_PFNUM_BITS)) - 1;
> +	int idx;
> +	int id;
> +
> +	idx = mlx5_lag_get_member_idx(esw->dev);
> +	if (idx < 0 || idx > max_idx)
> +		return 0;
> +
> +	/* Use only non-zero id's (2-255). */
> +	id = ida_alloc_range(&esw->offloads.vport_metadata_ida,
> +			     MLX5_ESW_METADATA_RSVD_UPLINK + 1,
> +			     vport_end_ida, GFP_KERNEL);
> +	if (id < 0)
> +		return 0;
> +	id = (idx << ESW_NESTED_VPORT_BITS) | id;
> +	return id;
> +}
> +
> +static void mlx5_nested_esw_match_metadata_free(struct mlx5_eswitch *esw,
> +						u32 metadata)
> +{
> +	u32 vport_bit_mask = (1 << ESW_NESTED_VPORT_BITS) - 1;
> +
> +	/* Metadata holds only ESW_NESTED_VPORT_BITS of actual ida id. */
> +	ida_free(&esw->offloads.vport_metadata_ida, metadata & vport_bit_mask);
> +}
> +
>  u32 mlx5_esw_match_metadata_alloc(struct mlx5_eswitch *esw)
>  {
>  	u32 vport_end_ida = (1 << ESW_VPORT_BITS) - 1;
> @@ -3659,6 +3701,9 @@ u32 mlx5_esw_match_metadata_alloc(struct mlx5_eswitch *esw)
>  	int pf_num;
>  	int id;
>  
> +	if (!mlx5_core_is_pf(esw->dev) && !mlx5_virt_lag_nonmember(esw->dev))
> +		return mlx5_nested_esw_match_metadata_alloc(esw);
> +
>  	/* Only 4 bits of pf_num */
>  	pf_num = mlx5_sd_pf_num_get(esw->dev);
>  	if (pf_num < 0 || pf_num > max_pf_num)
> @@ -3679,6 +3724,9 @@ void mlx5_esw_match_metadata_free(struct mlx5_eswitch *esw, u32 metadata)
>  {
>  	u32 vport_bit_mask = (1 << ESW_VPORT_BITS) - 1;
>  
> +	if (!mlx5_core_is_pf(esw->dev) && !mlx5_virt_lag_nonmember(esw->dev))
> +		return mlx5_nested_esw_match_metadata_free(esw, metadata);

[Severity: High]
Does negating mlx5_virt_lag_nonmember(esw->dev) in
mlx5_esw_match_metadata_alloc() and mlx5_esw_match_metadata_free() break
switchdev mode on VF and SF eswitches that do not support virt LAG?

In drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h,
mlx5_virt_lag_nonmember() is defined as:

static inline bool mlx5_virt_lag_nonmember(struct mlx5_core_dev *dev)
{
	return mlx5_virt_lag_is_supported(dev) && !mlx5_lag_dev(dev);
}

When mlx5_virt_lag_is_supported(esw->dev) is false, mlx5_lag_dev(esw->dev)
is NULL and mlx5_virt_lag_nonmember(esw->dev) returns false.

As a result, !mlx5_virt_lag_nonmember(esw->dev) evaluates to true, sending
non-virt-LAG VF and SF eswitches into mlx5_nested_esw_match_metadata_alloc():

esw_offloads_enable()
  esw_offloads_metadata_init()
    esw_offloads_vport_metadata_setup()
      mlx5_esw_match_metadata_alloc()
        mlx5_nested_esw_match_metadata_alloc()
          mlx5_lag_get_member_idx() -> -ENOENT

Because mlx5_lag_get_member_idx(esw->dev) returns -ENOENT when
mlx5_lag_dev(esw->dev) is NULL, mlx5_nested_esw_match_metadata_alloc()
returns 0 and esw_offloads_vport_metadata_setup() fails with -ENOSPC.

Should mlx5_esw_match_metadata_alloc() and mlx5_esw_match_metadata_free()
check whether the device is actually a virt LAG member (for example,
checking mlx5_virt_lag_is_supported(esw->dev) && mlx5_lag_dev(esw->dev),
or simply mlx5_lag_dev(esw->dev)) instead of !mlx5_virt_lag_nonmember(esw->dev)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103171009.563034-1-tariqt@nvidia.com?part=11