diff mbox series

[v8,04/16] crypto/mlx5: add basic operations

Message ID 20210715164126.54073-5-shirik@nvidia.com (mailing list archive)
State Changes Requested, archived
Delegated to: akhil goyal
Headers
Series drivers: introduce mlx5 crypto PMD |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Shiri Kuzin July 15, 2021, 4:41 p.m. UTC 
  The basic dev control operations are configure, close and get info.

Extended the existing support of configure and close:
	-mlx5_crypto_dev_configure- function used to configure device.
	-mlx5_crypto_dev_close-  function used to close a configured
	 device.

Added config struct to user private data with the fields socket id,
number of queue pairs and feature flags to be disabled.

Signed-off-by: Shiri Kuzin <shirik@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 drivers/crypto/mlx5/mlx5_crypto.c | 26 +++++++++++++++++++-------
 drivers/crypto/mlx5/mlx5_crypto.h |  1 +
 2 files changed, 20 insertions(+), 7 deletions(-)

Comments

Akhil Goyal July 16, 2021, 7:34 p.m. UTC | #1 
> The basic dev control operations are configure, close and get info.
> 
> Extended the existing support of configure and close:
> 	-mlx5_crypto_dev_configure- function used to configure device.
> 	-mlx5_crypto_dev_close-  function used to close a configured
> 	 device.
> 
> Added config struct to user private data with the fields socket id,
> number of queue pairs and feature flags to be disabled.
> 
> Signed-off-by: Shiri Kuzin <shirik@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
>  drivers/crypto/mlx5/mlx5_crypto.c | 26 +++++++++++++++++++-------
>  drivers/crypto/mlx5/mlx5_crypto.h |  1 +
>  2 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/crypto/mlx5/mlx5_crypto.c
> b/drivers/crypto/mlx5/mlx5_crypto.c
> index 3f0c97d081..a7e44deb9e 100644
> --- a/drivers/crypto/mlx5/mlx5_crypto.c
> +++ b/drivers/crypto/mlx5/mlx5_crypto.c
> @@ -105,22 +105,27 @@ mlx5_crypto_dev_infos_get(struct rte_cryptodev
> *dev,
>  	}
>  }
> 
> -static unsigned int
> -mlx5_crypto_sym_session_get_size(struct rte_cryptodev *dev
> __rte_unused)
> -{
> -	return sizeof(struct mlx5_crypto_session);
> -}
> -

I do not get the reason to remove above function, it was introduced in the previous patch.
It looks the patches are not properly split.

>  static int
>  mlx5_crypto_dev_configure(struct rte_cryptodev *dev,
> -		struct rte_cryptodev_config *config __rte_unused)
> +			  struct rte_cryptodev_config *config)
>  {
>  	struct mlx5_crypto_priv *priv = dev->data->dev_private;
> 
> +	if (config == NULL) {
> +		DRV_LOG(ERR, "Invalid crypto dev configure parameters.");
> +		return -EINVAL;
> +	}
> +	if ((config->ff_disable & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO)
> != 0) {
> +		DRV_LOG(ERR,
> +			"Disabled symmetric crypto feature is not
> supported.");
> +		return -ENOTSUP;
> +	}
>  	if (mlx5_crypto_dek_setup(priv) != 0) {
>  		DRV_LOG(ERR, "Dek hash list creation has failed.");
>  		return -ENOMEM;
>  	}
> +	priv->dev_config = *config;
> +	DRV_LOG(DEBUG, "Device %u was configured.", dev->driver_id);
>  	return 0;
>  }

The patch title and the patch do not match.
Title says, add basic operations, which should introduce the
Configure and close ops. But here the configure and close ops
Were already there and you are introducing some new checks
In them.

> 
> @@ -130,9 +135,16 @@ mlx5_crypto_dev_close(struct rte_cryptodev *dev)
>  	struct mlx5_crypto_priv *priv = dev->data->dev_private;
> 
>  	mlx5_crypto_dek_unset(priv);
> +	DRV_LOG(DEBUG, "Device %u was closed.", dev->driver_id);
>  	return 0;
>  }
Logging could have been added in the patch where dev_close was added.

> 
> +static unsigned int
> +mlx5_crypto_sym_session_get_size(struct rte_cryptodev *dev
> __rte_unused)
> +{
> +	return sizeof(struct mlx5_crypto_session);
> +}
> +
>  static int
>  mlx5_crypto_sym_session_configure(struct rte_cryptodev *dev,
>  				  struct rte_crypto_sym_xform *xform,
> diff --git a/drivers/crypto/mlx5/mlx5_crypto.h
> b/drivers/crypto/mlx5/mlx5_crypto.h
> index 167e9e57ad..a0df775407 100644
> --- a/drivers/crypto/mlx5/mlx5_crypto.h
> +++ b/drivers/crypto/mlx5/mlx5_crypto.h
> @@ -24,6 +24,7 @@ struct mlx5_crypto_priv {
>  	uint32_t pdn; /* Protection Domain number. */
>  	struct ibv_pd *pd;
>  	struct mlx5_hlist *dek_hlist; /* Dek hash list. */
> +	struct rte_cryptodev_config dev_config;
>  };
> 
>  struct mlx5_crypto_dek {
> --
> 2.27.0
Suanming Mou July 20, 2021, 8:33 a.m. UTC | #2 
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Saturday, July 17, 2021 3:34 AM
> To: Shiri Kuzin <shirik@nvidia.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>; david.marchand@redhat.com
> Subject: RE: [EXT] [PATCH v8 04/16] crypto/mlx5: add basic operations
> 
> > The basic dev control operations are configure, close and get info.
> >
> > Extended the existing support of configure and close:
> > 	-mlx5_crypto_dev_configure- function used to configure device.
> > 	-mlx5_crypto_dev_close-  function used to close a configured
> > 	 device.
> >
> > Added config struct to user private data with the fields socket id,
> > number of queue pairs and feature flags to be disabled.
> >
> > Signed-off-by: Shiri Kuzin <shirik@nvidia.com>
> > Acked-by: Matan Azrad <matan@nvidia.com>
> > ---
> >  drivers/crypto/mlx5/mlx5_crypto.c | 26 +++++++++++++++++++-------
> > drivers/crypto/mlx5/mlx5_crypto.h |  1 +
> >  2 files changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/crypto/mlx5/mlx5_crypto.c
> > b/drivers/crypto/mlx5/mlx5_crypto.c
> > index 3f0c97d081..a7e44deb9e 100644
> > --- a/drivers/crypto/mlx5/mlx5_crypto.c
> > +++ b/drivers/crypto/mlx5/mlx5_crypto.c
> > @@ -105,22 +105,27 @@ mlx5_crypto_dev_infos_get(struct rte_cryptodev
> > *dev,
> >  	}
> >  }
> >
> > -static unsigned int
> > -mlx5_crypto_sym_session_get_size(struct rte_cryptodev *dev
> > __rte_unused)
> > -{
> > -	return sizeof(struct mlx5_crypto_session);
> > -}
> > -
> 
> I do not get the reason to remove above function, it was introduced in the
> previous patch.
> It looks the patches are not properly split.

We are not removing the function here, in fact at the last of this patch, you can say it is still there.
It's just some code movement.

> 
> >  static int
> >  mlx5_crypto_dev_configure(struct rte_cryptodev *dev,
> > -		struct rte_cryptodev_config *config __rte_unused)
> > +			  struct rte_cryptodev_config *config)
> >  {
> >  	struct mlx5_crypto_priv *priv = dev->data->dev_private;
> >
> > +	if (config == NULL) {
> > +		DRV_LOG(ERR, "Invalid crypto dev configure parameters.");
> > +		return -EINVAL;
> > +	}
> > +	if ((config->ff_disable & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO)
> > != 0) {
> > +		DRV_LOG(ERR,
> > +			"Disabled symmetric crypto feature is not
> > supported.");
> > +		return -ENOTSUP;
> > +	}
> >  	if (mlx5_crypto_dek_setup(priv) != 0) {
> >  		DRV_LOG(ERR, "Dek hash list creation has failed.");
> >  		return -ENOMEM;
> >  	}
> > +	priv->dev_config = *config;
> > +	DRV_LOG(DEBUG, "Device %u was configured.", dev->driver_id);
> >  	return 0;
> >  }
> 
> The patch title and the patch do not match.
> Title says, add basic operations, which should introduce the Configure and close
> ops. But here the configure and close ops Were already there and you are
> introducing some new checks In them.

Good suggestion, will try to implement the configure and close dev in this patch.

> 
> >
> > @@ -130,9 +135,16 @@ mlx5_crypto_dev_close(struct rte_cryptodev *dev)
> >  	struct mlx5_crypto_priv *priv = dev->data->dev_private;
> >
> >  	mlx5_crypto_dek_unset(priv);
> > +	DRV_LOG(DEBUG, "Device %u was closed.", dev->driver_id);
> >  	return 0;
> >  }
> Logging could have been added in the patch where dev_close was added.
> 
> >
> > +static unsigned int
> > +mlx5_crypto_sym_session_get_size(struct rte_cryptodev *dev
> > __rte_unused)
> > +{
> > +	return sizeof(struct mlx5_crypto_session); }
> > +
> >  static int
> >  mlx5_crypto_sym_session_configure(struct rte_cryptodev *dev,
> >  				  struct rte_crypto_sym_xform *xform, diff --git
> > a/drivers/crypto/mlx5/mlx5_crypto.h
> > b/drivers/crypto/mlx5/mlx5_crypto.h
> > index 167e9e57ad..a0df775407 100644
> > --- a/drivers/crypto/mlx5/mlx5_crypto.h
> > +++ b/drivers/crypto/mlx5/mlx5_crypto.h
> > @@ -24,6 +24,7 @@ struct mlx5_crypto_priv {
> >  	uint32_t pdn; /* Protection Domain number. */
> >  	struct ibv_pd *pd;
> >  	struct mlx5_hlist *dek_hlist; /* Dek hash list. */
> > +	struct rte_cryptodev_config dev_config;
> >  };
> >
> >  struct mlx5_crypto_dek {
> > --
> > 2.27.0
diff mbox series

Patch

diff --git a/drivers/crypto/mlx5/mlx5_crypto.c b/drivers/crypto/mlx5/mlx5_crypto.c
index 3f0c97d081..a7e44deb9e 100644
--- a/drivers/crypto/mlx5/mlx5_crypto.c
+++ b/drivers/crypto/mlx5/mlx5_crypto.c
@@ -105,22 +105,27 @@  mlx5_crypto_dev_infos_get(struct rte_cryptodev *dev,
 	}
 }
 
-static unsigned int
-mlx5_crypto_sym_session_get_size(struct rte_cryptodev *dev __rte_unused)
-{
-	return sizeof(struct mlx5_crypto_session);
-}
-
 static int
 mlx5_crypto_dev_configure(struct rte_cryptodev *dev,
-		struct rte_cryptodev_config *config __rte_unused)
+			  struct rte_cryptodev_config *config)
 {
 	struct mlx5_crypto_priv *priv = dev->data->dev_private;
 
+	if (config == NULL) {
+		DRV_LOG(ERR, "Invalid crypto dev configure parameters.");
+		return -EINVAL;
+	}
+	if ((config->ff_disable & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO) != 0) {
+		DRV_LOG(ERR,
+			"Disabled symmetric crypto feature is not supported.");
+		return -ENOTSUP;
+	}
 	if (mlx5_crypto_dek_setup(priv) != 0) {
 		DRV_LOG(ERR, "Dek hash list creation has failed.");
 		return -ENOMEM;
 	}
+	priv->dev_config = *config;
+	DRV_LOG(DEBUG, "Device %u was configured.", dev->driver_id);
 	return 0;
 }
 
@@ -130,9 +135,16 @@  mlx5_crypto_dev_close(struct rte_cryptodev *dev)
 	struct mlx5_crypto_priv *priv = dev->data->dev_private;
 
 	mlx5_crypto_dek_unset(priv);
+	DRV_LOG(DEBUG, "Device %u was closed.", dev->driver_id);
 	return 0;
 }
 
+static unsigned int
+mlx5_crypto_sym_session_get_size(struct rte_cryptodev *dev __rte_unused)
+{
+	return sizeof(struct mlx5_crypto_session);
+}
+
 static int
 mlx5_crypto_sym_session_configure(struct rte_cryptodev *dev,
 				  struct rte_crypto_sym_xform *xform,
diff --git a/drivers/crypto/mlx5/mlx5_crypto.h b/drivers/crypto/mlx5/mlx5_crypto.h
index 167e9e57ad..a0df775407 100644
--- a/drivers/crypto/mlx5/mlx5_crypto.h
+++ b/drivers/crypto/mlx5/mlx5_crypto.h
@@ -24,6 +24,7 @@  struct mlx5_crypto_priv {
 	uint32_t pdn; /* Protection Domain number. */
 	struct ibv_pd *pd;
 	struct mlx5_hlist *dek_hlist; /* Dek hash list. */
+	struct rte_cryptodev_config dev_config;
 };
 
 struct mlx5_crypto_dek {