Re: [net 11/12] net/mlx5e: Revert parameters on errors when changing MTU and LRO state without reset

On Wed, 2021-01-27 at 15:00 -0500, Willem de Bruijn wrote:

On Wed, Jan 27, 2021 at 3:58 AM Saeed Mahameed [off-list ref]
wrote:

quoted

From: Maxim Mikityanskiy <redacted>

Sometimes, channel params are changed without recreating the
channels.
It happens in two basic cases: when the channels are closed, and
when
the parameter being changed doesn't affect how channels are
configured.
Such changes invoke a hardware command that might fail. The whole
operation should be reverted in such cases, but the code that
restores
the parameters' values in the driver was missing. This commit adds
this
handling.

Fixes: 2e20a151205b ("net/mlx5e: Fail safe mtu and lro setting")
Signed-off-by: Maxim Mikityanskiy <redacted>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 30 +++++++++++++--
----
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index ac76d32bad7d..a9d824a9cb05 100644

--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

@@ -3764,7 +3764,7 @@ static int set_feature_lro(struct net_device

*netdev, bool enable)
        struct mlx5e_priv *priv = netdev_priv(netdev);
        struct mlx5_core_dev *mdev = priv->mdev;
        struct mlx5e_channels new_channels = {};
-       struct mlx5e_params *old_params;
+       struct mlx5e_params *cur_params;
        int err = 0;
        bool reset;

@@ -3777,8 +3777,8 @@ static int set_feature_lro(struct net_device

*netdev, bool enable)
                goto out;
        }

-       old_params = &priv->channels.params;
-       if (enable && !MLX5E_GET_PFLAG(old_params,
MLX5E_PFLAG_RX_STRIDING_RQ)) {
+       cur_params = &priv->channels.params;
+       if (enable && !MLX5E_GET_PFLAG(cur_params,
MLX5E_PFLAG_RX_STRIDING_RQ)) {
                netdev_warn(netdev, "can't set LRO with legacy
RQ\n");
                err = -EINVAL;
                goto out;

@@ -3786,18 +3786,23 @@ static int set_feature_lro(struct

net_device *netdev, bool enable)

        reset = test_bit(MLX5E_STATE_OPENED, &priv->state);

-       new_channels.params = *old_params;
+       new_channels.params = *cur_params;
        new_channels.params.lro_en = enable;

-       if (old_params->rq_wq_type != MLX5_WQ_TYPE_CYCLIC) {
-               if (mlx5e_rx_mpwqe_is_linear_skb(mdev, old_params,
NULL) ==
+       if (cur_params->rq_wq_type != MLX5_WQ_TYPE_CYCLIC) {
+               if (mlx5e_rx_mpwqe_is_linear_skb(mdev, cur_params,
NULL) ==
                    mlx5e_rx_mpwqe_is_linear_skb(mdev,
&new_channels.params, NULL))
                        reset = false;
        }

        if (!reset) {
-               *old_params = new_channels.params;
+               struct mlx5e_params old_params;
+
+               old_params = *cur_params;
+               *cur_params = new_channels.params;
                err = mlx5e_modify_tirs_lro(priv);
+               if (err)
+                       *cur_params = old_params;

No need to explicitly save and restore all params if the only one
changed is lro_en?

not a big deal, this is a practice we follow in all of our unwind
procedures, 
the code will change in net-next to use a generic function that would
restore all params regardless of what changed and what not .. so we
figured to have a one flow that will be easy to replace in net-next.