Re: [PATCH net-next v1 10/17] net/mlx5: Clean IPsec FS add/delete rules

[Saeed Mahameed <saeedm@nvidia.com>]

On 19 Apr 13:13, Leon Romanovsky wrote:
>From: Leon Romanovsky <leonro@nvidia.com>
>
>Reuse existing struct to pass parameters instead of open code them.
>

Why? what do you mean "open code them" ? they are not open coded, they are
primitive for a reason ! If we go with this reasoning, then let's pass
mlx5e_priv to all functions and just forget about modularity.

>Reviewed-by: Raed Salem <raeds@nvidia.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
> .../mellanox/mlx5/core/en_accel/ipsec.c       | 10 +---
> .../mellanox/mlx5/core/en_accel/ipsec.h       |  7 +--
> .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 55 ++++++++++---------
> 3 files changed, 34 insertions(+), 38 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 537311a74bfb..81c9831ad286 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
>@@ -313,9 +313,7 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x)
> 	if (err)
> 		goto err_xfrm;
>
>-	err = mlx5e_accel_ipsec_fs_add_rule(priv, &sa_entry->attrs,
>-					    sa_entry->ipsec_obj_id,
>-					    &sa_entry->ipsec_rule);
>+	err = mlx5e_accel_ipsec_fs_add_rule(priv, sa_entry);

To add to my comment on the previous patch, in here the issue is more
severe as previously ipsec_fs.c was unaware of sa_entry object and used to
deal with pure fs related objects, you are peppering the code with sa_entry for
no reason, other than reducing function parameters from 4 to 2.
  
> 	if (err)
> 		goto err_hw_ctx;
>
>@@ -333,8 +331,7 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x)
> 	goto out;
>
> err_add_rule:
>-	mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->attrs,
>-				      &sa_entry->ipsec_rule);
>+	mlx5e_accel_ipsec_fs_del_rule(priv, sa_entry);
> err_hw_ctx:
> 	mlx5_ipsec_free_sa_ctx(sa_entry);
> err_xfrm:
>@@ -357,8 +354,7 @@ static void mlx5e_xfrm_free_state(struct xfrm_state *x)
> 	struct mlx5e_priv *priv = netdev_priv(x->xso.dev);
>
> 	cancel_work_sync(&sa_entry->modify_work.work);
>-	mlx5e_accel_ipsec_fs_del_rule(priv, &sa_entry->attrs,
>-				      &sa_entry->ipsec_rule);
>+	mlx5e_accel_ipsec_fs_del_rule(priv, sa_entry);
> 	mlx5_ipsec_free_sa_ctx(sa_entry);
> 	kfree(sa_entry);
> }
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
>index cdcb95f90623..af1467cbb7c7 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h
>@@ -176,12 +176,9 @@ struct xfrm_state *mlx5e_ipsec_sadb_rx_lookup(struct mlx5e_ipsec *dev,
> void mlx5e_accel_ipsec_fs_cleanup(struct mlx5e_ipsec *ipsec);
> int mlx5e_accel_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,
>-				  struct mlx5e_ipsec_rule *ipsec_rule);
>+				  struct mlx5e_ipsec_sa_entry *sa_entry);
> void mlx5e_accel_ipsec_fs_del_rule(struct mlx5e_priv *priv,
>-				   struct mlx5_accel_esp_xfrm_attrs *attrs,
>-				   struct mlx5e_ipsec_rule *ipsec_rule);
>+				   struct mlx5e_ipsec_sa_entry *sa_entry);
>
> int mlx5_ipsec_create_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry);
> void mlx5_ipsec_free_sa_ctx(struct mlx5e_ipsec_sa_entry *sa_entry);
>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 96ab2e9d6f9a..342828351254 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
>@@ -454,11 +454,12 @@ static void setup_fte_common(struct mlx5_accel_esp_xfrm_attrs *attrs,
> }
>
> static int rx_add_rule(struct mlx5e_priv *priv,
>-		       struct mlx5_accel_esp_xfrm_attrs *attrs,
>-		       u32 ipsec_obj_id,
>-		       struct mlx5e_ipsec_rule *ipsec_rule)
>+		       struct mlx5e_ipsec_sa_entry *sa_entry)
> {
> 	u8 action[MLX5_UN_SZ_BYTES(set_add_copy_action_in_auto)] = {};
>+	struct mlx5e_ipsec_rule *ipsec_rule = &sa_entry->ipsec_rule;
>+	struct mlx5_accel_esp_xfrm_attrs *attrs = &sa_entry->attrs;
>+	u32 ipsec_obj_id = sa_entry->ipsec_obj_id;
> 	struct mlx5_modify_hdr *modify_hdr = NULL;
> 	struct mlx5e_accel_fs_esp_prot *fs_prot;
> 	struct mlx5_flow_destination dest = {};
>@@ -532,9 +533,7 @@ static int rx_add_rule(struct mlx5e_priv *priv,
> }
>
> static int tx_add_rule(struct mlx5e_priv *priv,
>-		       struct mlx5_accel_esp_xfrm_attrs *attrs,
>-		       u32 ipsec_obj_id,
>-		       struct mlx5e_ipsec_rule *ipsec_rule)
>+		       struct mlx5e_ipsec_sa_entry *sa_entry)
> {
> 	struct mlx5_flow_act flow_act = {};
> 	struct mlx5_flow_handle *rule;
>@@ -551,7 +550,8 @@ static int tx_add_rule(struct mlx5e_priv *priv,
> 		goto out;
> 	}
>
>-	setup_fte_common(attrs, ipsec_obj_id, spec, &flow_act);
>+	setup_fte_common(&sa_entry->attrs, sa_entry->ipsec_obj_id, spec,
>+			 &flow_act);
>
> 	/* Add IPsec indicator in metadata_reg_a */
> 	spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_2;
>@@ -566,11 +566,11 @@ static int tx_add_rule(struct mlx5e_priv *priv,
> 	if (IS_ERR(rule)) {
> 		err = PTR_ERR(rule);
> 		netdev_err(priv->netdev, "fail to add ipsec rule attrs->action=0x%x, err=%d\n",
>-				attrs->action, err);
>+				sa_entry->attrs.action, err);
> 		goto out;
> 	}
>
>-	ipsec_rule->rule = rule;
>+	sa_entry->ipsec_rule.rule = rule;
>
> out:
> 	kvfree(spec);
>@@ -580,21 +580,25 @@ static int tx_add_rule(struct mlx5e_priv *priv,
> }
>
> static void rx_del_rule(struct mlx5e_priv *priv,
>-		struct mlx5_accel_esp_xfrm_attrs *attrs,
>-		struct mlx5e_ipsec_rule *ipsec_rule)
>+			struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>+	struct mlx5e_ipsec_rule *ipsec_rule = &sa_entry->ipsec_rule;
>+
> 	mlx5_del_flow_rules(ipsec_rule->rule);
> 	ipsec_rule->rule = NULL;
>
> 	mlx5_modify_header_dealloc(priv->mdev, ipsec_rule->set_modify_hdr);
> 	ipsec_rule->set_modify_hdr = NULL;
>
>-	rx_ft_put(priv, attrs->is_ipv6 ? ACCEL_FS_ESP6 : ACCEL_FS_ESP4);
>+	rx_ft_put(priv,
>+		  sa_entry->attrs.is_ipv6 ? ACCEL_FS_ESP6 : ACCEL_FS_ESP4);
> }
>
> static void tx_del_rule(struct mlx5e_priv *priv,
>-		struct mlx5e_ipsec_rule *ipsec_rule)
>+			struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>+	struct mlx5e_ipsec_rule *ipsec_rule = &sa_entry->ipsec_rule;
>+
> 	mlx5_del_flow_rules(ipsec_rule->rule);
> 	ipsec_rule->rule = NULL;
>
>@@ -602,24 +606,23 @@ static void tx_del_rule(struct mlx5e_priv *priv,
> }
>
> int mlx5e_accel_ipsec_fs_add_rule(struct mlx5e_priv *priv,
>-				  struct mlx5_accel_esp_xfrm_attrs *attrs,
>-				  u32 ipsec_obj_id,
>-				  struct mlx5e_ipsec_rule *ipsec_rule)
>+				  struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>-	if (attrs->action == MLX5_ACCEL_ESP_ACTION_DECRYPT)
>-		return rx_add_rule(priv, attrs, ipsec_obj_id, ipsec_rule);
>-	else
>-		return tx_add_rule(priv, attrs, ipsec_obj_id, ipsec_rule);
>+	if (sa_entry->attrs.action == MLX5_ACCEL_ESP_ACTION_ENCRYPT)
>+		return tx_add_rule(priv, sa_entry);
>+
>+	return rx_add_rule(priv, sa_entry);
> }
>
> void mlx5e_accel_ipsec_fs_del_rule(struct mlx5e_priv *priv,
>-		struct mlx5_accel_esp_xfrm_attrs *attrs,
>-		struct mlx5e_ipsec_rule *ipsec_rule)
>+				   struct mlx5e_ipsec_sa_entry *sa_entry)
> {
>-	if (attrs->action == MLX5_ACCEL_ESP_ACTION_DECRYPT)
>-		rx_del_rule(priv, attrs, ipsec_rule);
>-	else
>-		tx_del_rule(priv, ipsec_rule);
>+	if (sa_entry->attrs.action == MLX5_ACCEL_ESP_ACTION_ENCRYPT) {
>+		tx_del_rule(priv, sa_entry);
>+		return;
>+	}
>+
>+	rx_del_rule(priv, sa_entry);
> }
>
> void mlx5e_accel_ipsec_fs_cleanup(struct mlx5e_ipsec *ipsec)
>-- 
>2.35.1
>