Re: [PATCH net-next 2/2] net/mlx5e: Add DCB_CAP_DCBX_LLD_MANAGED support in setdcbx

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mlx5e_dcbnl_setdcbx() in
  drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c now returns success…
- [Medium] With the `if (mode & DCB_CAP_DCBX_LLD_MANAGED) return 1;`
  guard removed, mlx5e_dcbnl_setdcbx() looks only at the HOST bit…

--

net/mlx5e: Add DCB_CAP_DCBX_LLD_MANAGED support in setdcbx

This patch removes the early rejection of DCB_CAP_DCBX_LLD_MANAGED in
mlx5e_dcbnl_setdcbx(). An LLD_MANAGED request now switches the device to
firmware-controlled DCBX mode instead of failing.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> index 4eef929f5ae9..68deedb6f82e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> @@ -439,9 +439,6 @@ static u8 mlx5e_dcbnl_setdcbx(struct net_device *dev, u8 mode)
>  	struct mlx5e_dcbx *dcbx = &priv->dcbx;
>  	enum mlx5_dcbx_oper_mode target_mode;
>  
> -	if (mode & DCB_CAP_DCBX_LLD_MANAGED)
> -		return 1;
> -

[Severity: Medium]
An LLD_MANAGED request now returns success. If the firmware was already in
AUTO mode at probe, will LLD_MANAGED ever be reported back?

In the firmware-managed case, mlx5e_dcbnl_initialize() sets the cap
without LLD_MANAGED:

	priv->dcbx.cap = DCB_CAP_DCBX_VER_CEE |
			 DCB_CAP_DCBX_VER_IEEE;
	if (priv->dcbx.mode == MLX5E_DCBX_PARAM_VER_OPER_HOST)
		priv->dcbx.cap |= DCB_CAP_DCBX_HOST;

A later DCB_CMD_SDCBX with LLD_MANAGED gives new_is_host == false and
target_mode == MLX5E_DCBX_PARAM_VER_OPER_AUTO. That matches dcbx->mode,
so mlx5e_dcbnl_setdcbx() returns early and never reaches
dcbx->cap = mode:

	if (dcbx->mode == target_mode)
		return 0;

After that, mlx5e_dcbnl_getdcbx() still returns CEE|IEEE.
mlx5e_dcbnl_getcap(DCB_CAP_ATTR_DCBX) also leaves out LLD_MANAGED.

But after a HOST->AUTO switch through setdcbx, the same firmware state
reads back as CEE|IEEE|LLD_MANAGED:

	if (!new_is_host)
		mode = DCB_CAP_DCBX_VER_CEE |
		       DCB_CAP_DCBX_VER_IEEE |
		       DCB_CAP_DCBX_LLD_MANAGED;

Should mlx5e_dcbnl_initialize() also set DCB_CAP_DCBX_LLD_MANAGED when the
mode is AUTO? Then both paths would report the same state.

>  	if (!MLX5_CAP_GEN(priv->mdev, dcbx))
>  		return !new_is_host;
>  

[Severity: Medium]
What happens when a request sets both HOST and LLD_MANAGED?

dcbnl_setdcbx() in net/dcb/dcbnl.c passes the u8 from userspace straight
to the driver without checking it. The uAPI header in
include/uapi/linux/dcbnl.h defines HOST and LLD_MANAGED as opposite owners
of DCBX negotiation.

Only the HOST bit picks the target mode:

	bool new_is_host = mode & DCB_CAP_DCBX_HOST;

So a request like HOST|LLD_MANAGED|IEEE now selects
MLX5E_DCBX_PARAM_VER_OPER_HOST and switches the firmware to host mode.
That is the opposite of what LLD_MANAGED asks for.

The !new_is_host normalisation is skipped, so dcbx->cap = mode stores
HOST|LLD_MANAGED|IEEE. mlx5e_dcbnl_getdcbx() then reports that both the
host and the LLD own DCBX.

On devices without MLX5_CAP_GEN(priv->mdev, dcbx), the same request now
returns !new_is_host, which is 0 (success), and changes nothing. Before
this patch it returned 1.

Other drivers such as ixgbe, mlx4, i40e, ice and hns3 reject LLD_MANAGED
combinations they can't honour. Should this reject LLD_MANAGED when HOST is
also set, or else define what that combination means?

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