[dpdk-dev] [PATCH v2] Test/crypto: check valid test_stats before running test

[dpdk-dev] [PATCH v2] Test/crypto: check valid test_stats before running test

[Apeksha Gupta]

Test_stats is an optional cryptodev op and if it is not defined by the
PMD, it should not run the test cases for it.

Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
---
 app/test/test_cryptodev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index c624018ee..1ad650675 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -8801,6 +8801,10 @@ test_stats(void)
 	if (gbl_action_type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)
 		return -ENOTSUP;
 
+	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
+	if (dev->dev_ops->stats_get == 0)
+		return -ENOTSUP;
+
 	/* Verify the capabilities */
 	struct rte_cryptodev_sym_capability_idx cap_idx;
 	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH;
@@ -8820,7 +8824,6 @@ test_stats(void)
 		"rte_cryptodev_stats_get invalid dev failed");
 	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], 0) != 0),
 		"rte_cryptodev_stats_get invalid Param failed");
-	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
 	temp_pfn = dev->dev_ops->stats_get;
 	dev->dev_ops->stats_get = (cryptodev_stats_get_t)0;
 	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], &stats)
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] Test/crypto: check valid test_stats before running test

[Ruifeng Wang]

> -----Original Message-----
> From: Apeksha Gupta <apeksha.gupta@nxp.com>
> Sent: Friday, May 15, 2020 3:33 PM
> To: dev@dpdk.org
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; declan.doherty@intel.com;
> asomalap@amd.com; anoobj@marvell.com; roy.fan.zhang@intel.com;
> fiona.trahe@intel.com; rnagadheeraj@marvell.com; adwivedi@marvell.com;
> jianjay.zhou@huawei.com; pablo.de.lara.guarch@intel.com;
> adamx.dybkowski@intel.com; Akhil.goyal@nxp.com; Apeksha Gupta
> <apeksha.gupta@nxp.com>
> Subject: [PATCH v2] Test/crypto: check valid test_stats before running test
>
> Test_stats is an optional cryptodev op and if it is not defined by the PMD, it
> should not run the test cases for it.
>
> Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> ---
>  app/test/test_cryptodev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
> c624018ee..1ad650675 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -8801,6 +8801,10 @@ test_stats(void)
>  if (gbl_action_type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)
>  return -ENOTSUP;
>
> +dev = &rte_cryptodevs[ts_params->valid_devs[0]];
> +if (dev->dev_ops->stats_get == 0)
> +return -ENOTSUP;
> +
>  /* Verify the capabilities */
>  struct rte_cryptodev_sym_capability_idx cap_idx;
>  cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH; @@ -8820,7
> +8824,6 @@ test_stats(void)
>  "rte_cryptodev_stats_get invalid dev failed");
>  TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0],
> 0) != 0),
>  "rte_cryptodev_stats_get invalid Param failed");
> -dev = &rte_cryptodevs[ts_params->valid_devs[0]];
>  temp_pfn = dev->dev_ops->stats_get;
>  dev->dev_ops->stats_get = (cryptodev_stats_get_t)0;
>  TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0],
> &stats)
> --
> 2.17.1
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[dpdk-dev] [PATCH v3] Test/crypto: check valid test_stats before running test

[Apeksha Gupta]

Test_stats is an optional cryptodev op and if it is not defined by the
PMD, it should not run the test cases for it.

Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
---
 app/test/test_cryptodev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index c624018ee..1ad650675 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -8801,6 +8801,10 @@ test_stats(void)
 	if (gbl_action_type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)
 		return -ENOTSUP;
 
+	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
+	if (dev->dev_ops->stats_get == 0)
+		return -ENOTSUP;
+
 	/* Verify the capabilities */
 	struct rte_cryptodev_sym_capability_idx cap_idx;
 	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH;
@@ -8820,7 +8824,6 @@ test_stats(void)
 		"rte_cryptodev_stats_get invalid dev failed");
 	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], 0) != 0),
 		"rte_cryptodev_stats_get invalid Param failed");
-	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
 	temp_pfn = dev->dev_ops->stats_get;
 	dev->dev_ops->stats_get = (cryptodev_stats_get_t)0;
 	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], &stats)
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3] Test/crypto: check valid test_stats before running test

[Ruifeng Wang]

> -----Original Message-----
> From: Apeksha Gupta <apeksha.gupta@nxp.com>
> Sent: Friday, May 15, 2020 3:56 PM
> To: dev@dpdk.org
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; declan.doherty@intel.com;
> asomalap@amd.com; anoobj@marvell.com; roy.fan.zhang@intel.com;
> fiona.trahe@intel.com; rnagadheeraj@marvell.com; adwivedi@marvell.com;
> jianjay.zhou@huawei.com; pablo.de.lara.guarch@intel.com;
> adamx.dybkowski@intel.com; Akhil.goyal@nxp.com; Apeksha Gupta
> <apeksha.gupta@nxp.com>
> Subject: [PATCH v3] Test/crypto: check valid test_stats before running test
> 
> Test_stats is an optional cryptodev op and if it is not defined by the PMD, it
> should not run the test cases for it.
> 
> Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> ---
>  app/test/test_cryptodev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
> c624018ee..1ad650675 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -8801,6 +8801,10 @@ test_stats(void)
>  	if (gbl_action_type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)
>  		return -ENOTSUP;
> 
> +	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
> +	if (dev->dev_ops->stats_get == 0)
> +		return -ENOTSUP;
> +
>  	/* Verify the capabilities */
>  	struct rte_cryptodev_sym_capability_idx cap_idx;
>  	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH; @@ -8820,7
> +8824,6 @@ test_stats(void)
>  		"rte_cryptodev_stats_get invalid dev failed");
>  	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0],
> 0) != 0),
>  		"rte_cryptodev_stats_get invalid Param failed");
> -	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
>  	temp_pfn = dev->dev_ops->stats_get;
>  	dev->dev_ops->stats_get = (cryptodev_stats_get_t)0;
>  	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0],
> &stats)
> --
> 2.17.1
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Re: [dpdk-dev] [PATCH v3] Test/crypto: check valid test_stats before running test

[Ananyev, Konstantin]

> Test_stats is an optional cryptodev op and if it is not defined by the
> PMD, it should not run the test cases for it.
> 
> Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> ---
>  app/test/test_cryptodev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> index c624018ee..1ad650675 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -8801,6 +8801,10 @@ test_stats(void)
>  	if (gbl_action_type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)
>  		return -ENOTSUP;
> 
> +	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
> +	if (dev->dev_ops->stats_get == 0)
> +		return -ENOTSUP;
> +
>  

rte_cryptodevs[] and dev_ops supposed to be internal for cryptodev
(though in practice they are not).
It is not a good practice for the app to access them directly.


	/* Verify the capabilities */
>  	struct rte_cryptodev_sym_capability_idx cap_idx;
>  	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH;
> @@ -8820,7 +8824,6 @@ test_stats(void)
>  		"rte_cryptodev_stats_get invalid dev failed");
>  	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], 0) != 0),
>  		"rte_cryptodev_stats_get invalid Param failed");
> -	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
>  	temp_pfn = dev->dev_ops->stats_get;
>  	dev->dev_ops->stats_get = (cryptodev_stats_get_t)0;
>  	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], &stats)
> --
> 2.17.1

Re: [dpdk-dev] [PATCH v3] Test/crypto: check valid test_stats before running test

[Akhil Goyal]

Hi Konstantin,
> 
> > Test_stats is an optional cryptodev op and if it is not defined by the
> > PMD, it should not run the test cases for it.
> >
> > Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> > ---
> >  app/test/test_cryptodev.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > index c624018ee..1ad650675 100644
> > --- a/app/test/test_cryptodev.c
> > +++ b/app/test/test_cryptodev.c
> > @@ -8801,6 +8801,10 @@ test_stats(void)
> >  	if (gbl_action_type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)
> >  		return -ENOTSUP;
> >
> > +	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
> > +	if (dev->dev_ops->stats_get == 0)
> > +		return -ENOTSUP;
> > +
> >
> 
> rte_cryptodevs[] and dev_ops supposed to be internal for cryptodev
> (though in practice they are not).
> It is not a good practice for the app to access them directly.
> 

Yes, you are absolutely correct. The test case is not properly written.
App should not access the dev_ops directly.

Apeksha,
You can do the following change to fix this.

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 1ed2df898..aa14d42fa 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -8751,7 +8751,6 @@ test_stats(void)
        struct crypto_testsuite_params *ts_params = &testsuite_params;
        struct rte_cryptodev_stats stats;
        struct rte_cryptodev *dev;
-       cryptodev_stats_get_t temp_pfn;

        if (gbl_action_type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)
                return -ENOTSUP;
@@ -8769,19 +8768,16 @@ test_stats(void)
                        &cap_idx) == NULL)
                return -ENOTSUP;

+       if (rte_cryptodev_stats_get(ts_params->valid_devs[0], &stats)
+                       == -ENOTSUP)
+               return -ENOTSUP;
+
        rte_cryptodev_stats_reset(ts_params->valid_devs[0]);
        TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0] + 600,
                        &stats) == -ENODEV),
                "rte_cryptodev_stats_get invalid dev failed");
        TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], 0) != 0),
                "rte_cryptodev_stats_get invalid Param failed");
-       dev = &rte_cryptodevs[ts_params->valid_devs[0]];
-       temp_pfn = dev->dev_ops->stats_get;
-       dev->dev_ops->stats_get = (cryptodev_stats_get_t)0;
-       TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], &stats)
-                       == -ENOTSUP),
-               "rte_cryptodev_stats_get invalid Param failed");
-       dev->dev_ops->stats_get = temp_pfn;

        /* Test expected values */
        ut_setup();

[dpdk-dev] [PATCH v4] Test/crypto: check valid test_stats before running test

[Apeksha Gupta]

Test_stats is an optional cryptodev op and if it is not defined by the
PMD, it should not run the test cases for it.

Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
---
 app/test/test_cryptodev.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index c624018ee..83fee232d 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -8795,8 +8795,6 @@ test_stats(void)
 {
 	struct crypto_testsuite_params *ts_params = &testsuite_params;
 	struct rte_cryptodev_stats stats;
-	struct rte_cryptodev *dev;
-	cryptodev_stats_get_t temp_pfn;
 
 	if (gbl_action_type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO)
 		return -ENOTSUP;
@@ -8813,6 +8811,9 @@ test_stats(void)
 	if (rte_cryptodev_sym_capability_get(ts_params->valid_devs[0],
 			&cap_idx) == NULL)
 		return -ENOTSUP;
+	if (rte_cryptodev_stats_get(ts_params->valid_devs[0], &stats)
+			== -ENOTSUP)
+		return -ENOTSUP;
 
 	rte_cryptodev_stats_reset(ts_params->valid_devs[0]);
 	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0] + 600,
@@ -8820,13 +8821,6 @@ test_stats(void)
 		"rte_cryptodev_stats_get invalid dev failed");
 	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], 0) != 0),
 		"rte_cryptodev_stats_get invalid Param failed");
-	dev = &rte_cryptodevs[ts_params->valid_devs[0]];
-	temp_pfn = dev->dev_ops->stats_get;
-	dev->dev_ops->stats_get = (cryptodev_stats_get_t)0;
-	TEST_ASSERT((rte_cryptodev_stats_get(ts_params->valid_devs[0], &stats)
-			== -ENOTSUP),
-		"rte_cryptodev_stats_get invalid Param failed");
-	dev->dev_ops->stats_get = temp_pfn;
 
 	/* Test expected values */
 	ut_setup();
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4] Test/crypto: check valid test_stats before running test

[Akhil Goyal]

> Test_stats is an optional cryptodev op and if it is not defined by the
> PMD, it should not run the test cases for it.
> 
> Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> ---

Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Title and description of the patch updated as
    test/crypto: fix statistics case

    The test case - test_stats is directly accessing the
    cryptodev and its dev_ops which are internal to library
    and should not be used directly by the application.
    However, the test case is also missing to check for the
    error ENOTSUP. It should skip the case if the API returns
    ENOTSUP. This patch fixes these two issues.

    Fixes: 202d375c60bc ("app/test: add cryptodev unit and performance tests")
    Cc: stable@dpdk.org

    Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>

Applied to dpdk-next-crypto.

Thanks.