| Message ID | 20210126234345.202096-12-saeedm@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8355060f5ec381abda77659f91f56302203df535 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,01/12] net/mlx5: Fix memory leak on flow table creation error flow | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Pull request |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: linux-rdma@vger.kernel.org tariqt@mellanox.com leon@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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?
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.
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; goto out; } @@ -4074,9 +4079,16 @@ int mlx5e_change_mtu(struct net_device *netdev, int new_mtu, } if (!reset) { + unsigned int old_mtu = params->sw_mtu; + params->sw_mtu = new_mtu; - if (preactivate) - preactivate(priv, NULL); + if (preactivate) { + err = preactivate(priv, NULL); + if (err) { + params->sw_mtu = old_mtu; + goto out; + } + } netdev->mtu = params->sw_mtu; goto out; }