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
> -----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.
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
> -----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>
> 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
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();
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
> 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.