[PATCH net-next 0/2] net/mlx5e: Align setdcbx with DCBX capability flags

[PATCH net-next 0/2] net/mlx5e: Align setdcbx with DCBX capability flags

[Tariq Toukan]

Hi,

This series by Alexei aligns the mlx5e DCBX mode handling in the setdcbx
DCB callback with the DCBX capability flags.

Today mlx5e_dcbnl_setdcbx() applies two different policies: a request of
mode 0 switches the firmware to AUTO, while any other request is
accepted only if it carries DCB_CAP_DCBX_HOST. Requests with
DCB_CAP_DCBX_LLD_MANAGED are rejected outright, even though that flag
describes exactly the case where DCBX negotiation is done by the
firmware/LLD agent rather than by the host.

Patch 1 switches the firmware between HOST and AUTO modes based solely
on the DCB_CAP_DCBX_HOST bit, which folds the two code paths into one
and removes the dedicated switch-to-host helper. When moving to
firmware-controlled mode, CEE, IEEE and LLD_MANAGED are reported
together due to a firmware limitation.

Patch 2 drops the rejection of DCB_CAP_DCBX_LLD_MANAGED, so that
requesting it switches to firmware-controlled DCBX mode.

Regards,
Tariq

Alexei Lazar (2):
  net/mlx5e: Select DCBX HOST/AUTO from capability flags in setdcbx
  net/mlx5e: Add DCB_CAP_DCBX_LLD_MANAGED support in setdcbx

 .../ethernet/mellanox/mlx5/core/en_dcbnl.c    | 51 +++++--------------
 1 file changed, 14 insertions(+), 37 deletions(-)


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.44.0

[PATCH net-next 1/2] net/mlx5e: Select DCBX HOST/AUTO from capability flags in setdcbx

[Tariq Toukan]

From: Alexei Lazar <alazar@nvidia.com>

mlx5e_dcbnl_setdcbx() used two different policies: mode 0 switched
firmware to AUTO, and any other request was accepted only with
DCB_CAP_DCBX_HOST.

Switch the firmware HOST/AUTO modes based on the DCB_CAP_DCBX_HOST
bit. This better aligns with the DCBX capability flags. It also
allows folding the two different code paths into one.

When switching to firmware-controlled (non-host) mode, report CEE,
IEEE and LLD_MANAGED together due to a FW limitation.

Signed-off-by: Alexei Lazar <alazar@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Carolina Jubran <cjubran@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/en_dcbnl.c    | 48 ++++++-------------
 1 file changed, 14 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
index 741f75b5bfec..4eef929f5ae9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
@@ -99,25 +99,6 @@ static int mlx5e_dcbnl_set_dcbx_mode(struct mlx5e_priv *priv,
 	return mlx5_set_port_dcbx_param(mdev, param);
 }
 
-static int mlx5e_dcbnl_switch_to_host_mode(struct mlx5e_priv *priv)
-{
-	struct mlx5e_dcbx *dcbx = &priv->dcbx;
-	int err;
-
-	if (!MLX5_CAP_GEN(priv->mdev, dcbx))
-		return 0;
-
-	if (dcbx->mode == MLX5E_DCBX_PARAM_VER_OPER_HOST)
-		return 0;
-
-	err = mlx5e_dcbnl_set_dcbx_mode(priv, MLX5E_DCBX_PARAM_VER_OPER_HOST);
-	if (err)
-		return err;
-
-	dcbx->mode = MLX5E_DCBX_PARAM_VER_OPER_HOST;
-	return 0;
-}
-
 static int mlx5e_dcbnl_ieee_getets(struct net_device *netdev,
 				   struct ieee_ets *ets)
 {
@@ -453,32 +434,31 @@ static u8 mlx5e_dcbnl_getdcbx(struct net_device *dev)
 
 static u8 mlx5e_dcbnl_setdcbx(struct net_device *dev, u8 mode)
 {
+	bool new_is_host = mode & DCB_CAP_DCBX_HOST;
 	struct mlx5e_priv *priv = netdev_priv(dev);
 	struct mlx5e_dcbx *dcbx = &priv->dcbx;
+	enum mlx5_dcbx_oper_mode target_mode;
 
 	if (mode & DCB_CAP_DCBX_LLD_MANAGED)
 		return 1;
 
-	if ((!mode) && MLX5_CAP_GEN(priv->mdev, dcbx)) {
-		if (dcbx->mode == MLX5E_DCBX_PARAM_VER_OPER_AUTO)
-			return 0;
-
-		/* set dcbx to fw controlled */
-		if (!mlx5e_dcbnl_set_dcbx_mode(priv, MLX5E_DCBX_PARAM_VER_OPER_AUTO)) {
-			dcbx->mode = MLX5E_DCBX_PARAM_VER_OPER_AUTO;
-			dcbx->cap &= ~DCB_CAP_DCBX_HOST;
-			return 0;
-		}
+	if (!MLX5_CAP_GEN(priv->mdev, dcbx))
+		return !new_is_host;
 
-		return 1;
-	}
+	target_mode = new_is_host ? MLX5E_DCBX_PARAM_VER_OPER_HOST :
+				    MLX5E_DCBX_PARAM_VER_OPER_AUTO;
+	if (dcbx->mode == target_mode)
+		return 0;
 
-	if (!(mode & DCB_CAP_DCBX_HOST))
+	if (mlx5e_dcbnl_set_dcbx_mode(priv, target_mode))
 		return 1;
 
-	if (mlx5e_dcbnl_switch_to_host_mode(netdev_priv(dev)))
-		return 1;
+	if (!new_is_host)
+		mode = DCB_CAP_DCBX_VER_CEE |
+		       DCB_CAP_DCBX_VER_IEEE |
+		       DCB_CAP_DCBX_LLD_MANAGED;
 
+	dcbx->mode = target_mode;
 	dcbx->cap = mode;
 
 	return 0;
-- 
2.44.0

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

[Tariq Toukan]

From: Alexei Lazar <alazar@nvidia.com>

Add support for DCB_CAP_DCBX_LLD_MANAGED, which indicates that DCBX
negotiation is performed by the firmware/LLD agent rather than the host.

When DCB_CAP_DCBX_LLD_MANAGED is requested, switch to
firmware-controlled DCBX mode.

Signed-off-by: Alexei Lazar <alazar@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Carolina Jubran <cjubran@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c | 3 ---
 1 file changed, 3 deletions(-)

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;
-
 	if (!MLX5_CAP_GEN(priv->mdev, dcbx))
 		return !new_is_host;
 
-- 
2.44.0

Re: [PATCH net-next 1/2] net/mlx5e: Select DCBX HOST/AUTO from capability flags in setdcbx

[netdev-bot+sashiko]

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()
  (drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c) now returns…
- [Medium] The patch reports firmware AUTO mode as CEE|IEEE|LLD_MANAGED
  (0x0e), but only on the setdcbx HOST->AUTO transition path.

--

net/mlx5e: Select DCBX HOST/AUTO from capability flags in setdcbx

This commit changes mlx5e_dcbnl_setdcbx() so that the DCB_CAP_DCBX_HOST
bit selects firmware HOST or AUTO DCBX mode. That merges the two old code
paths into one. When it switches to firmware-controlled mode, it reports
CEE, IEEE and LLD_MANAGED together because of a firmware limitation.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> index 741f75b5bfec..4eef929f5ae9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c

[ ... ]

> @@ -453,32 +434,31 @@ static u8 mlx5e_dcbnl_getdcbx(struct net_device *dev)
>  
>  static u8 mlx5e_dcbnl_setdcbx(struct net_device *dev, u8 mode)
>  {
> +	bool new_is_host = mode & DCB_CAP_DCBX_HOST;
>  	struct mlx5e_priv *priv = netdev_priv(dev);
>  	struct mlx5e_dcbx *dcbx = &priv->dcbx;
> +	enum mlx5_dcbx_oper_mode target_mode;
>  
>  	if (mode & DCB_CAP_DCBX_LLD_MANAGED)
>  		return 1;
>  
> -	if ((!mode) && MLX5_CAP_GEN(priv->mdev, dcbx)) {
> -		if (dcbx->mode == MLX5E_DCBX_PARAM_VER_OPER_AUTO)
> -			return 0;
> -
> -		/* set dcbx to fw controlled */
> -		if (!mlx5e_dcbnl_set_dcbx_mode(priv, MLX5E_DCBX_PARAM_VER_OPER_AUTO)) {
> -			dcbx->mode = MLX5E_DCBX_PARAM_VER_OPER_AUTO;
> -			dcbx->cap &= ~DCB_CAP_DCBX_HOST;
> -			return 0;
> -		}
> +	if (!MLX5_CAP_GEN(priv->mdev, dcbx))
> +		return !new_is_host;
>  
> -		return 1;
> -	}
> +	target_mode = new_is_host ? MLX5E_DCBX_PARAM_VER_OPER_HOST :
> +				    MLX5E_DCBX_PARAM_VER_OPER_AUTO;
> +	if (dcbx->mode == target_mode)
> +		return 0;

[Severity: Medium]
Do these two early returns lose the requested DCBX flags?

If the device lacks the dcbx HCA capability, the first check returns 0
for any HOST request.

If firmware is already in the target mode, the second check returns 0.
One example is a host LLDP agent sending HOST|VER_IEEE (0x09) while the
port is already in HOST mode.

Neither path reaches the "dcbx->cap = mode;" at the end of the function.

Before this patch, both cases went through
mlx5e_dcbnl_switch_to_host_mode(). That helper returned 0 when the cap was
missing or the mode was already HOST, and the caller then did:

	dcbx->cap = mode;
	return 0;

Now dcbnl_setdcbx() reports success to userspace, but
mlx5e_dcbnl_getdcbx() keeps returning the initial CEE|IEEE|HOST (0x0d).
The same stale value goes into DCB_ATTR_DCBX in the IEEE/CEE get and
notify paths, and into event.dcbx for dcb_app notifier events.

fcoe_dcb_app_notification() in drivers/scsi/fcoe/fcoe.c consumes that
value:

	if (entry->dcbx & DCB_CAP_DCBX_VER_CEE)
		prio = ffs(entry->app.priority) - 1;

If VER_CEE is still set after userspace selected IEEE only, could FCoE
read an IEEE priority as a CEE bitmap?

The commit message only talks about folding the two code paths into one.
Was this change in behaviour intended? It is still present at the end of
the series.

>  
> -	if (!(mode & DCB_CAP_DCBX_HOST))
> +	if (mlx5e_dcbnl_set_dcbx_mode(priv, target_mode))
>  		return 1;
>  
> -	if (mlx5e_dcbnl_switch_to_host_mode(netdev_priv(dev)))
> -		return 1;
> +	if (!new_is_host)
> +		mode = DCB_CAP_DCBX_VER_CEE |
> +		       DCB_CAP_DCBX_VER_IEEE |
> +		       DCB_CAP_DCBX_LLD_MANAGED;

[Severity: Medium]
Does the reported AUTO state now depend on history rather than on the
firmware state?

This sets cap to CEE|IEEE|LLD_MANAGED (0x0e), but only after an actual
HOST to AUTO transition. mlx5e_dcbnl_initialize() was not updated. When
firmware is in a non-host mode at probe time, it still does:

	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;

That gives 0x0c, without LLD_MANAGED. A later non-host setdcbx request
then hits the "if (dcbx->mode == target_mode) return 0;" check above, so
LLD_MANAGED is never added.

So the same firmware state is reported as 0x0c after booting in AUTO, but
as 0x0e after a runtime switch to AUTO. The difference shows up in:

  - getdcbx
  - DCB_CAP_ATTR_DCBX, since mlx5e_dcbnl_getcap() ORs in dcbx->cap
  - DCB_ATTR_DCBX in the IEEE/CEE get and notify paths
  - event.dcbx

Before this patch, both paths reported 0x0c.

Should mlx5e_dcbnl_initialize() also set LLD_MANAGED in the AUTO case?
That would match "report CEE, IEEE and LLD_MANAGED together" from the
commit message.

There is a related problem in the same function. setdcbx still rejects
LLD_MANAGED at the top even though it now reports it, which breaks a get
then set round trip.

The later patch in this series, "net/mlx5e: Add DCB_CAP_DCBX_LLD_MANAGED
support in setdcbx", fixes that part by removing the check. The
history-dependent reporting is still present at the end of the series.

[ ... ]

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

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

[netdev-bot+sashiko]

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