Re: [PATCH net] Revert "net/mlx5: E-Switch, preserve max tx speed on vport state modification"

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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] The revert's correctness rests on a firmware claim that cannot
  be checked from the kernel tree, and that claim contradicts the commit…

--

Revert "net/mlx5: E-Switch, preserve max tx speed on vport state modification"

This reverts the query-and-write-back of max_tx_speed in
mlx5_modify_vport_admin_state() and mlx5_esw_adj_vport_modify(). The
commit message says FW treats a zero max_tx_speed in MODIFY_VPORT_STATE
as "not provided", so no query is needed to keep the current value.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/vport.c b/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> index 5774ec1174287..78f3d15b599d0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> @@ -93,30 +93,6 @@ int mlx5_modify_vport_admin_state(struct mlx5_core_dev *mdev, u8 opmod,
>  	lockdep_assert_held(&mdev->priv.eswitch->state_lock);
>  #endif
>  
> -	if (MLX5_CAP_ESW(mdev, esw_vport_state_max_tx_speed) &&
> -	    opmod == MLX5_VPORT_STATE_OP_MOD_ESW_VPORT &&
> -	    vport != MLX5_VPORT_UPLINK) {
> -		u32 speed = 0;
> -		int err;
> -
> -		err = mlx5_query_vport_max_tx_speed(mdev, opmod, vport,
> -						    other_vport, &speed, NULL);

[ ... ]

> -		MLX5_SET(modify_vport_state_in, in, max_tx_speed, speed);
> -	}
> -
>  	MLX5_SET(modify_vport_state_in, in, opcode,
>  		 MLX5_CMD_OP_MODIFY_VPORT_STATE);
>  	MLX5_SET(modify_vport_state_in, in, op_mod, opmod);

[Severity: Medium]

The commit being reverted, ad0ae7aefa7a, describes the opposite firmware
behavior:

  "When modifying vport state, the firmware interprets a zero in the max tx
  speed field as an intentional reset, which can overwrite previously set
  values."

This revert says:

  "FW skips writing it whenever it's 0, treating that as "not provided"
  rather than "reset to zero"."

Which of these is correct, and does it hold for every FW version?

Could the commit message explain why the earlier description was wrong?
For example, it could cite the PRM or a FW version where this behavior
was confirmed.

The command layout has valid bits for the connect fields but none for
max_tx_speed:

include/linux/mlx5/mlx5_ifc.h:mlx5_ifc_modify_vport_state_in_bits {
    ...
	u8         max_tx_speed[0x10];
	u8         ingress_connect[0x1];
	u8         egress_connect[0x1];
	u8         ingress_connect_valid[0x1];
	u8         egress_connect_valid[0x1];
    ...
}

So after this patch, mlx5_modify_vport_admin_state() and
mlx5_esw_adj_vport_modify() always send max_tx_speed=0. Whether that
keeps the current speed is decided entirely by FW.

Suppose some FW that advertises esw_vport_state_max_tx_speed does reset
on zero. Would routine admin state changes then clear the LAG-aggregated
speed programmed by mlx5_lag_modify_device_vports_speed()? For example:

ip link set <rep> down
  mlx5e_rep_close()
    mlx5_modify_vport_admin_state(priv->mdev,
                                  MLX5_VPORT_STATE_OP_MOD_ESW_VPORT,
                                  rep->vport, 1,
                                  MLX5_VPORT_ADMIN_STATE_DOWN);

The same applies to mlx5e_rep_open(), mlx5e_rep_change_carrier() and
mlx5_eswitch_set_vport_state(). It also applies to adjacent vport connect
and disconnect, which mlx5_esw_fdb_active() and mlx5_esw_fdb_inactive()
do through mlx5_esw_adj_vport_modify().

In that case vport->agg_max_tx_speed would still hold the aggregated
value. Nothing would reapply it until the next LAG event or
mlx5_esw_vport_enable(). That is the hardware/software mismatch that
ad0ae7aefa7a set out to fix.

Also, this patch carries a Fixes: tag, but the commit message only calls
the reverted code "unnecessary" and names no defect. The reverted code
added one query and wrote back the value FW had just reported.

Is there a user-visible problem being fixed here? If not, would this fit
better as a cleanup without the Fixes: tag?

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