Re: [igt-dev] [PATCH i-g-t 10/10] KUnit: replace abort with graceful skip

[Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>]

On Tue, 13 Jun 2023 11:11:00 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

> On Tuesday, 13 June 2023 10:41:03 CEST Mauro Carvalho Chehab wrote:
> > On Tue, 13 Jun 2023 09:27:26 +0200
> > Dominik Karol Piatkowski <dominik.karol.piatkowski@intel.com> wrote:
> >   
> > > Sample drm_buddy output with missing requirements:
> > > 	Starting subtest: drm_buddy_test
> > > 	(drm_buddy:32218) igt_kmod-WARNING: Unable to load KUnit
> > > 	Subtest drm_buddy_test: SKIP (0.001s)
> > > 
> > > Signed-off-by: Dominik Karol Piątkowski   
> <dominik.karol.piatkowski@intel.com>
> > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> > > ---
> > >  lib/igt_kmod.c | 20 +++++++++++++-------
> > >  1 file changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > > index 1511bdef4..04220f404 100644
> > > --- a/lib/igt_kmod.c
> > > +++ b/lib/igt_kmod.c
> > > @@ -763,27 +763,27 @@ static void __igt_kunit(const char *module_name,   
> const char *opts)
> > >  	int ret;
> > >  	struct ktap_test_results *results;
> > >  	struct ktap_test_results_element *temp;
> > > +	bool skip = false;
> > >  
> > >  	/* get normalized module name */
> > >  	if (igt_ktest_init(&tst, module_name) != 0) {
> > > -		igt_warn("Unable to initialize ktest for %s\n",   
> module_name);
> > > -		igt_fail(IGT_EXIT_SKIP);
> > > +		igt_skip("Unable to initialize ktest for %s\n",   
> module_name);
> > >  	}
> > >  
> > >  	if (igt_ktest_begin(&tst) != 0) {
> > > -		igt_warn("Unable to begin ktest for %s\n", module_name);
> > > -
> > >  		igt_ktest_fini(&tst);
> > > -		igt_fail(IGT_EXIT_SKIP);
> > > +		igt_skip("Unable to begin ktest for %s\n", module_name);
> > >  	}
> > >  
> > >  	if (tst.kmsg < 0) {
> > >  		igt_warn("Could not open /dev/kmsg\n");
> > > +		skip = true;  
> > 
> > Hmm.... not being able to read from /dev/kmsg doesn't seem a reason for
> > skipping the test.
> > 
> > IMO, the test should be skipped only if:
> > 
> > 1. The Kernel is not compiled with KUnit support;
> > 2. The KUnit test was not compiled.
> > 
> > Errors for all other conditions should be considered as failures.
> > 
> > Now, I'm not sure what would be the proper way to test for such
> > skip condition. I suspect you could check the error code at
> > igt_kmod_load(), calling igt_skip() if it returns -ENOENT,
> > e. g. on this part of __igt_kunit():
> > 
> >         /* The KUnit module is required for running any KUnit tests */
> >         if (igt_kmod_load("kunit", NULL) != 0 ||
> >             kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) !=   
> 0) {
> >                 igt_warn("Unable to load KUnit\n");
> >                 igt_fail(IGT_EXIT_FAILURE);
> >         }
> > 
> >         is_builtin = kmod_module_get_initstate(kunit_kmod) ==   
> KMOD_MODULE_BUILTIN;
> > 
> >         results = ktap_parser_start(f, is_builtin);
> > 
> >         if (igt_kmod_load(module_name, opts) != 0) {
> >                 igt_warn("Unable to load %s module\n", module_name);
> >                 ret = ktap_parser_stop();
> >                 igt_fail(IGT_EXIT_FAILURE);
> >         }
> > 
> > 
> > This could be replaced by something similar to:
> > 
> > 	err = igt_kmod_load("kunit", NULL);
> > 	igt_skip_on(err == ENOENT, "KUnit not supported");
> > 	igt_fail_on_f(err != 0, "Error %d when trying to load kunit", err);
> >         if (kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) !=   
> 0) {
> >                 igt_warn("Unable to load KUnit\n");
> >                 igt_fail(IGT_EXIT_FAILURE);
> >         }
> > 
> > 
> >         is_builtin = kmod_module_get_initstate(kunit_kmod) ==   
> KMOD_MODULE_BUILTIN;
> > 
> >         results = ktap_parser_start(f, is_builtin);
> > 
> >         err = igt_kmod_load(module_name, opts) 
> > 	igt_skip_on(err == ENOENT, "Module %s not found", module_name);
> > 	if (err) {
> >                 igt_warn("Error %d when trying to load %s module\n", err,   
> module_name);
> >                 ret = ktap_parser_stop();
> >                 igt_fail(IGT_EXIT_FAILURE);
> >         }
> > 
> > 
> > In order to explicitly check for (1) and (2).  
> 
> Why do we reinvent the wheel instead of following the pattern from 
> igt_kselftest()? 

Nobody is talking about reinventing the wheel. Selftests and KUnit tests
are executed on different ways.

For selftests, the same driver that was loaded before needs to be unloaded
and re-probed with a different argument.

For KUnit tests, a different module will be loaded, if the Kernel was
compiled with KUnit support for a given test suite.

Looking at KUnit modprobe time, if a module doesn't load because the file
doesn't exist (-ENOENT error), it means that:

- for KUnit: the KUnit module was not compiled.
- for selftests: this is really unlikely to happen, as all other
  previous IGT tests depend on having the driver module loaded.
  So, no need for any explicit check for this specific condition.

> We used to follow it, before the whole igt_kunit() body was 
> put under igt_subtest_with_dynamic() (patch 6/8 of v5, then squashed with 5/5, 
> IIRC).  That forced us to introduce a series of changes inside igt_kunit(), 
> which we now contest.  Was that really necessary?

This has nothing to do with using (or not) a dynamic IGT subtest.

It is related to how to report IGT errors while trying to run KUnit
tests.

Without this patch, every kind of errors will produce a failure;
after this patch, they'll all produce a skip.

IMO, what it is needed is to split KUnit errors on two different
categories:

1. "real" errors while running KUnit (like troubles reading dmesg);
2. the KUnit module to be modprobed doesn't exist.

for (1), igt_fail() is the proper error handling.

For (2), igt_skip() is the right error handling, as the Kernel under
tests doesn't have what it is needed to run KUnit, as the module was
not compiled.

Regards,
Mauro