On 13/04/2021 6:51 PM, Akhil Goyal wrote:
Hi Ciara, //snipnb_devs = rte_cryptodev_count(); if (nb_devs < 1) { RTE_LOG(WARNING, USER1, "No crypto devices found?\n"); @@ -838,6 +630,228 @@ testsuite_setup(void) return TEST_SUCCESS; } +static int +qat_testsuite_setup(void) +{ + return testsuite_params_setup(); +} + +static int +aesni_mb_testsuite_setup(void) +{ + int32_t nb_devs, ret; + nb_devs = rte_cryptodev_device_count_by_driver( + rte_cryptodev_driver_id_get( + RTE_STR(CRYPTODEV_NAME_AESNI_MB_PMD))); + if (nb_devs < 1) { + ret = rte_vdev_init(RTE_STR(CRYPTODEV_NAME_AESNI_MB_PMD), NULL); + + TEST_ASSERT(ret == 0, + "Failed to create instance of pmd : %s", + RTE_STR(CRYPTODEV_NAME_AESNI_MB_PMD)); + } + return testsuite_params_setup(); +}Why is it required to have a separate function for each of the PMD? I believe a single function with an argument should be sufficient. And in that single function a simple switch case may process the vdevs differently. As of now, it looks that moving the unnecessary duplicate code from one place to another. This was one cleanup we have been looking forward to from quite some time. Every time a new PMD comes, a separate function is formed and the length of the code increases. //snip
The virtual device creation code could be dropped completely if user is guaranteed to create the required crypto PMD from EAL devargs and we could assume that the first crypto device of the test suite type is always used?
-static struct unit_test_suite cryptodev_virtio_testsuite = { +static struct unit_test_suite cryptodev_virtio_sub_testsuite = { .suite_name = "Crypto VIRTIO Unit Test Suite", - .setup = testsuite_setup, - .teardown = testsuite_teardown, .unit_test_cases = { TEST_CASE_ST(ut_setup, ut_teardown, test_AES_cipheronly_all), @@ -13935,15 +14107,12 @@ static struct unit_test_suite cryptodev_virtio_testsuite = { } }; -static struct unit_test_suite cryptodev_caam_jr_testsuite = { - .suite_name = "Crypto CAAM JR Unit Test Suite", - .setup = testsuite_setup, - .teardown = testsuite_teardown, +static struct unit_test_suite cryptodev_caam_jr_sub_testsuite = { + .suite_name = "Crypto CAAM JR Sub Unit Test Suite", .unit_test_cases = { TEST_CASE_ST(ut_setup, ut_teardown, - test_device_configure_invalid_dev_id), - TEST_CASE_ST(ut_setup, ut_teardown, - test_multi_session), + test_device_configure_invalid_dev_id), + TEST_CASE_ST(ut_setup, ut_teardown, test_multi_session), TEST_CASE_ST(ut_setup, ut_teardown, test_AES_chain_all), TEST_CASE_ST(ut_setup, ut_teardown, test_3DES_chain_all), @@ -13955,58 +14124,28 @@ static struct unit_test_suite cryptodev_caam_jr_testsuite = { } };Splitting the complete testsuite into logical generic algo based sub testsuite Is a good idea. I appreciate that. But introducing PMD based test suite is not recommended. We have been trying from past few releases to clean this up. And this patch is again introducing the same. When I first saw this series, I saw only the algo based splitting and when it was run on the board, it was showing results in an organized way. But this was not expected that, PMD based test suites are reintroduced by Intel who helped in removing them in last few releases. This will make an unnecessary addition of duplicate code whenever a new PMD is introduced. I recommend to use a single parent suite - cryptodev_testsuite and there Can be multiple sub testsuites based on Algos etc. but not on the basis of PMD. Regards, Akhil
Hey Akhil, I understand the sentiment of this, we were just trying to avoid necessary failures by executing testsuites which aren't supported by the PMD under test, and we're confident that all testsuites/tests are correctly verifying their capabilities requirements. If we add some code into the testsuite setup functions to test capabilities required for the testsuites vs those required by the PMD then we could do as you are suggesting. If we can make this change quickly would you consider this patchset for inclusion in RC2?
