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

[Saeed Mahameed <saeed@kernel.org>]

On Wed, 2021-01-27 at 15:00 -0500, Willem de Bruijn wrote:
> On Wed, Jan 27, 2021 at 3:58 AM Saeed Mahameed <saeedm@nvidia.com>
> wrote:
> > 
> > From: Maxim Mikityanskiy <maximmi@mellanox.com>
> > 
> > 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 <maximmi@mellanox.com>
> > 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.