[i-g-t,v2,04/11] lib/kunit: Parse KTAP report from the main process thread

Commit Message

Janusz Krzysztofik Oct. 9, 2023, 12:27 p.m. UTC

There was an attempt to parse KTAP reports in the background while a kunit
test module is loading.  However, since dynamic sub-subtests can be
executed only from the main thread, that attempt was not quite successful,
as IGT results from all executed kunit test cases were generated only
after loading of kunit test module completed.

Now that the parser maintains its state and we can call it separately for
each input line of a KTAP report, it is perfectly possible to call the
parser from the main thread while the module is loading in the background,
and convert results from kunit test cases immediately to results of IGT
dynamic sub-subtests by running an igt_dynamic() section for each result
as soon as returned by the parser.

Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
result obtained from igt_ktap_parse() called from the main thread.

Also, drop no longer needed functions from igt_ktap soruces.

v3: Fix ktap structure not freed on lseek error,
  - fix initial SIGCHLD handler not restored,
  - fix missing handling of potential errors returned by sigaction,
  - fix potential race of read() vs. ptherad_kill(), use robust mutex for
    synchronization with modprobe thread,
  - fix potentially illegal use of igt_assert() called outside of
    dynamic sub-subtest section,
  - fix unsupported exit code potentially passed to igt_fail(),
  - no need to fail a dynamic sub-subtest on potential KTAP parser error
    after a valid result from the parser has been processed,
  - fix trailing newlines missing from error messages,
  - add more debug statements,
  - integrate common code around kunit_result_free() into it.
v2: Interrupt blocking read() on modprobe failure.

Signed-off-by: Janusz Krzysztofik <[e-mail]>
Acked-by: Mauro Carvalho Chehab <[e-mail]> # v2
---
 lib/igt_kmod.c | 261 +++++++++++++++++++----
 lib/igt_ktap.c | 568 -------------------------------------------------
 lib/igt_ktap.h |  22 --
 3 files changed, 222 insertions(+), 629 deletions(-)

Comments

Mauro Carvalho Chehab Oct. 10, 2023, 1:33 p.m. UTC | #1

On Mon,  9 Oct 2023 14:27:55 +0200
Janusz Krzysztofik <[e-mail]> wrote:

> There was an attempt to parse KTAP reports in the background while a kunit
> test module is loading.  However, since dynamic sub-subtests can be
> executed only from the main thread, that attempt was not quite successful,
> as IGT results from all executed kunit test cases were generated only
> after loading of kunit test module completed.
> 
> Now that the parser maintains its state and we can call it separately for
> each input line of a KTAP report, it is perfectly possible to call the
> parser from the main thread while the module is loading in the background,
> and convert results from kunit test cases immediately to results of IGT
> dynamic sub-subtests by running an igt_dynamic() section for each result
> as soon as returned by the parser.
> 
> Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> result obtained from igt_ktap_parse() called from the main thread.
> 
> Also, drop no longer needed functions from igt_ktap soruces.
> 
> v3: Fix ktap structure not freed on lseek error,
>   - fix initial SIGCHLD handler not restored,
>   - fix missing handling of potential errors returned by sigaction,
>   - fix potential race of read() vs. ptherad_kill(), use robust mutex for
>     synchronization with modprobe thread,
>   - fix potentially illegal use of igt_assert() called outside of
>     dynamic sub-subtest section,
>   - fix unsupported exit code potentially passed to igt_fail(),
>   - no need to fail a dynamic sub-subtest on potential KTAP parser error
>     after a valid result from the parser has been processed,
>   - fix trailing newlines missing from error messages,
>   - add more debug statements,
>   - integrate common code around kunit_result_free() into it.
> v2: Interrupt blocking read() on modprobe failure.
> 
> Signed-off-by: Janusz Krzysztofik <[e-mail]>
> Acked-by: Mauro Carvalho Chehab <[e-mail]> # v2
> ---
>  lib/igt_kmod.c | 261 +++++++++++++++++++----
>  lib/igt_ktap.c | 568 -------------------------------------------------
>  lib/igt_ktap.h |  22 --
>  3 files changed, 222 insertions(+), 629 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 426ae5b26f..7bca4cdaab 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright © 2016 Intel Corporation
> + * Copyright © 2016-2023 Intel Corporation
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a
>   * copy of this software and associated documentation files (the "Software"),
> @@ -26,7 +26,12 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <pthread.h>
> +#include <stdlib.h>
> +#include <string.h>
>  #include <sys/utsname.h>
> +#include <unistd.h>
> +
> +#include "assembler/brw_compat.h"	/* [un]likely() */
>  
>  #include "igt_aux.h"
>  #include "igt_core.h"
> @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
>  }
>  
>  struct modprobe_data {
> +	pthread_t parent;
>  	struct kmod_module *kmod;
>  	const char *opts;
>  	int err;
> +	pthread_mutex_t lock;
> +	pthread_t thread;
>  };
>  
>  static void *modprobe_task(void *arg)
> @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
>  
>  	data->err = modprobe(data->kmod, data->opts);
>  
> +	if (igt_debug_on(data->err)) {
> +		int err;
> +
> +		while (err = pthread_mutex_trylock(&data->lock),
> +		       err && !igt_debug_on(err != EBUSY))
> +			igt_debug_on(pthread_kill(data->parent, SIGCHLD));

I guess you need here an "igt_debug_on_once", as it doesn't make
sense to have a (potentially) endless loop here printing error
messages.

the other changes LGTM.

> +	} else {
> +		/* let main thread use mutex to detect modprobe completion */
> +		igt_debug_on(pthread_mutex_lock(&data->lock));
> +	}
> +
>  	return NULL;
>  }
>  
> +static void kunit_sigchld_handler(int signal)
> +{
> +	return;
> +}
> +
> +static int kunit_kmsg_result_get(struct igt_list_head *results,
> +				 struct modprobe_data *modprobe,
> +				 int fd, struct igt_ktap_results *ktap)
> +{
> +	struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> +			 *saved;
> +	char record[BUF_LEN + 1], *buf;
> +	unsigned long taints;
> +	int ret;
> +
> +	do {
> +		int err;
> +
> +		if (igt_debug_on(igt_kernel_tainted(&taints)))
> +			return -ENOTRECOVERABLE;
> +
> +		err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved));
> +		if (err == -1)
> +			return -errno;
> +		else if (unlikely(err))
> +			return err;
> +
> +		err = pthread_mutex_lock(&modprobe->lock);
> +		switch (err) {
> +		case EOWNERDEAD:
> +			/* leave the mutex unrecoverable */
> +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> +			__attribute__ ((fallthrough));
> +		case ENOTRECOVERABLE:
> +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> +			if (igt_debug_on(modprobe->err))
> +				return modprobe->err;
> +			break;
> +		case 0:
> +			break;
> +		default:
> +			igt_debug("pthread_mutex_lock() error: %d\n", err);
> +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> +			return -err;
> +		}
> +
> +		ret = read(fd, record, BUF_LEN);
> +
> +		if (!err) {
> +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> +		}
> +
> +		if (igt_debug_on(!ret))
> +			return -ENODATA;
> +		if (igt_debug_on(ret == -1))
> +			return -errno;
> +		if (unlikely(igt_debug_on(ret < 0)))
> +			break;
> +
> +		/* skip kmsg continuation lines */
> +		if (igt_debug_on(*record == ' '))
> +			continue;
> +
> +		/* NULL-terminate the record */
> +		record[ret] = '\0';
> +
> +		/* detect start of log message, continue if not found */
> +		buf = strchrnul(record, ';');
> +		if (igt_debug_on(*buf == '\0'))
> +			continue;
> +		buf++;
> +
> +		ret = igt_ktap_parse(buf, ktap);
> +		if (!ret || igt_debug_on(ret != -EINPROGRESS))
> +			break;
> +	} while (igt_list_empty(results));
> +
> +	return ret;
> +}
> +
> +static void kunit_result_free(struct igt_ktap_result **r,
> +			      char **suite_name, char **case_name)
> +{
> +	if (!*r)
> +		return;
> +
> +	igt_list_del(&(*r)->link);
> +
> +	if ((*r)->suite_name != *suite_name) {
> +		free(*suite_name);
> +		*suite_name = (*r)->suite_name;
> +	}
> +
> +	if ((*r)->case_name != *case_name) {
> +		free(*case_name);
> +		*case_name = (*r)->case_name;
> +	}
> +
> +	free((*r)->msg);
> +	free(*r);
> +	*r = NULL;
> +}
> +
>  static void __igt_kunit(struct igt_ktest *tst, const char *opts)
>  {
> -	struct modprobe_data modprobe = { tst->kmod, opts, 0, };
> -	struct kmod_module *kunit_kmod;
> -	bool is_builtin;
> -	struct ktap_test_results *results;
> -	pthread_t modprobe_thread;
> +	struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, };
> +	char *suite_name = NULL, *case_name = NULL;
> +	struct igt_ktap_result *r, *rn;
> +	struct igt_ktap_results *ktap;
> +	pthread_mutexattr_t attr;
> +	IGT_LIST_HEAD(results);
>  	unsigned long taints;
>  	int flags, ret;
>  
> @@ -780,60 +904,119 @@ static void __igt_kunit(struct igt_ktest *tst, const char *opts)
>  
>  	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
>  
> -	igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod));
> -	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> -	kmod_module_unref(kunit_kmod);
> +	igt_skip_on(pthread_mutexattr_init(&attr));
> +	igt_skip_on(pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST));
> +	igt_skip_on(pthread_mutex_init(&modprobe.lock, &attr));
>  
> -	results = ktap_parser_start(tst->kmsg, is_builtin);
> +	ktap = igt_ktap_alloc(&results);
> +	igt_require(ktap);
>  
> -	if (igt_debug_on(pthread_create(&modprobe_thread, NULL,
> +	if (igt_debug_on(pthread_create(&modprobe.thread, NULL,
>  					modprobe_task, &modprobe))) {
> -		ktap_parser_cancel();
> -		igt_ignore_warn(ktap_parser_stop());
> +		igt_ktap_free(ktap);
>  		igt_skip("Failed to create a modprobe thread\n");
>  	}
>  
> -	while (READ_ONCE(results->still_running) || !igt_list_empty(&results->list))
> -	{
> -		struct ktap_test_results_element *result;
> -
> -		if (!pthread_tryjoin_np(modprobe_thread, NULL) && modprobe.err) {
> -			ktap_parser_cancel();
> +	do {
> +		ret = kunit_kmsg_result_get(&results, &modprobe,
> +					    tst->kmsg, ktap);
> +		if (igt_debug_on(ret && ret != -EINPROGRESS))
>  			break;
> -		}
>  
> -		if (igt_kernel_tainted(&taints)) {
> -			ktap_parser_cancel();
> -			pthread_cancel(modprobe_thread);
> +		if (igt_debug_on(igt_list_empty(&results)))
>  			break;
> -		}
>  
> -		pthread_mutex_lock(&results->mutex);
> -		if (igt_list_empty(&results->list)) {
> -			pthread_mutex_unlock(&results->mutex);
> -			continue;
> -		}
> +		r = igt_list_first_entry(&results, r, link);
>  
> -		result = igt_list_first_entry(&results->list, result, link);
> +		igt_dynamic_f("%s-%s", r->suite_name, r->case_name) {
> +			if (r->code == IGT_EXIT_INVALID) {
> +				/* parametrized test case, get actual result */
> +				kunit_result_free(&r, &suite_name, &case_name);
>  
> -		igt_list_del(&result->link);
> -		pthread_mutex_unlock(&results->mutex);
> +				igt_assert(igt_list_empty(&results));
>  
> -		igt_dynamic(result->test_name) {
> -			igt_assert(READ_ONCE(result->passed));
> +				ret = kunit_kmsg_result_get(&results, &modprobe,
> +							    tst->kmsg, ktap);
> +				if (ret != -EINPROGRESS)
> +					igt_fail_on(ret);
> +
> +				igt_fail_on(igt_list_empty(&results));
> +
> +				r = igt_list_first_entry(&results, r, link);
> +
> +				igt_fail_on_f(strcmp(r->suite_name, suite_name),
> +					      "suite_name expected: %s, got: %s\n",
> +					      suite_name, r->suite_name);
> +				igt_fail_on_f(strcmp(r->case_name, case_name),
> +					      "case_name expected: %s, got: %s\n",
> +					      case_name, r->case_name);
> +			}
>  
> -			if (!pthread_tryjoin_np(modprobe_thread, NULL))
> +			igt_assert_neq(r->code, IGT_EXIT_INVALID);
> +
> +			if (r->msg && *r->msg) {
> +				igt_skip_on_f(r->code == IGT_EXIT_SKIP,
> +					      "%s\n", r->msg);
> +				igt_fail_on_f(r->code == IGT_EXIT_FAILURE,
> +					      "%s\n", r->msg);
> +				igt_abort_on_f(r->code == IGT_EXIT_ABORT,
> +					      "%s\n", r->msg);
> +			} else {
> +				igt_skip_on(r->code == IGT_EXIT_SKIP);
> +				igt_fail_on(r->code == IGT_EXIT_FAILURE);
> +				if (r->code == IGT_EXIT_ABORT)
> +					igt_fail(r->code);
> +			}
> +			igt_assert_eq(r->code, IGT_EXIT_SUCCESS);
> +
> +			switch (pthread_mutex_lock(&modprobe.lock)) {
> +			case 0:
> +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> +				break;
> +			case EOWNERDEAD:
> +				/* leave the mutex unrecoverable */
> +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> +				__attribute__ ((fallthrough));
> +			case ENOTRECOVERABLE:
>  				igt_assert_eq(modprobe.err, 0);
> +				break;
> +			default:
> +				igt_debug("pthread_mutex_lock() failed\n");
> +				break;
> +			}
>  
>  			igt_assert_eq(igt_kernel_tainted(&taints), 0);
>  		}
>  
> -		free(result);
> +		kunit_result_free(&r, &suite_name, &case_name);
> +
> +	} while (ret == -EINPROGRESS);
> +
> +	igt_list_for_each_entry_safe(r, rn, &results, link)
> +		kunit_result_free(&r, &suite_name, &case_name);
> +
> +	free(case_name);
> +	free(suite_name);
> +
> +	switch (pthread_mutex_lock(&modprobe.lock)) {
> +	case 0:
> +		igt_debug_on(pthread_cancel(modprobe.thread));
> +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> +		break;
> +	case EOWNERDEAD:
> +		/* leave the mutex unrecoverable */
> +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> +		break;
> +	case ENOTRECOVERABLE:
> +		break;
> +	default:
> +		igt_debug("pthread_mutex_lock() failed\n");
> +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> +		break;
>  	}
>  
> -	pthread_join(modprobe_thread, NULL);
> -
> -	ret = ktap_parser_stop();
> +	igt_ktap_free(ktap);
>  
>  	igt_skip_on(modprobe.err);
>  	igt_skip_on(igt_kernel_tainted(&taints));
> diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> index 5eac102417..53a6c63288 100644
> --- a/lib/igt_ktap.c
> +++ b/lib/igt_ktap.c
> @@ -4,17 +4,11 @@
>   * Copyright © 2023 Intel Corporation
>   */
>  
> -#include <ctype.h>
> -#include <limits.h>
> -#include <libkmod.h>
> -#include <pthread.h>
>  #include <errno.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <unistd.h>
>  
> -#include "igt_aux.h"
>  #include "igt_core.h"
>  #include "igt_ktap.h"
>  #include "igt_list.h"
> @@ -312,565 +306,3 @@ void igt_ktap_free(struct igt_ktap_results *ktap)
>  {
>  	free(ktap);
>  }
> -
> -#define DELIMITER "-"
> -
> -struct ktap_parser_args {
> -	int fd;
> -	bool is_builtin;
> -	int ret;
> -} ktap_args;
> -
> -static struct ktap_test_results results;
> -
> -static int log_to_end(enum igt_log_level level, int fd,
> -		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
> -
> -/**
> - * log_to_end:
> - * @level: #igt_log_level
> - * @record: record to store the read data
> - * @format: format string
> - * @...: optional arguments used in the format string
> - *
> - * This is an altered version of the generic structured logging helper function
> - * igt_log capable of reading to the end of a given line.
> - *
> - * Returns: 0 for success, or -2 if there's an error reading from the file
> - */
> -static int log_to_end(enum igt_log_level level, int fd,
> -		      char *record, const char *format, ...)
> -{
> -	va_list args;
> -	const char *lend;
> -
> -	/* Cutoff after newline character, in order to not display garbage */
> -	char *cutoff = strchr(record, '\n');
> -	if (cutoff) {
> -		if (cutoff - record < BUF_LEN)
> -			cutoff[1] = '\0';
> -	}
> -
> -	va_start(args, format);
> -	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
> -	va_end(args);
> -
> -	lend = strchrnul(record, '\n');
> -	while (*lend == '\0') {
> -		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
> -
> -		if (read(fd, record, BUF_LEN) < 0) {
> -			if (errno == EPIPE)
> -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -			else
> -				igt_warn("an error occurred while reading kmsg: %m\n");
> -
> -			return -2;
> -		}
> -
> -		lend = strchrnul(record, '\n');
> -	}
> -	return 0;
> -}
> -
> -/**
> - * lookup_value:
> - * @haystack: the string to search in
> - * @needle: the string to search for
> - *
> - * Returns: the value of the needle in the haystack, or -1 if not found.
> - */
> -static long lookup_value(const char *haystack, const char *needle)
> -{
> -	const char *needle_rptr;
> -	char *needle_end;
> -	long num;
> -
> -	needle_rptr = strcasestr(haystack, needle);
> -
> -	if (needle_rptr == NULL)
> -		return -1;
> -
> -	/* Skip search string and whitespaces after it */
> -	needle_rptr += strlen(needle);
> -
> -	num = strtol(needle_rptr, &needle_end, 10);
> -
> -	if (needle_rptr == needle_end)
> -		return -1;
> -
> -	if (num == LONG_MIN || num == LONG_MAX)
> -		return 0;
> -
> -	return num > 0 ? num : 0;
> -}
> -
> -/**
> - * tap_version_present:
> - * @record: buffer with tap data
> - * @print_info: whether tap version should be printed or not
> - *
> - * Returns:
> - * 0 if not found
> - * 1 if found
> - */
> -static int tap_version_present(char* record, bool print_info)
> -{
> -	/*
> -	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> -	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
> -	 *
> -	 * but actually isn't, as it currently depends on the KUnit module
> -	 * being built-in, so we can't rely on it every time
> -	 */
> -	const char *version_rptr = strcasestr(record, "TAP version ");
> -	char *cutoff;
> -
> -	if (version_rptr == NULL)
> -		return 0;
> -
> -	/* Cutoff after newline character, in order to not display garbage */
> -	cutoff = strchr(version_rptr, '\n');
> -	if (cutoff)
> -		cutoff[0] = '\0';
> -
> -	if (print_info)
> -		igt_info("%s\n", version_rptr);
> -
> -	return 1;
> -}
> -
> -/**
> - * find_next_tap_subtest:
> - * @fd: file descriptor
> - * @record: buffer used to read fd
> - * @is_builtin: whether KUnit is built-in or not
> - *
> - * Returns:
> - * 0 if there's missing information
> - * -1 if not found
> - * -2 if there are problems while reading the file.
> - * any other value corresponds to the amount of cases of the next (sub)test
> - */
> -static int find_next_tap_subtest(int fd, char *record, char *test_name, bool is_builtin)
> -{
> -	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
> -	long test_count;
> -	char *cutoff;
> -
> -	test_name[0] = '\0';
> -	test_name[BUF_LEN] = '\0';
> -
> -	test_lookup_str = " subtest: ";
> -	subtest_lookup_str = " test: ";
> -
> -	if (!tap_version_present(record, true))
> -		return -1;
> -
> -	if (is_builtin) {
> -		if (read(fd, record, BUF_LEN) < 0) {
> -			if (errno == EPIPE)
> -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -			else
> -				igt_warn("an error occurred while reading kmsg: %m\n");
> -
> -			return -2;
> -		}
> -	}
> -
> -	name_rptr = strcasestr(record, test_lookup_str);
> -	if (name_rptr != NULL) {
> -		name_rptr += strlen(test_lookup_str);
> -	} else {
> -		name_rptr = strcasestr(record, subtest_lookup_str);
> -		if (name_rptr != NULL)
> -			name_rptr += strlen(subtest_lookup_str);
> -	}
> -
> -	if (name_rptr == NULL) {
> -		if (!is_builtin)
> -			/* We've probably found nothing */
> -			return -1;
> -		igt_info("Missing test name\n");
> -	} else {
> -		strncpy(test_name, name_rptr, BUF_LEN);
> -		/* Cutoff after newline character, in order to not display garbage */
> -		cutoff = strchr(test_name, '\n');
> -		if (cutoff)
> -			cutoff[0] = '\0';
> -
> -		if (read(fd, record, BUF_LEN) < 0) {
> -			if (errno == EPIPE)
> -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -			else
> -				igt_warn("unknown error reading kmsg (%m)\n");
> -
> -			return -2;
> -		}
> -
> -		/* Now we can be sure we found tests */
> -		if (!is_builtin)
> -			igt_info("KUnit is not built-in, skipping version check...\n");
> -	}
> -
> -	/*
> -	 * Total test count will almost always appear as 0..N at the beginning
> -	 * of a run, so we use it to reliably identify a new run
> -	 */
> -	test_count = lookup_value(record, "..");
> -
> -	if (test_count <= 0) {
> -		igt_info("Missing test count\n");
> -		if (test_name[0] == '\0')
> -			return 0;
> -		if (log_to_end(IGT_LOG_INFO, fd, record,
> -				"Running some tests in: %s\n",
> -				test_name) < 0)
> -			return -2;
> -		return 0;
> -	} else if (test_name[0] == '\0') {
> -		igt_info("Running %ld tests...\n", test_count);
> -		return 0;
> -	}
> -
> -	if (log_to_end(IGT_LOG_INFO, fd, record,
> -			"Executing %ld tests in: %s\n",
> -			test_count, test_name) < 0)
> -		return -2;
> -
> -	return test_count;
> -}
> -
> -/**
> - * parse_kmsg_for_tap:
> - * @fd: file descriptor
> - * @record: buffer used to read fd
> - * @test_name: buffer to store the test name
> - *
> - * Returns:
> - * 1 if no results were found
> - * 0 if a test succeded
> - * -1 if a test failed
> - * -2 if there are problems reading the file
> - */
> -static int parse_kmsg_for_tap(int fd, char *record, char *test_name)
> -{
> -	const char *lstart, *ok_lookup_str, *nok_lookup_str,
> -	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
> -	char *test_name_end;
> -
> -	ok_lookup_str = "ok ";
> -	nok_lookup_str = "not ok ";
> -
> -	lstart = strchrnul(record, ';');
> -
> -	if (*lstart == '\0') {
> -		igt_warn("kmsg truncated: output malformed (%m)\n");
> -		return -2;
> -	}
> -
> -	lstart++;
> -	while (isspace(*lstart))
> -		lstart++;
> -
> -	nok_rptr = strstr(lstart, nok_lookup_str);
> -	if (nok_rptr != NULL) {
> -		nok_rptr += strlen(nok_lookup_str);
> -		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
> -			nok_rptr++;
> -		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
> -		while (!isspace(*test_name_end))
> -			test_name_end++;
> -		*test_name_end = '\0';
> -		if (log_to_end(IGT_LOG_WARN, fd, record,
> -			       "%s", lstart) < 0)
> -			return -2;
> -		return -1;
> -	}
> -
> -	comment_start = strchrnul(lstart, '#');
> -
> -	/* Check if we're still in a subtest */
> -	if (*comment_start != '\0') {
> -		comment_start++;
> -		value_parse_start = comment_start;
> -
> -		if (lookup_value(value_parse_start, "fail: ") > 0) {
> -			if (log_to_end(IGT_LOG_WARN, fd, record,
> -				       "%s", lstart) < 0)
> -				return -2;
> -			return -1;
> -		}
> -	}
> -
> -	ok_rptr = strstr(lstart, ok_lookup_str);
> -	if (ok_rptr != NULL) {
> -		ok_rptr += strlen(ok_lookup_str);
> -		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
> -			ok_rptr++;
> -		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
> -		while (!isspace(*test_name_end))
> -			test_name_end++;
> -		*test_name_end = '\0';
> -		return 0;
> -	}
> -
> -	return 1;
> -}
> -
> -/**
> - * parse_tap_level:
> - * @fd: file descriptor
> - * @base_test_name: test_name from upper recursion level
> - * @test_count: test_count of this level
> - * @failed_tests: top level failed_tests pointer
> - * @found_tests: top level found_tests pointer
> - * @is_builtin: whether the KUnit module is built-in or not
> - *
> - * Returns:
> - * 0 if succeded
> - * -1 if error occurred
> - */
> -__maybe_unused
> -static int parse_tap_level(int fd, char *base_test_name, int test_count, bool *failed_tests,
> -			   bool *found_tests, bool is_builtin)
> -{
> -	char record[BUF_LEN + 1];
> -	struct ktap_test_results_element *r;
> -	int internal_test_count;
> -	char test_name[BUF_LEN + 1];
> -	char base_test_name_for_next_level[BUF_LEN + 1];
> -
> -	for (int i = 0; i < test_count; i++) {
> -		if (read(fd, record, BUF_LEN) < 0) {
> -			if (errno == EPIPE)
> -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -			else
> -				igt_warn("error reading kmsg (%m)\n");
> -
> -			return -1;
> -		}
> -
> -		/* Sublevel found */
> -		if (tap_version_present(record, false))
> -		{
> -			internal_test_count = find_next_tap_subtest(fd, record, test_name,
> -								    is_builtin);
> -			switch (internal_test_count) {
> -			case -2:
> -				/* No more data to read */
> -				return -1;
> -			case -1:
> -				/* No test found */
> -				return -1;
> -			case 0:
> -				/* Tests found, but they're missing info */
> -				*found_tests = true;
> -				return -1;
> -			default:
> -				*found_tests = true;
> -
> -				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
> -				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
> -				    base_test_name_for_next_level[0])
> -					strncat(base_test_name_for_next_level, DELIMITER,
> -						BUF_LEN - strlen(base_test_name_for_next_level));
> -				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
> -				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
> -
> -				if (parse_tap_level(fd, base_test_name_for_next_level,
> -						    internal_test_count, failed_tests, found_tests,
> -						    is_builtin) == -1)
> -					return -1;
> -				break;
> -			}
> -		}
> -
> -		switch (parse_kmsg_for_tap(fd, record, test_name)) {
> -		case -2:
> -			return -1;
> -		case -1:
> -			*failed_tests = true;
> -
> -			r = malloc(sizeof(*r));
> -
> -			memcpy(r->test_name, base_test_name, BUF_LEN);
> -			if (strlen(r->test_name) < BUF_LEN - 1)
> -				if (r->test_name[0])
> -					strncat(r->test_name, DELIMITER,
> -						BUF_LEN - strlen(r->test_name));
> -			memcpy(r->test_name + strlen(r->test_name), test_name,
> -			       BUF_LEN - strlen(r->test_name));
> -			r->test_name[BUF_LEN] = '\0';
> -
> -			r->passed = false;
> -
> -			pthread_mutex_lock(&results.mutex);
> -			igt_list_add_tail(&r->link, &results.list);
> -			pthread_mutex_unlock(&results.mutex);
> -
> -			test_name[0] = '\0';
> -			break;
> -		case 0:
> -			r = malloc(sizeof(*r));
> -
> -			memcpy(r->test_name, base_test_name, BUF_LEN);
> -			if (strlen(r->test_name) < BUF_LEN - 1)
> -				if (r->test_name[0])
> -					strncat(r->test_name, DELIMITER,
> -						BUF_LEN - strlen(r->test_name));
> -			memcpy(r->test_name + strlen(r->test_name), test_name,
> -			       BUF_LEN - strlen(r->test_name));
> -			r->test_name[BUF_LEN] = '\0';
> -
> -			r->passed = true;
> -
> -			pthread_mutex_lock(&results.mutex);
> -			igt_list_add_tail(&r->link, &results.list);
> -			pthread_mutex_unlock(&results.mutex);
> -
> -			test_name[0] = '\0';
> -			break;
> -		default:
> -			break;
> -		}
> -	}
> -	return 0;
> -}
> -
> -/**
> - * igt_ktap_parser:
> - *
> - * This function parses the output of a ktap script and passes it to main thread.
> - */
> -void *igt_ktap_parser(void *unused)
> -{
> -	char record[BUF_LEN + 1], *buf, *suite_name = NULL, *case_name = NULL;
> -	struct igt_ktap_results *ktap = NULL;
> -	int fd = ktap_args.fd;
> -	IGT_LIST_HEAD(list);
> -	int err;
> -
> -	ktap = igt_ktap_alloc(&list);
> -	if (igt_debug_on(!ktap))
> -		goto igt_ktap_parser_end;
> -
> -	while (err = read(fd, record, BUF_LEN), err > 0) {
> -		struct igt_ktap_result *r, *rn;
> -
> -		/* skip kmsg continuation lines */
> -		if (igt_debug_on(*record == ' '))
> -			continue;
> -
> -		/* NULL-terminate the record */
> -		record[err] = '\0';
> -
> -		/* detect start of log message, continue if not found */
> -		buf = strchrnul(record, ';');
> -		if (igt_debug_on(*buf == '\0'))
> -			continue;
> -		buf++;
> -
> -		err = igt_ktap_parse(buf, ktap);
> -
> -		/* parsing error */
> -		if (err && err != -EINPROGRESS)
> -			goto igt_ktap_parser_end;
> -
> -		igt_list_for_each_entry_safe(r, rn, &list, link) {
> -			struct ktap_test_results_element *result = NULL;
> -			int code = r->code;
> -
> -			if (code != IGT_EXIT_INVALID)
> -				result = calloc(1, sizeof(*result));
> -
> -			if (result) {
> -				snprintf(result->test_name, sizeof(result->test_name),
> -					 "%s-%s", r->suite_name, r->case_name);
> -
> -				if (code == IGT_EXIT_SUCCESS)
> -					result->passed = true;
> -			}
> -
> -			igt_list_del(&r->link);
> -			if (r->suite_name != suite_name) {
> -				free(suite_name);
> -				suite_name = r->suite_name;
> -			}
> -			if (r->case_name != case_name) {
> -				free(case_name);
> -				case_name = r->case_name;
> -			}
> -			free(r->msg);
> -			free(r);
> -
> -			/*
> -			 * no extra result record expected on start
> -			 * of parametrized test case -- skip it
> -			 */
> -			if (code == IGT_EXIT_INVALID)
> -				continue;
> -
> -			if (!result) {
> -				err = -ENOMEM;
> -				goto igt_ktap_parser_end;
> -			}
> -
> -			pthread_mutex_lock(&results.mutex);
> -			igt_list_add_tail(&result->link, &results.list);
> -			pthread_mutex_unlock(&results.mutex);
> -		}
> -
> -		/* end of KTAP report */
> -		if (!err)
> -			goto igt_ktap_parser_end;
> -	}
> -
> -	if (err < 0) {
> -		if (errno == EPIPE)
> -			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -		else
> -			igt_warn("error reading kmsg (%m)\n");
> -	}
> -
> -igt_ktap_parser_end:
> -	free(suite_name);
> -	free(case_name);
> -
> -	if (!err)
> -		ktap_args.ret = IGT_EXIT_SUCCESS;
> -
> -	results.still_running = false;
> -
> -	if (ktap)
> -		igt_ktap_free(ktap);
> -
> -	return NULL;
> -}
> -
> -static pthread_t ktap_parser_thread;
> -
> -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin)
> -{
> -	IGT_INIT_LIST_HEAD(&results.list);
> -	pthread_mutex_init(&results.mutex, NULL);
> -	results.still_running = true;
> -
> -	ktap_args.fd = fd;
> -	ktap_args.is_builtin = is_builtin;
> -	ktap_args.ret = IGT_EXIT_FAILURE;
> -	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
> -
> -	return &results;
> -}
> -
> -void ktap_parser_cancel(void)
> -{
> -	pthread_cancel(ktap_parser_thread);
> -}
> -
> -int ktap_parser_stop(void)
> -{
> -	pthread_join(ktap_parser_thread, NULL);
> -	return ktap_args.ret;
> -}
> diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> index 6f8da3eab6..c422636bfc 100644
> --- a/lib/igt_ktap.h
> +++ b/lib/igt_ktap.h
> @@ -27,8 +27,6 @@
>  
>  #define BUF_LEN 4096
>  
> -#include <pthread.h>
> -
>  #include "igt_list.h"
>  
>  struct igt_ktap_result {
> @@ -45,24 +43,4 @@ struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results);
>  int igt_ktap_parse(const char *buf, struct igt_ktap_results *ktap);
>  void igt_ktap_free(struct igt_ktap_results *ktap);
>  
> -void *igt_ktap_parser(void *unused);
> -
> -typedef struct ktap_test_results_element {
> -	char test_name[BUF_LEN + 1];
> -	bool passed;
> -	struct igt_list_head link;
> -} ktap_test_results_element;
> -
> -struct ktap_test_results {
> -	struct igt_list_head list;
> -	pthread_mutex_t mutex;
> -	bool still_running;
> -};
> -
> -
> -
> -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin);
> -void ktap_parser_cancel(void);
> -int ktap_parser_stop(void);
> -
>  #endif /* IGT_KTAP_H */

Kamil Konieczny Oct. 10, 2023, 3:59 p.m. UTC | #2

Hi Janusz,
On 2023-10-09 at 14:27:55 +0200, Janusz Krzysztofik wrote:
> There was an attempt to parse KTAP reports in the background while a kunit
> test module is loading.  However, since dynamic sub-subtests can be
> executed only from the main thread, that attempt was not quite successful,
> as IGT results from all executed kunit test cases were generated only
> after loading of kunit test module completed.
> 
> Now that the parser maintains its state and we can call it separately for
> each input line of a KTAP report, it is perfectly possible to call the
> parser from the main thread while the module is loading in the background,
> and convert results from kunit test cases immediately to results of IGT
> dynamic sub-subtests by running an igt_dynamic() section for each result
> as soon as returned by the parser.
> 
> Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> result obtained from igt_ktap_parse() called from the main thread.
> 
> Also, drop no longer needed functions from igt_ktap soruces.
> 
> v3: Fix ktap structure not freed on lseek error,
>   - fix initial SIGCHLD handler not restored,
>   - fix missing handling of potential errors returned by sigaction,
>   - fix potential race of read() vs. ptherad_kill(), use robust mutex for
>     synchronization with modprobe thread,
>   - fix potentially illegal use of igt_assert() called outside of
>     dynamic sub-subtest section,
>   - fix unsupported exit code potentially passed to igt_fail(),
>   - no need to fail a dynamic sub-subtest on potential KTAP parser error
>     after a valid result from the parser has been processed,
>   - fix trailing newlines missing from error messages,
>   - add more debug statements,
>   - integrate common code around kunit_result_free() into it.
> v2: Interrupt blocking read() on modprobe failure.
> 
> Signed-off-by: Janusz Krzysztofik <[e-mail]>
> Acked-by: Mauro Carvalho Chehab <[e-mail]> # v2
> ---
>  lib/igt_kmod.c | 261 +++++++++++++++++++----
>  lib/igt_ktap.c | 568 -------------------------------------------------
>  lib/igt_ktap.h |  22 --
>  3 files changed, 222 insertions(+), 629 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 426ae5b26f..7bca4cdaab 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright © 2016 Intel Corporation
> + * Copyright © 2016-2023 Intel Corporation
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a
>   * copy of this software and associated documentation files (the "Software"),
> @@ -26,7 +26,12 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <pthread.h>
> +#include <stdlib.h>
> +#include <string.h>
>  #include <sys/utsname.h>
> +#include <unistd.h>
> +
> +#include "assembler/brw_compat.h"	/* [un]likely() */
------------ ^^^^^^^^^
Do we really need this?

>  
>  #include "igt_aux.h"
>  #include "igt_core.h"
> @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
>  }
>  
>  struct modprobe_data {
> +	pthread_t parent;
--- ^^^^^^^^^^^^^^^^
Please move it below to other related thread data.
Also consider a comment why(or for what purpose)
did you put this here.

>  	struct kmod_module *kmod;
>  	const char *opts;
>  	int err;
> +	pthread_mutex_t lock;
> +	pthread_t thread;
>  };
>  
>  static void *modprobe_task(void *arg)
> @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
>  
>  	data->err = modprobe(data->kmod, data->opts);
>  
> +	if (igt_debug_on(data->err)) {
> +		int err;
> +
> +		while (err = pthread_mutex_trylock(&data->lock),
> +		       err && !igt_debug_on(err != EBUSY))
> +			igt_debug_on(pthread_kill(data->parent, SIGCHLD));
> +	} else {
> +		/* let main thread use mutex to detect modprobe completion */
> +		igt_debug_on(pthread_mutex_lock(&data->lock));
> +	}
> +
>  	return NULL;
>  }
>  
> +static void kunit_sigchld_handler(int signal)
> +{
> +	return;
--- ^^^^^^^
Why not removing this? checkpatch complains about return from void.

> +}
> +
> +static int kunit_kmsg_result_get(struct igt_list_head *results,
> +				 struct modprobe_data *modprobe,
> +				 int fd, struct igt_ktap_results *ktap)
> +{
> +	struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> +			 *saved;
> +	char record[BUF_LEN + 1], *buf;
> +	unsigned long taints;
> +	int ret;
> +
> +	do {
> +		int err;
> +
> +		if (igt_debug_on(igt_kernel_tainted(&taints)))
> +			return -ENOTRECOVERABLE;
> +
> +		err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved));
> +		if (err == -1)
> +			return -errno;
> +		else if (unlikely(err))
> +			return err;
> +
> +		err = pthread_mutex_lock(&modprobe->lock);
> +		switch (err) {
> +		case EOWNERDEAD:
> +			/* leave the mutex unrecoverable */
> +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> +			__attribute__ ((fallthrough));
> +		case ENOTRECOVERABLE:
> +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> +			if (igt_debug_on(modprobe->err))
> +				return modprobe->err;

Why no 'return -err;' here?

> +			break;
> +		case 0:
> +			break;
> +		default:
> +			igt_debug("pthread_mutex_lock() error: %d\n", err);
> +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> +			return -err;
> +		}
> +
> +		ret = read(fd, record, BUF_LEN);
> +
> +		if (!err) {
        ^^^^^^^^^^^
Looks strange here.

> +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> +		}
> +
> +		if (igt_debug_on(!ret))
> +			return -ENODATA;
> +		if (igt_debug_on(ret == -1))
> +			return -errno;
> +		if (unlikely(igt_debug_on(ret < 0)))
> +			break;
> +
> +		/* skip kmsg continuation lines */
> +		if (igt_debug_on(*record == ' '))
----------- ^^^^^^^^^^^^
Why debug here? imho it is not needed.

> +			continue;
> +
> +		/* NULL-terminate the record */
> +		record[ret] = '\0';
> +
> +		/* detect start of log message, continue if not found */
> +		buf = strchrnul(record, ';');
> +		if (igt_debug_on(*buf == '\0'))
----------- ^^^^^^^^^^^^
Same here, you just continue loop if no ';'?

> +			continue;
> +		buf++;
> +
> +		ret = igt_ktap_parse(buf, ktap);
> +		if (!ret || igt_debug_on(ret != -EINPROGRESS))
> +			break;
> +	} while (igt_list_empty(results));
> +
> +	return ret;
> +}
> +
> +static void kunit_result_free(struct igt_ktap_result **r,
> +			      char **suite_name, char **case_name)
> +{
> +	if (!*r)
> +		return;
> +
> +	igt_list_del(&(*r)->link);
> +
> +	if ((*r)->suite_name != *suite_name) {
> +		free(*suite_name);
> +		*suite_name = (*r)->suite_name;
> +	}
> +
> +	if ((*r)->case_name != *case_name) {
> +		free(*case_name);
> +		*case_name = (*r)->case_name;
> +	}
> +
> +	free((*r)->msg);
> +	free(*r);
> +	*r = NULL;
> +}
> +
>  static void __igt_kunit(struct igt_ktest *tst, const char *opts)
>  {
> -	struct modprobe_data modprobe = { tst->kmod, opts, 0, };
> -	struct kmod_module *kunit_kmod;
> -	bool is_builtin;
> -	struct ktap_test_results *results;
> -	pthread_t modprobe_thread;
> +	struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, };
> +	char *suite_name = NULL, *case_name = NULL;
> +	struct igt_ktap_result *r, *rn;
> +	struct igt_ktap_results *ktap;
> +	pthread_mutexattr_t attr;
> +	IGT_LIST_HEAD(results);
>  	unsigned long taints;
>  	int flags, ret;
>  
> @@ -780,60 +904,119 @@ static void __igt_kunit(struct igt_ktest *tst, const char *opts)
>  
>  	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
>  
> -	igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod));
> -	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> -	kmod_module_unref(kunit_kmod);
> +	igt_skip_on(pthread_mutexattr_init(&attr));
> +	igt_skip_on(pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST));
> +	igt_skip_on(pthread_mutex_init(&modprobe.lock, &attr));
>  
> -	results = ktap_parser_start(tst->kmsg, is_builtin);
> +	ktap = igt_ktap_alloc(&results);
> +	igt_require(ktap);
>  
> -	if (igt_debug_on(pthread_create(&modprobe_thread, NULL,
> +	if (igt_debug_on(pthread_create(&modprobe.thread, NULL,
>  					modprobe_task, &modprobe))) {
> -		ktap_parser_cancel();
> -		igt_ignore_warn(ktap_parser_stop());
> +		igt_ktap_free(ktap);
>  		igt_skip("Failed to create a modprobe thread\n");
>  	}
>  
> -	while (READ_ONCE(results->still_running) || !igt_list_empty(&results->list))
> -	{
> -		struct ktap_test_results_element *result;
> -
> -		if (!pthread_tryjoin_np(modprobe_thread, NULL) && modprobe.err) {
> -			ktap_parser_cancel();
> +	do {
> +		ret = kunit_kmsg_result_get(&results, &modprobe,
> +					    tst->kmsg, ktap);
> +		if (igt_debug_on(ret && ret != -EINPROGRESS))
>  			break;
> -		}
>  
> -		if (igt_kernel_tainted(&taints)) {
> -			ktap_parser_cancel();
> -			pthread_cancel(modprobe_thread);
> +		if (igt_debug_on(igt_list_empty(&results)))
>  			break;
> -		}
>  
> -		pthread_mutex_lock(&results->mutex);
> -		if (igt_list_empty(&results->list)) {
> -			pthread_mutex_unlock(&results->mutex);
> -			continue;
> -		}
> +		r = igt_list_first_entry(&results, r, link);
>  
> -		result = igt_list_first_entry(&results->list, result, link);
> +		igt_dynamic_f("%s-%s", r->suite_name, r->case_name) {
> +			if (r->code == IGT_EXIT_INVALID) {
> +				/* parametrized test case, get actual result */
> +				kunit_result_free(&r, &suite_name, &case_name);
>  
> -		igt_list_del(&result->link);
> -		pthread_mutex_unlock(&results->mutex);
> +				igt_assert(igt_list_empty(&results));
>  
> -		igt_dynamic(result->test_name) {
> -			igt_assert(READ_ONCE(result->passed));
> +				ret = kunit_kmsg_result_get(&results, &modprobe,
> +							    tst->kmsg, ktap);
> +				if (ret != -EINPROGRESS)
> +					igt_fail_on(ret);
> +
> +				igt_fail_on(igt_list_empty(&results));
> +
> +				r = igt_list_first_entry(&results, r, link);
> +
> +				igt_fail_on_f(strcmp(r->suite_name, suite_name),
> +					      "suite_name expected: %s, got: %s\n",
> +					      suite_name, r->suite_name);
> +				igt_fail_on_f(strcmp(r->case_name, case_name),
> +					      "case_name expected: %s, got: %s\n",
> +					      case_name, r->case_name);
> +			}
>  
> -			if (!pthread_tryjoin_np(modprobe_thread, NULL))
> +			igt_assert_neq(r->code, IGT_EXIT_INVALID);
> +
> +			if (r->msg && *r->msg) {
> +				igt_skip_on_f(r->code == IGT_EXIT_SKIP,
> +					      "%s\n", r->msg);
> +				igt_fail_on_f(r->code == IGT_EXIT_FAILURE,
> +					      "%s\n", r->msg);
> +				igt_abort_on_f(r->code == IGT_EXIT_ABORT,
> +					      "%s\n", r->msg);
> +			} else {
> +				igt_skip_on(r->code == IGT_EXIT_SKIP);
> +				igt_fail_on(r->code == IGT_EXIT_FAILURE);
> +				if (r->code == IGT_EXIT_ABORT)
> +					igt_fail(r->code);
> +			}
> +			igt_assert_eq(r->code, IGT_EXIT_SUCCESS);
> +
> +			switch (pthread_mutex_lock(&modprobe.lock)) {
> +			case 0:
> +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> +				break;
> +			case EOWNERDEAD:
> +				/* leave the mutex unrecoverable */
> +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> +				__attribute__ ((fallthrough));
> +			case ENOTRECOVERABLE:
>  				igt_assert_eq(modprobe.err, 0);
> +				break;

imho we should also break a do-while loop in case of error.

> +			default:
> +				igt_debug("pthread_mutex_lock() failed\n");
> +				break;
> +			}
>  
>  			igt_assert_eq(igt_kernel_tainted(&taints), 0);
>  		}
>  
> -		free(result);
> +		kunit_result_free(&r, &suite_name, &case_name);
> +
> +	} while (ret == -EINPROGRESS);
> +
> +	igt_list_for_each_entry_safe(r, rn, &results, link)
> +		kunit_result_free(&r, &suite_name, &case_name);
> +
> +	free(case_name);
> +	free(suite_name);
> +
> +	switch (pthread_mutex_lock(&modprobe.lock)) {
> +	case 0:
> +		igt_debug_on(pthread_cancel(modprobe.thread));
> +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> +		break;
> +	case EOWNERDEAD:
> +		/* leave the mutex unrecoverable */
> +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> +		break;
> +	case ENOTRECOVERABLE:
> +		break;
> +	default:
> +		igt_debug("pthread_mutex_lock() failed\n");
> +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> +		break;
>  	}
>  
> -	pthread_join(modprobe_thread, NULL);
> -
> -	ret = ktap_parser_stop();
> +	igt_ktap_free(ktap);
>  
>  	igt_skip_on(modprobe.err);
>  	igt_skip_on(igt_kernel_tainted(&taints));
> diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> index 5eac102417..53a6c63288 100644
> --- a/lib/igt_ktap.c
> +++ b/lib/igt_ktap.c
> @@ -4,17 +4,11 @@
>   * Copyright © 2023 Intel Corporation
>   */
>  
> -#include <ctype.h>
> -#include <limits.h>
> -#include <libkmod.h>
> -#include <pthread.h>
>  #include <errno.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <unistd.h>
>  
> -#include "igt_aux.h"
>  #include "igt_core.h"
>  #include "igt_ktap.h"
>  #include "igt_list.h"
> @@ -312,565 +306,3 @@ void igt_ktap_free(struct igt_ktap_results *ktap)
>  {
>  	free(ktap);
>  }
> -
> -#define DELIMITER "-"
> -
> -struct ktap_parser_args {
> -	int fd;
> -	bool is_builtin;
> -	int ret;
> -} ktap_args;
> -
> -static struct ktap_test_results results;
> -
> -static int log_to_end(enum igt_log_level level, int fd,
> -		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
> -
> -/**
> - * log_to_end:
> - * @level: #igt_log_level
> - * @record: record to store the read data
> - * @format: format string
> - * @...: optional arguments used in the format string
> - *
> - * This is an altered version of the generic structured logging helper function
> - * igt_log capable of reading to the end of a given line.
> - *
> - * Returns: 0 for success, or -2 if there's an error reading from the file
> - */
> -static int log_to_end(enum igt_log_level level, int fd,
> -		      char *record, const char *format, ...)
> -{
> -	va_list args;
> -	const char *lend;
> -
> -	/* Cutoff after newline character, in order to not display garbage */
> -	char *cutoff = strchr(record, '\n');
> -	if (cutoff) {
> -		if (cutoff - record < BUF_LEN)
> -			cutoff[1] = '\0';
> -	}
> -
> -	va_start(args, format);
> -	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
> -	va_end(args);
> -
> -	lend = strchrnul(record, '\n');
> -	while (*lend == '\0') {
> -		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
> -
> -		if (read(fd, record, BUF_LEN) < 0) {
> -			if (errno == EPIPE)
> -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -			else
> -				igt_warn("an error occurred while reading kmsg: %m\n");
> -
> -			return -2;
> -		}
> -
> -		lend = strchrnul(record, '\n');
> -	}
> -	return 0;
> -}
> -
> -/**
> - * lookup_value:
> - * @haystack: the string to search in
> - * @needle: the string to search for
> - *
> - * Returns: the value of the needle in the haystack, or -1 if not found.
> - */
> -static long lookup_value(const char *haystack, const char *needle)
> -{
> -	const char *needle_rptr;
> -	char *needle_end;
> -	long num;
> -
> -	needle_rptr = strcasestr(haystack, needle);
> -
> -	if (needle_rptr == NULL)
> -		return -1;
> -
> -	/* Skip search string and whitespaces after it */
> -	needle_rptr += strlen(needle);
> -
> -	num = strtol(needle_rptr, &needle_end, 10);
> -
> -	if (needle_rptr == needle_end)
> -		return -1;
> -
> -	if (num == LONG_MIN || num == LONG_MAX)
> -		return 0;
> -
> -	return num > 0 ? num : 0;
> -}
> -
> -/**
> - * tap_version_present:
> - * @record: buffer with tap data
> - * @print_info: whether tap version should be printed or not
> - *
> - * Returns:
> - * 0 if not found
> - * 1 if found
> - */
> -static int tap_version_present(char* record, bool print_info)
> -{
> -	/*
> -	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> -	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines

Could you save these two lines comment in some other place?

Regards,
Kamil

> -	 *
> -	 * but actually isn't, as it currently depends on the KUnit module
> -	 * being built-in, so we can't rely on it every time
> -	 */
> -	const char *version_rptr = strcasestr(record, "TAP version ");
> -	char *cutoff;
> -
> -	if (version_rptr == NULL)
> -		return 0;
> -
> -	/* Cutoff after newline character, in order to not display garbage */
> -	cutoff = strchr(version_rptr, '\n');
> -	if (cutoff)
> -		cutoff[0] = '\0';
> -
> -	if (print_info)
> -		igt_info("%s\n", version_rptr);
> -
> -	return 1;
> -}
> -
> -/**
> - * find_next_tap_subtest:
> - * @fd: file descriptor
> - * @record: buffer used to read fd
> - * @is_builtin: whether KUnit is built-in or not
> - *
> - * Returns:
> - * 0 if there's missing information
> - * -1 if not found
> - * -2 if there are problems while reading the file.
> - * any other value corresponds to the amount of cases of the next (sub)test
> - */
> -static int find_next_tap_subtest(int fd, char *record, char *test_name, bool is_builtin)
> -{
> -	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
> -	long test_count;
> -	char *cutoff;
> -
> -	test_name[0] = '\0';
> -	test_name[BUF_LEN] = '\0';
> -
> -	test_lookup_str = " subtest: ";
> -	subtest_lookup_str = " test: ";
> -
> -	if (!tap_version_present(record, true))
> -		return -1;
> -
> -	if (is_builtin) {
> -		if (read(fd, record, BUF_LEN) < 0) {
> -			if (errno == EPIPE)
> -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -			else
> -				igt_warn("an error occurred while reading kmsg: %m\n");
> -
> -			return -2;
> -		}
> -	}
> -
> -	name_rptr = strcasestr(record, test_lookup_str);
> -	if (name_rptr != NULL) {
> -		name_rptr += strlen(test_lookup_str);
> -	} else {
> -		name_rptr = strcasestr(record, subtest_lookup_str);
> -		if (name_rptr != NULL)
> -			name_rptr += strlen(subtest_lookup_str);
> -	}
> -
> -	if (name_rptr == NULL) {
> -		if (!is_builtin)
> -			/* We've probably found nothing */
> -			return -1;
> -		igt_info("Missing test name\n");
> -	} else {
> -		strncpy(test_name, name_rptr, BUF_LEN);
> -		/* Cutoff after newline character, in order to not display garbage */
> -		cutoff = strchr(test_name, '\n');
> -		if (cutoff)
> -			cutoff[0] = '\0';
> -
> -		if (read(fd, record, BUF_LEN) < 0) {
> -			if (errno == EPIPE)
> -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -			else
> -				igt_warn("unknown error reading kmsg (%m)\n");
> -
> -			return -2;
> -		}
> -
> -		/* Now we can be sure we found tests */
> -		if (!is_builtin)
> -			igt_info("KUnit is not built-in, skipping version check...\n");
> -	}
> -
> -	/*
> -	 * Total test count will almost always appear as 0..N at the beginning
> -	 * of a run, so we use it to reliably identify a new run
> -	 */
> -	test_count = lookup_value(record, "..");
> -
> -	if (test_count <= 0) {
> -		igt_info("Missing test count\n");
> -		if (test_name[0] == '\0')
> -			return 0;
> -		if (log_to_end(IGT_LOG_INFO, fd, record,
> -				"Running some tests in: %s\n",
> -				test_name) < 0)
> -			return -2;
> -		return 0;
> -	} else if (test_name[0] == '\0') {
> -		igt_info("Running %ld tests...\n", test_count);
> -		return 0;
> -	}
> -
> -	if (log_to_end(IGT_LOG_INFO, fd, record,
> -			"Executing %ld tests in: %s\n",
> -			test_count, test_name) < 0)
> -		return -2;
> -
> -	return test_count;
> -}
> -
> -/**
> - * parse_kmsg_for_tap:
> - * @fd: file descriptor
> - * @record: buffer used to read fd
> - * @test_name: buffer to store the test name
> - *
> - * Returns:
> - * 1 if no results were found
> - * 0 if a test succeded
> - * -1 if a test failed
> - * -2 if there are problems reading the file
> - */
> -static int parse_kmsg_for_tap(int fd, char *record, char *test_name)
> -{
> -	const char *lstart, *ok_lookup_str, *nok_lookup_str,
> -	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
> -	char *test_name_end;
> -
> -	ok_lookup_str = "ok ";
> -	nok_lookup_str = "not ok ";
> -
> -	lstart = strchrnul(record, ';');
> -
> -	if (*lstart == '\0') {
> -		igt_warn("kmsg truncated: output malformed (%m)\n");
> -		return -2;
> -	}
> -
> -	lstart++;
> -	while (isspace(*lstart))
> -		lstart++;
> -
> -	nok_rptr = strstr(lstart, nok_lookup_str);
> -	if (nok_rptr != NULL) {
> -		nok_rptr += strlen(nok_lookup_str);
> -		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
> -			nok_rptr++;
> -		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
> -		while (!isspace(*test_name_end))
> -			test_name_end++;
> -		*test_name_end = '\0';
> -		if (log_to_end(IGT_LOG_WARN, fd, record,
> -			       "%s", lstart) < 0)
> -			return -2;
> -		return -1;
> -	}
> -
> -	comment_start = strchrnul(lstart, '#');
> -
> -	/* Check if we're still in a subtest */
> -	if (*comment_start != '\0') {
> -		comment_start++;
> -		value_parse_start = comment_start;
> -
> -		if (lookup_value(value_parse_start, "fail: ") > 0) {
> -			if (log_to_end(IGT_LOG_WARN, fd, record,
> -				       "%s", lstart) < 0)
> -				return -2;
> -			return -1;
> -		}
> -	}
> -
> -	ok_rptr = strstr(lstart, ok_lookup_str);
> -	if (ok_rptr != NULL) {
> -		ok_rptr += strlen(ok_lookup_str);
> -		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
> -			ok_rptr++;
> -		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
> -		while (!isspace(*test_name_end))
> -			test_name_end++;
> -		*test_name_end = '\0';
> -		return 0;
> -	}
> -
> -	return 1;
> -}
> -
> -/**
> - * parse_tap_level:
> - * @fd: file descriptor
> - * @base_test_name: test_name from upper recursion level
> - * @test_count: test_count of this level
> - * @failed_tests: top level failed_tests pointer
> - * @found_tests: top level found_tests pointer
> - * @is_builtin: whether the KUnit module is built-in or not
> - *
> - * Returns:
> - * 0 if succeded
> - * -1 if error occurred
> - */
> -__maybe_unused
> -static int parse_tap_level(int fd, char *base_test_name, int test_count, bool *failed_tests,
> -			   bool *found_tests, bool is_builtin)
> -{
> -	char record[BUF_LEN + 1];
> -	struct ktap_test_results_element *r;
> -	int internal_test_count;
> -	char test_name[BUF_LEN + 1];
> -	char base_test_name_for_next_level[BUF_LEN + 1];
> -
> -	for (int i = 0; i < test_count; i++) {
> -		if (read(fd, record, BUF_LEN) < 0) {
> -			if (errno == EPIPE)
> -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -			else
> -				igt_warn("error reading kmsg (%m)\n");
> -
> -			return -1;
> -		}
> -
> -		/* Sublevel found */
> -		if (tap_version_present(record, false))
> -		{
> -			internal_test_count = find_next_tap_subtest(fd, record, test_name,
> -								    is_builtin);
> -			switch (internal_test_count) {
> -			case -2:
> -				/* No more data to read */
> -				return -1;
> -			case -1:
> -				/* No test found */
> -				return -1;
> -			case 0:
> -				/* Tests found, but they're missing info */
> -				*found_tests = true;
> -				return -1;
> -			default:
> -				*found_tests = true;
> -
> -				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
> -				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
> -				    base_test_name_for_next_level[0])
> -					strncat(base_test_name_for_next_level, DELIMITER,
> -						BUF_LEN - strlen(base_test_name_for_next_level));
> -				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
> -				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
> -
> -				if (parse_tap_level(fd, base_test_name_for_next_level,
> -						    internal_test_count, failed_tests, found_tests,
> -						    is_builtin) == -1)
> -					return -1;
> -				break;
> -			}
> -		}
> -
> -		switch (parse_kmsg_for_tap(fd, record, test_name)) {
> -		case -2:
> -			return -1;
> -		case -1:
> -			*failed_tests = true;
> -
> -			r = malloc(sizeof(*r));
> -
> -			memcpy(r->test_name, base_test_name, BUF_LEN);
> -			if (strlen(r->test_name) < BUF_LEN - 1)
> -				if (r->test_name[0])
> -					strncat(r->test_name, DELIMITER,
> -						BUF_LEN - strlen(r->test_name));
> -			memcpy(r->test_name + strlen(r->test_name), test_name,
> -			       BUF_LEN - strlen(r->test_name));
> -			r->test_name[BUF_LEN] = '\0';
> -
> -			r->passed = false;
> -
> -			pthread_mutex_lock(&results.mutex);
> -			igt_list_add_tail(&r->link, &results.list);
> -			pthread_mutex_unlock(&results.mutex);
> -
> -			test_name[0] = '\0';
> -			break;
> -		case 0:
> -			r = malloc(sizeof(*r));
> -
> -			memcpy(r->test_name, base_test_name, BUF_LEN);
> -			if (strlen(r->test_name) < BUF_LEN - 1)
> -				if (r->test_name[0])
> -					strncat(r->test_name, DELIMITER,
> -						BUF_LEN - strlen(r->test_name));
> -			memcpy(r->test_name + strlen(r->test_name), test_name,
> -			       BUF_LEN - strlen(r->test_name));
> -			r->test_name[BUF_LEN] = '\0';
> -
> -			r->passed = true;
> -
> -			pthread_mutex_lock(&results.mutex);
> -			igt_list_add_tail(&r->link, &results.list);
> -			pthread_mutex_unlock(&results.mutex);
> -
> -			test_name[0] = '\0';
> -			break;
> -		default:
> -			break;
> -		}
> -	}
> -	return 0;
> -}
> -
> -/**
> - * igt_ktap_parser:
> - *
> - * This function parses the output of a ktap script and passes it to main thread.
> - */
> -void *igt_ktap_parser(void *unused)
> -{
> -	char record[BUF_LEN + 1], *buf, *suite_name = NULL, *case_name = NULL;
> -	struct igt_ktap_results *ktap = NULL;
> -	int fd = ktap_args.fd;
> -	IGT_LIST_HEAD(list);
> -	int err;
> -
> -	ktap = igt_ktap_alloc(&list);
> -	if (igt_debug_on(!ktap))
> -		goto igt_ktap_parser_end;
> -
> -	while (err = read(fd, record, BUF_LEN), err > 0) {
> -		struct igt_ktap_result *r, *rn;
> -
> -		/* skip kmsg continuation lines */
> -		if (igt_debug_on(*record == ' '))
> -			continue;
> -
> -		/* NULL-terminate the record */
> -		record[err] = '\0';
> -
> -		/* detect start of log message, continue if not found */
> -		buf = strchrnul(record, ';');
> -		if (igt_debug_on(*buf == '\0'))
> -			continue;
> -		buf++;
> -
> -		err = igt_ktap_parse(buf, ktap);
> -
> -		/* parsing error */
> -		if (err && err != -EINPROGRESS)
> -			goto igt_ktap_parser_end;
> -
> -		igt_list_for_each_entry_safe(r, rn, &list, link) {
> -			struct ktap_test_results_element *result = NULL;
> -			int code = r->code;
> -
> -			if (code != IGT_EXIT_INVALID)
> -				result = calloc(1, sizeof(*result));
> -
> -			if (result) {
> -				snprintf(result->test_name, sizeof(result->test_name),
> -					 "%s-%s", r->suite_name, r->case_name);
> -
> -				if (code == IGT_EXIT_SUCCESS)
> -					result->passed = true;
> -			}
> -
> -			igt_list_del(&r->link);
> -			if (r->suite_name != suite_name) {
> -				free(suite_name);
> -				suite_name = r->suite_name;
> -			}
> -			if (r->case_name != case_name) {
> -				free(case_name);
> -				case_name = r->case_name;
> -			}
> -			free(r->msg);
> -			free(r);
> -
> -			/*
> -			 * no extra result record expected on start
> -			 * of parametrized test case -- skip it
> -			 */
> -			if (code == IGT_EXIT_INVALID)
> -				continue;
> -
> -			if (!result) {
> -				err = -ENOMEM;
> -				goto igt_ktap_parser_end;
> -			}
> -
> -			pthread_mutex_lock(&results.mutex);
> -			igt_list_add_tail(&result->link, &results.list);
> -			pthread_mutex_unlock(&results.mutex);
> -		}
> -
> -		/* end of KTAP report */
> -		if (!err)
> -			goto igt_ktap_parser_end;
> -	}
> -
> -	if (err < 0) {
> -		if (errno == EPIPE)
> -			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> -		else
> -			igt_warn("error reading kmsg (%m)\n");
> -	}
> -
> -igt_ktap_parser_end:
> -	free(suite_name);
> -	free(case_name);
> -
> -	if (!err)
> -		ktap_args.ret = IGT_EXIT_SUCCESS;
> -
> -	results.still_running = false;
> -
> -	if (ktap)
> -		igt_ktap_free(ktap);
> -
> -	return NULL;
> -}
> -
> -static pthread_t ktap_parser_thread;
> -
> -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin)
> -{
> -	IGT_INIT_LIST_HEAD(&results.list);
> -	pthread_mutex_init(&results.mutex, NULL);
> -	results.still_running = true;
> -
> -	ktap_args.fd = fd;
> -	ktap_args.is_builtin = is_builtin;
> -	ktap_args.ret = IGT_EXIT_FAILURE;
> -	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
> -
> -	return &results;
> -}
> -
> -void ktap_parser_cancel(void)
> -{
> -	pthread_cancel(ktap_parser_thread);
> -}
> -
> -int ktap_parser_stop(void)
> -{
> -	pthread_join(ktap_parser_thread, NULL);
> -	return ktap_args.ret;
> -}
> diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> index 6f8da3eab6..c422636bfc 100644
> --- a/lib/igt_ktap.h
> +++ b/lib/igt_ktap.h
> @@ -27,8 +27,6 @@
>  
>  #define BUF_LEN 4096
>  
> -#include <pthread.h>
> -
>  #include "igt_list.h"
>  
>  struct igt_ktap_result {
> @@ -45,24 +43,4 @@ struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results);
>  int igt_ktap_parse(const char *buf, struct igt_ktap_results *ktap);
>  void igt_ktap_free(struct igt_ktap_results *ktap);
>  
> -void *igt_ktap_parser(void *unused);
> -
> -typedef struct ktap_test_results_element {
> -	char test_name[BUF_LEN + 1];
> -	bool passed;
> -	struct igt_list_head link;
> -} ktap_test_results_element;
> -
> -struct ktap_test_results {
> -	struct igt_list_head list;
> -	pthread_mutex_t mutex;
> -	bool still_running;
> -};
> -
> -
> -
> -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin);
> -void ktap_parser_cancel(void);
> -int ktap_parser_stop(void);
> -
>  #endif /* IGT_KTAP_H */
> -- 
> 2.42.0
>

Janusz Krzysztofik Oct. 10, 2023, 5:49 p.m. UTC | #3

Hi Kamil,

Thanks for review.

On Tuesday, 10 October 2023 17:59:56 CEST Kamil Konieczny wrote:
> Hi Janusz,
> On 2023-10-09 at 14:27:55 +0200, Janusz Krzysztofik wrote:
> > There was an attempt to parse KTAP reports in the background while a kunit
> > test module is loading.  However, since dynamic sub-subtests can be
> > executed only from the main thread, that attempt was not quite successful,
> > as IGT results from all executed kunit test cases were generated only
> > after loading of kunit test module completed.
> > 
> > Now that the parser maintains its state and we can call it separately for
> > each input line of a KTAP report, it is perfectly possible to call the
> > parser from the main thread while the module is loading in the background,
> > and convert results from kunit test cases immediately to results of IGT
> > dynamic sub-subtests by running an igt_dynamic() section for each result
> > as soon as returned by the parser.
> > 
> > Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> > result obtained from igt_ktap_parse() called from the main thread.
> > 
> > Also, drop no longer needed functions from igt_ktap soruces.
> > 
> > v3: Fix ktap structure not freed on lseek error,
> >   - fix initial SIGCHLD handler not restored,
> >   - fix missing handling of potential errors returned by sigaction,
> >   - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> >     synchronization with modprobe thread,
> >   - fix potentially illegal use of igt_assert() called outside of
> >     dynamic sub-subtest section,
> >   - fix unsupported exit code potentially passed to igt_fail(),
> >   - no need to fail a dynamic sub-subtest on potential KTAP parser error
> >     after a valid result from the parser has been processed,
> >   - fix trailing newlines missing from error messages,
> >   - add more debug statements,
> >   - integrate common code around kunit_result_free() into it.
> > v2: Interrupt blocking read() on modprobe failure.
> > 
> > Signed-off-by: Janusz Krzysztofik <[e-mail]>
> > Acked-by: Mauro Carvalho Chehab <[e-mail]> # v2
> > ---
> >  lib/igt_kmod.c | 261 +++++++++++++++++++----
> >  lib/igt_ktap.c | 568 -------------------------------------------------
> >  lib/igt_ktap.h |  22 --
> >  3 files changed, 222 insertions(+), 629 deletions(-)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 426ae5b26f..7bca4cdaab 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -1,5 +1,5 @@
> >  /*
> > - * Copyright © 2016 Intel Corporation
> > + * Copyright © 2016-2023 Intel Corporation
> >   *
> >   * Permission is hereby granted, free of charge, to any person obtaining a
> >   * copy of this software and associated documentation files (the "Software"),
> > @@ -26,7 +26,12 @@
> >  #include <errno.h>
> >  #include <fcntl.h>
> >  #include <pthread.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> >  #include <sys/utsname.h>
> > +#include <unistd.h>
> > +
> > +#include "assembler/brw_compat.h"	/* [un]likely() */
> ------------ ^^^^^^^^^
> Do we really need this?

I think the correct question is if wee really need [un]likely().  I'm using it 
to document unlikely cases, which is a widely accepted method of documenting 
cases like that, I believe.  Having that clarified, I hope you just tell me if 
you think we need those cases documented, and how, if not that way.

> 
> >  
> >  #include "igt_aux.h"
> >  #include "igt_core.h"
> > @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> >  }
> >  
> >  struct modprobe_data {
> > +	pthread_t parent;
> --- ^^^^^^^^^^^^^^^^
> Please move it below to other related thread data.
> Also consider a comment why(or for what purpose)
> did you put this here.

No problem to move it to the bottom, if that's important to you, but regarding 
the comment, do you think that the purpose of this field of the structure, 
compared to other fields, is so unclear form review of the code, despite its 
name, that it requires a comment, unlike the other fields?

> 
> >  	struct kmod_module *kmod;
> >  	const char *opts;
> >  	int err;
> > +	pthread_mutex_t lock;
> > +	pthread_t thread;
> >  };
> >  
> >  static void *modprobe_task(void *arg)
> > @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
> >  
> >  	data->err = modprobe(data->kmod, data->opts);
> >  
> > +	if (igt_debug_on(data->err)) {
> > +		int err;
> > +
> > +		while (err = pthread_mutex_trylock(&data->lock),
> > +		       err && !igt_debug_on(err != EBUSY))
> > +			igt_debug_on(pthread_kill(data->parent, SIGCHLD));
> > +	} else {
> > +		/* let main thread use mutex to detect modprobe completion */
> > +		igt_debug_on(pthread_mutex_lock(&data->lock));
> > +	}
> > +
> >  	return NULL;
> >  }
> >  
> > +static void kunit_sigchld_handler(int signal)
> > +{
> > +	return;
> --- ^^^^^^^
> Why not removing this? checkpatch complains about return from void.

OK.

> 
> > +}
> > +
> > +static int kunit_kmsg_result_get(struct igt_list_head *results,
> > +				 struct modprobe_data *modprobe,
> > +				 int fd, struct igt_ktap_results *ktap)
> > +{
> > +	struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> > +			 *saved;
> > +	char record[BUF_LEN + 1], *buf;
> > +	unsigned long taints;
> > +	int ret;
> > +
> > +	do {
> > +		int err;
> > +
> > +		if (igt_debug_on(igt_kernel_tainted(&taints)))
> > +			return -ENOTRECOVERABLE;
> > +
> > +		err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved));
> > +		if (err == -1)
> > +			return -errno;
> > +		else if (unlikely(err))
> > +			return err;
> > +
> > +		err = pthread_mutex_lock(&modprobe->lock);
> > +		switch (err) {
> > +		case EOWNERDEAD:
> > +			/* leave the mutex unrecoverable */
> > +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> > +			__attribute__ ((fallthrough));
> > +		case ENOTRECOVERABLE:
> > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > +			if (igt_debug_on(modprobe->err))
> > +				return modprobe->err;
> 
> Why no 'return -err;' here?

Because modprobe->err, which can tell us more about what has gone bad in the 
modprobe thread, is more important here than err which only informs us about 
completion of the modprobe thread, also if successful, isn't it?

> 
> > +			break;
> > +		case 0:
> > +			break;
> > +		default:
> > +			igt_debug("pthread_mutex_lock() error: %d\n", err);
> > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > +			return -err;
> > +		}
> > +
> > +		ret = read(fd, record, BUF_LEN);
> > +
> > +		if (!err) {
>         ^^^^^^^^^^^
> Looks strange here.

Here err still carries a return code from pthread_mutex_lock(), then !err 
means that the modprobe thread was still running and we have 1) acquired 
the mutex which now we have to release, 2) installed a non-default 
SIGCHLD handler that we should now reset.  Have you got a better idea 
how to encode that so it looks less strange?  Would a comment that reminds 
a reader where that err comes from be sufficient?

> 
> > +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > +		}
> > +
> > +		if (igt_debug_on(!ret))
> > +			return -ENODATA;
> > +		if (igt_debug_on(ret == -1))
> > +			return -errno;
> > +		if (unlikely(igt_debug_on(ret < 0)))
> > +			break;
> > +
> > +		/* skip kmsg continuation lines */
> > +		if (igt_debug_on(*record == ' '))
> ----------- ^^^^^^^^^^^^
> Why debug here? imho it is not needed.

Because we don't expect continuation lines while reading a KTAP report. Since 
the kernel log may still contain them, we want to know about them when 
something wrong happens to our KTAP parser.  That's the purpose of all those 
debug_on() calls: provide as much information on unexpected conditions as 
possible when the test fails, isn't it?

> 
> > +			continue;
> > +
> > +		/* NULL-terminate the record */
> > +		record[ret] = '\0';
> > +
> > +		/* detect start of log message, continue if not found */
> > +		buf = strchrnul(record, ';');
> > +		if (igt_debug_on(*buf == '\0'))
> ----------- ^^^^^^^^^^^^
> Same here, you just continue loop if no ';'?

That means we are getting incorrectly formatted records from /dev/kmsg, and 
I'm sure we would like to be informed about that if something goes wrong with 
the test, wouldn't we?

> 
> > +			continue;
> > +		buf++;
> > +
> > +		ret = igt_ktap_parse(buf, ktap);
> > +		if (!ret || igt_debug_on(ret != -EINPROGRESS))
> > +			break;
> > +	} while (igt_list_empty(results));
> > +
> > +	return ret;
> > +}
> > +
> > +static void kunit_result_free(struct igt_ktap_result **r,
> > +			      char **suite_name, char **case_name)
> > +{
> > +	if (!*r)
> > +		return;
> > +
> > +	igt_list_del(&(*r)->link);
> > +
> > +	if ((*r)->suite_name != *suite_name) {
> > +		free(*suite_name);
> > +		*suite_name = (*r)->suite_name;
> > +	}
> > +
> > +	if ((*r)->case_name != *case_name) {
> > +		free(*case_name);
> > +		*case_name = (*r)->case_name;
> > +	}
> > +
> > +	free((*r)->msg);
> > +	free(*r);
> > +	*r = NULL;
> > +}
> > +
> >  static void __igt_kunit(struct igt_ktest *tst, const char *opts)
> >  {
> > -	struct modprobe_data modprobe = { tst->kmod, opts, 0, };
> > -	struct kmod_module *kunit_kmod;
> > -	bool is_builtin;
> > -	struct ktap_test_results *results;
> > -	pthread_t modprobe_thread;
> > +	struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, };
> > +	char *suite_name = NULL, *case_name = NULL;
> > +	struct igt_ktap_result *r, *rn;
> > +	struct igt_ktap_results *ktap;
> > +	pthread_mutexattr_t attr;
> > +	IGT_LIST_HEAD(results);
> >  	unsigned long taints;
> >  	int flags, ret;
> >  
> > @@ -780,60 +904,119 @@ static void __igt_kunit(struct igt_ktest *tst, const char *opts)
> >  
> >  	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
> >  
> > -	igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod));
> > -	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> > -	kmod_module_unref(kunit_kmod);
> > +	igt_skip_on(pthread_mutexattr_init(&attr));
> > +	igt_skip_on(pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST));
> > +	igt_skip_on(pthread_mutex_init(&modprobe.lock, &attr));
> >  
> > -	results = ktap_parser_start(tst->kmsg, is_builtin);
> > +	ktap = igt_ktap_alloc(&results);
> > +	igt_require(ktap);
> >  
> > -	if (igt_debug_on(pthread_create(&modprobe_thread, NULL,
> > +	if (igt_debug_on(pthread_create(&modprobe.thread, NULL,
> >  					modprobe_task, &modprobe))) {
> > -		ktap_parser_cancel();
> > -		igt_ignore_warn(ktap_parser_stop());
> > +		igt_ktap_free(ktap);
> >  		igt_skip("Failed to create a modprobe thread\n");
> >  	}
> >  
> > -	while (READ_ONCE(results->still_running) || !igt_list_empty(&results->list))
> > -	{
> > -		struct ktap_test_results_element *result;
> > -
> > -		if (!pthread_tryjoin_np(modprobe_thread, NULL) && modprobe.err) {
> > -			ktap_parser_cancel();
> > +	do {
> > +		ret = kunit_kmsg_result_get(&results, &modprobe,
> > +					    tst->kmsg, ktap);
> > +		if (igt_debug_on(ret && ret != -EINPROGRESS))
> >  			break;
> > -		}
> >  
> > -		if (igt_kernel_tainted(&taints)) {
> > -			ktap_parser_cancel();
> > -			pthread_cancel(modprobe_thread);
> > +		if (igt_debug_on(igt_list_empty(&results)))
> >  			break;
> > -		}
> >  
> > -		pthread_mutex_lock(&results->mutex);
> > -		if (igt_list_empty(&results->list)) {
> > -			pthread_mutex_unlock(&results->mutex);
> > -			continue;
> > -		}
> > +		r = igt_list_first_entry(&results, r, link);
> >  
> > -		result = igt_list_first_entry(&results->list, result, link);
> > +		igt_dynamic_f("%s-%s", r->suite_name, r->case_name) {
> > +			if (r->code == IGT_EXIT_INVALID) {
> > +				/* parametrized test case, get actual result */
> > +				kunit_result_free(&r, &suite_name, &case_name);
> >  
> > -		igt_list_del(&result->link);
> > -		pthread_mutex_unlock(&results->mutex);
> > +				igt_assert(igt_list_empty(&results));
> >  
> > -		igt_dynamic(result->test_name) {
> > -			igt_assert(READ_ONCE(result->passed));
> > +				ret = kunit_kmsg_result_get(&results, &modprobe,
> > +							    tst->kmsg, ktap);
> > +				if (ret != -EINPROGRESS)
> > +					igt_fail_on(ret);
> > +
> > +				igt_fail_on(igt_list_empty(&results));
> > +
> > +				r = igt_list_first_entry(&results, r, link);
> > +
> > +				igt_fail_on_f(strcmp(r->suite_name, suite_name),
> > +					      "suite_name expected: %s, got: %s\n",
> > +					      suite_name, r->suite_name);
> > +				igt_fail_on_f(strcmp(r->case_name, case_name),
> > +					      "case_name expected: %s, got: %s\n",
> > +					      case_name, r->case_name);
> > +			}
> >  
> > -			if (!pthread_tryjoin_np(modprobe_thread, NULL))
> > +			igt_assert_neq(r->code, IGT_EXIT_INVALID);
> > +
> > +			if (r->msg && *r->msg) {
> > +				igt_skip_on_f(r->code == IGT_EXIT_SKIP,
> > +					      "%s\n", r->msg);
> > +				igt_fail_on_f(r->code == IGT_EXIT_FAILURE,
> > +					      "%s\n", r->msg);
> > +				igt_abort_on_f(r->code == IGT_EXIT_ABORT,
> > +					      "%s\n", r->msg);
> > +			} else {
> > +				igt_skip_on(r->code == IGT_EXIT_SKIP);
> > +				igt_fail_on(r->code == IGT_EXIT_FAILURE);
> > +				if (r->code == IGT_EXIT_ABORT)
> > +					igt_fail(r->code);
> > +			}
> > +			igt_assert_eq(r->code, IGT_EXIT_SUCCESS);
> > +
> > +			switch (pthread_mutex_lock(&modprobe.lock)) {
> > +			case 0:
> > +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > +				break;
> > +			case EOWNERDEAD:
> > +				/* leave the mutex unrecoverable */
> > +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > +				__attribute__ ((fallthrough));
> > +			case ENOTRECOVERABLE:
> >  				igt_assert_eq(modprobe.err, 0);
> > +				break;
> 
> imho we should also break a do-while loop in case of error.

We are inside igt_dynamic() body, then I don't think we can safely break the 
outer loop from here, only pass, skip, or fail.

If we are not able to check whether the modprobe thread hasn't returned an 
error then I don't think that's a good reason for failing or skipping the 
current dynamic sub-subtest since we have its KTAP results already collected 
and we should just report them in that case, I believe.

If the error persists then the loop will get broken on next attempt to lock 
the mutex before entering blocking read(), next dynamic sub-subtests won't be 
executed, and the whole subtest will inform the user about that incompleteness 
of results from dynamic sub-subtests by returning SKIP.  Isn't that 
sufficient?

> 
> > +			default:
> > +				igt_debug("pthread_mutex_lock() failed\n");
> > +				break;
> > +			}
> >  
> >  			igt_assert_eq(igt_kernel_tainted(&taints), 0);
> >  		}
> >  
> > -		free(result);
> > +		kunit_result_free(&r, &suite_name, &case_name);
> > +
> > +	} while (ret == -EINPROGRESS);
> > +
> > +	igt_list_for_each_entry_safe(r, rn, &results, link)
> > +		kunit_result_free(&r, &suite_name, &case_name);
> > +
> > +	free(case_name);
> > +	free(suite_name);
> > +
> > +	switch (pthread_mutex_lock(&modprobe.lock)) {
> > +	case 0:
> > +		igt_debug_on(pthread_cancel(modprobe.thread));
> > +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> > +		break;
> > +	case EOWNERDEAD:
> > +		/* leave the mutex unrecoverable */
> > +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > +		break;
> > +	case ENOTRECOVERABLE:
> > +		break;
> > +	default:
> > +		igt_debug("pthread_mutex_lock() failed\n");
> > +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> > +		break;
> >  	}
> >  
> > -	pthread_join(modprobe_thread, NULL);
> > -
> > -	ret = ktap_parser_stop();
> > +	igt_ktap_free(ktap);
> >  
> >  	igt_skip_on(modprobe.err);
> >  	igt_skip_on(igt_kernel_tainted(&taints));
> > diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> > index 5eac102417..53a6c63288 100644
> > --- a/lib/igt_ktap.c
> > +++ b/lib/igt_ktap.c
> > @@ -4,17 +4,11 @@
> >   * Copyright © 2023 Intel Corporation
> >   */
> >  
> > -#include <ctype.h>
> > -#include <limits.h>
> > -#include <libkmod.h>
> > -#include <pthread.h>
> >  #include <errno.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > -#include <unistd.h>
> >  
> > -#include "igt_aux.h"
> >  #include "igt_core.h"
> >  #include "igt_ktap.h"
> >  #include "igt_list.h"
> > @@ -312,565 +306,3 @@ void igt_ktap_free(struct igt_ktap_results *ktap)
> >  {
> >  	free(ktap);
> >  }
> > -
> > -#define DELIMITER "-"
> > -
> > -struct ktap_parser_args {
> > -	int fd;
> > -	bool is_builtin;
> > -	int ret;
> > -} ktap_args;
> > -
> > -static struct ktap_test_results results;
> > -
> > -static int log_to_end(enum igt_log_level level, int fd,
> > -		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
> > -
> > -/**
> > - * log_to_end:
> > - * @level: #igt_log_level
> > - * @record: record to store the read data
> > - * @format: format string
> > - * @...: optional arguments used in the format string
> > - *
> > - * This is an altered version of the generic structured logging helper function
> > - * igt_log capable of reading to the end of a given line.
> > - *
> > - * Returns: 0 for success, or -2 if there's an error reading from the file
> > - */
> > -static int log_to_end(enum igt_log_level level, int fd,
> > -		      char *record, const char *format, ...)
> > -{
> > -	va_list args;
> > -	const char *lend;
> > -
> > -	/* Cutoff after newline character, in order to not display garbage */
> > -	char *cutoff = strchr(record, '\n');
> > -	if (cutoff) {
> > -		if (cutoff - record < BUF_LEN)
> > -			cutoff[1] = '\0';
> > -	}
> > -
> > -	va_start(args, format);
> > -	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
> > -	va_end(args);
> > -
> > -	lend = strchrnul(record, '\n');
> > -	while (*lend == '\0') {
> > -		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
> > -
> > -		if (read(fd, record, BUF_LEN) < 0) {
> > -			if (errno == EPIPE)
> > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -			else
> > -				igt_warn("an error occurred while reading kmsg: %m\n");
> > -
> > -			return -2;
> > -		}
> > -
> > -		lend = strchrnul(record, '\n');
> > -	}
> > -	return 0;
> > -}
> > -
> > -/**
> > - * lookup_value:
> > - * @haystack: the string to search in
> > - * @needle: the string to search for
> > - *
> > - * Returns: the value of the needle in the haystack, or -1 if not found.
> > - */
> > -static long lookup_value(const char *haystack, const char *needle)
> > -{
> > -	const char *needle_rptr;
> > -	char *needle_end;
> > -	long num;
> > -
> > -	needle_rptr = strcasestr(haystack, needle);
> > -
> > -	if (needle_rptr == NULL)
> > -		return -1;
> > -
> > -	/* Skip search string and whitespaces after it */
> > -	needle_rptr += strlen(needle);
> > -
> > -	num = strtol(needle_rptr, &needle_end, 10);
> > -
> > -	if (needle_rptr == needle_end)
> > -		return -1;
> > -
> > -	if (num == LONG_MIN || num == LONG_MAX)
> > -		return 0;
> > -
> > -	return num > 0 ? num : 0;
> > -}
> > -
> > -/**
> > - * tap_version_present:
> > - * @record: buffer with tap data
> > - * @print_info: whether tap version should be printed or not
> > - *
> > - * Returns:
> > - * 0 if not found
> > - * 1 if found
> > - */
> > -static int tap_version_present(char* record, bool print_info)
> > -{
> > -	/*
> > -	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> > -	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
> 
> Could you save these two lines comment in some other place?

Why?  Do you think that just that information from a multi-paragraph 
description of KTAP format is of particular importance?

Instead, let me add just the reference to the KTAP standard to a comment above 
the current parser implementation, OK?

Thanks,
Janusz

> 
> Regards,
> Kamil
> 
> > -	 *
> > -	 * but actually isn't, as it currently depends on the KUnit module
> > -	 * being built-in, so we can't rely on it every time
> > -	 */
> > -	const char *version_rptr = strcasestr(record, "TAP version ");
> > -	char *cutoff;
> > -
> > -	if (version_rptr == NULL)
> > -		return 0;
> > -
> > -	/* Cutoff after newline character, in order to not display garbage */
> > -	cutoff = strchr(version_rptr, '\n');
> > -	if (cutoff)
> > -		cutoff[0] = '\0';
> > -
> > -	if (print_info)
> > -		igt_info("%s\n", version_rptr);
> > -
> > -	return 1;
> > -}
> > -
> > -/**
> > - * find_next_tap_subtest:
> > - * @fd: file descriptor
> > - * @record: buffer used to read fd
> > - * @is_builtin: whether KUnit is built-in or not
> > - *
> > - * Returns:
> > - * 0 if there's missing information
> > - * -1 if not found
> > - * -2 if there are problems while reading the file.
> > - * any other value corresponds to the amount of cases of the next (sub)test
> > - */
> > -static int find_next_tap_subtest(int fd, char *record, char *test_name, bool is_builtin)
> > -{
> > -	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
> > -	long test_count;
> > -	char *cutoff;
> > -
> > -	test_name[0] = '\0';
> > -	test_name[BUF_LEN] = '\0';
> > -
> > -	test_lookup_str = " subtest: ";
> > -	subtest_lookup_str = " test: ";
> > -
> > -	if (!tap_version_present(record, true))
> > -		return -1;
> > -
> > -	if (is_builtin) {
> > -		if (read(fd, record, BUF_LEN) < 0) {
> > -			if (errno == EPIPE)
> > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -			else
> > -				igt_warn("an error occurred while reading kmsg: %m\n");
> > -
> > -			return -2;
> > -		}
> > -	}
> > -
> > -	name_rptr = strcasestr(record, test_lookup_str);
> > -	if (name_rptr != NULL) {
> > -		name_rptr += strlen(test_lookup_str);
> > -	} else {
> > -		name_rptr = strcasestr(record, subtest_lookup_str);
> > -		if (name_rptr != NULL)
> > -			name_rptr += strlen(subtest_lookup_str);
> > -	}
> > -
> > -	if (name_rptr == NULL) {
> > -		if (!is_builtin)
> > -			/* We've probably found nothing */
> > -			return -1;
> > -		igt_info("Missing test name\n");
> > -	} else {
> > -		strncpy(test_name, name_rptr, BUF_LEN);
> > -		/* Cutoff after newline character, in order to not display garbage */
> > -		cutoff = strchr(test_name, '\n');
> > -		if (cutoff)
> > -			cutoff[0] = '\0';
> > -
> > -		if (read(fd, record, BUF_LEN) < 0) {
> > -			if (errno == EPIPE)
> > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -			else
> > -				igt_warn("unknown error reading kmsg (%m)\n");
> > -
> > -			return -2;
> > -		}
> > -
> > -		/* Now we can be sure we found tests */
> > -		if (!is_builtin)
> > -			igt_info("KUnit is not built-in, skipping version check...\n");
> > -	}
> > -
> > -	/*
> > -	 * Total test count will almost always appear as 0..N at the beginning
> > -	 * of a run, so we use it to reliably identify a new run
> > -	 */
> > -	test_count = lookup_value(record, "..");
> > -
> > -	if (test_count <= 0) {
> > -		igt_info("Missing test count\n");
> > -		if (test_name[0] == '\0')
> > -			return 0;
> > -		if (log_to_end(IGT_LOG_INFO, fd, record,
> > -				"Running some tests in: %s\n",
> > -				test_name) < 0)
> > -			return -2;
> > -		return 0;
> > -	} else if (test_name[0] == '\0') {
> > -		igt_info("Running %ld tests...\n", test_count);
> > -		return 0;
> > -	}
> > -
> > -	if (log_to_end(IGT_LOG_INFO, fd, record,
> > -			"Executing %ld tests in: %s\n",
> > -			test_count, test_name) < 0)
> > -		return -2;
> > -
> > -	return test_count;
> > -}
> > -
> > -/**
> > - * parse_kmsg_for_tap:
> > - * @fd: file descriptor
> > - * @record: buffer used to read fd
> > - * @test_name: buffer to store the test name
> > - *
> > - * Returns:
> > - * 1 if no results were found
> > - * 0 if a test succeded
> > - * -1 if a test failed
> > - * -2 if there are problems reading the file
> > - */
> > -static int parse_kmsg_for_tap(int fd, char *record, char *test_name)
> > -{
> > -	const char *lstart, *ok_lookup_str, *nok_lookup_str,
> > -	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
> > -	char *test_name_end;
> > -
> > -	ok_lookup_str = "ok ";
> > -	nok_lookup_str = "not ok ";
> > -
> > -	lstart = strchrnul(record, ';');
> > -
> > -	if (*lstart == '\0') {
> > -		igt_warn("kmsg truncated: output malformed (%m)\n");
> > -		return -2;
> > -	}
> > -
> > -	lstart++;
> > -	while (isspace(*lstart))
> > -		lstart++;
> > -
> > -	nok_rptr = strstr(lstart, nok_lookup_str);
> > -	if (nok_rptr != NULL) {
> > -		nok_rptr += strlen(nok_lookup_str);
> > -		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
> > -			nok_rptr++;
> > -		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
> > -		while (!isspace(*test_name_end))
> > -			test_name_end++;
> > -		*test_name_end = '\0';
> > -		if (log_to_end(IGT_LOG_WARN, fd, record,
> > -			       "%s", lstart) < 0)
> > -			return -2;
> > -		return -1;
> > -	}
> > -
> > -	comment_start = strchrnul(lstart, '#');
> > -
> > -	/* Check if we're still in a subtest */
> > -	if (*comment_start != '\0') {
> > -		comment_start++;
> > -		value_parse_start = comment_start;
> > -
> > -		if (lookup_value(value_parse_start, "fail: ") > 0) {
> > -			if (log_to_end(IGT_LOG_WARN, fd, record,
> > -				       "%s", lstart) < 0)
> > -				return -2;
> > -			return -1;
> > -		}
> > -	}
> > -
> > -	ok_rptr = strstr(lstart, ok_lookup_str);
> > -	if (ok_rptr != NULL) {
> > -		ok_rptr += strlen(ok_lookup_str);
> > -		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
> > -			ok_rptr++;
> > -		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
> > -		while (!isspace(*test_name_end))
> > -			test_name_end++;
> > -		*test_name_end = '\0';
> > -		return 0;
> > -	}
> > -
> > -	return 1;
> > -}
> > -
> > -/**
> > - * parse_tap_level:
> > - * @fd: file descriptor
> > - * @base_test_name: test_name from upper recursion level
> > - * @test_count: test_count of this level
> > - * @failed_tests: top level failed_tests pointer
> > - * @found_tests: top level found_tests pointer
> > - * @is_builtin: whether the KUnit module is built-in or not
> > - *
> > - * Returns:
> > - * 0 if succeded
> > - * -1 if error occurred
> > - */
> > -__maybe_unused
> > -static int parse_tap_level(int fd, char *base_test_name, int test_count, bool *failed_tests,
> > -			   bool *found_tests, bool is_builtin)
> > -{
> > -	char record[BUF_LEN + 1];
> > -	struct ktap_test_results_element *r;
> > -	int internal_test_count;
> > -	char test_name[BUF_LEN + 1];
> > -	char base_test_name_for_next_level[BUF_LEN + 1];
> > -
> > -	for (int i = 0; i < test_count; i++) {
> > -		if (read(fd, record, BUF_LEN) < 0) {
> > -			if (errno == EPIPE)
> > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -			else
> > -				igt_warn("error reading kmsg (%m)\n");
> > -
> > -			return -1;
> > -		}
> > -
> > -		/* Sublevel found */
> > -		if (tap_version_present(record, false))
> > -		{
> > -			internal_test_count = find_next_tap_subtest(fd, record, test_name,
> > -								    is_builtin);
> > -			switch (internal_test_count) {
> > -			case -2:
> > -				/* No more data to read */
> > -				return -1;
> > -			case -1:
> > -				/* No test found */
> > -				return -1;
> > -			case 0:
> > -				/* Tests found, but they're missing info */
> > -				*found_tests = true;
> > -				return -1;
> > -			default:
> > -				*found_tests = true;
> > -
> > -				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
> > -				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
> > -				    base_test_name_for_next_level[0])
> > -					strncat(base_test_name_for_next_level, DELIMITER,
> > -						BUF_LEN - strlen(base_test_name_for_next_level));
> > -				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
> > -				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
> > -
> > -				if (parse_tap_level(fd, base_test_name_for_next_level,
> > -						    internal_test_count, failed_tests, found_tests,
> > -						    is_builtin) == -1)
> > -					return -1;
> > -				break;
> > -			}
> > -		}
> > -
> > -		switch (parse_kmsg_for_tap(fd, record, test_name)) {
> > -		case -2:
> > -			return -1;
> > -		case -1:
> > -			*failed_tests = true;
> > -
> > -			r = malloc(sizeof(*r));
> > -
> > -			memcpy(r->test_name, base_test_name, BUF_LEN);
> > -			if (strlen(r->test_name) < BUF_LEN - 1)
> > -				if (r->test_name[0])
> > -					strncat(r->test_name, DELIMITER,
> > -						BUF_LEN - strlen(r->test_name));
> > -			memcpy(r->test_name + strlen(r->test_name), test_name,
> > -			       BUF_LEN - strlen(r->test_name));
> > -			r->test_name[BUF_LEN] = '\0';
> > -
> > -			r->passed = false;
> > -
> > -			pthread_mutex_lock(&results.mutex);
> > -			igt_list_add_tail(&r->link, &results.list);
> > -			pthread_mutex_unlock(&results.mutex);
> > -
> > -			test_name[0] = '\0';
> > -			break;
> > -		case 0:
> > -			r = malloc(sizeof(*r));
> > -
> > -			memcpy(r->test_name, base_test_name, BUF_LEN);
> > -			if (strlen(r->test_name) < BUF_LEN - 1)
> > -				if (r->test_name[0])
> > -					strncat(r->test_name, DELIMITER,
> > -						BUF_LEN - strlen(r->test_name));
> > -			memcpy(r->test_name + strlen(r->test_name), test_name,
> > -			       BUF_LEN - strlen(r->test_name));
> > -			r->test_name[BUF_LEN] = '\0';
> > -
> > -			r->passed = true;
> > -
> > -			pthread_mutex_lock(&results.mutex);
> > -			igt_list_add_tail(&r->link, &results.list);
> > -			pthread_mutex_unlock(&results.mutex);
> > -
> > -			test_name[0] = '\0';
> > -			break;
> > -		default:
> > -			break;
> > -		}
> > -	}
> > -	return 0;
> > -}
> > -
> > -/**
> > - * igt_ktap_parser:
> > - *
> > - * This function parses the output of a ktap script and passes it to main thread.
> > - */
> > -void *igt_ktap_parser(void *unused)
> > -{
> > -	char record[BUF_LEN + 1], *buf, *suite_name = NULL, *case_name = NULL;
> > -	struct igt_ktap_results *ktap = NULL;
> > -	int fd = ktap_args.fd;
> > -	IGT_LIST_HEAD(list);
> > -	int err;
> > -
> > -	ktap = igt_ktap_alloc(&list);
> > -	if (igt_debug_on(!ktap))
> > -		goto igt_ktap_parser_end;
> > -
> > -	while (err = read(fd, record, BUF_LEN), err > 0) {
> > -		struct igt_ktap_result *r, *rn;
> > -
> > -		/* skip kmsg continuation lines */
> > -		if (igt_debug_on(*record == ' '))
> > -			continue;
> > -
> > -		/* NULL-terminate the record */
> > -		record[err] = '\0';
> > -
> > -		/* detect start of log message, continue if not found */
> > -		buf = strchrnul(record, ';');
> > -		if (igt_debug_on(*buf == '\0'))
> > -			continue;
> > -		buf++;
> > -
> > -		err = igt_ktap_parse(buf, ktap);
> > -
> > -		/* parsing error */
> > -		if (err && err != -EINPROGRESS)
> > -			goto igt_ktap_parser_end;
> > -
> > -		igt_list_for_each_entry_safe(r, rn, &list, link) {
> > -			struct ktap_test_results_element *result = NULL;
> > -			int code = r->code;
> > -
> > -			if (code != IGT_EXIT_INVALID)
> > -				result = calloc(1, sizeof(*result));
> > -
> > -			if (result) {
> > -				snprintf(result->test_name, sizeof(result->test_name),
> > -					 "%s-%s", r->suite_name, r->case_name);
> > -
> > -				if (code == IGT_EXIT_SUCCESS)
> > -					result->passed = true;
> > -			}
> > -
> > -			igt_list_del(&r->link);
> > -			if (r->suite_name != suite_name) {
> > -				free(suite_name);
> > -				suite_name = r->suite_name;
> > -			}
> > -			if (r->case_name != case_name) {
> > -				free(case_name);
> > -				case_name = r->case_name;
> > -			}
> > -			free(r->msg);
> > -			free(r);
> > -
> > -			/*
> > -			 * no extra result record expected on start
> > -			 * of parametrized test case -- skip it
> > -			 */
> > -			if (code == IGT_EXIT_INVALID)
> > -				continue;
> > -
> > -			if (!result) {
> > -				err = -ENOMEM;
> > -				goto igt_ktap_parser_end;
> > -			}
> > -
> > -			pthread_mutex_lock(&results.mutex);
> > -			igt_list_add_tail(&result->link, &results.list);
> > -			pthread_mutex_unlock(&results.mutex);
> > -		}
> > -
> > -		/* end of KTAP report */
> > -		if (!err)
> > -			goto igt_ktap_parser_end;
> > -	}
> > -
> > -	if (err < 0) {
> > -		if (errno == EPIPE)
> > -			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -		else
> > -			igt_warn("error reading kmsg (%m)\n");
> > -	}
> > -
> > -igt_ktap_parser_end:
> > -	free(suite_name);
> > -	free(case_name);
> > -
> > -	if (!err)
> > -		ktap_args.ret = IGT_EXIT_SUCCESS;
> > -
> > -	results.still_running = false;
> > -
> > -	if (ktap)
> > -		igt_ktap_free(ktap);
> > -
> > -	return NULL;
> > -}
> > -
> > -static pthread_t ktap_parser_thread;
> > -
> > -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin)
> > -{
> > -	IGT_INIT_LIST_HEAD(&results.list);
> > -	pthread_mutex_init(&results.mutex, NULL);
> > -	results.still_running = true;
> > -
> > -	ktap_args.fd = fd;
> > -	ktap_args.is_builtin = is_builtin;
> > -	ktap_args.ret = IGT_EXIT_FAILURE;
> > -	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
> > -
> > -	return &results;
> > -}
> > -
> > -void ktap_parser_cancel(void)
> > -{
> > -	pthread_cancel(ktap_parser_thread);
> > -}
> > -
> > -int ktap_parser_stop(void)
> > -{
> > -	pthread_join(ktap_parser_thread, NULL);
> > -	return ktap_args.ret;
> > -}
> > diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> > index 6f8da3eab6..c422636bfc 100644
> > --- a/lib/igt_ktap.h
> > +++ b/lib/igt_ktap.h
> > @@ -27,8 +27,6 @@
> >  
> >  #define BUF_LEN 4096
> >  
> > -#include <pthread.h>
> > -
> >  #include "igt_list.h"
> >  
> >  struct igt_ktap_result {
> > @@ -45,24 +43,4 @@ struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results);
> >  int igt_ktap_parse(const char *buf, struct igt_ktap_results *ktap);
> >  void igt_ktap_free(struct igt_ktap_results *ktap);
> >  
> > -void *igt_ktap_parser(void *unused);
> > -
> > -typedef struct ktap_test_results_element {
> > -	char test_name[BUF_LEN + 1];
> > -	bool passed;
> > -	struct igt_list_head link;
> > -} ktap_test_results_element;
> > -
> > -struct ktap_test_results {
> > -	struct igt_list_head list;
> > -	pthread_mutex_t mutex;
> > -	bool still_running;
> > -};
> > -
> > -
> > -
> > -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin);
> > -void ktap_parser_cancel(void);
> > -int ktap_parser_stop(void);
> > -
> >  #endif /* IGT_KTAP_H */
>

Janusz Krzysztofik Oct. 10, 2023, 5:54 p.m. UTC | #4

Hi Mauro,

Thanks for review.

On Tuesday, 10 October 2023 15:33:57 CEST Mauro Carvalho Chehab wrote:
> On Mon,  9 Oct 2023 14:27:55 +0200
> Janusz Krzysztofik <[e-mail]> wrote:
> 
> > There was an attempt to parse KTAP reports in the background while a kunit
> > test module is loading.  However, since dynamic sub-subtests can be
> > executed only from the main thread, that attempt was not quite successful,
> > as IGT results from all executed kunit test cases were generated only
> > after loading of kunit test module completed.
> > 
> > Now that the parser maintains its state and we can call it separately for
> > each input line of a KTAP report, it is perfectly possible to call the
> > parser from the main thread while the module is loading in the background,
> > and convert results from kunit test cases immediately to results of IGT
> > dynamic sub-subtests by running an igt_dynamic() section for each result
> > as soon as returned by the parser.
> > 
> > Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> > result obtained from igt_ktap_parse() called from the main thread.
> > 
> > Also, drop no longer needed functions from igt_ktap soruces.
> > 
> > v3: Fix ktap structure not freed on lseek error,
> >   - fix initial SIGCHLD handler not restored,
> >   - fix missing handling of potential errors returned by sigaction,
> >   - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> >     synchronization with modprobe thread,
> >   - fix potentially illegal use of igt_assert() called outside of
> >     dynamic sub-subtest section,
> >   - fix unsupported exit code potentially passed to igt_fail(),
> >   - no need to fail a dynamic sub-subtest on potential KTAP parser error
> >     after a valid result from the parser has been processed,
> >   - fix trailing newlines missing from error messages,
> >   - add more debug statements,
> >   - integrate common code around kunit_result_free() into it.
> > v2: Interrupt blocking read() on modprobe failure.
> > 
> > Signed-off-by: Janusz Krzysztofik <[e-mail]>
> > Acked-by: Mauro Carvalho Chehab <[e-mail]> # v2
> > ---
> >  lib/igt_kmod.c | 261 +++++++++++++++++++----
> >  lib/igt_ktap.c | 568 -------------------------------------------------
> >  lib/igt_ktap.h |  22 --
> >  3 files changed, 222 insertions(+), 629 deletions(-)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 426ae5b26f..7bca4cdaab 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -1,5 +1,5 @@
> >  /*
> > - * Copyright © 2016 Intel Corporation
> > + * Copyright © 2016-2023 Intel Corporation
> >   *
> >   * Permission is hereby granted, free of charge, to any person obtaining a
> >   * copy of this software and associated documentation files (the "Software"),
> > @@ -26,7 +26,12 @@
> >  #include <errno.h>
> >  #include <fcntl.h>
> >  #include <pthread.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> >  #include <sys/utsname.h>
> > +#include <unistd.h>
> > +
> > +#include "assembler/brw_compat.h"	/* [un]likely() */
> >  
> >  #include "igt_aux.h"
> >  #include "igt_core.h"
> > @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> >  }
> >  
> >  struct modprobe_data {
> > +	pthread_t parent;
> >  	struct kmod_module *kmod;
> >  	const char *opts;
> >  	int err;
> > +	pthread_mutex_t lock;
> > +	pthread_t thread;
> >  };
> >  
> >  static void *modprobe_task(void *arg)
> > @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
> >  
> >  	data->err = modprobe(data->kmod, data->opts);
> >  
> > +	if (igt_debug_on(data->err)) {
> > +		int err;
> > +
> > +		while (err = pthread_mutex_trylock(&data->lock),
> > +		       err && !igt_debug_on(err != EBUSY))
> > +			igt_debug_on(pthread_kill(data->parent, SIGCHLD));
> 
> I guess you need here an "igt_debug_on_once", as it doesn't make
> sense to have a (potentially) endless loop here printing error
> messages.

Right.  Since we don't have igt_debug_on_once() implemented, I'll open code 
that improvement.

Thanks,
Janusz

> 
> the other changes LGTM.
> 
> > +	} else {
> > +		/* let main thread use mutex to detect modprobe completion */
> > +		igt_debug_on(pthread_mutex_lock(&data->lock));
> > +	}
> > +
> >  	return NULL;
> >  }
> >  
> > +static void kunit_sigchld_handler(int signal)
> > +{
> > +	return;
> > +}
> > +
> > +static int kunit_kmsg_result_get(struct igt_list_head *results,
> > +				 struct modprobe_data *modprobe,
> > +				 int fd, struct igt_ktap_results *ktap)
> > +{
> > +	struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> > +			 *saved;
> > +	char record[BUF_LEN + 1], *buf;
> > +	unsigned long taints;
> > +	int ret;
> > +
> > +	do {
> > +		int err;
> > +
> > +		if (igt_debug_on(igt_kernel_tainted(&taints)))
> > +			return -ENOTRECOVERABLE;
> > +
> > +		err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved));
> > +		if (err == -1)
> > +			return -errno;
> > +		else if (unlikely(err))
> > +			return err;
> > +
> > +		err = pthread_mutex_lock(&modprobe->lock);
> > +		switch (err) {
> > +		case EOWNERDEAD:
> > +			/* leave the mutex unrecoverable */
> > +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> > +			__attribute__ ((fallthrough));
> > +		case ENOTRECOVERABLE:
> > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > +			if (igt_debug_on(modprobe->err))
> > +				return modprobe->err;
> > +			break;
> > +		case 0:
> > +			break;
> > +		default:
> > +			igt_debug("pthread_mutex_lock() error: %d\n", err);
> > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > +			return -err;
> > +		}
> > +
> > +		ret = read(fd, record, BUF_LEN);
> > +
> > +		if (!err) {
> > +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > +		}
> > +
> > +		if (igt_debug_on(!ret))
> > +			return -ENODATA;
> > +		if (igt_debug_on(ret == -1))
> > +			return -errno;
> > +		if (unlikely(igt_debug_on(ret < 0)))
> > +			break;
> > +
> > +		/* skip kmsg continuation lines */
> > +		if (igt_debug_on(*record == ' '))
> > +			continue;
> > +
> > +		/* NULL-terminate the record */
> > +		record[ret] = '\0';
> > +
> > +		/* detect start of log message, continue if not found */
> > +		buf = strchrnul(record, ';');
> > +		if (igt_debug_on(*buf == '\0'))
> > +			continue;
> > +		buf++;
> > +
> > +		ret = igt_ktap_parse(buf, ktap);
> > +		if (!ret || igt_debug_on(ret != -EINPROGRESS))
> > +			break;
> > +	} while (igt_list_empty(results));
> > +
> > +	return ret;
> > +}
> > +
> > +static void kunit_result_free(struct igt_ktap_result **r,
> > +			      char **suite_name, char **case_name)
> > +{
> > +	if (!*r)
> > +		return;
> > +
> > +	igt_list_del(&(*r)->link);
> > +
> > +	if ((*r)->suite_name != *suite_name) {
> > +		free(*suite_name);
> > +		*suite_name = (*r)->suite_name;
> > +	}
> > +
> > +	if ((*r)->case_name != *case_name) {
> > +		free(*case_name);
> > +		*case_name = (*r)->case_name;
> > +	}
> > +
> > +	free((*r)->msg);
> > +	free(*r);
> > +	*r = NULL;
> > +}
> > +
> >  static void __igt_kunit(struct igt_ktest *tst, const char *opts)
> >  {
> > -	struct modprobe_data modprobe = { tst->kmod, opts, 0, };
> > -	struct kmod_module *kunit_kmod;
> > -	bool is_builtin;
> > -	struct ktap_test_results *results;
> > -	pthread_t modprobe_thread;
> > +	struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, };
> > +	char *suite_name = NULL, *case_name = NULL;
> > +	struct igt_ktap_result *r, *rn;
> > +	struct igt_ktap_results *ktap;
> > +	pthread_mutexattr_t attr;
> > +	IGT_LIST_HEAD(results);
> >  	unsigned long taints;
> >  	int flags, ret;
> >  
> > @@ -780,60 +904,119 @@ static void __igt_kunit(struct igt_ktest *tst, const char *opts)
> >  
> >  	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
> >  
> > -	igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod));
> > -	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> > -	kmod_module_unref(kunit_kmod);
> > +	igt_skip_on(pthread_mutexattr_init(&attr));
> > +	igt_skip_on(pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST));
> > +	igt_skip_on(pthread_mutex_init(&modprobe.lock, &attr));
> >  
> > -	results = ktap_parser_start(tst->kmsg, is_builtin);
> > +	ktap = igt_ktap_alloc(&results);
> > +	igt_require(ktap);
> >  
> > -	if (igt_debug_on(pthread_create(&modprobe_thread, NULL,
> > +	if (igt_debug_on(pthread_create(&modprobe.thread, NULL,
> >  					modprobe_task, &modprobe))) {
> > -		ktap_parser_cancel();
> > -		igt_ignore_warn(ktap_parser_stop());
> > +		igt_ktap_free(ktap);
> >  		igt_skip("Failed to create a modprobe thread\n");
> >  	}
> >  
> > -	while (READ_ONCE(results->still_running) || !igt_list_empty(&results->list))
> > -	{
> > -		struct ktap_test_results_element *result;
> > -
> > -		if (!pthread_tryjoin_np(modprobe_thread, NULL) && modprobe.err) {
> > -			ktap_parser_cancel();
> > +	do {
> > +		ret = kunit_kmsg_result_get(&results, &modprobe,
> > +					    tst->kmsg, ktap);
> > +		if (igt_debug_on(ret && ret != -EINPROGRESS))
> >  			break;
> > -		}
> >  
> > -		if (igt_kernel_tainted(&taints)) {
> > -			ktap_parser_cancel();
> > -			pthread_cancel(modprobe_thread);
> > +		if (igt_debug_on(igt_list_empty(&results)))
> >  			break;
> > -		}
> >  
> > -		pthread_mutex_lock(&results->mutex);
> > -		if (igt_list_empty(&results->list)) {
> > -			pthread_mutex_unlock(&results->mutex);
> > -			continue;
> > -		}
> > +		r = igt_list_first_entry(&results, r, link);
> >  
> > -		result = igt_list_first_entry(&results->list, result, link);
> > +		igt_dynamic_f("%s-%s", r->suite_name, r->case_name) {
> > +			if (r->code == IGT_EXIT_INVALID) {
> > +				/* parametrized test case, get actual result */
> > +				kunit_result_free(&r, &suite_name, &case_name);
> >  
> > -		igt_list_del(&result->link);
> > -		pthread_mutex_unlock(&results->mutex);
> > +				igt_assert(igt_list_empty(&results));
> >  
> > -		igt_dynamic(result->test_name) {
> > -			igt_assert(READ_ONCE(result->passed));
> > +				ret = kunit_kmsg_result_get(&results, &modprobe,
> > +							    tst->kmsg, ktap);
> > +				if (ret != -EINPROGRESS)
> > +					igt_fail_on(ret);
> > +
> > +				igt_fail_on(igt_list_empty(&results));
> > +
> > +				r = igt_list_first_entry(&results, r, link);
> > +
> > +				igt_fail_on_f(strcmp(r->suite_name, suite_name),
> > +					      "suite_name expected: %s, got: %s\n",
> > +					      suite_name, r->suite_name);
> > +				igt_fail_on_f(strcmp(r->case_name, case_name),
> > +					      "case_name expected: %s, got: %s\n",
> > +					      case_name, r->case_name);
> > +			}
> >  
> > -			if (!pthread_tryjoin_np(modprobe_thread, NULL))
> > +			igt_assert_neq(r->code, IGT_EXIT_INVALID);
> > +
> > +			if (r->msg && *r->msg) {
> > +				igt_skip_on_f(r->code == IGT_EXIT_SKIP,
> > +					      "%s\n", r->msg);
> > +				igt_fail_on_f(r->code == IGT_EXIT_FAILURE,
> > +					      "%s\n", r->msg);
> > +				igt_abort_on_f(r->code == IGT_EXIT_ABORT,
> > +					      "%s\n", r->msg);
> > +			} else {
> > +				igt_skip_on(r->code == IGT_EXIT_SKIP);
> > +				igt_fail_on(r->code == IGT_EXIT_FAILURE);
> > +				if (r->code == IGT_EXIT_ABORT)
> > +					igt_fail(r->code);
> > +			}
> > +			igt_assert_eq(r->code, IGT_EXIT_SUCCESS);
> > +
> > +			switch (pthread_mutex_lock(&modprobe.lock)) {
> > +			case 0:
> > +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > +				break;
> > +			case EOWNERDEAD:
> > +				/* leave the mutex unrecoverable */
> > +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > +				__attribute__ ((fallthrough));
> > +			case ENOTRECOVERABLE:
> >  				igt_assert_eq(modprobe.err, 0);
> > +				break;
> > +			default:
> > +				igt_debug("pthread_mutex_lock() failed\n");
> > +				break;
> > +			}
> >  
> >  			igt_assert_eq(igt_kernel_tainted(&taints), 0);
> >  		}
> >  
> > -		free(result);
> > +		kunit_result_free(&r, &suite_name, &case_name);
> > +
> > +	} while (ret == -EINPROGRESS);
> > +
> > +	igt_list_for_each_entry_safe(r, rn, &results, link)
> > +		kunit_result_free(&r, &suite_name, &case_name);
> > +
> > +	free(case_name);
> > +	free(suite_name);
> > +
> > +	switch (pthread_mutex_lock(&modprobe.lock)) {
> > +	case 0:
> > +		igt_debug_on(pthread_cancel(modprobe.thread));
> > +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> > +		break;
> > +	case EOWNERDEAD:
> > +		/* leave the mutex unrecoverable */
> > +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > +		break;
> > +	case ENOTRECOVERABLE:
> > +		break;
> > +	default:
> > +		igt_debug("pthread_mutex_lock() failed\n");
> > +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> > +		break;
> >  	}
> >  
> > -	pthread_join(modprobe_thread, NULL);
> > -
> > -	ret = ktap_parser_stop();
> > +	igt_ktap_free(ktap);
> >  
> >  	igt_skip_on(modprobe.err);
> >  	igt_skip_on(igt_kernel_tainted(&taints));
> > diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> > index 5eac102417..53a6c63288 100644
> > --- a/lib/igt_ktap.c
> > +++ b/lib/igt_ktap.c
> > @@ -4,17 +4,11 @@
> >   * Copyright © 2023 Intel Corporation
> >   */
> >  
> > -#include <ctype.h>
> > -#include <limits.h>
> > -#include <libkmod.h>
> > -#include <pthread.h>
> >  #include <errno.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > -#include <unistd.h>
> >  
> > -#include "igt_aux.h"
> >  #include "igt_core.h"
> >  #include "igt_ktap.h"
> >  #include "igt_list.h"
> > @@ -312,565 +306,3 @@ void igt_ktap_free(struct igt_ktap_results *ktap)
> >  {
> >  	free(ktap);
> >  }
> > -
> > -#define DELIMITER "-"
> > -
> > -struct ktap_parser_args {
> > -	int fd;
> > -	bool is_builtin;
> > -	int ret;
> > -} ktap_args;
> > -
> > -static struct ktap_test_results results;
> > -
> > -static int log_to_end(enum igt_log_level level, int fd,
> > -		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
> > -
> > -/**
> > - * log_to_end:
> > - * @level: #igt_log_level
> > - * @record: record to store the read data
> > - * @format: format string
> > - * @...: optional arguments used in the format string
> > - *
> > - * This is an altered version of the generic structured logging helper function
> > - * igt_log capable of reading to the end of a given line.
> > - *
> > - * Returns: 0 for success, or -2 if there's an error reading from the file
> > - */
> > -static int log_to_end(enum igt_log_level level, int fd,
> > -		      char *record, const char *format, ...)
> > -{
> > -	va_list args;
> > -	const char *lend;
> > -
> > -	/* Cutoff after newline character, in order to not display garbage */
> > -	char *cutoff = strchr(record, '\n');
> > -	if (cutoff) {
> > -		if (cutoff - record < BUF_LEN)
> > -			cutoff[1] = '\0';
> > -	}
> > -
> > -	va_start(args, format);
> > -	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
> > -	va_end(args);
> > -
> > -	lend = strchrnul(record, '\n');
> > -	while (*lend == '\0') {
> > -		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
> > -
> > -		if (read(fd, record, BUF_LEN) < 0) {
> > -			if (errno == EPIPE)
> > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -			else
> > -				igt_warn("an error occurred while reading kmsg: %m\n");
> > -
> > -			return -2;
> > -		}
> > -
> > -		lend = strchrnul(record, '\n');
> > -	}
> > -	return 0;
> > -}
> > -
> > -/**
> > - * lookup_value:
> > - * @haystack: the string to search in
> > - * @needle: the string to search for
> > - *
> > - * Returns: the value of the needle in the haystack, or -1 if not found.
> > - */
> > -static long lookup_value(const char *haystack, const char *needle)
> > -{
> > -	const char *needle_rptr;
> > -	char *needle_end;
> > -	long num;
> > -
> > -	needle_rptr = strcasestr(haystack, needle);
> > -
> > -	if (needle_rptr == NULL)
> > -		return -1;
> > -
> > -	/* Skip search string and whitespaces after it */
> > -	needle_rptr += strlen(needle);
> > -
> > -	num = strtol(needle_rptr, &needle_end, 10);
> > -
> > -	if (needle_rptr == needle_end)
> > -		return -1;
> > -
> > -	if (num == LONG_MIN || num == LONG_MAX)
> > -		return 0;
> > -
> > -	return num > 0 ? num : 0;
> > -}
> > -
> > -/**
> > - * tap_version_present:
> > - * @record: buffer with tap data
> > - * @print_info: whether tap version should be printed or not
> > - *
> > - * Returns:
> > - * 0 if not found
> > - * 1 if found
> > - */
> > -static int tap_version_present(char* record, bool print_info)
> > -{
> > -	/*
> > -	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> > -	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
> > -	 *
> > -	 * but actually isn't, as it currently depends on the KUnit module
> > -	 * being built-in, so we can't rely on it every time
> > -	 */
> > -	const char *version_rptr = strcasestr(record, "TAP version ");
> > -	char *cutoff;
> > -
> > -	if (version_rptr == NULL)
> > -		return 0;
> > -
> > -	/* Cutoff after newline character, in order to not display garbage */
> > -	cutoff = strchr(version_rptr, '\n');
> > -	if (cutoff)
> > -		cutoff[0] = '\0';
> > -
> > -	if (print_info)
> > -		igt_info("%s\n", version_rptr);
> > -
> > -	return 1;
> > -}
> > -
> > -/**
> > - * find_next_tap_subtest:
> > - * @fd: file descriptor
> > - * @record: buffer used to read fd
> > - * @is_builtin: whether KUnit is built-in or not
> > - *
> > - * Returns:
> > - * 0 if there's missing information
> > - * -1 if not found
> > - * -2 if there are problems while reading the file.
> > - * any other value corresponds to the amount of cases of the next (sub)test
> > - */
> > -static int find_next_tap_subtest(int fd, char *record, char *test_name, bool is_builtin)
> > -{
> > -	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
> > -	long test_count;
> > -	char *cutoff;
> > -
> > -	test_name[0] = '\0';
> > -	test_name[BUF_LEN] = '\0';
> > -
> > -	test_lookup_str = " subtest: ";
> > -	subtest_lookup_str = " test: ";
> > -
> > -	if (!tap_version_present(record, true))
> > -		return -1;
> > -
> > -	if (is_builtin) {
> > -		if (read(fd, record, BUF_LEN) < 0) {
> > -			if (errno == EPIPE)
> > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -			else
> > -				igt_warn("an error occurred while reading kmsg: %m\n");
> > -
> > -			return -2;
> > -		}
> > -	}
> > -
> > -	name_rptr = strcasestr(record, test_lookup_str);
> > -	if (name_rptr != NULL) {
> > -		name_rptr += strlen(test_lookup_str);
> > -	} else {
> > -		name_rptr = strcasestr(record, subtest_lookup_str);
> > -		if (name_rptr != NULL)
> > -			name_rptr += strlen(subtest_lookup_str);
> > -	}
> > -
> > -	if (name_rptr == NULL) {
> > -		if (!is_builtin)
> > -			/* We've probably found nothing */
> > -			return -1;
> > -		igt_info("Missing test name\n");
> > -	} else {
> > -		strncpy(test_name, name_rptr, BUF_LEN);
> > -		/* Cutoff after newline character, in order to not display garbage */
> > -		cutoff = strchr(test_name, '\n');
> > -		if (cutoff)
> > -			cutoff[0] = '\0';
> > -
> > -		if (read(fd, record, BUF_LEN) < 0) {
> > -			if (errno == EPIPE)
> > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -			else
> > -				igt_warn("unknown error reading kmsg (%m)\n");
> > -
> > -			return -2;
> > -		}
> > -
> > -		/* Now we can be sure we found tests */
> > -		if (!is_builtin)
> > -			igt_info("KUnit is not built-in, skipping version check...\n");
> > -	}
> > -
> > -	/*
> > -	 * Total test count will almost always appear as 0..N at the beginning
> > -	 * of a run, so we use it to reliably identify a new run
> > -	 */
> > -	test_count = lookup_value(record, "..");
> > -
> > -	if (test_count <= 0) {
> > -		igt_info("Missing test count\n");
> > -		if (test_name[0] == '\0')
> > -			return 0;
> > -		if (log_to_end(IGT_LOG_INFO, fd, record,
> > -				"Running some tests in: %s\n",
> > -				test_name) < 0)
> > -			return -2;
> > -		return 0;
> > -	} else if (test_name[0] == '\0') {
> > -		igt_info("Running %ld tests...\n", test_count);
> > -		return 0;
> > -	}
> > -
> > -	if (log_to_end(IGT_LOG_INFO, fd, record,
> > -			"Executing %ld tests in: %s\n",
> > -			test_count, test_name) < 0)
> > -		return -2;
> > -
> > -	return test_count;
> > -}
> > -
> > -/**
> > - * parse_kmsg_for_tap:
> > - * @fd: file descriptor
> > - * @record: buffer used to read fd
> > - * @test_name: buffer to store the test name
> > - *
> > - * Returns:
> > - * 1 if no results were found
> > - * 0 if a test succeded
> > - * -1 if a test failed
> > - * -2 if there are problems reading the file
> > - */
> > -static int parse_kmsg_for_tap(int fd, char *record, char *test_name)
> > -{
> > -	const char *lstart, *ok_lookup_str, *nok_lookup_str,
> > -	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
> > -	char *test_name_end;
> > -
> > -	ok_lookup_str = "ok ";
> > -	nok_lookup_str = "not ok ";
> > -
> > -	lstart = strchrnul(record, ';');
> > -
> > -	if (*lstart == '\0') {
> > -		igt_warn("kmsg truncated: output malformed (%m)\n");
> > -		return -2;
> > -	}
> > -
> > -	lstart++;
> > -	while (isspace(*lstart))
> > -		lstart++;
> > -
> > -	nok_rptr = strstr(lstart, nok_lookup_str);
> > -	if (nok_rptr != NULL) {
> > -		nok_rptr += strlen(nok_lookup_str);
> > -		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
> > -			nok_rptr++;
> > -		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
> > -		while (!isspace(*test_name_end))
> > -			test_name_end++;
> > -		*test_name_end = '\0';
> > -		if (log_to_end(IGT_LOG_WARN, fd, record,
> > -			       "%s", lstart) < 0)
> > -			return -2;
> > -		return -1;
> > -	}
> > -
> > -	comment_start = strchrnul(lstart, '#');
> > -
> > -	/* Check if we're still in a subtest */
> > -	if (*comment_start != '\0') {
> > -		comment_start++;
> > -		value_parse_start = comment_start;
> > -
> > -		if (lookup_value(value_parse_start, "fail: ") > 0) {
> > -			if (log_to_end(IGT_LOG_WARN, fd, record,
> > -				       "%s", lstart) < 0)
> > -				return -2;
> > -			return -1;
> > -		}
> > -	}
> > -
> > -	ok_rptr = strstr(lstart, ok_lookup_str);
> > -	if (ok_rptr != NULL) {
> > -		ok_rptr += strlen(ok_lookup_str);
> > -		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
> > -			ok_rptr++;
> > -		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
> > -		while (!isspace(*test_name_end))
> > -			test_name_end++;
> > -		*test_name_end = '\0';
> > -		return 0;
> > -	}
> > -
> > -	return 1;
> > -}
> > -
> > -/**
> > - * parse_tap_level:
> > - * @fd: file descriptor
> > - * @base_test_name: test_name from upper recursion level
> > - * @test_count: test_count of this level
> > - * @failed_tests: top level failed_tests pointer
> > - * @found_tests: top level found_tests pointer
> > - * @is_builtin: whether the KUnit module is built-in or not
> > - *
> > - * Returns:
> > - * 0 if succeded
> > - * -1 if error occurred
> > - */
> > -__maybe_unused
> > -static int parse_tap_level(int fd, char *base_test_name, int test_count, bool *failed_tests,
> > -			   bool *found_tests, bool is_builtin)
> > -{
> > -	char record[BUF_LEN + 1];
> > -	struct ktap_test_results_element *r;
> > -	int internal_test_count;
> > -	char test_name[BUF_LEN + 1];
> > -	char base_test_name_for_next_level[BUF_LEN + 1];
> > -
> > -	for (int i = 0; i < test_count; i++) {
> > -		if (read(fd, record, BUF_LEN) < 0) {
> > -			if (errno == EPIPE)
> > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -			else
> > -				igt_warn("error reading kmsg (%m)\n");
> > -
> > -			return -1;
> > -		}
> > -
> > -		/* Sublevel found */
> > -		if (tap_version_present(record, false))
> > -		{
> > -			internal_test_count = find_next_tap_subtest(fd, record, test_name,
> > -								    is_builtin);
> > -			switch (internal_test_count) {
> > -			case -2:
> > -				/* No more data to read */
> > -				return -1;
> > -			case -1:
> > -				/* No test found */
> > -				return -1;
> > -			case 0:
> > -				/* Tests found, but they're missing info */
> > -				*found_tests = true;
> > -				return -1;
> > -			default:
> > -				*found_tests = true;
> > -
> > -				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
> > -				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
> > -				    base_test_name_for_next_level[0])
> > -					strncat(base_test_name_for_next_level, DELIMITER,
> > -						BUF_LEN - strlen(base_test_name_for_next_level));
> > -				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
> > -				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
> > -
> > -				if (parse_tap_level(fd, base_test_name_for_next_level,
> > -						    internal_test_count, failed_tests, found_tests,
> > -						    is_builtin) == -1)
> > -					return -1;
> > -				break;
> > -			}
> > -		}
> > -
> > -		switch (parse_kmsg_for_tap(fd, record, test_name)) {
> > -		case -2:
> > -			return -1;
> > -		case -1:
> > -			*failed_tests = true;
> > -
> > -			r = malloc(sizeof(*r));
> > -
> > -			memcpy(r->test_name, base_test_name, BUF_LEN);
> > -			if (strlen(r->test_name) < BUF_LEN - 1)
> > -				if (r->test_name[0])
> > -					strncat(r->test_name, DELIMITER,
> > -						BUF_LEN - strlen(r->test_name));
> > -			memcpy(r->test_name + strlen(r->test_name), test_name,
> > -			       BUF_LEN - strlen(r->test_name));
> > -			r->test_name[BUF_LEN] = '\0';
> > -
> > -			r->passed = false;
> > -
> > -			pthread_mutex_lock(&results.mutex);
> > -			igt_list_add_tail(&r->link, &results.list);
> > -			pthread_mutex_unlock(&results.mutex);
> > -
> > -			test_name[0] = '\0';
> > -			break;
> > -		case 0:
> > -			r = malloc(sizeof(*r));
> > -
> > -			memcpy(r->test_name, base_test_name, BUF_LEN);
> > -			if (strlen(r->test_name) < BUF_LEN - 1)
> > -				if (r->test_name[0])
> > -					strncat(r->test_name, DELIMITER,
> > -						BUF_LEN - strlen(r->test_name));
> > -			memcpy(r->test_name + strlen(r->test_name), test_name,
> > -			       BUF_LEN - strlen(r->test_name));
> > -			r->test_name[BUF_LEN] = '\0';
> > -
> > -			r->passed = true;
> > -
> > -			pthread_mutex_lock(&results.mutex);
> > -			igt_list_add_tail(&r->link, &results.list);
> > -			pthread_mutex_unlock(&results.mutex);
> > -
> > -			test_name[0] = '\0';
> > -			break;
> > -		default:
> > -			break;
> > -		}
> > -	}
> > -	return 0;
> > -}
> > -
> > -/**
> > - * igt_ktap_parser:
> > - *
> > - * This function parses the output of a ktap script and passes it to main thread.
> > - */
> > -void *igt_ktap_parser(void *unused)
> > -{
> > -	char record[BUF_LEN + 1], *buf, *suite_name = NULL, *case_name = NULL;
> > -	struct igt_ktap_results *ktap = NULL;
> > -	int fd = ktap_args.fd;
> > -	IGT_LIST_HEAD(list);
> > -	int err;
> > -
> > -	ktap = igt_ktap_alloc(&list);
> > -	if (igt_debug_on(!ktap))
> > -		goto igt_ktap_parser_end;
> > -
> > -	while (err = read(fd, record, BUF_LEN), err > 0) {
> > -		struct igt_ktap_result *r, *rn;
> > -
> > -		/* skip kmsg continuation lines */
> > -		if (igt_debug_on(*record == ' '))
> > -			continue;
> > -
> > -		/* NULL-terminate the record */
> > -		record[err] = '\0';
> > -
> > -		/* detect start of log message, continue if not found */
> > -		buf = strchrnul(record, ';');
> > -		if (igt_debug_on(*buf == '\0'))
> > -			continue;
> > -		buf++;
> > -
> > -		err = igt_ktap_parse(buf, ktap);
> > -
> > -		/* parsing error */
> > -		if (err && err != -EINPROGRESS)
> > -			goto igt_ktap_parser_end;
> > -
> > -		igt_list_for_each_entry_safe(r, rn, &list, link) {
> > -			struct ktap_test_results_element *result = NULL;
> > -			int code = r->code;
> > -
> > -			if (code != IGT_EXIT_INVALID)
> > -				result = calloc(1, sizeof(*result));
> > -
> > -			if (result) {
> > -				snprintf(result->test_name, sizeof(result->test_name),
> > -					 "%s-%s", r->suite_name, r->case_name);
> > -
> > -				if (code == IGT_EXIT_SUCCESS)
> > -					result->passed = true;
> > -			}
> > -
> > -			igt_list_del(&r->link);
> > -			if (r->suite_name != suite_name) {
> > -				free(suite_name);
> > -				suite_name = r->suite_name;
> > -			}
> > -			if (r->case_name != case_name) {
> > -				free(case_name);
> > -				case_name = r->case_name;
> > -			}
> > -			free(r->msg);
> > -			free(r);
> > -
> > -			/*
> > -			 * no extra result record expected on start
> > -			 * of parametrized test case -- skip it
> > -			 */
> > -			if (code == IGT_EXIT_INVALID)
> > -				continue;
> > -
> > -			if (!result) {
> > -				err = -ENOMEM;
> > -				goto igt_ktap_parser_end;
> > -			}
> > -
> > -			pthread_mutex_lock(&results.mutex);
> > -			igt_list_add_tail(&result->link, &results.list);
> > -			pthread_mutex_unlock(&results.mutex);
> > -		}
> > -
> > -		/* end of KTAP report */
> > -		if (!err)
> > -			goto igt_ktap_parser_end;
> > -	}
> > -
> > -	if (err < 0) {
> > -		if (errno == EPIPE)
> > -			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > -		else
> > -			igt_warn("error reading kmsg (%m)\n");
> > -	}
> > -
> > -igt_ktap_parser_end:
> > -	free(suite_name);
> > -	free(case_name);
> > -
> > -	if (!err)
> > -		ktap_args.ret = IGT_EXIT_SUCCESS;
> > -
> > -	results.still_running = false;
> > -
> > -	if (ktap)
> > -		igt_ktap_free(ktap);
> > -
> > -	return NULL;
> > -}
> > -
> > -static pthread_t ktap_parser_thread;
> > -
> > -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin)
> > -{
> > -	IGT_INIT_LIST_HEAD(&results.list);
> > -	pthread_mutex_init(&results.mutex, NULL);
> > -	results.still_running = true;
> > -
> > -	ktap_args.fd = fd;
> > -	ktap_args.is_builtin = is_builtin;
> > -	ktap_args.ret = IGT_EXIT_FAILURE;
> > -	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
> > -
> > -	return &results;
> > -}
> > -
> > -void ktap_parser_cancel(void)
> > -{
> > -	pthread_cancel(ktap_parser_thread);
> > -}
> > -
> > -int ktap_parser_stop(void)
> > -{
> > -	pthread_join(ktap_parser_thread, NULL);
> > -	return ktap_args.ret;
> > -}
> > diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> > index 6f8da3eab6..c422636bfc 100644
> > --- a/lib/igt_ktap.h
> > +++ b/lib/igt_ktap.h
> > @@ -27,8 +27,6 @@
> >  
> >  #define BUF_LEN 4096
> >  
> > -#include <pthread.h>
> > -
> >  #include "igt_list.h"
> >  
> >  struct igt_ktap_result {
> > @@ -45,24 +43,4 @@ struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results);
> >  int igt_ktap_parse(const char *buf, struct igt_ktap_results *ktap);
> >  void igt_ktap_free(struct igt_ktap_results *ktap);
> >  
> > -void *igt_ktap_parser(void *unused);
> > -
> > -typedef struct ktap_test_results_element {
> > -	char test_name[BUF_LEN + 1];
> > -	bool passed;
> > -	struct igt_list_head link;
> > -} ktap_test_results_element;
> > -
> > -struct ktap_test_results {
> > -	struct igt_list_head list;
> > -	pthread_mutex_t mutex;
> > -	bool still_running;
> > -};
> > -
> > -
> > -
> > -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin);
> > -void ktap_parser_cancel(void);
> > -int ktap_parser_stop(void);
> > -
> >  #endif /* IGT_KTAP_H */
>

Kamil Konieczny Oct. 11, 2023, 1 p.m. UTC | #5

Hi Janusz,
On 2023-10-10 at 19:49:35 +0200, Janusz Krzysztofik wrote:
> Hi Kamil,
> 
> Thanks for review.
> 
> On Tuesday, 10 October 2023 17:59:56 CEST Kamil Konieczny wrote:
> > Hi Janusz,
> > On 2023-10-09 at 14:27:55 +0200, Janusz Krzysztofik wrote:
> > > There was an attempt to parse KTAP reports in the background while a kunit
> > > test module is loading.  However, since dynamic sub-subtests can be
> > > executed only from the main thread, that attempt was not quite successful,
> > > as IGT results from all executed kunit test cases were generated only
> > > after loading of kunit test module completed.
> > > 
> > > Now that the parser maintains its state and we can call it separately for
> > > each input line of a KTAP report, it is perfectly possible to call the
> > > parser from the main thread while the module is loading in the background,
> > > and convert results from kunit test cases immediately to results of IGT
> > > dynamic sub-subtests by running an igt_dynamic() section for each result
> > > as soon as returned by the parser.
> > > 
> > > Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> > > result obtained from igt_ktap_parse() called from the main thread.
> > > 
> > > Also, drop no longer needed functions from igt_ktap soruces.
> > > 
> > > v3: Fix ktap structure not freed on lseek error,
> > >   - fix initial SIGCHLD handler not restored,
> > >   - fix missing handling of potential errors returned by sigaction,
> > >   - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> > >     synchronization with modprobe thread,
> > >   - fix potentially illegal use of igt_assert() called outside of
> > >     dynamic sub-subtest section,
> > >   - fix unsupported exit code potentially passed to igt_fail(),
> > >   - no need to fail a dynamic sub-subtest on potential KTAP parser error
> > >     after a valid result from the parser has been processed,
> > >   - fix trailing newlines missing from error messages,
> > >   - add more debug statements,
> > >   - integrate common code around kunit_result_free() into it.
> > > v2: Interrupt blocking read() on modprobe failure.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <[e-mail]>
> > > Acked-by: Mauro Carvalho Chehab <[e-mail]> # v2
> > > ---
> > >  lib/igt_kmod.c | 261 +++++++++++++++++++----
> > >  lib/igt_ktap.c | 568 -------------------------------------------------
> > >  lib/igt_ktap.h |  22 --
> > >  3 files changed, 222 insertions(+), 629 deletions(-)
> > > 
> > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > > index 426ae5b26f..7bca4cdaab 100644
> > > --- a/lib/igt_kmod.c
> > > +++ b/lib/igt_kmod.c
> > > @@ -1,5 +1,5 @@
> > >  /*
> > > - * Copyright © 2016 Intel Corporation
> > > + * Copyright © 2016-2023 Intel Corporation
> > >   *
> > >   * Permission is hereby granted, free of charge, to any person obtaining a
> > >   * copy of this software and associated documentation files (the "Software"),
> > > @@ -26,7 +26,12 @@
> > >  #include <errno.h>
> > >  #include <fcntl.h>
> > >  #include <pthread.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > >  #include <sys/utsname.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "assembler/brw_compat.h"	/* [un]likely() */
> > ------------ ^^^^^^^^^
> > Do we really need this?
> 
> I think the correct question is if wee really need [un]likely().  I'm using it 
> to document unlikely cases, which is a widely accepted method of documenting 
> cases like that, I believe.  Having that clarified, I hope you just tell me if 
> you think we need those cases documented, and how, if not that way.
> 

Now I see, it is for unlikely() - ok it can stay.

> > 
> > >  
> > >  #include "igt_aux.h"
> > >  #include "igt_core.h"
> > > @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> > >  }
> > >  
> > >  struct modprobe_data {
> > > +	pthread_t parent;
> > --- ^^^^^^^^^^^^^^^^
> > Please move it below to other related thread data.
> > Also consider a comment why(or for what purpose)
> > did you put this here.
> 
> No problem to move it to the bottom, if that's important to you, but regarding 
> the comment, do you think that the purpose of this field of the structure, 
> compared to other fields, is so unclear form review of the code, despite its 
> name, that it requires a comment, unlike the other fields?
> 

I am ok with only move.

> > 
> > >  	struct kmod_module *kmod;
> > >  	const char *opts;
> > >  	int err;
> > > +	pthread_mutex_t lock;
> > > +	pthread_t thread;
> > >  };
> > >  
> > >  static void *modprobe_task(void *arg)
> > > @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
> > >  
> > >  	data->err = modprobe(data->kmod, data->opts);
> > >  
> > > +	if (igt_debug_on(data->err)) {
> > > +		int err;
> > > +
> > > +		while (err = pthread_mutex_trylock(&data->lock),
> > > +		       err && !igt_debug_on(err != EBUSY))
> > > +			igt_debug_on(pthread_kill(data->parent, SIGCHLD));
> > > +	} else {
> > > +		/* let main thread use mutex to detect modprobe completion */
> > > +		igt_debug_on(pthread_mutex_lock(&data->lock));
> > > +	}
> > > +
> > >  	return NULL;
> > >  }
> > >  
> > > +static void kunit_sigchld_handler(int signal)
> > > +{
> > > +	return;
> > --- ^^^^^^^
> > Why not removing this? checkpatch complains about return from void.
> 
> OK.
> 
> > 
> > > +}
> > > +
> > > +static int kunit_kmsg_result_get(struct igt_list_head *results,
> > > +				 struct modprobe_data *modprobe,
> > > +				 int fd, struct igt_ktap_results *ktap)
> > > +{
> > > +	struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> > > +			 *saved;
> > > +	char record[BUF_LEN + 1], *buf;
> > > +	unsigned long taints;
> > > +	int ret;
> > > +
> > > +	do {
> > > +		int err;
> > > +
> > > +		if (igt_debug_on(igt_kernel_tainted(&taints)))
> > > +			return -ENOTRECOVERABLE;
> > > +
> > > +		err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved));
> > > +		if (err == -1)
> > > +			return -errno;
> > > +		else if (unlikely(err))
> > > +			return err;
> > > +
> > > +		err = pthread_mutex_lock(&modprobe->lock);
> > > +		switch (err) {
> > > +		case EOWNERDEAD:
> > > +			/* leave the mutex unrecoverable */
> > > +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> > > +			__attribute__ ((fallthrough));
> > > +		case ENOTRECOVERABLE:
> > > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > > +			if (igt_debug_on(modprobe->err))
> > > +				return modprobe->err;
> > 
> > Why no 'return -err;' here?
> 
> Because modprobe->err, which can tell us more about what has gone bad in the 
> modprobe thread, is more important here than err which only informs us about 
> completion of the modprobe thread, also if successful, isn't it?
> 

Thank you for explanation.

> > 
> > > +			break;
> > > +		case 0:
> > > +			break;
> > > +		default:
> > > +			igt_debug("pthread_mutex_lock() error: %d\n", err);
> > > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > > +			return -err;
> > > +		}
> > > +
> > > +		ret = read(fd, record, BUF_LEN);
> > > +
> > > +		if (!err) {
> >         ^^^^^^^^^^^
> > Looks strange here.
> 
> Here err still carries a return code from pthread_mutex_lock(), then !err 
> means that the modprobe thread was still running and we have 1) acquired 
> the mutex which now we have to release, 2) installed a non-default 
> SIGCHLD handler that we should now reset.  Have you got a better idea 
> how to encode that so it looks less strange?  Would a comment that reminds 
> a reader where that err comes from be sufficient?
> 

Yes please add comment here.

> > 
> > > +			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
> > > +			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
> > > +		}
> > > +
> > > +		if (igt_debug_on(!ret))
> > > +			return -ENODATA;
> > > +		if (igt_debug_on(ret == -1))
> > > +			return -errno;
> > > +		if (unlikely(igt_debug_on(ret < 0)))
> > > +			break;
> > > +
> > > +		/* skip kmsg continuation lines */
> > > +		if (igt_debug_on(*record == ' '))
> > ----------- ^^^^^^^^^^^^
> > Why debug here? imho it is not needed.
> 
> Because we don't expect continuation lines while reading a KTAP report. Since 
> the kernel log may still contain them, we want to know about them when 
> something wrong happens to our KTAP parser.  That's the purpose of all those 
> debug_on() calls: provide as much information on unexpected conditions as 
> possible when the test fails, isn't it?
> 

Ok.

> > 
> > > +			continue;
> > > +
> > > +		/* NULL-terminate the record */
> > > +		record[ret] = '\0';
> > > +
> > > +		/* detect start of log message, continue if not found */
> > > +		buf = strchrnul(record, ';');
> > > +		if (igt_debug_on(*buf == '\0'))
> > ----------- ^^^^^^^^^^^^
> > Same here, you just continue loop if no ';'?
> 
> That means we are getting incorrectly formatted records from /dev/kmsg, and 
> I'm sure we would like to be informed about that if something goes wrong with 
> the test, wouldn't we?
> 

Ok.

> > 
> > > +			continue;
> > > +		buf++;
> > > +
> > > +		ret = igt_ktap_parse(buf, ktap);
> > > +		if (!ret || igt_debug_on(ret != -EINPROGRESS))
> > > +			break;
> > > +	} while (igt_list_empty(results));
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void kunit_result_free(struct igt_ktap_result **r,
> > > +			      char **suite_name, char **case_name)
> > > +{
> > > +	if (!*r)
> > > +		return;
> > > +
> > > +	igt_list_del(&(*r)->link);
> > > +
> > > +	if ((*r)->suite_name != *suite_name) {
> > > +		free(*suite_name);
> > > +		*suite_name = (*r)->suite_name;
> > > +	}
> > > +
> > > +	if ((*r)->case_name != *case_name) {
> > > +		free(*case_name);
> > > +		*case_name = (*r)->case_name;
> > > +	}
> > > +
> > > +	free((*r)->msg);
> > > +	free(*r);
> > > +	*r = NULL;
> > > +}
> > > +
> > >  static void __igt_kunit(struct igt_ktest *tst, const char *opts)
> > >  {
> > > -	struct modprobe_data modprobe = { tst->kmod, opts, 0, };
> > > -	struct kmod_module *kunit_kmod;
> > > -	bool is_builtin;
> > > -	struct ktap_test_results *results;
> > > -	pthread_t modprobe_thread;
> > > +	struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, };
> > > +	char *suite_name = NULL, *case_name = NULL;
> > > +	struct igt_ktap_result *r, *rn;
> > > +	struct igt_ktap_results *ktap;
> > > +	pthread_mutexattr_t attr;
> > > +	IGT_LIST_HEAD(results);
> > >  	unsigned long taints;
> > >  	int flags, ret;
> > >  
> > > @@ -780,60 +904,119 @@ static void __igt_kunit(struct igt_ktest *tst, const char *opts)
> > >  
> > >  	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
> > >  
> > > -	igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod));
> > > -	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> > > -	kmod_module_unref(kunit_kmod);
> > > +	igt_skip_on(pthread_mutexattr_init(&attr));
> > > +	igt_skip_on(pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST));
> > > +	igt_skip_on(pthread_mutex_init(&modprobe.lock, &attr));
> > >  
> > > -	results = ktap_parser_start(tst->kmsg, is_builtin);
> > > +	ktap = igt_ktap_alloc(&results);
> > > +	igt_require(ktap);
> > >  
> > > -	if (igt_debug_on(pthread_create(&modprobe_thread, NULL,
> > > +	if (igt_debug_on(pthread_create(&modprobe.thread, NULL,
> > >  					modprobe_task, &modprobe))) {
> > > -		ktap_parser_cancel();
> > > -		igt_ignore_warn(ktap_parser_stop());
> > > +		igt_ktap_free(ktap);
> > >  		igt_skip("Failed to create a modprobe thread\n");
> > >  	}
> > >  
> > > -	while (READ_ONCE(results->still_running) || !igt_list_empty(&results->list))
> > > -	{
> > > -		struct ktap_test_results_element *result;
> > > -
> > > -		if (!pthread_tryjoin_np(modprobe_thread, NULL) && modprobe.err) {
> > > -			ktap_parser_cancel();
> > > +	do {
> > > +		ret = kunit_kmsg_result_get(&results, &modprobe,
> > > +					    tst->kmsg, ktap);
> > > +		if (igt_debug_on(ret && ret != -EINPROGRESS))
> > >  			break;
> > > -		}
> > >  
> > > -		if (igt_kernel_tainted(&taints)) {
> > > -			ktap_parser_cancel();
> > > -			pthread_cancel(modprobe_thread);
> > > +		if (igt_debug_on(igt_list_empty(&results)))
> > >  			break;
> > > -		}
> > >  
> > > -		pthread_mutex_lock(&results->mutex);
> > > -		if (igt_list_empty(&results->list)) {
> > > -			pthread_mutex_unlock(&results->mutex);
> > > -			continue;
> > > -		}
> > > +		r = igt_list_first_entry(&results, r, link);
> > >  
> > > -		result = igt_list_first_entry(&results->list, result, link);
> > > +		igt_dynamic_f("%s-%s", r->suite_name, r->case_name) {
> > > +			if (r->code == IGT_EXIT_INVALID) {
> > > +				/* parametrized test case, get actual result */
> > > +				kunit_result_free(&r, &suite_name, &case_name);
> > >  
> > > -		igt_list_del(&result->link);
> > > -		pthread_mutex_unlock(&results->mutex);
> > > +				igt_assert(igt_list_empty(&results));
> > >  
> > > -		igt_dynamic(result->test_name) {
> > > -			igt_assert(READ_ONCE(result->passed));
> > > +				ret = kunit_kmsg_result_get(&results, &modprobe,
> > > +							    tst->kmsg, ktap);
> > > +				if (ret != -EINPROGRESS)
> > > +					igt_fail_on(ret);
> > > +
> > > +				igt_fail_on(igt_list_empty(&results));
> > > +
> > > +				r = igt_list_first_entry(&results, r, link);
> > > +
> > > +				igt_fail_on_f(strcmp(r->suite_name, suite_name),
> > > +					      "suite_name expected: %s, got: %s\n",
> > > +					      suite_name, r->suite_name);
> > > +				igt_fail_on_f(strcmp(r->case_name, case_name),
> > > +					      "case_name expected: %s, got: %s\n",
> > > +					      case_name, r->case_name);
> > > +			}
> > >  
> > > -			if (!pthread_tryjoin_np(modprobe_thread, NULL))
> > > +			igt_assert_neq(r->code, IGT_EXIT_INVALID);
> > > +
> > > +			if (r->msg && *r->msg) {
> > > +				igt_skip_on_f(r->code == IGT_EXIT_SKIP,
> > > +					      "%s\n", r->msg);
> > > +				igt_fail_on_f(r->code == IGT_EXIT_FAILURE,
> > > +					      "%s\n", r->msg);
> > > +				igt_abort_on_f(r->code == IGT_EXIT_ABORT,
> > > +					      "%s\n", r->msg);
> > > +			} else {
> > > +				igt_skip_on(r->code == IGT_EXIT_SKIP);
> > > +				igt_fail_on(r->code == IGT_EXIT_FAILURE);
> > > +				if (r->code == IGT_EXIT_ABORT)
> > > +					igt_fail(r->code);
> > > +			}
> > > +			igt_assert_eq(r->code, IGT_EXIT_SUCCESS);
> > > +
> > > +			switch (pthread_mutex_lock(&modprobe.lock)) {
> > > +			case 0:
> > > +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > > +				break;
> > > +			case EOWNERDEAD:
> > > +				/* leave the mutex unrecoverable */
> > > +				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > > +				__attribute__ ((fallthrough));
> > > +			case ENOTRECOVERABLE:
> > >  				igt_assert_eq(modprobe.err, 0);
> > > +				break;
> > 
> > imho we should also break a do-while loop in case of error.
> 
> We are inside igt_dynamic() body, then I don't think we can safely break the 
> outer loop from here, only pass, skip, or fail.
> 
> If we are not able to check whether the modprobe thread hasn't returned an 
> error then I don't think that's a good reason for failing or skipping the 
> current dynamic sub-subtest since we have its KTAP results already collected 
> and we should just report them in that case, I believe.
> 
> If the error persists then the loop will get broken on next attempt to lock 
> the mutex before entering blocking read(), next dynamic sub-subtests won't be 
> executed, and the whole subtest will inform the user about that incompleteness 
> of results from dynamic sub-subtests by returning SKIP.  Isn't that 
> sufficient?
> 

Yes it is, thank you for explaining it.

> > 
> > > +			default:
> > > +				igt_debug("pthread_mutex_lock() failed\n");
> > > +				break;
> > > +			}
> > >  
> > >  			igt_assert_eq(igt_kernel_tainted(&taints), 0);
> > >  		}
> > >  
> > > -		free(result);
> > > +		kunit_result_free(&r, &suite_name, &case_name);
> > > +
> > > +	} while (ret == -EINPROGRESS);
> > > +
> > > +	igt_list_for_each_entry_safe(r, rn, &results, link)
> > > +		kunit_result_free(&r, &suite_name, &case_name);
> > > +
> > > +	free(case_name);
> > > +	free(suite_name);
> > > +
> > > +	switch (pthread_mutex_lock(&modprobe.lock)) {
> > > +	case 0:
> > > +		igt_debug_on(pthread_cancel(modprobe.thread));
> > > +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > > +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> > > +		break;
> > > +	case EOWNERDEAD:
> > > +		/* leave the mutex unrecoverable */
> > > +		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
> > > +		break;
> > > +	case ENOTRECOVERABLE:
> > > +		break;
> > > +	default:
> > > +		igt_debug("pthread_mutex_lock() failed\n");
> > > +		igt_debug_on(pthread_join(modprobe.thread, NULL));
> > > +		break;
> > >  	}
> > >  
> > > -	pthread_join(modprobe_thread, NULL);
> > > -
> > > -	ret = ktap_parser_stop();
> > > +	igt_ktap_free(ktap);
> > >  
> > >  	igt_skip_on(modprobe.err);
> > >  	igt_skip_on(igt_kernel_tainted(&taints));
> > > diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> > > index 5eac102417..53a6c63288 100644
> > > --- a/lib/igt_ktap.c
> > > +++ b/lib/igt_ktap.c
> > > @@ -4,17 +4,11 @@
> > >   * Copyright © 2023 Intel Corporation
> > >   */
> > >  
> > > -#include <ctype.h>
> > > -#include <limits.h>
> > > -#include <libkmod.h>
> > > -#include <pthread.h>
> > >  #include <errno.h>
> > >  #include <stdio.h>
> > >  #include <stdlib.h>
> > >  #include <string.h>
> > > -#include <unistd.h>
> > >  
> > > -#include "igt_aux.h"
> > >  #include "igt_core.h"
> > >  #include "igt_ktap.h"
> > >  #include "igt_list.h"
> > > @@ -312,565 +306,3 @@ void igt_ktap_free(struct igt_ktap_results *ktap)
> > >  {
> > >  	free(ktap);
> > >  }
> > > -
> > > -#define DELIMITER "-"
> > > -
> > > -struct ktap_parser_args {
> > > -	int fd;
> > > -	bool is_builtin;
> > > -	int ret;
> > > -} ktap_args;
> > > -
> > > -static struct ktap_test_results results;
> > > -
> > > -static int log_to_end(enum igt_log_level level, int fd,
> > > -		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
> > > -
> > > -/**
> > > - * log_to_end:
> > > - * @level: #igt_log_level
> > > - * @record: record to store the read data
> > > - * @format: format string
> > > - * @...: optional arguments used in the format string
> > > - *
> > > - * This is an altered version of the generic structured logging helper function
> > > - * igt_log capable of reading to the end of a given line.
> > > - *
> > > - * Returns: 0 for success, or -2 if there's an error reading from the file
> > > - */
> > > -static int log_to_end(enum igt_log_level level, int fd,
> > > -		      char *record, const char *format, ...)
> > > -{
> > > -	va_list args;
> > > -	const char *lend;
> > > -
> > > -	/* Cutoff after newline character, in order to not display garbage */
> > > -	char *cutoff = strchr(record, '\n');
> > > -	if (cutoff) {
> > > -		if (cutoff - record < BUF_LEN)
> > > -			cutoff[1] = '\0';
> > > -	}
> > > -
> > > -	va_start(args, format);
> > > -	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
> > > -	va_end(args);
> > > -
> > > -	lend = strchrnul(record, '\n');
> > > -	while (*lend == '\0') {
> > > -		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
> > > -
> > > -		if (read(fd, record, BUF_LEN) < 0) {
> > > -			if (errno == EPIPE)
> > > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -			else
> > > -				igt_warn("an error occurred while reading kmsg: %m\n");
> > > -
> > > -			return -2;
> > > -		}
> > > -
> > > -		lend = strchrnul(record, '\n');
> > > -	}
> > > -	return 0;
> > > -}
> > > -
> > > -/**
> > > - * lookup_value:
> > > - * @haystack: the string to search in
> > > - * @needle: the string to search for
> > > - *
> > > - * Returns: the value of the needle in the haystack, or -1 if not found.
> > > - */
> > > -static long lookup_value(const char *haystack, const char *needle)
> > > -{
> > > -	const char *needle_rptr;
> > > -	char *needle_end;
> > > -	long num;
> > > -
> > > -	needle_rptr = strcasestr(haystack, needle);
> > > -
> > > -	if (needle_rptr == NULL)
> > > -		return -1;
> > > -
> > > -	/* Skip search string and whitespaces after it */
> > > -	needle_rptr += strlen(needle);
> > > -
> > > -	num = strtol(needle_rptr, &needle_end, 10);
> > > -
> > > -	if (needle_rptr == needle_end)
> > > -		return -1;
> > > -
> > > -	if (num == LONG_MIN || num == LONG_MAX)
> > > -		return 0;
> > > -
> > > -	return num > 0 ? num : 0;
> > > -}
> > > -
> > > -/**
> > > - * tap_version_present:
> > > - * @record: buffer with tap data
> > > - * @print_info: whether tap version should be printed or not
> > > - *
> > > - * Returns:
> > > - * 0 if not found
> > > - * 1 if found
> > > - */
> > > -static int tap_version_present(char* record, bool print_info)
> > > -{
> > > -	/*
> > > -	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> > > -	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
> > 
> > Could you save these two lines comment in some other place?
> 
> Why?  Do you think that just that information from a multi-paragraph 
> description of KTAP format is of particular importance?
> 
> Instead, let me add just the reference to the KTAP standard to a comment above 
> the current parser implementation, OK?

Seems good, please add the reference.

Regards,
Kamil

> 
> Thanks,
> Janusz
> 
> > 
> > Regards,
> > Kamil
> > 
> > > -	 *
> > > -	 * but actually isn't, as it currently depends on the KUnit module
> > > -	 * being built-in, so we can't rely on it every time
> > > -	 */
> > > -	const char *version_rptr = strcasestr(record, "TAP version ");
> > > -	char *cutoff;
> > > -
> > > -	if (version_rptr == NULL)
> > > -		return 0;
> > > -
> > > -	/* Cutoff after newline character, in order to not display garbage */
> > > -	cutoff = strchr(version_rptr, '\n');
> > > -	if (cutoff)
> > > -		cutoff[0] = '\0';
> > > -
> > > -	if (print_info)
> > > -		igt_info("%s\n", version_rptr);
> > > -
> > > -	return 1;
> > > -}
> > > -
> > > -/**
> > > - * find_next_tap_subtest:
> > > - * @fd: file descriptor
> > > - * @record: buffer used to read fd
> > > - * @is_builtin: whether KUnit is built-in or not
> > > - *
> > > - * Returns:
> > > - * 0 if there's missing information
> > > - * -1 if not found
> > > - * -2 if there are problems while reading the file.
> > > - * any other value corresponds to the amount of cases of the next (sub)test
> > > - */
> > > -static int find_next_tap_subtest(int fd, char *record, char *test_name, bool is_builtin)
> > > -{
> > > -	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
> > > -	long test_count;
> > > -	char *cutoff;
> > > -
> > > -	test_name[0] = '\0';
> > > -	test_name[BUF_LEN] = '\0';
> > > -
> > > -	test_lookup_str = " subtest: ";
> > > -	subtest_lookup_str = " test: ";
> > > -
> > > -	if (!tap_version_present(record, true))
> > > -		return -1;
> > > -
> > > -	if (is_builtin) {
> > > -		if (read(fd, record, BUF_LEN) < 0) {
> > > -			if (errno == EPIPE)
> > > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -			else
> > > -				igt_warn("an error occurred while reading kmsg: %m\n");
> > > -
> > > -			return -2;
> > > -		}
> > > -	}
> > > -
> > > -	name_rptr = strcasestr(record, test_lookup_str);
> > > -	if (name_rptr != NULL) {
> > > -		name_rptr += strlen(test_lookup_str);
> > > -	} else {
> > > -		name_rptr = strcasestr(record, subtest_lookup_str);
> > > -		if (name_rptr != NULL)
> > > -			name_rptr += strlen(subtest_lookup_str);
> > > -	}
> > > -
> > > -	if (name_rptr == NULL) {
> > > -		if (!is_builtin)
> > > -			/* We've probably found nothing */
> > > -			return -1;
> > > -		igt_info("Missing test name\n");
> > > -	} else {
> > > -		strncpy(test_name, name_rptr, BUF_LEN);
> > > -		/* Cutoff after newline character, in order to not display garbage */
> > > -		cutoff = strchr(test_name, '\n');
> > > -		if (cutoff)
> > > -			cutoff[0] = '\0';
> > > -
> > > -		if (read(fd, record, BUF_LEN) < 0) {
> > > -			if (errno == EPIPE)
> > > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -			else
> > > -				igt_warn("unknown error reading kmsg (%m)\n");
> > > -
> > > -			return -2;
> > > -		}
> > > -
> > > -		/* Now we can be sure we found tests */
> > > -		if (!is_builtin)
> > > -			igt_info("KUnit is not built-in, skipping version check...\n");
> > > -	}
> > > -
> > > -	/*
> > > -	 * Total test count will almost always appear as 0..N at the beginning
> > > -	 * of a run, so we use it to reliably identify a new run
> > > -	 */
> > > -	test_count = lookup_value(record, "..");
> > > -
> > > -	if (test_count <= 0) {
> > > -		igt_info("Missing test count\n");
> > > -		if (test_name[0] == '\0')
> > > -			return 0;
> > > -		if (log_to_end(IGT_LOG_INFO, fd, record,
> > > -				"Running some tests in: %s\n",
> > > -				test_name) < 0)
> > > -			return -2;
> > > -		return 0;
> > > -	} else if (test_name[0] == '\0') {
> > > -		igt_info("Running %ld tests...\n", test_count);
> > > -		return 0;
> > > -	}
> > > -
> > > -	if (log_to_end(IGT_LOG_INFO, fd, record,
> > > -			"Executing %ld tests in: %s\n",
> > > -			test_count, test_name) < 0)
> > > -		return -2;
> > > -
> > > -	return test_count;
> > > -}
> > > -
> > > -/**
> > > - * parse_kmsg_for_tap:
> > > - * @fd: file descriptor
> > > - * @record: buffer used to read fd
> > > - * @test_name: buffer to store the test name
> > > - *
> > > - * Returns:
> > > - * 1 if no results were found
> > > - * 0 if a test succeded
> > > - * -1 if a test failed
> > > - * -2 if there are problems reading the file
> > > - */
> > > -static int parse_kmsg_for_tap(int fd, char *record, char *test_name)
> > > -{
> > > -	const char *lstart, *ok_lookup_str, *nok_lookup_str,
> > > -	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
> > > -	char *test_name_end;
> > > -
> > > -	ok_lookup_str = "ok ";
> > > -	nok_lookup_str = "not ok ";
> > > -
> > > -	lstart = strchrnul(record, ';');
> > > -
> > > -	if (*lstart == '\0') {
> > > -		igt_warn("kmsg truncated: output malformed (%m)\n");
> > > -		return -2;
> > > -	}
> > > -
> > > -	lstart++;
> > > -	while (isspace(*lstart))
> > > -		lstart++;
> > > -
> > > -	nok_rptr = strstr(lstart, nok_lookup_str);
> > > -	if (nok_rptr != NULL) {
> > > -		nok_rptr += strlen(nok_lookup_str);
> > > -		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
> > > -			nok_rptr++;
> > > -		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
> > > -		while (!isspace(*test_name_end))
> > > -			test_name_end++;
> > > -		*test_name_end = '\0';
> > > -		if (log_to_end(IGT_LOG_WARN, fd, record,
> > > -			       "%s", lstart) < 0)
> > > -			return -2;
> > > -		return -1;
> > > -	}
> > > -
> > > -	comment_start = strchrnul(lstart, '#');
> > > -
> > > -	/* Check if we're still in a subtest */
> > > -	if (*comment_start != '\0') {
> > > -		comment_start++;
> > > -		value_parse_start = comment_start;
> > > -
> > > -		if (lookup_value(value_parse_start, "fail: ") > 0) {
> > > -			if (log_to_end(IGT_LOG_WARN, fd, record,
> > > -				       "%s", lstart) < 0)
> > > -				return -2;
> > > -			return -1;
> > > -		}
> > > -	}
> > > -
> > > -	ok_rptr = strstr(lstart, ok_lookup_str);
> > > -	if (ok_rptr != NULL) {
> > > -		ok_rptr += strlen(ok_lookup_str);
> > > -		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
> > > -			ok_rptr++;
> > > -		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
> > > -		while (!isspace(*test_name_end))
> > > -			test_name_end++;
> > > -		*test_name_end = '\0';
> > > -		return 0;
> > > -	}
> > > -
> > > -	return 1;
> > > -}
> > > -
> > > -/**
> > > - * parse_tap_level:
> > > - * @fd: file descriptor
> > > - * @base_test_name: test_name from upper recursion level
> > > - * @test_count: test_count of this level
> > > - * @failed_tests: top level failed_tests pointer
> > > - * @found_tests: top level found_tests pointer
> > > - * @is_builtin: whether the KUnit module is built-in or not
> > > - *
> > > - * Returns:
> > > - * 0 if succeded
> > > - * -1 if error occurred
> > > - */
> > > -__maybe_unused
> > > -static int parse_tap_level(int fd, char *base_test_name, int test_count, bool *failed_tests,
> > > -			   bool *found_tests, bool is_builtin)
> > > -{
> > > -	char record[BUF_LEN + 1];
> > > -	struct ktap_test_results_element *r;
> > > -	int internal_test_count;
> > > -	char test_name[BUF_LEN + 1];
> > > -	char base_test_name_for_next_level[BUF_LEN + 1];
> > > -
> > > -	for (int i = 0; i < test_count; i++) {
> > > -		if (read(fd, record, BUF_LEN) < 0) {
> > > -			if (errno == EPIPE)
> > > -				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -			else
> > > -				igt_warn("error reading kmsg (%m)\n");
> > > -
> > > -			return -1;
> > > -		}
> > > -
> > > -		/* Sublevel found */
> > > -		if (tap_version_present(record, false))
> > > -		{
> > > -			internal_test_count = find_next_tap_subtest(fd, record, test_name,
> > > -								    is_builtin);
> > > -			switch (internal_test_count) {
> > > -			case -2:
> > > -				/* No more data to read */
> > > -				return -1;
> > > -			case -1:
> > > -				/* No test found */
> > > -				return -1;
> > > -			case 0:
> > > -				/* Tests found, but they're missing info */
> > > -				*found_tests = true;
> > > -				return -1;
> > > -			default:
> > > -				*found_tests = true;
> > > -
> > > -				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
> > > -				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
> > > -				    base_test_name_for_next_level[0])
> > > -					strncat(base_test_name_for_next_level, DELIMITER,
> > > -						BUF_LEN - strlen(base_test_name_for_next_level));
> > > -				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
> > > -				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
> > > -
> > > -				if (parse_tap_level(fd, base_test_name_for_next_level,
> > > -						    internal_test_count, failed_tests, found_tests,
> > > -						    is_builtin) == -1)
> > > -					return -1;
> > > -				break;
> > > -			}
> > > -		}
> > > -
> > > -		switch (parse_kmsg_for_tap(fd, record, test_name)) {
> > > -		case -2:
> > > -			return -1;
> > > -		case -1:
> > > -			*failed_tests = true;
> > > -
> > > -			r = malloc(sizeof(*r));
> > > -
> > > -			memcpy(r->test_name, base_test_name, BUF_LEN);
> > > -			if (strlen(r->test_name) < BUF_LEN - 1)
> > > -				if (r->test_name[0])
> > > -					strncat(r->test_name, DELIMITER,
> > > -						BUF_LEN - strlen(r->test_name));
> > > -			memcpy(r->test_name + strlen(r->test_name), test_name,
> > > -			       BUF_LEN - strlen(r->test_name));
> > > -			r->test_name[BUF_LEN] = '\0';
> > > -
> > > -			r->passed = false;
> > > -
> > > -			pthread_mutex_lock(&results.mutex);
> > > -			igt_list_add_tail(&r->link, &results.list);
> > > -			pthread_mutex_unlock(&results.mutex);
> > > -
> > > -			test_name[0] = '\0';
> > > -			break;
> > > -		case 0:
> > > -			r = malloc(sizeof(*r));
> > > -
> > > -			memcpy(r->test_name, base_test_name, BUF_LEN);
> > > -			if (strlen(r->test_name) < BUF_LEN - 1)
> > > -				if (r->test_name[0])
> > > -					strncat(r->test_name, DELIMITER,
> > > -						BUF_LEN - strlen(r->test_name));
> > > -			memcpy(r->test_name + strlen(r->test_name), test_name,
> > > -			       BUF_LEN - strlen(r->test_name));
> > > -			r->test_name[BUF_LEN] = '\0';
> > > -
> > > -			r->passed = true;
> > > -
> > > -			pthread_mutex_lock(&results.mutex);
> > > -			igt_list_add_tail(&r->link, &results.list);
> > > -			pthread_mutex_unlock(&results.mutex);
> > > -
> > > -			test_name[0] = '\0';
> > > -			break;
> > > -		default:
> > > -			break;
> > > -		}
> > > -	}
> > > -	return 0;
> > > -}
> > > -
> > > -/**
> > > - * igt_ktap_parser:
> > > - *
> > > - * This function parses the output of a ktap script and passes it to main thread.
> > > - */
> > > -void *igt_ktap_parser(void *unused)
> > > -{
> > > -	char record[BUF_LEN + 1], *buf, *suite_name = NULL, *case_name = NULL;
> > > -	struct igt_ktap_results *ktap = NULL;
> > > -	int fd = ktap_args.fd;
> > > -	IGT_LIST_HEAD(list);
> > > -	int err;
> > > -
> > > -	ktap = igt_ktap_alloc(&list);
> > > -	if (igt_debug_on(!ktap))
> > > -		goto igt_ktap_parser_end;
> > > -
> > > -	while (err = read(fd, record, BUF_LEN), err > 0) {
> > > -		struct igt_ktap_result *r, *rn;
> > > -
> > > -		/* skip kmsg continuation lines */
> > > -		if (igt_debug_on(*record == ' '))
> > > -			continue;
> > > -
> > > -		/* NULL-terminate the record */
> > > -		record[err] = '\0';
> > > -
> > > -		/* detect start of log message, continue if not found */
> > > -		buf = strchrnul(record, ';');
> > > -		if (igt_debug_on(*buf == '\0'))
> > > -			continue;
> > > -		buf++;
> > > -
> > > -		err = igt_ktap_parse(buf, ktap);
> > > -
> > > -		/* parsing error */
> > > -		if (err && err != -EINPROGRESS)
> > > -			goto igt_ktap_parser_end;
> > > -
> > > -		igt_list_for_each_entry_safe(r, rn, &list, link) {
> > > -			struct ktap_test_results_element *result = NULL;
> > > -			int code = r->code;
> > > -
> > > -			if (code != IGT_EXIT_INVALID)
> > > -				result = calloc(1, sizeof(*result));
> > > -
> > > -			if (result) {
> > > -				snprintf(result->test_name, sizeof(result->test_name),
> > > -					 "%s-%s", r->suite_name, r->case_name);
> > > -
> > > -				if (code == IGT_EXIT_SUCCESS)
> > > -					result->passed = true;
> > > -			}
> > > -
> > > -			igt_list_del(&r->link);
> > > -			if (r->suite_name != suite_name) {
> > > -				free(suite_name);
> > > -				suite_name = r->suite_name;
> > > -			}
> > > -			if (r->case_name != case_name) {
> > > -				free(case_name);
> > > -				case_name = r->case_name;
> > > -			}
> > > -			free(r->msg);
> > > -			free(r);
> > > -
> > > -			/*
> > > -			 * no extra result record expected on start
> > > -			 * of parametrized test case -- skip it
> > > -			 */
> > > -			if (code == IGT_EXIT_INVALID)
> > > -				continue;
> > > -
> > > -			if (!result) {
> > > -				err = -ENOMEM;
> > > -				goto igt_ktap_parser_end;
> > > -			}
> > > -
> > > -			pthread_mutex_lock(&results.mutex);
> > > -			igt_list_add_tail(&result->link, &results.list);
> > > -			pthread_mutex_unlock(&results.mutex);
> > > -		}
> > > -
> > > -		/* end of KTAP report */
> > > -		if (!err)
> > > -			goto igt_ktap_parser_end;
> > > -	}
> > > -
> > > -	if (err < 0) {
> > > -		if (errno == EPIPE)
> > > -			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> > > -		else
> > > -			igt_warn("error reading kmsg (%m)\n");
> > > -	}
> > > -
> > > -igt_ktap_parser_end:
> > > -	free(suite_name);
> > > -	free(case_name);
> > > -
> > > -	if (!err)
> > > -		ktap_args.ret = IGT_EXIT_SUCCESS;
> > > -
> > > -	results.still_running = false;
> > > -
> > > -	if (ktap)
> > > -		igt_ktap_free(ktap);
> > > -
> > > -	return NULL;
> > > -}
> > > -
> > > -static pthread_t ktap_parser_thread;
> > > -
> > > -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin)
> > > -{
> > > -	IGT_INIT_LIST_HEAD(&results.list);
> > > -	pthread_mutex_init(&results.mutex, NULL);
> > > -	results.still_running = true;
> > > -
> > > -	ktap_args.fd = fd;
> > > -	ktap_args.is_builtin = is_builtin;
> > > -	ktap_args.ret = IGT_EXIT_FAILURE;
> > > -	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
> > > -
> > > -	return &results;
> > > -}
> > > -
> > > -void ktap_parser_cancel(void)
> > > -{
> > > -	pthread_cancel(ktap_parser_thread);
> > > -}
> > > -
> > > -int ktap_parser_stop(void)
> > > -{
> > > -	pthread_join(ktap_parser_thread, NULL);
> > > -	return ktap_args.ret;
> > > -}
> > > diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> > > index 6f8da3eab6..c422636bfc 100644
> > > --- a/lib/igt_ktap.h
> > > +++ b/lib/igt_ktap.h
> > > @@ -27,8 +27,6 @@
> > >  
> > >  #define BUF_LEN 4096
> > >  
> > > -#include <pthread.h>
> > > -
> > >  #include "igt_list.h"
> > >  
> > >  struct igt_ktap_result {
> > > @@ -45,24 +43,4 @@ struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results);
> > >  int igt_ktap_parse(const char *buf, struct igt_ktap_results *ktap);
> > >  void igt_ktap_free(struct igt_ktap_results *ktap);
> > >  
> > > -void *igt_ktap_parser(void *unused);
> > > -
> > > -typedef struct ktap_test_results_element {
> > > -	char test_name[BUF_LEN + 1];
> > > -	bool passed;
> > > -	struct igt_list_head link;
> > > -} ktap_test_results_element;
> > > -
> > > -struct ktap_test_results {
> > > -	struct igt_list_head list;
> > > -	pthread_mutex_t mutex;
> > > -	bool still_running;
> > > -};
> > > -
> > > -
> > > -
> > > -struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin);
> > > -void ktap_parser_cancel(void);
> > > -int ktap_parser_stop(void);
> > > -
> > >  #endif /* IGT_KTAP_H */
> > 
> 
> 
> 
>

Patch

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 426ae5b26f..7bca4cdaab 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -1,5 +1,5 @@ 
 /*
- * Copyright © 2016 Intel Corporation
+ * Copyright © 2016-2023 Intel Corporation
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -26,7 +26,12 @@ 
 #include <errno.h>
 #include <fcntl.h>
 #include <pthread.h>
+#include <stdlib.h>
+#include <string.h>
 #include <sys/utsname.h>
+#include <unistd.h>
+
+#include "assembler/brw_compat.h"	/* [un]likely() */
 
 #include "igt_aux.h"
 #include "igt_core.h"
@@ -748,9 +753,12 @@  void igt_kselftest_get_tests(struct kmod_module *kmod,
 }
 
 struct modprobe_data {
+	pthread_t parent;
 	struct kmod_module *kmod;
 	const char *opts;
 	int err;
+	pthread_mutex_t lock;
+	pthread_t thread;
 };
 
 static void *modprobe_task(void *arg)
@@ -759,16 +767,132 @@  static void *modprobe_task(void *arg)
 
 	data->err = modprobe(data->kmod, data->opts);
 
+	if (igt_debug_on(data->err)) {
+		int err;
+
+		while (err = pthread_mutex_trylock(&data->lock),
+		       err && !igt_debug_on(err != EBUSY))
+			igt_debug_on(pthread_kill(data->parent, SIGCHLD));
+	} else {
+		/* let main thread use mutex to detect modprobe completion */
+		igt_debug_on(pthread_mutex_lock(&data->lock));
+	}
+
 	return NULL;
 }
 
+static void kunit_sigchld_handler(int signal)
+{
+	return;
+}
+
+static int kunit_kmsg_result_get(struct igt_list_head *results,
+				 struct modprobe_data *modprobe,
+				 int fd, struct igt_ktap_results *ktap)
+{
+	struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
+			 *saved;
+	char record[BUF_LEN + 1], *buf;
+	unsigned long taints;
+	int ret;
+
+	do {
+		int err;
+
+		if (igt_debug_on(igt_kernel_tainted(&taints)))
+			return -ENOTRECOVERABLE;
+
+		err = igt_debug_on(sigaction(SIGCHLD, &sigchld, saved));
+		if (err == -1)
+			return -errno;
+		else if (unlikely(err))
+			return err;
+
+		err = pthread_mutex_lock(&modprobe->lock);
+		switch (err) {
+		case EOWNERDEAD:
+			/* leave the mutex unrecoverable */
+			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
+			__attribute__ ((fallthrough));
+		case ENOTRECOVERABLE:
+			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
+			if (igt_debug_on(modprobe->err))
+				return modprobe->err;
+			break;
+		case 0:
+			break;
+		default:
+			igt_debug("pthread_mutex_lock() error: %d\n", err);
+			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
+			return -err;
+		}
+
+		ret = read(fd, record, BUF_LEN);
+
+		if (!err) {
+			igt_debug_on(pthread_mutex_unlock(&modprobe->lock));
+			igt_debug_on(sigaction(SIGCHLD, saved, NULL));
+		}
+
+		if (igt_debug_on(!ret))
+			return -ENODATA;
+		if (igt_debug_on(ret == -1))
+			return -errno;
+		if (unlikely(igt_debug_on(ret < 0)))
+			break;
+
+		/* skip kmsg continuation lines */
+		if (igt_debug_on(*record == ' '))
+			continue;
+
+		/* NULL-terminate the record */
+		record[ret] = '\0';
+
+		/* detect start of log message, continue if not found */
+		buf = strchrnul(record, ';');
+		if (igt_debug_on(*buf == '\0'))
+			continue;
+		buf++;
+
+		ret = igt_ktap_parse(buf, ktap);
+		if (!ret || igt_debug_on(ret != -EINPROGRESS))
+			break;
+	} while (igt_list_empty(results));
+
+	return ret;
+}
+
+static void kunit_result_free(struct igt_ktap_result **r,
+			      char **suite_name, char **case_name)
+{
+	if (!*r)
+		return;
+
+	igt_list_del(&(*r)->link);
+
+	if ((*r)->suite_name != *suite_name) {
+		free(*suite_name);
+		*suite_name = (*r)->suite_name;
+	}
+
+	if ((*r)->case_name != *case_name) {
+		free(*case_name);
+		*case_name = (*r)->case_name;
+	}
+
+	free((*r)->msg);
+	free(*r);
+	*r = NULL;
+}
+
 static void __igt_kunit(struct igt_ktest *tst, const char *opts)
 {
-	struct modprobe_data modprobe = { tst->kmod, opts, 0, };
-	struct kmod_module *kunit_kmod;
-	bool is_builtin;
-	struct ktap_test_results *results;
-	pthread_t modprobe_thread;
+	struct modprobe_data modprobe = { pthread_self(), tst->kmod, opts, 0, };
+	char *suite_name = NULL, *case_name = NULL;
+	struct igt_ktap_result *r, *rn;
+	struct igt_ktap_results *ktap;
+	pthread_mutexattr_t attr;
+	IGT_LIST_HEAD(results);
 	unsigned long taints;
 	int flags, ret;
 
@@ -780,60 +904,119 @@  static void __igt_kunit(struct igt_ktest *tst, const char *opts)
 
 	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
 
-	igt_skip_on(kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod));
-	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
-	kmod_module_unref(kunit_kmod);
+	igt_skip_on(pthread_mutexattr_init(&attr));
+	igt_skip_on(pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST));
+	igt_skip_on(pthread_mutex_init(&modprobe.lock, &attr));
 
-	results = ktap_parser_start(tst->kmsg, is_builtin);
+	ktap = igt_ktap_alloc(&results);
+	igt_require(ktap);
 
-	if (igt_debug_on(pthread_create(&modprobe_thread, NULL,
+	if (igt_debug_on(pthread_create(&modprobe.thread, NULL,
 					modprobe_task, &modprobe))) {
-		ktap_parser_cancel();
-		igt_ignore_warn(ktap_parser_stop());
+		igt_ktap_free(ktap);
 		igt_skip("Failed to create a modprobe thread\n");
 	}
 
-	while (READ_ONCE(results->still_running) || !igt_list_empty(&results->list))
-	{
-		struct ktap_test_results_element *result;
-
-		if (!pthread_tryjoin_np(modprobe_thread, NULL) && modprobe.err) {
-			ktap_parser_cancel();
+	do {
+		ret = kunit_kmsg_result_get(&results, &modprobe,
+					    tst->kmsg, ktap);
+		if (igt_debug_on(ret && ret != -EINPROGRESS))
 			break;
-		}
 
-		if (igt_kernel_tainted(&taints)) {
-			ktap_parser_cancel();
-			pthread_cancel(modprobe_thread);
+		if (igt_debug_on(igt_list_empty(&results)))
 			break;
-		}
 
-		pthread_mutex_lock(&results->mutex);
-		if (igt_list_empty(&results->list)) {
-			pthread_mutex_unlock(&results->mutex);
-			continue;
-		}
+		r = igt_list_first_entry(&results, r, link);
 
-		result = igt_list_first_entry(&results->list, result, link);
+		igt_dynamic_f("%s-%s", r->suite_name, r->case_name) {
+			if (r->code == IGT_EXIT_INVALID) {
+				/* parametrized test case, get actual result */
+				kunit_result_free(&r, &suite_name, &case_name);
 
-		igt_list_del(&result->link);
-		pthread_mutex_unlock(&results->mutex);
+				igt_assert(igt_list_empty(&results));
 
-		igt_dynamic(result->test_name) {
-			igt_assert(READ_ONCE(result->passed));
+				ret = kunit_kmsg_result_get(&results, &modprobe,
+							    tst->kmsg, ktap);
+				if (ret != -EINPROGRESS)
+					igt_fail_on(ret);
+
+				igt_fail_on(igt_list_empty(&results));
+
+				r = igt_list_first_entry(&results, r, link);
+
+				igt_fail_on_f(strcmp(r->suite_name, suite_name),
+					      "suite_name expected: %s, got: %s\n",
+					      suite_name, r->suite_name);
+				igt_fail_on_f(strcmp(r->case_name, case_name),
+					      "case_name expected: %s, got: %s\n",
+					      case_name, r->case_name);
+			}
 
-			if (!pthread_tryjoin_np(modprobe_thread, NULL))
+			igt_assert_neq(r->code, IGT_EXIT_INVALID);
+
+			if (r->msg && *r->msg) {
+				igt_skip_on_f(r->code == IGT_EXIT_SKIP,
+					      "%s\n", r->msg);
+				igt_fail_on_f(r->code == IGT_EXIT_FAILURE,
+					      "%s\n", r->msg);
+				igt_abort_on_f(r->code == IGT_EXIT_ABORT,
+					      "%s\n", r->msg);
+			} else {
+				igt_skip_on(r->code == IGT_EXIT_SKIP);
+				igt_fail_on(r->code == IGT_EXIT_FAILURE);
+				if (r->code == IGT_EXIT_ABORT)
+					igt_fail(r->code);
+			}
+			igt_assert_eq(r->code, IGT_EXIT_SUCCESS);
+
+			switch (pthread_mutex_lock(&modprobe.lock)) {
+			case 0:
+				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
+				break;
+			case EOWNERDEAD:
+				/* leave the mutex unrecoverable */
+				igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
+				__attribute__ ((fallthrough));
+			case ENOTRECOVERABLE:
 				igt_assert_eq(modprobe.err, 0);
+				break;
+			default:
+				igt_debug("pthread_mutex_lock() failed\n");
+				break;
+			}
 
 			igt_assert_eq(igt_kernel_tainted(&taints), 0);
 		}
 
-		free(result);
+		kunit_result_free(&r, &suite_name, &case_name);
+
+	} while (ret == -EINPROGRESS);
+
+	igt_list_for_each_entry_safe(r, rn, &results, link)
+		kunit_result_free(&r, &suite_name, &case_name);
+
+	free(case_name);
+	free(suite_name);
+
+	switch (pthread_mutex_lock(&modprobe.lock)) {
+	case 0:
+		igt_debug_on(pthread_cancel(modprobe.thread));
+		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
+		igt_debug_on(pthread_join(modprobe.thread, NULL));
+		break;
+	case EOWNERDEAD:
+		/* leave the mutex unrecoverable */
+		igt_debug_on(pthread_mutex_unlock(&modprobe.lock));
+		break;
+	case ENOTRECOVERABLE:
+		break;
+	default:
+		igt_debug("pthread_mutex_lock() failed\n");
+		igt_debug_on(pthread_join(modprobe.thread, NULL));
+		break;
 	}
 
-	pthread_join(modprobe_thread, NULL);
-
-	ret = ktap_parser_stop();
+	igt_ktap_free(ktap);
 
 	igt_skip_on(modprobe.err);
 	igt_skip_on(igt_kernel_tainted(&taints));
diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
index 5eac102417..53a6c63288 100644
--- a/lib/igt_ktap.c
+++ b/lib/igt_ktap.c
@@ -4,17 +4,11 @@ 
  * Copyright © 2023 Intel Corporation
  */
 
-#include <ctype.h>
-#include <limits.h>
-#include <libkmod.h>
-#include <pthread.h>
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <unistd.h>
 
-#include "igt_aux.h"
 #include "igt_core.h"
 #include "igt_ktap.h"
 #include "igt_list.h"
@@ -312,565 +306,3 @@  void igt_ktap_free(struct igt_ktap_results *ktap)
 {
 	free(ktap);
 }
-
-#define DELIMITER "-"
-
-struct ktap_parser_args {
-	int fd;
-	bool is_builtin;
-	int ret;
-} ktap_args;
-
-static struct ktap_test_results results;
-
-static int log_to_end(enum igt_log_level level, int fd,
-		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
-
-/**
- * log_to_end:
- * @level: #igt_log_level
- * @record: record to store the read data
- * @format: format string
- * @...: optional arguments used in the format string
- *
- * This is an altered version of the generic structured logging helper function
- * igt_log capable of reading to the end of a given line.
- *
- * Returns: 0 for success, or -2 if there's an error reading from the file
- */
-static int log_to_end(enum igt_log_level level, int fd,
-		      char *record, const char *format, ...)
-{
-	va_list args;
-	const char *lend;
-
-	/* Cutoff after newline character, in order to not display garbage */
-	char *cutoff = strchr(record, '\n');
-	if (cutoff) {
-		if (cutoff - record < BUF_LEN)
-			cutoff[1] = '\0';
-	}
-
-	va_start(args, format);
-	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
-	va_end(args);
-
-	lend = strchrnul(record, '\n');
-	while (*lend == '\0') {
-		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
-
-		if (read(fd, record, BUF_LEN) < 0) {
-			if (errno == EPIPE)
-				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
-			else
-				igt_warn("an error occurred while reading kmsg: %m\n");
-
-			return -2;
-		}
-
-		lend = strchrnul(record, '\n');
-	}
-	return 0;
-}
-
-/**
- * lookup_value:
- * @haystack: the string to search in
- * @needle: the string to search for
- *
- * Returns: the value of the needle in the haystack, or -1 if not found.
- */
-static long lookup_value(const char *haystack, const char *needle)
-{
-	const char *needle_rptr;
-	char *needle_end;
-	long num;
-
-	needle_rptr = strcasestr(haystack, needle);
-
-	if (needle_rptr == NULL)
-		return -1;
-
-	/* Skip search string and whitespaces after it */
-	needle_rptr += strlen(needle);
-
-	num = strtol(needle_rptr, &needle_end, 10);
-
-	if (needle_rptr == needle_end)
-		return -1;
-
-	if (num == LONG_MIN || num == LONG_MAX)
-		return 0;
-
-	return num > 0 ? num : 0;
-}
-
-/**
- * tap_version_present:
- * @record: buffer with tap data
- * @print_info: whether tap version should be printed or not
- *
- * Returns:
- * 0 if not found
- * 1 if found
- */
-static int tap_version_present(char* record, bool print_info)
-{
-	/*
-	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
-	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
-	 *
-	 * but actually isn't, as it currently depends on the KUnit module
-	 * being built-in, so we can't rely on it every time
-	 */
-	const char *version_rptr = strcasestr(record, "TAP version ");
-	char *cutoff;
-
-	if (version_rptr == NULL)
-		return 0;
-
-	/* Cutoff after newline character, in order to not display garbage */
-	cutoff = strchr(version_rptr, '\n');
-	if (cutoff)
-		cutoff[0] = '\0';
-
-	if (print_info)
-		igt_info("%s\n", version_rptr);
-
-	return 1;
-}
-
-/**
- * find_next_tap_subtest:
- * @fd: file descriptor
- * @record: buffer used to read fd
- * @is_builtin: whether KUnit is built-in or not
- *
- * Returns:
- * 0 if there's missing information
- * -1 if not found
- * -2 if there are problems while reading the file.
- * any other value corresponds to the amount of cases of the next (sub)test
- */
-static int find_next_tap_subtest(int fd, char *record, char *test_name, bool is_builtin)
-{
-	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
-	long test_count;
-	char *cutoff;
-
-	test_name[0] = '\0';
-	test_name[BUF_LEN] = '\0';
-
-	test_lookup_str = " subtest: ";
-	subtest_lookup_str = " test: ";
-
-	if (!tap_version_present(record, true))
-		return -1;
-
-	if (is_builtin) {
-		if (read(fd, record, BUF_LEN) < 0) {
-			if (errno == EPIPE)
-				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
-			else
-				igt_warn("an error occurred while reading kmsg: %m\n");
-
-			return -2;
-		}
-	}
-
-	name_rptr = strcasestr(record, test_lookup_str);
-	if (name_rptr != NULL) {
-		name_rptr += strlen(test_lookup_str);
-	} else {
-		name_rptr = strcasestr(record, subtest_lookup_str);
-		if (name_rptr != NULL)
-			name_rptr += strlen(subtest_lookup_str);
-	}
-
-	if (name_rptr == NULL) {
-		if (!is_builtin)
-			/* We've probably found nothing */
-			return -1;
-		igt_info("Missing test name\n");
-	} else {
-		strncpy(test_name, name_rptr, BUF_LEN);
-		/* Cutoff after newline character, in order to not display garbage */
-		cutoff = strchr(test_name, '\n');
-		if (cutoff)
-			cutoff[0] = '\0';
-
-		if (read(fd, record, BUF_LEN) < 0) {
-			if (errno == EPIPE)
-				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
-			else
-				igt_warn("unknown error reading kmsg (%m)\n");
-
-			return -2;
-		}
-
-		/* Now we can be sure we found tests */
-		if (!is_builtin)
-			igt_info("KUnit is not built-in, skipping version check...\n");
-	}
-
-	/*
-	 * Total test count will almost always appear as 0..N at the beginning
-	 * of a run, so we use it to reliably identify a new run
-	 */
-	test_count = lookup_value(record, "..");
-
-	if (test_count <= 0) {
-		igt_info("Missing test count\n");
-		if (test_name[0] == '\0')
-			return 0;
-		if (log_to_end(IGT_LOG_INFO, fd, record,
-				"Running some tests in: %s\n",
-				test_name) < 0)
-			return -2;
-		return 0;
-	} else if (test_name[0] == '\0') {
-		igt_info("Running %ld tests...\n", test_count);
-		return 0;
-	}
-
-	if (log_to_end(IGT_LOG_INFO, fd, record,
-			"Executing %ld tests in: %s\n",
-			test_count, test_name) < 0)
-		return -2;
-
-	return test_count;
-}
-
-/**
- * parse_kmsg_for_tap:
- * @fd: file descriptor
- * @record: buffer used to read fd
- * @test_name: buffer to store the test name
- *
- * Returns:
- * 1 if no results were found
- * 0 if a test succeded
- * -1 if a test failed
- * -2 if there are problems reading the file
- */
-static int parse_kmsg_for_tap(int fd, char *record, char *test_name)
-{
-	const char *lstart, *ok_lookup_str, *nok_lookup_str,
-	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
-	char *test_name_end;
-
-	ok_lookup_str = "ok ";
-	nok_lookup_str = "not ok ";
-
-	lstart = strchrnul(record, ';');
-
-	if (*lstart == '\0') {
-		igt_warn("kmsg truncated: output malformed (%m)\n");
-		return -2;
-	}
-
-	lstart++;
-	while (isspace(*lstart))
-		lstart++;
-
-	nok_rptr = strstr(lstart, nok_lookup_str);
-	if (nok_rptr != NULL) {
-		nok_rptr += strlen(nok_lookup_str);
-		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
-			nok_rptr++;
-		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
-		while (!isspace(*test_name_end))
-			test_name_end++;
-		*test_name_end = '\0';
-		if (log_to_end(IGT_LOG_WARN, fd, record,
-			       "%s", lstart) < 0)
-			return -2;
-		return -1;
-	}
-
-	comment_start = strchrnul(lstart, '#');
-
-	/* Check if we're still in a subtest */
-	if (*comment_start != '\0') {
-		comment_start++;
-		value_parse_start = comment_start;
-
-		if (lookup_value(value_parse_start, "fail: ") > 0) {
-			if (log_to_end(IGT_LOG_WARN, fd, record,
-				       "%s", lstart) < 0)
-				return -2;
-			return -1;
-		}
-	}
-
-	ok_rptr = strstr(lstart, ok_lookup_str);
-	if (ok_rptr != NULL) {
-		ok_rptr += strlen(ok_lookup_str);
-		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
-			ok_rptr++;
-		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
-		while (!isspace(*test_name_end))
-			test_name_end++;
-		*test_name_end = '\0';
-		return 0;
-	}
-
-	return 1;
-}
-
-/**
- * parse_tap_level:
- * @fd: file descriptor
- * @base_test_name: test_name from upper recursion level
- * @test_count: test_count of this level
- * @failed_tests: top level failed_tests pointer
- * @found_tests: top level found_tests pointer
- * @is_builtin: whether the KUnit module is built-in or not
- *
- * Returns:
- * 0 if succeded
- * -1 if error occurred
- */
-__maybe_unused
-static int parse_tap_level(int fd, char *base_test_name, int test_count, bool *failed_tests,
-			   bool *found_tests, bool is_builtin)
-{
-	char record[BUF_LEN + 1];
-	struct ktap_test_results_element *r;
-	int internal_test_count;
-	char test_name[BUF_LEN + 1];
-	char base_test_name_for_next_level[BUF_LEN + 1];
-
-	for (int i = 0; i < test_count; i++) {
-		if (read(fd, record, BUF_LEN) < 0) {
-			if (errno == EPIPE)
-				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
-			else
-				igt_warn("error reading kmsg (%m)\n");
-
-			return -1;
-		}
-
-		/* Sublevel found */
-		if (tap_version_present(record, false))
-		{
-			internal_test_count = find_next_tap_subtest(fd, record, test_name,
-								    is_builtin);
-			switch (internal_test_count) {
-			case -2:
-				/* No more data to read */
-				return -1;
-			case -1:
-				/* No test found */
-				return -1;
-			case 0:
-				/* Tests found, but they're missing info */
-				*found_tests = true;
-				return -1;
-			default:
-				*found_tests = true;
-
-				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
-				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
-				    base_test_name_for_next_level[0])
-					strncat(base_test_name_for_next_level, DELIMITER,
-						BUF_LEN - strlen(base_test_name_for_next_level));
-				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
-				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
-
-				if (parse_tap_level(fd, base_test_name_for_next_level,
-						    internal_test_count, failed_tests, found_tests,
-						    is_builtin) == -1)
-					return -1;
-				break;
-			}
-		}
-
-		switch (parse_kmsg_for_tap(fd, record, test_name)) {
-		case -2:
-			return -1;
-		case -1:
-			*failed_tests = true;
-
-			r = malloc(sizeof(*r));
-
-			memcpy(r->test_name, base_test_name, BUF_LEN);
-			if (strlen(r->test_name) < BUF_LEN - 1)
-				if (r->test_name[0])
-					strncat(r->test_name, DELIMITER,
-						BUF_LEN - strlen(r->test_name));
-			memcpy(r->test_name + strlen(r->test_name), test_name,
-			       BUF_LEN - strlen(r->test_name));
-			r->test_name[BUF_LEN] = '\0';
-
-			r->passed = false;
-
-			pthread_mutex_lock(&results.mutex);
-			igt_list_add_tail(&r->link, &results.list);
-			pthread_mutex_unlock(&results.mutex);
-
-			test_name[0] = '\0';
-			break;
-		case 0:
-			r = malloc(sizeof(*r));
-
-			memcpy(r->test_name, base_test_name, BUF_LEN);
-			if (strlen(r->test_name) < BUF_LEN - 1)
-				if (r->test_name[0])
-					strncat(r->test_name, DELIMITER,
-						BUF_LEN - strlen(r->test_name));
-			memcpy(r->test_name + strlen(r->test_name), test_name,
-			       BUF_LEN - strlen(r->test_name));
-			r->test_name[BUF_LEN] = '\0';
-
-			r->passed = true;
-
-			pthread_mutex_lock(&results.mutex);
-			igt_list_add_tail(&r->link, &results.list);
-			pthread_mutex_unlock(&results.mutex);
-
-			test_name[0] = '\0';
-			break;
-		default:
-			break;
-		}
-	}
-	return 0;
-}
-
-/**
- * igt_ktap_parser:
- *
- * This function parses the output of a ktap script and passes it to main thread.
- */
-void *igt_ktap_parser(void *unused)
-{
-	char record[BUF_LEN + 1], *buf, *suite_name = NULL, *case_name = NULL;
-	struct igt_ktap_results *ktap = NULL;
-	int fd = ktap_args.fd;
-	IGT_LIST_HEAD(list);
-	int err;
-
-	ktap = igt_ktap_alloc(&list);
-	if (igt_debug_on(!ktap))
-		goto igt_ktap_parser_end;
-
-	while (err = read(fd, record, BUF_LEN), err > 0) {
-		struct igt_ktap_result *r, *rn;
-
-		/* skip kmsg continuation lines */
-		if (igt_debug_on(*record == ' '))
-			continue;
-
-		/* NULL-terminate the record */
-		record[err] = '\0';
-
-		/* detect start of log message, continue if not found */
-		buf = strchrnul(record, ';');
-		if (igt_debug_on(*buf == '\0'))
-			continue;
-		buf++;
-
-		err = igt_ktap_parse(buf, ktap);
-
-		/* parsing error */
-		if (err && err != -EINPROGRESS)
-			goto igt_ktap_parser_end;
-
-		igt_list_for_each_entry_safe(r, rn, &list, link) {
-			struct ktap_test_results_element *result = NULL;
-			int code = r->code;
-
-			if (code != IGT_EXIT_INVALID)
-				result = calloc(1, sizeof(*result));
-
-			if (result) {
-				snprintf(result->test_name, sizeof(result->test_name),
-					 "%s-%s", r->suite_name, r->case_name);
-
-				if (code == IGT_EXIT_SUCCESS)
-					result->passed = true;
-			}
-
-			igt_list_del(&r->link);
-			if (r->suite_name != suite_name) {
-				free(suite_name);
-				suite_name = r->suite_name;
-			}
-			if (r->case_name != case_name) {
-				free(case_name);
-				case_name = r->case_name;
-			}
-			free(r->msg);
-			free(r);
-
-			/*
-			 * no extra result record expected on start
-			 * of parametrized test case -- skip it
-			 */
-			if (code == IGT_EXIT_INVALID)
-				continue;
-
-			if (!result) {
-				err = -ENOMEM;
-				goto igt_ktap_parser_end;
-			}
-
-			pthread_mutex_lock(&results.mutex);
-			igt_list_add_tail(&result->link, &results.list);
-			pthread_mutex_unlock(&results.mutex);
-		}
-
-		/* end of KTAP report */
-		if (!err)
-			goto igt_ktap_parser_end;
-	}
-
-	if (err < 0) {
-		if (errno == EPIPE)
-			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
-		else
-			igt_warn("error reading kmsg (%m)\n");
-	}
-
-igt_ktap_parser_end:
-	free(suite_name);
-	free(case_name);
-
-	if (!err)
-		ktap_args.ret = IGT_EXIT_SUCCESS;
-
-	results.still_running = false;
-
-	if (ktap)
-		igt_ktap_free(ktap);
-
-	return NULL;
-}
-
-static pthread_t ktap_parser_thread;
-
-struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin)
-{
-	IGT_INIT_LIST_HEAD(&results.list);
-	pthread_mutex_init(&results.mutex, NULL);
-	results.still_running = true;
-
-	ktap_args.fd = fd;
-	ktap_args.is_builtin = is_builtin;
-	ktap_args.ret = IGT_EXIT_FAILURE;
-	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
-
-	return &results;
-}
-
-void ktap_parser_cancel(void)
-{
-	pthread_cancel(ktap_parser_thread);
-}
-
-int ktap_parser_stop(void)
-{
-	pthread_join(ktap_parser_thread, NULL);
-	return ktap_args.ret;
-}
diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
index 6f8da3eab6..c422636bfc 100644
--- a/lib/igt_ktap.h
+++ b/lib/igt_ktap.h
@@ -27,8 +27,6 @@ 
 
 #define BUF_LEN 4096
 
-#include <pthread.h>
-
 #include "igt_list.h"
 
 struct igt_ktap_result {
@@ -45,24 +43,4 @@  struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results);
 int igt_ktap_parse(const char *buf, struct igt_ktap_results *ktap);
 void igt_ktap_free(struct igt_ktap_results *ktap);
 
-void *igt_ktap_parser(void *unused);
-
-typedef struct ktap_test_results_element {
-	char test_name[BUF_LEN + 1];
-	bool passed;
-	struct igt_list_head link;
-} ktap_test_results_element;
-
-struct ktap_test_results {
-	struct igt_list_head list;
-	pthread_mutex_t mutex;
-	bool still_running;
-};
-
-
-
-struct ktap_test_results *ktap_parser_start(int fd, bool is_builtin);
-void ktap_parser_cancel(void);
-int ktap_parser_stop(void);
-
 #endif /* IGT_KTAP_H */