[PATCH i-g-t v3 0/6] lib/kunit: Support writable filter* parameters of kunit module

[PATCH i-g-t v3 0/6] lib/kunit: Support writable filter* parameters of kunit module

[Janusz Krzysztofik]

Instead of wasting resources on reloading the base Kunit module each time
a different set of filter parameters is needed, try to write the required
values to sysfs representation of those parameters.  If that fails (e.g.
on older LTS kernels with read-only filter parameters), fall back to
reloading the module.

This change also provides a workaround for the issue of impossibility to
unload the base Kunit module on Xe platforms, available as soon as the
module supports writable filter parameters.

While being at it, fine tune processing of skips on errors during test
case list collection phase.

v3: Don't read-compare-write, just write the values (Lucas),
  - skip calling igt_sysfs_set() when the string to be written to
    filter_action is empty (Lucas),
  - warn if we leave the filter_action set to "skip" while setting a non-
    default value of the filter parameter,
  - transform generic kunit_set_params() to kunit_set_filtering().
v2: Work around ineffective writes of empty strings to sysfs module
    parameter files (Lucas) by using human readable non-empty strings that
    give the same results as default NULLs,
  - drop fallback to reload of base Kunit module method if assigning new
    values to module parameters via sysfs fails (Lucas), instead use the
    existing fallback to blind execution like if getting a list of test
    cases was not supported at all,
  - split move of open_parameters() helper up in the source file as well
    as cleanup of base KUnit module unloading to separate patches (Kamil),
  - skip on empty list of test cases (new patch),
  - address the issue of commit description suggesting two separate
    changes combined in one patch (Kamil).

Janusz Krzysztofik (6):
  lib/kunit: Skip on empty list of test cases
  lib/kmode: Prepare open_parameters() helper for reuse by kunit
  lib/kunit: Unload base KUnit module only before reloading it
  lib/kunit: Support writable filter* parameters of kunit module
  lib/kunit: Report early kernel taints explicitly
  lib/kunit: Process module remove error after list errors

 lib/igt_kmod.c | 115 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 79 insertions(+), 36 deletions(-)

-- 
2.43.0

[PATCH i-g-t v3 1/6] lib/kunit: Skip on empty list of test cases

[Janusz Krzysztofik]

If loading the base KUnit module in list mode succeeds but our KTAP
parser or test suite filter returns an empty list of test cases then,
instead of calling igt_skip, we now unintentionally fall back to legacy
mode as if the module didn't support filter parameters.  Fix it.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_kmod.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 250ab2107b..ad69173151 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -1163,6 +1163,7 @@ static void kunit_get_tests(struct igt_list_head *tests,
 
 	igt_skip_on_f(err,
 		      "KTAP parser failed while getting a list of test cases\n");
+	igt_skip_on(igt_list_empty(tests));
 }
 
 static void __igt_kunit(struct igt_ktest *tst,
-- 
2.43.0

[PATCH i-g-t v3 2/6] lib/kmode: Prepare open_parameters() helper for reuse by kunit

[Janusz Krzysztofik]

Move the open_parameters() helper code up in the source file, above the
kunit related functions, so it is available to follow-up changes of the
kunit code.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_kmod.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index ad69173151..d1091bdc79 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -803,6 +803,14 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
 	kmod_module_info_free_list(pre);
 }
 
+static int open_parameters(const char *module_name)
+{
+	char path[256];
+
+	snprintf(path, sizeof(path), "/sys/module/%s/parameters", module_name);
+	return open(path, O_RDONLY);
+}
+
 struct modprobe_data {
 	struct kmod_module *kmod;
 	const char *opts;
@@ -1402,14 +1410,6 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
 	igt_ktest_fini(&tst);
 }
 
-static int open_parameters(const char *module_name)
-{
-	char path[256];
-
-	snprintf(path, sizeof(path), "/sys/module/%s/parameters", module_name);
-	return open(path, O_RDONLY);
-}
-
 int igt_ktest_init(struct igt_ktest *tst,
 		   const char *module_name)
 {
-- 
2.43.0

[PATCH i-g-t v3 3/6] lib/kunit: Unload base KUnit module only before reloading it

[Janusz Krzysztofik]

In preparation for replacement of the current method of reloading base
KUnit module with updating the module parameters via sysfs when a new set
of those parameters is needed, unload the module right before it is now
reloaded with new parameters, and stop unloading it in other places.
A follow-up patch will remove all those unloads still left, but even then
it will be shorter and more clean when based on top of this one.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
---
 lib/igt_kmod.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index d1091bdc79..d2c28d0a64 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -1130,6 +1130,7 @@ static void kunit_get_tests(struct igt_list_head *tests,
 	 * iterating over KTAP results collected from blind execution of all
 	 * Kunit test cases provided by a Kunit test module.
 	 */
+	igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
 	if (igt_debug_on(igt_kmod_load("kunit",
 				       "filter=module=none filter_action=skip")))
 		return;
@@ -1167,7 +1168,6 @@ static void kunit_get_tests(struct igt_list_head *tests,
 	}
 
 	igt_skip_on(kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE));
-	igt_skip_on(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
 
 	igt_skip_on_f(err,
 		      "KTAP parser failed while getting a list of test cases\n");
@@ -1200,6 +1200,17 @@ static void __igt_kunit(struct igt_ktest *tst,
 			      t->case_name) {
 
 			if (!modprobe.thread) {
+				/*
+				 * Since we have successfully loaded the kunit
+				 * base module with non-default parameters in
+				 * order to get a list of test cases, now we
+				 * have to unload it so it is then automatically
+				 * reloaded with default parameter values when
+				 * we load the test module again for execution.
+				 */
+				igt_skip_on(igt_kmod_unload("kunit",
+							    KMOD_REMOVE_FORCE));
+
 				igt_assert_eq(pthread_mutexattr_init(&attr), 0);
 				igt_assert_eq(pthread_mutexattr_setrobust(&attr,
 							  PTHREAD_MUTEX_ROBUST),
@@ -1364,15 +1375,6 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
 		igt_skip_on(igt_ktest_init(&tst, module_name));
 		igt_skip_on(igt_ktest_begin(&tst));
 
-		/*
-		 * Since we need to load kunit base module with specific
-		 * options in order to get a list of test cases, make
-		 * sure that the module is not loaded.  However, since
-		 * unload may fail if kunit base module is not loaded,
-		 * ignore any failures, we'll fail later if still loaded.
-		 */
-		igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
-
 		igt_assert(igt_list_empty(&tests));
 	}
 
@@ -1404,7 +1406,6 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
 		kunit_results_free(&tests, &suite_name, &case_name);
 
 		igt_ktest_end(&tst);
-		igt_debug_on(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
 	}
 
 	igt_ktest_fini(&tst);
-- 
2.43.0

[PATCH i-g-t v3 4/6] lib/kunit: Support writable filter* parameters of kunit module

[Janusz Krzysztofik]

Instead of wasting resources on reloading the base Kunit module each time
a different set of filter parameters is needed, try to write the required
values to sysfs representation of those parameters.  If that fails (e.g.
on older LTS kernels with read-only filter parameters), fall back to
legacy "no list of test cases available" mode as we do on kernels with
those module parameters not supported at all.

This modification will also serve as a workaround for the issue of
impossibility to unload the base Kunit module on Xe platforms, available
to IGT as soon as the module supports writable filter parameters.

v3: Don't read-compare-write, just write the values (Lucas),
  - skip calling igt_sysfs_set() when the string to be written to
    filter_action is empty (Lucas),
  - warn if we leave the filter_action set to "skip" while setting a non-
    default value of the filter parameter,
  - transform generic kunit_set_params() to kunit_set_filtering().
v2: Work around ineffective writes of empty strings to sysfs module
    parameter files (Lucas) by using human readable non-empty strings that
    give the same results as default NULLs,
  - drop fallback to reload of base Kunit module method if assigning new
    values to module parameters via sysfs fails (Lucas), instead use the
    existing fallback to blind execution like if getting a list of test
    cases was not supported at all,
  - split move of open_parameters() helper up in the source file as well
    as cleanup of base KUnit module unloading to separate patches (Kamil),
  - address the issue of the second paragraph of commit description
    suggesting two separate changes combined in one patch (Kamil) by
    rewording the description.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_kmod.c | 91 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 26 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index d2c28d0a64..83416d2aa4 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -811,6 +811,49 @@ static int open_parameters(const char *module_name)
 	return open(path, O_RDONLY);
 }
 
+static bool kunit_set_filtering(const char *filter_glob, const char *filter,
+				const char *filter_action)
+{
+	int params;
+	bool ret;
+
+	params = open_parameters("kunit");
+	ret = !igt_debug_on(params < 0);
+
+	/*
+	 * Default values of the KUnit base module filtering parameters
+	 * are all NULLs.  Reapplying those NULLs over sysfs once
+	 * overwritten with non-NULL strings seems not possible.
+	 * As a workaround, we use human-readable strings that exhibit
+	 * the same behaviour as if default NULLs were in place.
+	 */
+	if (ret)
+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_glob",
+						   filter_glob ?: "*"));
+	if (ret)
+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter",
+						   filter ?: "module!=none"));
+	/*
+	 * In case of filter_action parameter there is no obvious human-
+	 * readable string that behaves like default NULL.  However, if NULL
+	 * is requested for both filter and filter_action then that's not a
+	 * problem for us since even if filter_action was previously set to
+	 * "skip" we can leave it as is because our "module!=none" default
+	 * filter guarantees that no test cases are filtered out to be skipped.
+	 */
+	if (ret)
+		ret = !igt_warn_on_f(!filter_action && filter,
+				     "test error: filter_action=NULL requires filter=NULL\n");
+	if (ret && filter_action)
+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_action",
+						   filter_action));
+
+	if (params >= 0)
+		close(params);
+
+	return ret;
+}
+
 struct modprobe_data {
 	struct kmod_module *kmod;
 	const char *opts;
@@ -1116,24 +1159,26 @@ static void kunit_get_tests(struct igt_list_head *tests,
 	/*
 	 * To get a list of test cases provided by a kunit test module, ask the
 	 * generic kunit module to respond with SKIP result for each test found.
-	 * We could also use action=list kunit parameter to get the listing,
-	 * however, parsing a KTAP report -- something that we already can do
-	 * perfectly -- seems to be more safe than extracting a test case list
-	 * of unknown length from /dev/kmsg.
-	 *
-	 * TODO: drop the following workaround, which is required by LTS kernel
-	 *       v6.1 not capable of listing test cases when built as a module.
-	 * If loading the kunit module with required parameters fails then
-	 * assume that we are running on a kernel with missing test case listing
-	 * capabilities.  Dont's skip but just return with empty list of test
-	 * cases, that should tell the caller to use a legacy method of
-	 * iterating over KTAP results collected from blind execution of all
-	 * Kunit test cases provided by a Kunit test module.
+	 * We could also try to use action=list kunit parameter to get the
+	 * listing, however, parsing a structured KTAP report -- something that
+	 * we already can do perfectly -- seems to be more safe than extracting
+	 * a free text list of unknown length from /dev/kmsg.
 	 */
-	igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
-	if (igt_debug_on(igt_kmod_load("kunit",
-				       "filter=module=none filter_action=skip")))
+	if (igt_debug_on(!kunit_set_filtering(NULL, "module=none", "skip"))) {
+		/*
+		 * TODO: drop the following workaround, required by LTS kernel
+		 *       v6.1 not capable of listing test cases when built as as
+		 *       module, when no longer needed.
+		 * If updating writable filter parameters of the kunit base
+		 * module with required values fails then assume we are running
+		 * on a kernel with missing test case listing capabilities.
+		 * Don't skip but just return with empty list of test cases,
+		 * which should tell the caller to use a legacy method of
+		 * iterating over KTAP results collected from blind execution
+		 * of all Kunit test cases provided by a Kunit test module.
+		 */
 		return;
+	}
 
 	igt_skip_on(modprobe(tst->kmod, opts));
 
@@ -1200,16 +1245,7 @@ static void __igt_kunit(struct igt_ktest *tst,
 			      t->case_name) {
 
 			if (!modprobe.thread) {
-				/*
-				 * Since we have successfully loaded the kunit
-				 * base module with non-default parameters in
-				 * order to get a list of test cases, now we
-				 * have to unload it so it is then automatically
-				 * reloaded with default parameter values when
-				 * we load the test module again for execution.
-				 */
-				igt_skip_on(igt_kmod_unload("kunit",
-							    KMOD_REMOVE_FORCE));
+				igt_require(kunit_set_filtering(NULL, NULL, NULL));
 
 				igt_assert_eq(pthread_mutexattr_init(&attr), 0);
 				igt_assert_eq(pthread_mutexattr_setrobust(&attr,
@@ -1378,6 +1414,9 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
 		igt_assert(igt_list_empty(&tests));
 	}
 
+	/* We need the base KUnit module loaded if not built-in */
+	igt_ignore_warn(igt_kmod_load("kunit", NULL));
+
 	/*
 	 * We need to use igt_subtest here, as otherwise it may crash with:
 	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
-- 
2.43.0

[PATCH i-g-t v3 5/6] lib/kunit: Report early kernel taints explicitly

[Janusz Krzysztofik]

When we find the kernel tainted after loading a KUnit test module in
list only mode, report that taint immediately as the reason for skip
instead of executing and blaming our KTAP parser.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_kmod.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 83416d2aa4..0ac4ac7a02 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -1146,6 +1146,7 @@ static void kunit_get_tests(struct igt_list_head *tests,
 	char *suite_name = NULL, *case_name = NULL;
 	struct igt_ktap_result *r, *rn;
 	struct igt_ktap_results *ktap;
+	unsigned long taints;
 	int flags, err;
 
 	igt_skip_on_f(tst->kmsg < 0, "Could not open /dev/kmsg\n");
@@ -1181,6 +1182,7 @@ static void kunit_get_tests(struct igt_list_head *tests,
 	}
 
 	igt_skip_on(modprobe(tst->kmod, opts));
+	igt_skip_on(igt_kernel_tainted(&taints));
 
 	ktap = igt_ktap_alloc(tests);
 	igt_require(ktap);
-- 
2.43.0

[PATCH i-g-t v3 6/6] lib/kunit: Process module remove error after list errors

[Janusz Krzysztofik]

Skip on any error from test case list gathering first, then, in
preparation for executing those test cases, on an error from unloading the
test module loaded in list only mode, so it is more clear if listing the
test cases was successful or not.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_kmod.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 0ac4ac7a02..3163434aa6 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -1214,11 +1214,11 @@ static void kunit_get_tests(struct igt_list_head *tests,
 		free(case_name);
 	}
 
-	igt_skip_on(kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE));
-
 	igt_skip_on_f(err,
 		      "KTAP parser failed while getting a list of test cases\n");
 	igt_skip_on(igt_list_empty(tests));
+
+	igt_skip_on(kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE));
 }
 
 static void __igt_kunit(struct igt_ktest *tst,
-- 
2.43.0

Re: [PATCH i-g-t v3 4/6] lib/kunit: Support writable filter* parameters of kunit module

[Lucas De Marchi]

On Thu, Feb 01, 2024 at 07:59:20PM +0100, Janusz Krzysztofik wrote:
>Instead of wasting resources on reloading the base Kunit module each time
>a different set of filter parameters is needed, try to write the required
>values to sysfs representation of those parameters.  If that fails (e.g.
>on older LTS kernels with read-only filter parameters), fall back to
>legacy "no list of test cases available" mode as we do on kernels with
>those module parameters not supported at all.
>
>This modification will also serve as a workaround for the issue of
>impossibility to unload the base Kunit module on Xe platforms, available
>to IGT as soon as the module supports writable filter parameters.
>
>v3: Don't read-compare-write, just write the values (Lucas),
>  - skip calling igt_sysfs_set() when the string to be written to
>    filter_action is empty (Lucas),
>  - warn if we leave the filter_action set to "skip" while setting a non-
>    default value of the filter parameter,
>  - transform generic kunit_set_params() to kunit_set_filtering().
>v2: Work around ineffective writes of empty strings to sysfs module
>    parameter files (Lucas) by using human readable non-empty strings that
>    give the same results as default NULLs,
>  - drop fallback to reload of base Kunit module method if assigning new
>    values to module parameters via sysfs fails (Lucas), instead use the
>    existing fallback to blind execution like if getting a list of test
>    cases was not supported at all,
>  - split move of open_parameters() helper up in the source file as well
>    as cleanup of base KUnit module unloading to separate patches (Kamil),
>  - address the issue of the second paragraph of commit description
>    suggesting two separate changes combined in one patch (Kamil) by
>    rewording the description.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>---
> lib/igt_kmod.c | 91 +++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 65 insertions(+), 26 deletions(-)
>
>diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>index d2c28d0a64..83416d2aa4 100644
>--- a/lib/igt_kmod.c
>+++ b/lib/igt_kmod.c
>@@ -811,6 +811,49 @@ static int open_parameters(const char *module_name)
> 	return open(path, O_RDONLY);
> }
>
>+static bool kunit_set_filtering(const char *filter_glob, const char *filter,
>+				const char *filter_action)
>+{
>+	int params;
>+	bool ret;
>+
>+	params = open_parameters("kunit");
>+	ret = !igt_debug_on(params < 0);

why proceed? just return early when it fails pre-conditions

>+
>+	/*
>+	 * Default values of the KUnit base module filtering parameters
>+	 * are all NULLs.  Reapplying those NULLs over sysfs once
>+	 * overwritten with non-NULL strings seems not possible.
>+	 * As a workaround, we use human-readable strings that exhibit
>+	 * the same behaviour as if default NULLs were in place.
>+	 */
>+	if (ret)
>+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_glob",
>+						   filter_glob ?: "*"));
>+	if (ret)
>+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter",
>+						   filter ?: "module!=none"));
>+	/*
>+	 * In case of filter_action parameter there is no obvious human-
>+	 * readable string that behaves like default NULL.  However, if NULL

that is not true.. it's just igt_sysfs not doing the right thing to
write an empty string, that does behave like NULL on the kernel side.
I'd drop these lines and just justify with the lines below why we are
not setting it here.

with a small change below we can also move these to use igt_require()

>+	 * is requested for both filter and filter_action then that's not a
>+	 * problem for us since even if filter_action was previously set to
>+	 * "skip" we can leave it as is because our "module!=none" default
>+	 * filter guarantees that no test cases are filtered out to be skipped.
>+	 */
>+	if (ret)
>+		ret = !igt_warn_on_f(!filter_action && filter,
>+				     "test error: filter_action=NULL requires filter=NULL\n");
>+	if (ret && filter_action)
>+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_action",
>+						   filter_action));
>+
>+	if (params >= 0)
>+		close(params);
>+
>+	return ret;
>+}
>+
> struct modprobe_data {
> 	struct kmod_module *kmod;
> 	const char *opts;
>@@ -1116,24 +1159,26 @@ static void kunit_get_tests(struct igt_list_head *tests,
> 	/*
> 	 * To get a list of test cases provided by a kunit test module, ask the
> 	 * generic kunit module to respond with SKIP result for each test found.
>-	 * We could also use action=list kunit parameter to get the listing,
>-	 * however, parsing a KTAP report -- something that we already can do
>-	 * perfectly -- seems to be more safe than extracting a test case list
>-	 * of unknown length from /dev/kmsg.
>-	 *
>-	 * TODO: drop the following workaround, which is required by LTS kernel
>-	 *       v6.1 not capable of listing test cases when built as a module.
>-	 * If loading the kunit module with required parameters fails then
>-	 * assume that we are running on a kernel with missing test case listing
>-	 * capabilities.  Dont's skip but just return with empty list of test
>-	 * cases, that should tell the caller to use a legacy method of
>-	 * iterating over KTAP results collected from blind execution of all
>-	 * Kunit test cases provided by a Kunit test module.
>+	 * We could also try to use action=list kunit parameter to get the
>+	 * listing, however, parsing a structured KTAP report -- something that
>+	 * we already can do perfectly -- seems to be more safe than extracting
>+	 * a free text list of unknown length from /dev/kmsg.
> 	 */
>-	igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
>-	if (igt_debug_on(igt_kmod_load("kunit",
>-				       "filter=module=none filter_action=skip")))
>+	if (igt_debug_on(!kunit_set_filtering(NULL, "module=none", "skip"))) {

probably in the caller or very early:

	if (!igt_syfs_has_rw_attr())
		return fallback_kunit_execution();

I think it's too different this approach from the previous one, that
results in the long comments trying to explain each different
step.

Humn... I guess we can do that on top, so if this works, seems ok for
now.

Lucas De Marchi

>+		/*
>+		 * TODO: drop the following workaround, required by LTS kernel
>+		 *       v6.1 not capable of listing test cases when built as as
>+		 *       module, when no longer needed.
>+		 * If updating writable filter parameters of the kunit base
>+		 * module with required values fails then assume we are running
>+		 * on a kernel with missing test case listing capabilities.
>+		 * Don't skip but just return with empty list of test cases,
>+		 * which should tell the caller to use a legacy method of
>+		 * iterating over KTAP results collected from blind execution
>+		 * of all Kunit test cases provided by a Kunit test module.
>+		 */
> 		return;
>+	}
>
> 	igt_skip_on(modprobe(tst->kmod, opts));
>
>@@ -1200,16 +1245,7 @@ static void __igt_kunit(struct igt_ktest *tst,
> 			      t->case_name) {
>
> 			if (!modprobe.thread) {
>-				/*
>-				 * Since we have successfully loaded the kunit
>-				 * base module with non-default parameters in
>-				 * order to get a list of test cases, now we
>-				 * have to unload it so it is then automatically
>-				 * reloaded with default parameter values when
>-				 * we load the test module again for execution.
>-				 */
>-				igt_skip_on(igt_kmod_unload("kunit",
>-							    KMOD_REMOVE_FORCE));
>+				igt_require(kunit_set_filtering(NULL, NULL, NULL));
>
> 				igt_assert_eq(pthread_mutexattr_init(&attr), 0);
> 				igt_assert_eq(pthread_mutexattr_setrobust(&attr,
>@@ -1378,6 +1414,9 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
> 		igt_assert(igt_list_empty(&tests));
> 	}
>
>+	/* We need the base KUnit module loaded if not built-in */
>+	igt_ignore_warn(igt_kmod_load("kunit", NULL));
>+
> 	/*
> 	 * We need to use igt_subtest here, as otherwise it may crash with:
> 	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
>-- 
>2.43.0
>

✗ CI.Patch_applied: failure for lib/kunit: Support writable filter* parameters of kunit module (rev3)

[Patchwork]

== Series Details ==

Series: lib/kunit: Support writable filter* parameters of kunit module (rev3)
URL   : https://patchwork.freedesktop.org/series/129175/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 3e58a4940 drm-tip: 2024y-02m-01d-11h-13m-29s UTC integration manifest
=== git am output follows ===
error: lib/igt_kmod.c: does not exist in index
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: lib/kunit: Skip on empty list of test cases
Patch failed at 0001 lib/kunit: Skip on empty list of test cases
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [PATCH i-g-t v3 4/6] lib/kunit: Support writable filter* parameters of kunit module

[Janusz Krzysztofik]

On Thursday, 1 February 2024 21:26:35 CET Lucas De Marchi wrote:
> On Thu, Feb 01, 2024 at 07:59:20PM +0100, Janusz Krzysztofik wrote:
> >Instead of wasting resources on reloading the base Kunit module each time
> >a different set of filter parameters is needed, try to write the required
> >values to sysfs representation of those parameters.  If that fails (e.g.
> >on older LTS kernels with read-only filter parameters), fall back to
> >legacy "no list of test cases available" mode as we do on kernels with
> >those module parameters not supported at all.
> >
> >This modification will also serve as a workaround for the issue of
> >impossibility to unload the base Kunit module on Xe platforms, available
> >to IGT as soon as the module supports writable filter parameters.
> >
> >v3: Don't read-compare-write, just write the values (Lucas),
> >  - skip calling igt_sysfs_set() when the string to be written to
> >    filter_action is empty (Lucas),
> >  - warn if we leave the filter_action set to "skip" while setting a non-
> >    default value of the filter parameter,
> >  - transform generic kunit_set_params() to kunit_set_filtering().
> >v2: Work around ineffective writes of empty strings to sysfs module
> >    parameter files (Lucas) by using human readable non-empty strings that
> >    give the same results as default NULLs,
> >  - drop fallback to reload of base Kunit module method if assigning new
> >    values to module parameters via sysfs fails (Lucas), instead use the
> >    existing fallback to blind execution like if getting a list of test
> >    cases was not supported at all,
> >  - split move of open_parameters() helper up in the source file as well
> >    as cleanup of base KUnit module unloading to separate patches (Kamil),
> >  - address the issue of the second paragraph of commit description
> >    suggesting two separate changes combined in one patch (Kamil) by
> >    rewording the description.
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> >---
> > lib/igt_kmod.c | 91 +++++++++++++++++++++++++++++++++++---------------
> > 1 file changed, 65 insertions(+), 26 deletions(-)
> >
> >diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> >index d2c28d0a64..83416d2aa4 100644
> >--- a/lib/igt_kmod.c
> >+++ b/lib/igt_kmod.c
> >@@ -811,6 +811,49 @@ static int open_parameters(const char *module_name)
> > 	return open(path, O_RDONLY);
> > }
> >
> >+static bool kunit_set_filtering(const char *filter_glob, const char *filter,
> >+				const char *filter_action)
> >+{
> >+	int params;
> >+	bool ret;
> >+
> >+	params = open_parameters("kunit");
> >+	ret = !igt_debug_on(params < 0);
> 
> why proceed? just return early when it fails pre-conditions

Right.

> >+
> >+	/*
> >+	 * Default values of the KUnit base module filtering parameters
> >+	 * are all NULLs.  Reapplying those NULLs over sysfs once
> >+	 * overwritten with non-NULL strings seems not possible.
> >+	 * As a workaround, we use human-readable strings that exhibit
> >+	 * the same behaviour as if default NULLs were in place.
> >+	 */
> >+	if (ret)
> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_glob",
> >+						   filter_glob ?: "*"));
> >+	if (ret)
> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter",
> >+						   filter ?: "module!=none"));
> >+	/*
> >+	 * In case of filter_action parameter there is no obvious human-
> >+	 * readable string that behaves like default NULL.  However, if NULL
> 
> that is not true.. it's just igt_sysfs not doing the right thing to

That depends on how we feel about empty strings, whether they are human-
readable or not.  For me they're not quite, e.g. when simply reading them back 
from sysfs it's not possible to distinguished them from potential white space, 
unless some graphical representation of non-printing characters is used.  
Maybe it would be more clear if kernel was using a convention to display empty 
strings as e.g. "(empty)", like it displays NULL as "(null)".

> write an empty string, that does behave like NULL on the kernel side.

Then maybe let's better send the empty string to igt_sysfs_set() instead of 
skipping that, and replace my "human-readable" with "non-NULL" in the 
comments.  Thanks to "module!=none" we are still on the safe side, even before 
igt_sysfs_set() is fixed.

> I'd drop these lines and just justify with the lines below why we are
> not setting it here.
> 
> with a small change below we can also move these to use igt_require()
> 
> >+	 * is requested for both filter and filter_action then that's not a
> >+	 * problem for us since even if filter_action was previously set to
> >+	 * "skip" we can leave it as is because our "module!=none" default
> >+	 * filter guarantees that no test cases are filtered out to be skipped.
> >+	 */
> >+	if (ret)
> >+		ret = !igt_warn_on_f(!filter_action && filter,
> >+				     "test error: filter_action=NULL requires filter=NULL\n");
> >+	if (ret && filter_action)
> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_action",
> >+						   filter_action));
> >+
> >+	if (params >= 0)
> >+		close(params);
> >+
> >+	return ret;
> >+}
> >+
> > struct modprobe_data {
> > 	struct kmod_module *kmod;
> > 	const char *opts;
> >@@ -1116,24 +1159,26 @@ static void kunit_get_tests(struct igt_list_head *tests,
> > 	/*
> > 	 * To get a list of test cases provided by a kunit test module, ask the
> > 	 * generic kunit module to respond with SKIP result for each test found.
> >-	 * We could also use action=list kunit parameter to get the listing,
> >-	 * however, parsing a KTAP report -- something that we already can do
> >-	 * perfectly -- seems to be more safe than extracting a test case list
> >-	 * of unknown length from /dev/kmsg.
> >-	 *
> >-	 * TODO: drop the following workaround, which is required by LTS kernel
> >-	 *       v6.1 not capable of listing test cases when built as a module.
> >-	 * If loading the kunit module with required parameters fails then
> >-	 * assume that we are running on a kernel with missing test case listing
> >-	 * capabilities.  Dont's skip but just return with empty list of test
> >-	 * cases, that should tell the caller to use a legacy method of
> >-	 * iterating over KTAP results collected from blind execution of all
> >-	 * Kunit test cases provided by a Kunit test module.
> >+	 * We could also try to use action=list kunit parameter to get the
> >+	 * listing, however, parsing a structured KTAP report -- something that
> >+	 * we already can do perfectly -- seems to be more safe than extracting
> >+	 * a free text list of unknown length from /dev/kmsg.
> > 	 */
> >-	igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
> >-	if (igt_debug_on(igt_kmod_load("kunit",
> >-				       "filter=module=none filter_action=skip")))
> >+	if (igt_debug_on(!kunit_set_filtering(NULL, "module=none", "skip"))) {
> 
> probably in the caller or very early:
> 
> 	if (!igt_syfs_has_rw_attr())
> 		return fallback_kunit_execution();
> 
> I think it's too different this approach from the previous one, that
> results in the long comments trying to explain each different
> step.

OK.

> 
> Humn... I guess we can do that on top, so if this works, seems ok for
> now.

OK.

Thanks,
Janusz

> 
> Lucas De Marchi
> 
> >+		/*
> >+		 * TODO: drop the following workaround, required by LTS kernel
> >+		 *       v6.1 not capable of listing test cases when built as as
> >+		 *       module, when no longer needed.
> >+		 * If updating writable filter parameters of the kunit base
> >+		 * module with required values fails then assume we are running
> >+		 * on a kernel with missing test case listing capabilities.
> >+		 * Don't skip but just return with empty list of test cases,
> >+		 * which should tell the caller to use a legacy method of
> >+		 * iterating over KTAP results collected from blind execution
> >+		 * of all Kunit test cases provided by a Kunit test module.
> >+		 */
> > 		return;
> >+	}
> >
> > 	igt_skip_on(modprobe(tst->kmod, opts));
> >
> >@@ -1200,16 +1245,7 @@ static void __igt_kunit(struct igt_ktest *tst,
> > 			      t->case_name) {
> >
> > 			if (!modprobe.thread) {
> >-				/*
> >-				 * Since we have successfully loaded the kunit
> >-				 * base module with non-default parameters in
> >-				 * order to get a list of test cases, now we
> >-				 * have to unload it so it is then automatically
> >-				 * reloaded with default parameter values when
> >-				 * we load the test module again for execution.
> >-				 */
> >-				igt_skip_on(igt_kmod_unload("kunit",
> >-							    KMOD_REMOVE_FORCE));
> >+				igt_require(kunit_set_filtering(NULL, NULL, NULL));
> >
> > 				igt_assert_eq(pthread_mutexattr_init(&attr), 0);
> > 				igt_assert_eq(pthread_mutexattr_setrobust(&attr,
> >@@ -1378,6 +1414,9 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
> > 		igt_assert(igt_list_empty(&tests));
> > 	}
> >
> >+	/* We need the base KUnit module loaded if not built-in */
> >+	igt_ignore_warn(igt_kmod_load("kunit", NULL));
> >+
> > 	/*
> > 	 * We need to use igt_subtest here, as otherwise it may crash with:
> > 	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
> 

Re: [PATCH i-g-t v3 3/6] lib/kunit: Unload base KUnit module only before reloading it

[Kamil Konieczny]

Hi Janusz,
On 2024-02-01 at 19:59:19 +0100, Janusz Krzysztofik wrote:
> In preparation for replacement of the current method of reloading base
> KUnit module with updating the module parameters via sysfs when a new set
> of those parameters is needed, unload the module right before it is now
> reloaded with new parameters, and stop unloading it in other places.
> A follow-up patch will remove all those unloads still left, but even then
> it will be shorter and more clean when based on top of this one.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>

Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  lib/igt_kmod.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index d1091bdc79..d2c28d0a64 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -1130,6 +1130,7 @@ static void kunit_get_tests(struct igt_list_head *tests,
>  	 * iterating over KTAP results collected from blind execution of all
>  	 * Kunit test cases provided by a Kunit test module.
>  	 */
> +	igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
>  	if (igt_debug_on(igt_kmod_load("kunit",
>  				       "filter=module=none filter_action=skip")))
>  		return;
> @@ -1167,7 +1168,6 @@ static void kunit_get_tests(struct igt_list_head *tests,
>  	}
>  
>  	igt_skip_on(kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE));
> -	igt_skip_on(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
>  
>  	igt_skip_on_f(err,
>  		      "KTAP parser failed while getting a list of test cases\n");
> @@ -1200,6 +1200,17 @@ static void __igt_kunit(struct igt_ktest *tst,
>  			      t->case_name) {
>  
>  			if (!modprobe.thread) {
> +				/*
> +				 * Since we have successfully loaded the kunit
> +				 * base module with non-default parameters in
> +				 * order to get a list of test cases, now we
> +				 * have to unload it so it is then automatically
> +				 * reloaded with default parameter values when
> +				 * we load the test module again for execution.
> +				 */
> +				igt_skip_on(igt_kmod_unload("kunit",
> +							    KMOD_REMOVE_FORCE));
> +
>  				igt_assert_eq(pthread_mutexattr_init(&attr), 0);
>  				igt_assert_eq(pthread_mutexattr_setrobust(&attr,
>  							  PTHREAD_MUTEX_ROBUST),
> @@ -1364,15 +1375,6 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
>  		igt_skip_on(igt_ktest_init(&tst, module_name));
>  		igt_skip_on(igt_ktest_begin(&tst));
>  
> -		/*
> -		 * Since we need to load kunit base module with specific
> -		 * options in order to get a list of test cases, make
> -		 * sure that the module is not loaded.  However, since
> -		 * unload may fail if kunit base module is not loaded,
> -		 * ignore any failures, we'll fail later if still loaded.
> -		 */
> -		igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
> -
>  		igt_assert(igt_list_empty(&tests));
>  	}
>  
> @@ -1404,7 +1406,6 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
>  		kunit_results_free(&tests, &suite_name, &case_name);
>  
>  		igt_ktest_end(&tst);
> -		igt_debug_on(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
>  	}
>  
>  	igt_ktest_fini(&tst);
> -- 
> 2.43.0
> 

Re: Re: [PATCH i-g-t v3 4/6] lib/kunit: Support writable filter* parameters of kunit module

[Lucas De Marchi]

On Fri, Feb 02, 2024 at 11:14:49AM +0100, Janusz Krzysztofik wrote:
>On Thursday, 1 February 2024 21:26:35 CET Lucas De Marchi wrote:
>> On Thu, Feb 01, 2024 at 07:59:20PM +0100, Janusz Krzysztofik wrote:
>> >Instead of wasting resources on reloading the base Kunit module each time
>> >a different set of filter parameters is needed, try to write the required
>> >values to sysfs representation of those parameters.  If that fails (e.g.
>> >on older LTS kernels with read-only filter parameters), fall back to
>> >legacy "no list of test cases available" mode as we do on kernels with
>> >those module parameters not supported at all.
>> >
>> >This modification will also serve as a workaround for the issue of
>> >impossibility to unload the base Kunit module on Xe platforms, available
>> >to IGT as soon as the module supports writable filter parameters.
>> >
>> >v3: Don't read-compare-write, just write the values (Lucas),
>> >  - skip calling igt_sysfs_set() when the string to be written to
>> >    filter_action is empty (Lucas),
>> >  - warn if we leave the filter_action set to "skip" while setting a non-
>> >    default value of the filter parameter,
>> >  - transform generic kunit_set_params() to kunit_set_filtering().
>> >v2: Work around ineffective writes of empty strings to sysfs module
>> >    parameter files (Lucas) by using human readable non-empty strings that
>> >    give the same results as default NULLs,
>> >  - drop fallback to reload of base Kunit module method if assigning new
>> >    values to module parameters via sysfs fails (Lucas), instead use the
>> >    existing fallback to blind execution like if getting a list of test
>> >    cases was not supported at all,
>> >  - split move of open_parameters() helper up in the source file as well
>> >    as cleanup of base KUnit module unloading to separate patches (Kamil),
>> >  - address the issue of the second paragraph of commit description
>> >    suggesting two separate changes combined in one patch (Kamil) by
>> >    rewording the description.
>> >
>> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>> >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> >Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> >---
>> > lib/igt_kmod.c | 91 +++++++++++++++++++++++++++++++++++---------------
>> > 1 file changed, 65 insertions(+), 26 deletions(-)
>> >
>> >diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>> >index d2c28d0a64..83416d2aa4 100644
>> >--- a/lib/igt_kmod.c
>> >+++ b/lib/igt_kmod.c
>> >@@ -811,6 +811,49 @@ static int open_parameters(const char *module_name)
>> > 	return open(path, O_RDONLY);
>> > }
>> >
>> >+static bool kunit_set_filtering(const char *filter_glob, const char *filter,
>> >+				const char *filter_action)
>> >+{
>> >+	int params;
>> >+	bool ret;
>> >+
>> >+	params = open_parameters("kunit");
>> >+	ret = !igt_debug_on(params < 0);
>>
>> why proceed? just return early when it fails pre-conditions
>
>Right.
>
>> >+
>> >+	/*
>> >+	 * Default values of the KUnit base module filtering parameters
>> >+	 * are all NULLs.  Reapplying those NULLs over sysfs once
>> >+	 * overwritten with non-NULL strings seems not possible.
>> >+	 * As a workaround, we use human-readable strings that exhibit
>> >+	 * the same behaviour as if default NULLs were in place.
>> >+	 */
>> >+	if (ret)
>> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_glob",
>> >+						   filter_glob ?: "*"));
>> >+	if (ret)
>> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter",
>> >+						   filter ?: "module!=none"));
>> >+	/*
>> >+	 * In case of filter_action parameter there is no obvious human-
>> >+	 * readable string that behaves like default NULL.  However, if NULL
>>
>> that is not true.. it's just igt_sysfs not doing the right thing to
>
>That depends on how we feel about empty strings, whether they are human-
>readable or not.  For me they're not quite, e.g. when simply reading them back
>from sysfs it's not possible to distinguished them from potential white space,
>unless some graphical representation of non-printing characters is used.
>Maybe it would be more clear if kernel was using a convention to display empty
>strings as e.g. "(empty)", like it displays NULL as "(null)".

you can't print NULL. You can print empty like it's done now. That's not
going to change ever, as it would break perfectly usable and sane
userspace.


>
>> write an empty string, that does behave like NULL on the kernel side.
>
>Then maybe let's better send the empty string to igt_sysfs_set() instead of
>skipping that, and replace my "human-readable" with "non-NULL" in the

as long as the "human-readable" comment is removed, fine. Because empty
is empty, it's not a non-printing char. Let me try to make it clearer:

	# echo -ne a > /sys/module/firmware_class/parameters/path
	# hexdump -C /sys/module/firmware_class/parameters/path
	00000000  61 0a                                             |a.|
	00000002

There are 2 chars: 'a' and '\n', with the latter always inserted by the
kernel.

	# echo -ne "\0" > /sys/module/firmware_class/parameters/path
	# hexdump -C /sys/module/firmware_class/parameters/path
	00000000  0a                                                |.|
	00000001

there is 1 char, just the \n inserted by the kernel. Nobody would ever
ask for the kernel to start printing "(newline)" instead of \n.

Yes, the kernel side could behave differently depending if it's NULL
vs empty. kunit doesn't and shouldn't.


Lucas De Marchi

>comments.  Thanks to "module!=none" we are still on the safe side, even before
>igt_sysfs_set() is fixed.
>
>> I'd drop these lines and just justify with the lines below why we are
>> not setting it here.
>>
>> with a small change below we can also move these to use igt_require()
>>
>> >+	 * is requested for both filter and filter_action then that's not a
>> >+	 * problem for us since even if filter_action was previously set to
>> >+	 * "skip" we can leave it as is because our "module!=none" default
>> >+	 * filter guarantees that no test cases are filtered out to be skipped.
>> >+	 */
>> >+	if (ret)
>> >+		ret = !igt_warn_on_f(!filter_action && filter,
>> >+				     "test error: filter_action=NULL requires filter=NULL\n");
>> >+	if (ret && filter_action)
>> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_action",
>> >+						   filter_action));
>> >+
>> >+	if (params >= 0)
>> >+		close(params);
>> >+
>> >+	return ret;
>> >+}
>> >+
>> > struct modprobe_data {
>> > 	struct kmod_module *kmod;
>> > 	const char *opts;
>> >@@ -1116,24 +1159,26 @@ static void kunit_get_tests(struct igt_list_head *tests,
>> > 	/*
>> > 	 * To get a list of test cases provided by a kunit test module, ask the
>> > 	 * generic kunit module to respond with SKIP result for each test found.
>> >-	 * We could also use action=list kunit parameter to get the listing,
>> >-	 * however, parsing a KTAP report -- something that we already can do
>> >-	 * perfectly -- seems to be more safe than extracting a test case list
>> >-	 * of unknown length from /dev/kmsg.
>> >-	 *
>> >-	 * TODO: drop the following workaround, which is required by LTS kernel
>> >-	 *       v6.1 not capable of listing test cases when built as a module.
>> >-	 * If loading the kunit module with required parameters fails then
>> >-	 * assume that we are running on a kernel with missing test case listing
>> >-	 * capabilities.  Dont's skip but just return with empty list of test
>> >-	 * cases, that should tell the caller to use a legacy method of
>> >-	 * iterating over KTAP results collected from blind execution of all
>> >-	 * Kunit test cases provided by a Kunit test module.
>> >+	 * We could also try to use action=list kunit parameter to get the
>> >+	 * listing, however, parsing a structured KTAP report -- something that
>> >+	 * we already can do perfectly -- seems to be more safe than extracting
>> >+	 * a free text list of unknown length from /dev/kmsg.
>> > 	 */
>> >-	igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
>> >-	if (igt_debug_on(igt_kmod_load("kunit",
>> >-				       "filter=module=none filter_action=skip")))
>> >+	if (igt_debug_on(!kunit_set_filtering(NULL, "module=none", "skip"))) {
>>
>> probably in the caller or very early:
>>
>> 	if (!igt_syfs_has_rw_attr())
>> 		return fallback_kunit_execution();
>>
>> I think it's too different this approach from the previous one, that
>> results in the long comments trying to explain each different
>> step.
>
>OK.
>
>>
>> Humn... I guess we can do that on top, so if this works, seems ok for
>> now.
>
>OK.
>
>Thanks,
>Janusz
>
>>
>> Lucas De Marchi
>>
>> >+		/*
>> >+		 * TODO: drop the following workaround, required by LTS kernel
>> >+		 *       v6.1 not capable of listing test cases when built as as
>> >+		 *       module, when no longer needed.
>> >+		 * If updating writable filter parameters of the kunit base
>> >+		 * module with required values fails then assume we are running
>> >+		 * on a kernel with missing test case listing capabilities.
>> >+		 * Don't skip but just return with empty list of test cases,
>> >+		 * which should tell the caller to use a legacy method of
>> >+		 * iterating over KTAP results collected from blind execution
>> >+		 * of all Kunit test cases provided by a Kunit test module.
>> >+		 */
>> > 		return;
>> >+	}
>> >
>> > 	igt_skip_on(modprobe(tst->kmod, opts));
>> >
>> >@@ -1200,16 +1245,7 @@ static void __igt_kunit(struct igt_ktest *tst,
>> > 			      t->case_name) {
>> >
>> > 			if (!modprobe.thread) {
>> >-				/*
>> >-				 * Since we have successfully loaded the kunit
>> >-				 * base module with non-default parameters in
>> >-				 * order to get a list of test cases, now we
>> >-				 * have to unload it so it is then automatically
>> >-				 * reloaded with default parameter values when
>> >-				 * we load the test module again for execution.
>> >-				 */
>> >-				igt_skip_on(igt_kmod_unload("kunit",
>> >-							    KMOD_REMOVE_FORCE));
>> >+				igt_require(kunit_set_filtering(NULL, NULL, NULL));
>> >
>> > 				igt_assert_eq(pthread_mutexattr_init(&attr), 0);
>> > 				igt_assert_eq(pthread_mutexattr_setrobust(&attr,
>> >@@ -1378,6 +1414,9 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
>> > 		igt_assert(igt_list_empty(&tests));
>> > 	}
>> >
>> >+	/* We need the base KUnit module loaded if not built-in */
>> >+	igt_ignore_warn(igt_kmod_load("kunit", NULL));
>> >+
>> > 	/*
>> > 	 * We need to use igt_subtest here, as otherwise it may crash with:
>> > 	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
>>
>
>
>
>

Re: [PATCH i-g-t v3 4/6] lib/kunit: Support writable filter* parameters of kunit module

[Janusz Krzysztofik]

On Friday, 2 February 2024 16:57:46 CET Lucas De Marchi wrote:
> On Fri, Feb 02, 2024 at 11:14:49AM +0100, Janusz Krzysztofik wrote:
> >On Thursday, 1 February 2024 21:26:35 CET Lucas De Marchi wrote:
> >> On Thu, Feb 01, 2024 at 07:59:20PM +0100, Janusz Krzysztofik wrote:
> >> >Instead of wasting resources on reloading the base Kunit module each time
> >> >a different set of filter parameters is needed, try to write the required
> >> >values to sysfs representation of those parameters.  If that fails (e.g.
> >> >on older LTS kernels with read-only filter parameters), fall back to
> >> >legacy "no list of test cases available" mode as we do on kernels with
> >> >those module parameters not supported at all.
> >> >
> >> >This modification will also serve as a workaround for the issue of
> >> >impossibility to unload the base Kunit module on Xe platforms, available
> >> >to IGT as soon as the module supports writable filter parameters.
> >> >
> >> >v3: Don't read-compare-write, just write the values (Lucas),
> >> >  - skip calling igt_sysfs_set() when the string to be written to
> >> >    filter_action is empty (Lucas),
> >> >  - warn if we leave the filter_action set to "skip" while setting a non-
> >> >    default value of the filter parameter,
> >> >  - transform generic kunit_set_params() to kunit_set_filtering().
> >> >v2: Work around ineffective writes of empty strings to sysfs module
> >> >    parameter files (Lucas) by using human readable non-empty strings that
> >> >    give the same results as default NULLs,
> >> >  - drop fallback to reload of base Kunit module method if assigning new
> >> >    values to module parameters via sysfs fails (Lucas), instead use the
> >> >    existing fallback to blind execution like if getting a list of test
> >> >    cases was not supported at all,
> >> >  - split move of open_parameters() helper up in the source file as well
> >> >    as cleanup of base KUnit module unloading to separate patches (Kamil),
> >> >  - address the issue of the second paragraph of commit description
> >> >    suggesting two separate changes combined in one patch (Kamil) by
> >> >    rewording the description.
> >> >
> >> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >> >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >> >Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> >> >---
> >> > lib/igt_kmod.c | 91 +++++++++++++++++++++++++++++++++++---------------
> >> > 1 file changed, 65 insertions(+), 26 deletions(-)
> >> >
> >> >diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> >> >index d2c28d0a64..83416d2aa4 100644
> >> >--- a/lib/igt_kmod.c
> >> >+++ b/lib/igt_kmod.c
> >> >@@ -811,6 +811,49 @@ static int open_parameters(const char *module_name)
> >> > 	return open(path, O_RDONLY);
> >> > }
> >> >
> >> >+static bool kunit_set_filtering(const char *filter_glob, const char *filter,
> >> >+				const char *filter_action)
> >> >+{
> >> >+	int params;
> >> >+	bool ret;
> >> >+
> >> >+	params = open_parameters("kunit");
> >> >+	ret = !igt_debug_on(params < 0);
> >>
> >> why proceed? just return early when it fails pre-conditions
> >
> >Right.
> >
> >> >+
> >> >+	/*
> >> >+	 * Default values of the KUnit base module filtering parameters
> >> >+	 * are all NULLs.  Reapplying those NULLs over sysfs once
> >> >+	 * overwritten with non-NULL strings seems not possible.
> >> >+	 * As a workaround, we use human-readable strings that exhibit
> >> >+	 * the same behaviour as if default NULLs were in place.
> >> >+	 */
> >> >+	if (ret)
> >> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_glob",
> >> >+						   filter_glob ?: "*"));
> >> >+	if (ret)
> >> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter",
> >> >+						   filter ?: "module!=none"));
> >> >+	/*
> >> >+	 * In case of filter_action parameter there is no obvious human-
> >> >+	 * readable string that behaves like default NULL.  However, if NULL
> >>
> >> that is not true.. it's just igt_sysfs not doing the right thing to
> >
> >That depends on how we feel about empty strings, whether they are human-
> >readable or not.  For me they're not quite, e.g. when simply reading them back
> >from sysfs it's not possible to distinguished them from potential white space,
> >unless some graphical representation of non-printing characters is used.
> >Maybe it would be more clear if kernel was using a convention to display empty
> >strings as e.g. "(empty)", like it displays NULL as "(null)".
> 
> you can't print NULL. You can print empty like it's done now. That's not
> going to change ever, as it would break perfectly usable and sane
> userspace.
> 
> 
> >
> >> write an empty string, that does behave like NULL on the kernel side.
> >
> >Then maybe let's better send the empty string to igt_sysfs_set() instead of
> >skipping that, and replace my "human-readable" with "non-NULL" in the
> 
> as long as the "human-readable" comment is removed, fine. Because empty

I'm going to follow this approach.

> is empty, it's not a non-printing char. Let me try to make it clearer:
> 
> 	# echo -ne a > /sys/module/firmware_class/parameters/path
> 	# hexdump -C /sys/module/firmware_class/parameters/path
> 	00000000  61 0a                                             |a.|
> 	00000002
> 
> There are 2 chars: 'a' and '\n', with the latter always inserted by the
> kernel.
> 
> 	# echo -ne "\0" > /sys/module/firmware_class/parameters/path
> 	# hexdump -C /sys/module/firmware_class/parameters/path
> 	00000000  0a                                                |.|
> 	00000001
> 
> there is 1 char, just the \n inserted by the kernel. Nobody would ever
> ask for the kernel to start printing "(newline)" instead of \n.
> 
> Yes, the kernel side could behave differently depending if it's NULL
> vs empty. kunit doesn't and shouldn't.
> 
> 
> Lucas De Marchi
> 
> >comments.  Thanks to "module!=none" we are still on the safe side, even before
> >igt_sysfs_set() is fixed.
> >
> >> I'd drop these lines and just justify with the lines below why we are
> >> not setting it here.
> >>
> >> with a small change below we can also move these to use igt_require()

I'd rather avoid calling igt_require() before we close params.  Instead, I'll 
convert that to a classic unwind on error pattern.

> >>
> >> >+	 * is requested for both filter and filter_action then that's not a
> >> >+	 * problem for us since even if filter_action was previously set to
> >> >+	 * "skip" we can leave it as is because our "module!=none" default
> >> >+	 * filter guarantees that no test cases are filtered out to be skipped.
> >> >+	 */
> >> >+	if (ret)
> >> >+		ret = !igt_warn_on_f(!filter_action && filter,
> >> >+				     "test error: filter_action=NULL requires filter=NULL\n");
> >> >+	if (ret && filter_action)
> >> >+		ret = !igt_debug_on(!igt_sysfs_set(params, "filter_action",
> >> >+						   filter_action));
> >> >+
> >> >+	if (params >= 0)
> >> >+		close(params);
> >> >+
> >> >+	return ret;
> >> >+}
> >> >+
> >> > struct modprobe_data {
> >> > 	struct kmod_module *kmod;
> >> > 	const char *opts;
> >> >@@ -1116,24 +1159,26 @@ static void kunit_get_tests(struct igt_list_head *tests,
> >> > 	/*
> >> > 	 * To get a list of test cases provided by a kunit test module, ask the
> >> > 	 * generic kunit module to respond with SKIP result for each test found.
> >> >-	 * We could also use action=list kunit parameter to get the listing,
> >> >-	 * however, parsing a KTAP report -- something that we already can do
> >> >-	 * perfectly -- seems to be more safe than extracting a test case list
> >> >-	 * of unknown length from /dev/kmsg.
> >> >-	 *
> >> >-	 * TODO: drop the following workaround, which is required by LTS kernel
> >> >-	 *       v6.1 not capable of listing test cases when built as a module.
> >> >-	 * If loading the kunit module with required parameters fails then
> >> >-	 * assume that we are running on a kernel with missing test case listing
> >> >-	 * capabilities.  Dont's skip but just return with empty list of test
> >> >-	 * cases, that should tell the caller to use a legacy method of
> >> >-	 * iterating over KTAP results collected from blind execution of all
> >> >-	 * Kunit test cases provided by a Kunit test module.
> >> >+	 * We could also try to use action=list kunit parameter to get the
> >> >+	 * listing, however, parsing a structured KTAP report -- something that
> >> >+	 * we already can do perfectly -- seems to be more safe than extracting
> >> >+	 * a free text list of unknown length from /dev/kmsg.
> >> > 	 */
> >> >-	igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
> >> >-	if (igt_debug_on(igt_kmod_load("kunit",
> >> >-				       "filter=module=none filter_action=skip")))
> >> >+	if (igt_debug_on(!kunit_set_filtering(NULL, "module=none", "skip"))) {
> >>
> >> probably in the caller or very early:
> >>
> >> 	if (!igt_syfs_has_rw_attr())
> >> 		return fallback_kunit_execution();
> >>
> >> I think it's too different this approach from the previous one, that
> >> results in the long comments trying to explain each different
> >> step.
> >
> >OK.

I think I've found a nice way to resolve the issue of multiple TODOs on 
several steps back to the same legacy path without introducing your proposed 
check-permissions-before-write.  Please expect a separate patch that will 
replace v3 1/6.

> >
> >>
> >> Humn... I guess we can do that on top, so if this works, seems ok for
> >> now.
> >
> >OK.

Since still in progress, please expect v4 soon.

Thanks,
Janusz

> >
> >Thanks,
> >Janusz
> >
> >>
> >> Lucas De Marchi
> >>
> >> >+		/*
> >> >+		 * TODO: drop the following workaround, required by LTS kernel
> >> >+		 *       v6.1 not capable of listing test cases when built as as
> >> >+		 *       module, when no longer needed.
> >> >+		 * If updating writable filter parameters of the kunit base
> >> >+		 * module with required values fails then assume we are running
> >> >+		 * on a kernel with missing test case listing capabilities.
> >> >+		 * Don't skip but just return with empty list of test cases,
> >> >+		 * which should tell the caller to use a legacy method of
> >> >+		 * iterating over KTAP results collected from blind execution
> >> >+		 * of all Kunit test cases provided by a Kunit test module.
> >> >+		 */
> >> > 		return;
> >> >+	}
> >> >
> >> > 	igt_skip_on(modprobe(tst->kmod, opts));
> >> >
> >> >@@ -1200,16 +1245,7 @@ static void __igt_kunit(struct igt_ktest *tst,
> >> > 			      t->case_name) {
> >> >
> >> > 			if (!modprobe.thread) {
> >> >-				/*
> >> >-				 * Since we have successfully loaded the kunit
> >> >-				 * base module with non-default parameters in
> >> >-				 * order to get a list of test cases, now we
> >> >-				 * have to unload it so it is then automatically
> >> >-				 * reloaded with default parameter values when
> >> >-				 * we load the test module again for execution.
> >> >-				 */
> >> >-				igt_skip_on(igt_kmod_unload("kunit",
> >> >-							    KMOD_REMOVE_FORCE));
> >> >+				igt_require(kunit_set_filtering(NULL, NULL, NULL));
> >> >
> >> > 				igt_assert_eq(pthread_mutexattr_init(&attr), 0);
> >> > 				igt_assert_eq(pthread_mutexattr_setrobust(&attr,
> >> >@@ -1378,6 +1414,9 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
> >> > 		igt_assert(igt_list_empty(&tests));
> >> > 	}
> >> >
> >> >+	/* We need the base KUnit module loaded if not built-in */
> >> >+	igt_ignore_warn(igt_kmod_load("kunit", NULL));
> >> >+
> >> > 	/*
> >> > 	 * We need to use igt_subtest here, as otherwise it may crash with:
> >> > 	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
> >>
> >
> >
> >
> >
>