Re: [PATCH mlx5-next 01/17] net/mlx5: Simplify IPsec flow steering init/cleanup functions

[Leon Romanovsky <leon@kernel.org>]

On Sun, Apr 10, 2022 at 09:46:20AM -0700, Saeed Mahameed wrote:
> On 10 Apr 11:28, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Cleanup IPsec FS initialization and cleanup functions.
> 
> Can you be more clear about what are you cleaning up ?
> 
> unfolding/joining static functions shouldn't be considered as cleanup.

And how would you describe extensive usage of one time called functions
that have no use as standalone ones?

This patch makes sure that all flow steering initialized and cleaned at
one place and allows me to present coherent picture of what is needed
for IPsec FS.

You should focus on the end result of this series rather on single patch.
 15 files changed, 320 insertions(+), 839 deletions(-)

> 
> Also i don't agree these patches should go to mlx5-next, we need to avoid
> bloating  mlx5-next.
> Many of these patches are purely ipsec, yes i understand you are heavily
> modifying include/linux/mlx5/accel.h but from what I can tell, it's not
> affecting rdma.

I'm deleting include/linux/mlx5/accel.h, it is not needed.
But I don't care about the target, it can be net-next and not
mlx5-next.

> 
> Please give me a chance to review the whole series until next week as i am
> out of office this week.

I see this problematic as next week will be combination of my Passover
vacation and paternity leave at the same time.

Thanks

> 
> > 
> > Reviewed-by: Raed Salem <raeds@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> > .../mellanox/mlx5/core/en_accel/ipsec.c       |  4 +-
> > .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 73 ++++++-------------
> > .../mellanox/mlx5/core/en_accel/ipsec_fs.h    |  4 +-
> > 3 files changed, 27 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > index c280a18ff002..5a10755dd4f1 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > @@ -424,7 +424,7 @@ int mlx5e_ipsec_init(struct mlx5e_priv *priv)
> > 	}
> > 
> > 	priv->ipsec = ipsec;
> > -	mlx5e_accel_ipsec_fs_init(priv);
> > +	mlx5e_ipsec_fs_init(ipsec);
> > 	netdev_dbg(priv->netdev, "IPSec attached to netdevice\n");
> > 	return 0;
> > }
> > @@ -436,7 +436,7 @@ void mlx5e_ipsec_cleanup(struct mlx5e_priv *priv)
> > 	if (!ipsec)
> > 		return;
> > 
> > -	mlx5e_accel_ipsec_fs_cleanup(priv);
> > +	mlx5e_ipsec_fs_cleanup(ipsec);
> > 	destroy_workqueue(ipsec->wq);
> > 
> > 	kfree(ipsec);
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
> > index 66b529e36ea1..869b5692e9b9 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
> > @@ -632,81 +632,54 @@ void mlx5e_accel_ipsec_fs_del_rule(struct mlx5e_priv *priv,
> > 		tx_del_rule(priv, ipsec_rule);
> > }
> > 
> > -static void fs_cleanup_tx(struct mlx5e_priv *priv)
> > -{
> > -	mutex_destroy(&priv->ipsec->tx_fs->mutex);
> > -	WARN_ON(priv->ipsec->tx_fs->refcnt);
> > -	kfree(priv->ipsec->tx_fs);
> > -	priv->ipsec->tx_fs = NULL;
> > -}
> > -
> > -static void fs_cleanup_rx(struct mlx5e_priv *priv)
> > +void mlx5e_ipsec_fs_cleanup(struct mlx5e_ipsec *ipsec)
> > {
> > 	struct mlx5e_accel_fs_esp_prot *fs_prot;
> > 	struct mlx5e_accel_fs_esp *accel_esp;
> > 	enum accel_fs_esp_type i;
> > 
> > -	accel_esp = priv->ipsec->rx_fs;
> > +	if (!ipsec->rx_fs)
> > +		return;
> > +
> > +	mutex_destroy(&ipsec->tx_fs->mutex);
> > +	WARN_ON(ipsec->tx_fs->refcnt);
> > +	kfree(ipsec->tx_fs);
> > +
> > +	accel_esp = ipsec->rx_fs;
> > 	for (i = 0; i < ACCEL_FS_ESP_NUM_TYPES; i++) {
> > 		fs_prot = &accel_esp->fs_prot[i];
> > 		mutex_destroy(&fs_prot->prot_mutex);
> > 		WARN_ON(fs_prot->refcnt);
> > 	}
> > -	kfree(priv->ipsec->rx_fs);
> > -	priv->ipsec->rx_fs = NULL;
> > -}
> > -
> > -static int fs_init_tx(struct mlx5e_priv *priv)
> > -{
> > -	priv->ipsec->tx_fs =
> > -		kzalloc(sizeof(struct mlx5e_ipsec_tx), GFP_KERNEL);
> > -	if (!priv->ipsec->tx_fs)
> > -		return -ENOMEM;
> > -
> > -	mutex_init(&priv->ipsec->tx_fs->mutex);
> > -	return 0;
> > +	kfree(ipsec->rx_fs);
> > }
> > 
> > -static int fs_init_rx(struct mlx5e_priv *priv)
> > +int mlx5e_ipsec_fs_init(struct mlx5e_ipsec *ipsec)
> > {
> > 	struct mlx5e_accel_fs_esp_prot *fs_prot;
> > 	struct mlx5e_accel_fs_esp *accel_esp;
> > 	enum accel_fs_esp_type i;
> > +	int err = -ENOMEM;
> > 
> > -	priv->ipsec->rx_fs =
> > -		kzalloc(sizeof(struct mlx5e_accel_fs_esp), GFP_KERNEL);
> > -	if (!priv->ipsec->rx_fs)
> > +	ipsec->tx_fs = kzalloc(sizeof(*ipsec->tx_fs), GFP_KERNEL);
> > +	if (!ipsec->tx_fs)
> > 		return -ENOMEM;
> > 
> > -	accel_esp = priv->ipsec->rx_fs;
> > +	ipsec->rx_fs = kzalloc(sizeof(*ipsec->rx_fs), GFP_KERNEL);
> > +	if (!ipsec->rx_fs)
> > +		goto err_rx;
> > +
> > +	mutex_init(&ipsec->tx_fs->mutex);
> > +
> > +	accel_esp = ipsec->rx_fs;
> > 	for (i = 0; i < ACCEL_FS_ESP_NUM_TYPES; i++) {
> > 		fs_prot = &accel_esp->fs_prot[i];
> > 		mutex_init(&fs_prot->prot_mutex);
> > 	}
> > 
> > 	return 0;
> > -}
> > -
> > -void mlx5e_accel_ipsec_fs_cleanup(struct mlx5e_priv *priv)
> > -{
> > -	if (!priv->ipsec->rx_fs)
> > -		return;
> > -
> > -	fs_cleanup_tx(priv);
> > -	fs_cleanup_rx(priv);
> > -}
> > -
> > -int mlx5e_accel_ipsec_fs_init(struct mlx5e_priv *priv)
> > -{
> > -	int err;
> > -
> > -	err = fs_init_tx(priv);
> > -	if (err)
> > -		return err;
> > -
> > -	err = fs_init_rx(priv);
> > -	if (err)
> > -		fs_cleanup_tx(priv);
> > 
> > +err_rx:
> > +	kfree(ipsec->tx_fs);
> > 	return err;
> > }
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.h
> > index b70953979709..8e0e829ab58f 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.h
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.h
> > @@ -9,8 +9,8 @@
> > #include "ipsec_offload.h"
> > #include "en/fs.h"
> > 
> > -void mlx5e_accel_ipsec_fs_cleanup(struct mlx5e_priv *priv);
> > -int mlx5e_accel_ipsec_fs_init(struct mlx5e_priv *priv);
> > +void mlx5e_ipsec_fs_cleanup(struct mlx5e_ipsec *ipsec);
> > +int mlx5e_ipsec_fs_init(struct mlx5e_ipsec *ipsec);
> > int mlx5e_accel_ipsec_fs_add_rule(struct mlx5e_priv *priv,
> > 				  struct mlx5_accel_esp_xfrm_attrs *attrs,
> > 				  u32 ipsec_obj_id,
> > -- 
> > 2.35.1
> >