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

[Janusz Krzysztofik]

Hi Lucas,

On Tuesday, 6 February 2024 11:00:28 CET 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.
> 
> 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.
> 
> v4: Return early when opening module parameters directory fails (Lucas),
>   - use error unwind pattern on errors from sysfs writes,
>   - replace "human-readable" with "non-NULL" in comments about strings
>     used as equivalents of default NULLs,
>   - use empty string as filter_action NULL equivalent, but verify if
>     successfully applied when critical,
>   - refresh commit description by removing a statement that addressed an
>     update to a comment on legacy path, no longer in scope of this patch.

With your closing statement in your initial comments to v3:
"... if this works, seems ok for now."
(https://patchwork.freedesktop.org/patch/576817/?series=129174&rev=3#comment_1052127)
you seemed to accept v3 of this patch even if you had some ideas for further 
updates.  In v4 I tried to address those ideas, one way or another (please see 
v4 cover letter for more information).  Since you haven't commented that new 
version, may I assume you are OK with it?

Thanks,
Janusz


> 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 at linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> ---
>  lib/igt_kmod.c | 75 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 62 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 2a1b09c0bd..5931121944 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -811,6 +811,63 @@ 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");
> +	if (igt_debug_on(params < 0))
> +		return false;
> +
> +	/*
> +	 * 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 non-NULL strings that exhibit
> +	 * the same behaviour as if default NULLs were in place.
> +	 */
> +	ret = !igt_debug_on(!igt_sysfs_set(params, "filter_glob",
> +					   filter_glob ?: "*"));
> +	if (!ret)
> +		goto close;
> +
> +	ret = !igt_debug_on(!igt_sysfs_set(params, "filter",
> +					   filter ?: "module!=none"));
> +	if (!ret)
> +		goto close;
> +
> +	ret = !igt_debug_on(!igt_sysfs_set(params, "filter_action",
> +					   filter_action ?: ""));
> +
> +	/*
> +	 * TODO: Drop the extra check below as soon as igt_sysfs_set()
> +	 *	 can correctly process empty strings which we are using
> +	 *	 as filter_action NULL equivalent.
> +	 *
> +	 * We need this check only when NULL is requested for "filter_action"
> +	 * and not for "filter" parameter, otherwise, even if "filter_action"
> +	 * was previously set to "skip", we don't care since our
> +	 * "module!=none" default filter guarantees that no test cases are
> +	 * filtered out to be processed as "filter_action" says.
> +	 */
> +	if (ret && !filter_action && filter) {
> +		filter_action = igt_sysfs_get(params, "filter_action");
> +
> +		ret = !(igt_debug_on_f(!filter_action,
> +				       "open() failed\n") ||
> +			igt_debug_on_f(strlen(filter_action),
> +				       "empty string not applied\n"));
> +		free((char *)filter_action);
> +	}
> +
> +close:
> +	close(params);
> +
> +	return ret;
> +}
> +
>  struct modprobe_data {
>  	struct kmod_module *kmod;
>  	const char *opts;
> @@ -1121,9 +1178,7 @@ static bool kunit_get_tests(struct igt_list_head *tests,
>  	 * perfectly -- seems to be more safe than extracting a test case 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")))
>  		return false;
>  
>  	igt_skip_on(modprobe(tst->kmod, opts));
> @@ -1192,16 +1247,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,
> @@ -1370,6 +1416,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
>