Hi Konstantin <snip>
> > Subject: [PATCH] test/ipsec: fix test suite setup function > > > > Check for valid crypto_null devices before continuing. > > > > Fixes: 05fe65eb66b2 ("test/ipsec: introduce functional test") > > Signed-off-by: Bernard Iremonger <bernard.iremon...@intel.com> > > --- > > test/test/test_ipsec.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/test/test/test_ipsec.c b/test/test/test_ipsec.c index > > ff1a1c4..4dfc55b 100644 > > --- a/test/test/test_ipsec.c > > +++ b/test/test/test_ipsec.c > > @@ -46,6 +46,8 @@ > > #define BURST_SIZE 32 > > #define REORDER_PKTS 1 > > > > +static int gbl_driver_id; > > + > > Why do you need that global here? test_ipsec.c is based on test_cryptodev.c. gbl_driver_id used to store the ID of the required driver. > > > struct user_params { > > enum rte_crypto_sym_xform_type auth; > > enum rte_crypto_sym_xform_type cipher; @@ -218,7 +220,7 @@ > > testsuite_setup(void) { > > struct ipsec_testsuite_params *ts_params = &testsuite_params; > > struct rte_cryptodev_info info; > > - uint32_t nb_devs, dev_id; > > + uint32_t i, nb_devs, dev_id; > > size_t sess_sz; > > > > memset(ts_params, 0, sizeof(*ts_params)); @@ -251,7 +253,18 @@ > > testsuite_setup(void) > > return TEST_FAILED; > > } > > > > - ts_params->valid_devs[ts_params->valid_dev_count++] = 0; > > + gbl_driver_id = rte_cryptodev_driver_id_get( > > + RTE_STR(CRYPTODEV_NAME_NULL_PMD)); These tests only work with the crypto_null PMD's, gbl_driver_id is set to the crypto_null PMD id here. > > + > > + /* Create list of valid crypto devs */ > > + for (i = 0; i < nb_devs; i++) { > > + rte_cryptodev_info_get(i, &info); > > + if (info.driver_id == gbl_driver_id) > > + ts_params->valid_devs[ts_params->valid_dev_count++] > = i; > > + } > > I think you need to check driver capabilities, instead of relying on driver > name. I don't think it is necessary to check the driver capabilities. This is how it is done in test_cryptodev.c. I think it makes sense to mirror the test_cryptodev.c implementation. > > + > > + if (ts_params->valid_dev_count < 1) > > + return TEST_FAILED; > > > > /* Set up all the qps on the first of the valid devices found */ > > dev_id = ts_params->valid_devs[0]; > > If we always use just valid_dev[0] to determine private session size, why do > you > keep going though all devs in the loop above? There may be several crypto devs present for example, crypto_aesni_mb0, crypto_aseni_mb1, crypto_null0 and crypto_null1. The valid_dev[] array will contain all devs of the requested type, in this case crypto_null0 and crypto_null1. > Another thing, as I mentioned off-line - later you still use all vald_devs[] > to init > session: > s = rte_cryptodev_sym_session_create(qp->mp_session); > if (s == NULL) > return -ENOMEM; > > /* initiliaze SA crypto session for all supported devices */ > for (i = 0; i != devnum; i++) { > rc = rte_cryptodev_sym_session_init(devid[i], s, > ut->crypto_xforms, qp->mp_session_private); > if (rc != 0) > break; > } > > I think we need either to determine max private session size based on *all* > valid_devs[], or just use one device that can do NULL algorithm. The valid_devs[] array only contains crypto_null PMD's The code is using the crypto_null PMD only. > As we always enqueue/dequeuer into valid_devs[0] - I think there is no point > to > have an arrays here, just single valid_dev should be sufficient. The test program may be started with several crypto_dev PMD's for example: test -c f -n 4 --vdev crypto_aesni_mb0 --vdev crypto_null0 --vdev crypto_aesni_mb1 --vdev crypto_dev_null1 In this case the valid_devs[] array will contain crypto_dev_null0 and crypto_dev_null1. > Konstantin > > Regards, Bernard.