Re: [PATCH v12 net-next 2/9] net/mlx5e: trim stack use in PCIe congestion threshold helper

[David Laight]

On Fri, 8 May 2026 09:19:05 +0530
Ratheesh Kannoth <rkannoth@xxxxxxxxxxx> wrote:

> union devlink_param_value grew when U64 array parameters were added.
> Keeping a four-element array of that union in
> mlx5e_pcie_cong_get_thresh_config() inflated the stack frame past the
> -Wframe-larger-than limit.
> 
> Read each driverinit value into a single reused union, then store the
> four u16 thresholds in struct mlx5e_pcie_cong_thresh field order via a
> temporary u16 pointer to config.
> 
> Signed-off-by: Ratheesh Kannoth <rkannoth@xxxxxxxxxxx>
> ---
>  .../mellanox/mlx5/core/en/pcie_cong_event.c   | 34 +++++++++++--------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/pcie_cong_event.c b/drivers/net/ethernet/mellanox/mlx5/core/en/pcie_cong_event.c
> index 2eb666a46f39..88e76be3a73d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/pcie_cong_event.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/pcie_cong_event.c
> @@ -252,28 +252,32 @@ static int
>  mlx5e_pcie_cong_get_thresh_config(struct mlx5_core_dev *dev,
>  				  struct mlx5e_pcie_cong_thresh *config)
>  {
> +	enum {
> +		INBOUND_HIGH,
> +		INBOUND_LOW,
> +		OUTBOUND_HIGH,
> +		OUTBOUND_LOW,
> +	};
> +
>  	u32 ids[4] = {

Someone will suggest that should be 'static const'.
It may make the code smaller.

> -		MLX5_DEVLINK_PARAM_ID_PCIE_CONG_IN_LOW,
> -		MLX5_DEVLINK_PARAM_ID_PCIE_CONG_IN_HIGH,
> -		MLX5_DEVLINK_PARAM_ID_PCIE_CONG_OUT_LOW,
> -		MLX5_DEVLINK_PARAM_ID_PCIE_CONG_OUT_HIGH,
> +		[INBOUND_LOW] = MLX5_DEVLINK_PARAM_ID_PCIE_CONG_IN_LOW,
> +		[INBOUND_HIGH] = MLX5_DEVLINK_PARAM_ID_PCIE_CONG_IN_HIGH,
> +		[OUTBOUND_LOW] = MLX5_DEVLINK_PARAM_ID_PCIE_CONG_OUT_LOW,
> +		[OUTBOUND_HIGH] = MLX5_DEVLINK_PARAM_ID_PCIE_CONG_OUT_HIGH,
>  	};
> -	struct devlink *devlink = priv_to_devlink(dev);
> -	union devlink_param_value val[4];
>  
> -	for (int i = 0; i < 4; i++) {
> -		u32 id = ids[i];
> -		int err;
> +	struct devlink *devlink = priv_to_devlink(dev);
> +	union devlink_param_value val;
> +	u16 *dst = (u16 *)config;

You can't do that - far too fragile.
Maybe &config->inbound_low - but even that assumes the values are in order.
A safer way would be using a temporary 'u16 val16[4]'.
(Or even overwrite ids[] with the result.)

But the code might even be smaller if you just unroll the loop:
	err = devl_param_driverinit_value_get(devlink, MLX5_DEVLINK_PARAM_ID_PCIE_CONG_IN_LOW, &val);
	if (err)
		return err;
	config->inbound_low = val.vu16;
	err = devl_param_driverinit_value_get(devlink, MLX5_DEVLINK_PARAM_ID_PCIE_CONG_IN_HIGH, &val);
	if (err)
		return err;
	config->inbound_high = val.vu16;
	err = devl_param_driverinit_value_get(devlink, MLX5_DEVLINK_PARAM_ID_PCIE_CONG_OUT_LOW, &val);
	if (err)
		return err;
	config->outbound_low = val.vu16;
	err = devl_param_driverinit_value_get(devlink, MLX5_DEVLINK_PARAM_ID_PCIE_CONG_OUT_HIGH, &val);
	if (err)
		return err;
	config->outbound_high = val.vu16;

-- David


> +	int err;
>  
> -		err = devl_param_driverinit_value_get(devlink, id, &val[i]);
> +	for (int i = 0; i < ARRAY_SIZE(ids); i++) {
> +		err = devl_param_driverinit_value_get(devlink, ids[i], &val);
>  		if (err)
>  			return err;
> -	}
>  
> -	config->inbound_low = val[0].vu16;
> -	config->inbound_high = val[1].vu16;
> -	config->outbound_low = val[2].vu16;
> -	config->outbound_high = val[3].vu16;
> +		dst[i] = val.vu16;
> +	}
>  
>  	return 0;
>  }