[PATCH i-g-t 2/3] lib/igt_kmod: Stop handling _kunit suffix

[Janusz Krzysztofik]

On Tuesday, 5 November 2024 15:58:04 CET Lucas De Marchi wrote:
> On Tue, Nov 05, 2024 at 11:50:05AM +0100, Janusz Krzysztofik wrote:
> >Hi Lucas,
> >
> >On Tuesday, 5 November 2024 06:52:54 CET Lucas De Marchi wrote:
> >> It's not used anywhere anymore. Remove it.
> >
> >I'm not sure.  I added this option based on KUnit documentation.  Has the
> >documentation changed since then?  I haven't been watching it.  Have you
> 
> I don't see a suffix _kunit for module names mentioned anywhere.

See https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-file-and-module-names

"... The _kunit suffix is preferred, as it makes the distinction between KUnit 
 and non-KUnit tests clearer."

Thanks,
Janusz


> Documentation mentions that the config option usually ends with
> _KUNIT_TEST: https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#configuring-the-kernel
> 
> looking for modules ending in _kunit:
> 	$ git grep '=.*_kunit\.o'
> 	drivers/net/ethernet/microchip/vcap/Makefile:obj-$(CONFIG_VCAP_KUNIT_TEST) +=  vcap_model_kunit.o
> 	kernel/Makefile:obj-$(CONFIG_RESOURCE_KUNIT_TEST) += resource_kunit.o
> 	lib/Makefile:obj-$(CONFIG_STRING_KUNIT_TEST) += string_kunit.o
> 	lib/Makefile:obj-$(CONFIG_STRING_HELPERS_KUNIT_TEST) += string_helpers_kunit.o
> 	lib/Makefile:obj-$(CONFIG_CPUMASK_KUNIT_TEST) += cpumask_kunit.o
> 	lib/Makefile:obj-$(CONFIG_BITFIELD_KUNIT) += bitfield_kunit.o
> 	lib/Makefile:obj-$(CONFIG_CHECKSUM_KUNIT) += checksum_kunit.o
> 	lib/Makefile:obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> 	lib/Makefile:obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o
> 	lib/Makefile:obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
> 	lib/Makefile:obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_kunit.o
> 	lib/Makefile:obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
> 	lib/Makefile:obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
> 	lib/Makefile:obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o
> 	lib/Makefile:obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o
> 	lib/Makefile:obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
> 	lib/math/Makefile:obj-$(CONFIG_INT_POW_TEST)  += tests/int_pow_kunit.o
> 	lib/math/tests/Makefile:obj-$(CONFIG_INT_POW_TEST) += int_pow_kunit.o
> 	rust/Makefile:obj-$(CONFIG_RUST_KERNEL_DOCTESTS) += doctests_kernel_generated_kunit.o
> 	sound/core/Makefile:obj-$(CONFIG_SND_CORE_TEST) += sound_kunit.o
> 
> vs:
> 
> 	$ git grep '=.*_test\.o' | wc -l 
> 	87
> 
> I think the behavior is a bit surprising because when passing NULL it
> actually runs *all* the testsuites. It's not a shortcut to handle
> modules like e.g. usercopy_kunit.o / siphash_kunit.o
> that have the testsuite called usercopy / siphash. Instead it's
> a way to group the tests for igt reporting purposes. We could very well
> just keep the suffix or use a hardcoded "all". Or did I misread the
> handling of this arg?
> 
> Currently there are no users in igt for modules ending in _kunit and I
> don't think we are going to add one any time soon. We can always come
> back and revert this if needed.
> 
> >checked if core drm doesn't use that suffix?   If it is still documented then
> >what if we decide to run kunit tests from other subsytems (dma-buf? iommu?)
> 
> Using lib/stackinit_kunit.c as example: we could do either
> 
> 	igt_kunit("stackinit_kunit", NULL, NULL)
> 	or
> 	igt_kunit("stackinit_kunit", "stackinit", NULL)
> 
> The difference would only be the name of the igt subtest.
> 
> Lucas De Marchi
> 
> >and it occurs they still use it?
> >
> >Thanks,
> >Janusz
> >
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> >> ---
> >>  lib/igt_kmod.c | 5 +----
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> >> index 4eeab79ec..58edac8fa 100644
> >> --- a/lib/igt_kmod.c
> >> +++ b/lib/igt_kmod.c
> >> @@ -1189,7 +1189,7 @@ static void __igt_kunit(struct igt_ktest *tst,
> >>   *	   if NULL then test cases from all test suites provided by the module
> >>   *	   are executed as dynamic sub-subtests of one IGT subtest, which name
> >>   *	   is derived from the module name by cutting off its optional trailing
> >> - *	   _test or _kunit suffix
> >> + *	   _test suffix
> >>   * @opts: options to load the module
> >>   *
> >>   * Loads the test module, parses its (k)tap dmesg output, then unloads it
> >> @@ -1213,9 +1213,6 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
> >>  		if (!igt_debug_on(!subtest)) {
> >>  			char *suffix = strstr(subtest, "_test");
> >>
> >> -			if (!suffix)
> >> -				suffix = strstr(subtest, "_kunit");
> >> -
> >>  			if (suffix)
> >>  				*suffix = '\0';
> >>  		}
> >>
> >
> >
> >
> >
>