diff mbox series

[3/3] test/event_crypto_adapter: fix crypto null dev creation

Message ID 20230822131649.3509986-3-asasidharan@marvell.com (mailing list archive)
State Accepted, archived
Delegated to: Jerin Jacob
Headers
Series [1/3] event/cnxk: fix return values for capability API |

Checks

Context Check Description
ci/loongarch-compilation success Compilation OK
ci/checkpatch success coding style OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS

Commit Message

Aakash Sasidharan Aug. 22, 2023, 1:16 p.m. UTC 
  Create crypto null device only if no other crypto devices
were found.

Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
Cc: abhinandan.gujjar@intel.com

Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
---
 app/test/test_event_crypto_adapter.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Anoob Joseph Aug. 23, 2023, 4 a.m. UTC | #1 
> Create crypto null device only if no other crypto devices were found.
> 
> Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> Cc: abhinandan.gujjar@intel.com
> 
> Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> ---
>  app/test/test_event_crypto_adapter.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 

Acked-by: Anoob Joseph <anoobj@marvell.com>
Gujjar, Abhinandan S Aug. 23, 2023, 4:48 a.m. UTC | #2 
> -----Original Message-----
> From: Aakash Sasidharan <asasidharan@marvell.com>
> Sent: Tuesday, August 22, 2023 6:47 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Akhil Goyal
> <gakhil@marvell.com>
> Cc: jerinj@marvell.com; sthotton@marvell.com; anoobj@marvell.com;
> dev@dpdk.org; asasidharan@marvell.com
> Subject: [PATCH 3/3] test/event_crypto_adapter: fix crypto null dev creation
> 
> Create crypto null device only if no other crypto devices were found.
> 
> Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> Cc: abhinandan.gujjar@intel.com
> 
> Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> ---
>  app/test/test_event_crypto_adapter.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test/test_event_crypto_adapter.c
> b/app/test/test_event_crypto_adapter.c
> index 00b20fa443..0c56744ba0 100644
> --- a/app/test/test_event_crypto_adapter.c
> +++ b/app/test/test_event_crypto_adapter.c
> @@ -1072,11 +1072,10 @@ configure_cryptodev(void)
>  		return TEST_FAILED;
>  	}
> 
> -	/* Create a NULL crypto device */
> -	nb_devs = rte_cryptodev_device_count_by_driver(
> -			rte_cryptodev_driver_id_get(
> -			RTE_STR(CRYPTODEV_NAME_NULL_PMD)));
> +
> +	nb_devs = rte_cryptodev_count();
The test is intended to do on NULL cipher with NULL PMD. 
Right now, the test does cipher with " RTE_CRYPTO_CIPHER_NULL".
With this changes, does this test work, if you provide any other cryptodev like AESNI, QAT, OpenSSL other than NULL PMD?
>  	if (!nb_devs) {
> +		/* Create a NULL crypto device */
>  		ret = rte_vdev_init(
>  			RTE_STR(CRYPTODEV_NAME_NULL_PMD), NULL);
> 
> --
> 2.25.1
Anoob Joseph Aug. 23, 2023, 5:47 a.m. UTC | #3 
Hi Abhinandan,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> Sent: Wednesday, August 23, 2023 10:19 AM
> To: Aakash Sasidharan <asasidharan@marvell.com>; Akhil Goyal
> <gakhil@marvell.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Shijith Thotton
> <sthotton@marvell.com>; Anoob Joseph <anoobj@marvell.com>;
> dev@dpdk.org
> Subject: [EXT] RE: [PATCH 3/3] test/event_crypto_adapter: fix crypto null
> dev creation
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> 
> > -----Original Message-----
> > From: Aakash Sasidharan <asasidharan@marvell.com>
> > Sent: Tuesday, August 22, 2023 6:47 PM
> > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Akhil Goyal
> > <gakhil@marvell.com>
> > Cc: jerinj@marvell.com; sthotton@marvell.com; anoobj@marvell.com;
> > dev@dpdk.org; asasidharan@marvell.com
> > Subject: [PATCH 3/3] test/event_crypto_adapter: fix crypto null dev
> > creation
> >
> > Create crypto null device only if no other crypto devices were found.
> >
> > Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> > Cc: abhinandan.gujjar@intel.com
> >
> > Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> > ---
> >  app/test/test_event_crypto_adapter.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/app/test/test_event_crypto_adapter.c
> > b/app/test/test_event_crypto_adapter.c
> > index 00b20fa443..0c56744ba0 100644
> > --- a/app/test/test_event_crypto_adapter.c
> > +++ b/app/test/test_event_crypto_adapter.c
> > @@ -1072,11 +1072,10 @@ configure_cryptodev(void)
> >  		return TEST_FAILED;
> >  	}
> >
> > -	/* Create a NULL crypto device */
> > -	nb_devs = rte_cryptodev_device_count_by_driver(
> > -			rte_cryptodev_driver_id_get(
> > -			RTE_STR(CRYPTODEV_NAME_NULL_PMD)));
> > +
> > +	nb_devs = rte_cryptodev_count();
> The test is intended to do on NULL cipher with NULL PMD.

[Anoob] This test is intended to verify any cryptodev with crypto adapter. Why do we want to restrict the usage with NULL devices only?

> Right now, the test does cipher with " RTE_CRYPTO_CIPHER_NULL".

[Anoob] It has support for asymmetric cases also.

> With this changes, does this test work, if you provide any other cryptodev
> like AESNI, QAT, OpenSSL other than NULL PMD?

[Anoob] If NULL cipher is supported by those PMDs, it should be. Because, this being a standard test application should stick to standard semantics and so shouldn't care about whether the PMD under test is NULL or QAT. If NULL cipher is not supported by the PMD, then the case should get skipped at capability layer. Or are you saying that crypto adapter is only supposed to be used with NULL PMD?

Also, what we have seen is, if there are other cryptodevs (like cnxk), that would be the first cryptodev. And the test application is only testing CDEV_ID 0. So creating a NULL crypto PMD when other PMDs are available is pointless.

> >  	if (!nb_devs) {
> > +		/* Create a NULL crypto device */
> >  		ret = rte_vdev_init(
> >  			RTE_STR(CRYPTODEV_NAME_NULL_PMD), NULL);
> >
> > --
> > 2.25.1
Gujjar, Abhinandan S Aug. 23, 2023, 6:17 a.m. UTC | #4 
Hi Anoob,

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Wednesday, August 23, 2023 11:17 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Shijith Thotton
> <sthotton@marvell.com>; dev@dpdk.org; Aakash Sasidharan
> <asasidharan@marvell.com>; Akhil Goyal <gakhil@marvell.com>
> Subject: RE: [PATCH 3/3] test/event_crypto_adapter: fix crypto null dev
> creation
> 
> Hi Abhinandan,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> > Sent: Wednesday, August 23, 2023 10:19 AM
> > To: Aakash Sasidharan <asasidharan@marvell.com>; Akhil Goyal
> > <gakhil@marvell.com>
> > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Shijith Thotton
> > <sthotton@marvell.com>; Anoob Joseph <anoobj@marvell.com>;
> > dev@dpdk.org
> > Subject: [EXT] RE: [PATCH 3/3] test/event_crypto_adapter: fix crypto
> > null dev creation
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> >
> >
> > > -----Original Message-----
> > > From: Aakash Sasidharan <asasidharan@marvell.com>
> > > Sent: Tuesday, August 22, 2023 6:47 PM
> > > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Akhil Goyal
> > > <gakhil@marvell.com>
> > > Cc: jerinj@marvell.com; sthotton@marvell.com; anoobj@marvell.com;
> > > dev@dpdk.org; asasidharan@marvell.com
> > > Subject: [PATCH 3/3] test/event_crypto_adapter: fix crypto null dev
> > > creation
> > >
> > > Create crypto null device only if no other crypto devices were found.
> > >
> > > Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> > > Cc: abhinandan.gujjar@intel.com
> > >
> > > Signed-off-by: Aakash Sasidharan <asasidharan@marvell.com>
> > > ---
> > >  app/test/test_event_crypto_adapter.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/app/test/test_event_crypto_adapter.c
> > > b/app/test/test_event_crypto_adapter.c
> > > index 00b20fa443..0c56744ba0 100644
> > > --- a/app/test/test_event_crypto_adapter.c
> > > +++ b/app/test/test_event_crypto_adapter.c
> > > @@ -1072,11 +1072,10 @@ configure_cryptodev(void)
> > >  		return TEST_FAILED;
> > >  	}
> > >
> > > -	/* Create a NULL crypto device */
> > > -	nb_devs = rte_cryptodev_device_count_by_driver(
> > > -			rte_cryptodev_driver_id_get(
> > > -			RTE_STR(CRYPTODEV_NAME_NULL_PMD)));
> > > +
> > > +	nb_devs = rte_cryptodev_count();
> > The test is intended to do on NULL cipher with NULL PMD.
> 
> [Anoob] This test is intended to verify any cryptodev with crypto adapter.
> Why do we want to restrict the usage with NULL devices only?
No such restriction. Please refer below comment.
> 
> > Right now, the test does cipher with " RTE_CRYPTO_CIPHER_NULL".
> 
> [Anoob] It has support for asymmetric cases also.
Well, I was talking about cipher algo used in the test in the context of keeping it simple to verify the crypto adapter as it is agnostic to any underlying device.
Not sym vs asym support.
> 
> > With this changes, does this test work, if you provide any other
> > cryptodev like AESNI, QAT, OpenSSL other than NULL PMD?
> 
> [Anoob] If NULL cipher is supported by those PMDs, it should be. Because,
> this being a standard test application should stick to standard semantics and
> so shouldn't care about whether the PMD under test is NULL or QAT. If NULL
> cipher is not supported by the PMD, then the case should get skipped at
> capability layer. Or are you saying that crypto adapter is only supposed to be
> used with NULL PMD?
It is not about supporting other PMDs.
This is about keeping the test case simple with minimal conf overhead so that it is easy for testing and maintaining.
From the code, I see that few PMDs supports NULL too without much changes to the app. It should be ok.
> 
> Also, what we have seen is, if there are other cryptodevs (like cnxk), that
> would be the first cryptodev. And the test application is only testing CDEV_ID
> 0. So creating a NULL crypto PMD when other PMDs are available is pointless.
Sounds good.

Acked-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> 
> > >  	if (!nb_devs) {
> > > +		/* Create a NULL crypto device */
> > >  		ret = rte_vdev_init(
> > >  			RTE_STR(CRYPTODEV_NAME_NULL_PMD), NULL);
> > >
> > > --
> > > 2.25.1
Jerin Jacob Aug. 23, 2023, 2:20 p.m. UTC | #5 
On Wed, Aug 23, 2023 at 11:47 AM Gujjar, Abhinandan S
<abhinandan.gujjar@intel.com> wrote:
>

> It is not about supporting other PMDs.
> This is about keeping the test case simple with minimal conf overhead so that it is easy for testing and maintaining.
> From the code, I see that few PMDs supports NULL too without much changes to the app. It should be ok.
> >
> > Also, what we have seen is, if there are other cryptodevs (like cnxk), that
> > would be the first cryptodev. And the test application is only testing CDEV_ID
> > 0. So creating a NULL crypto PMD when other PMDs are available is pointless.
> Sounds good.
>
> Acked-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>

Series applied to dpdk-next-net-eventdev/for-main. Thanks
diff mbox series

Patch

diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 00b20fa443..0c56744ba0 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -1072,11 +1072,10 @@  configure_cryptodev(void)
 		return TEST_FAILED;
 	}
 
-	/* Create a NULL crypto device */
-	nb_devs = rte_cryptodev_device_count_by_driver(
-			rte_cryptodev_driver_id_get(
-			RTE_STR(CRYPTODEV_NAME_NULL_PMD)));
+
+	nb_devs = rte_cryptodev_count();
 	if (!nb_devs) {
+		/* Create a NULL crypto device */
 		ret = rte_vdev_init(
 			RTE_STR(CRYPTODEV_NAME_NULL_PMD), NULL);