Re: [PATCH i-g-t v2 02/11] lib/xe: Make debugfs_file_has_wa() aware of the debugfs file layout

[Matt Atwood <matthew.s.atwood@intel.com>]

On Wed, Sep 23, 2026 at 06:41:09PM -0300, Gustavo Sousa wrote:
> Currently debugfs_file_has_wa() uses strstr() to check if a workaround
> name is present in the debugfs dump.  That's a bit fragile because
> that solution isn't aware of the layout of a workaround debugfs dump.
> The strstr() call could match a substring of a workaround or even a
> "section line" of the file.
> 
> Make workaround matching more robust by adding a proper parser for the
> debugfs dump.
> 
> While at it, let's make sure that we have a lib test to validate the
> new logic.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
Reviewed-by: Matt Atwood <matthew.s.atwood@intel.com>
> ---
>  lib/tests/meson.build |  1 +
>  lib/tests/xe_wa.c     | 42 +++++++++++++++++++++++++++++++
>  lib/tests/xe_wa.h     | 12 +++++++++
>  lib/xe/xe_wa.c        | 69 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 121 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/tests/meson.build b/lib/tests/meson.build
> index f1db66d07c28..aa8cb37c2252 100644
> --- a/lib/tests/meson.build
> +++ b/lib/tests/meson.build
> @@ -28,6 +28,7 @@ lib_tests = [
>  	'igt_thread',
>  	'igt_types',
>  	'i915_perf_data_alignment',
> +	'xe_wa',
>  ]
>  
>  lib_fail_tests = [
> diff --git a/lib/tests/xe_wa.c b/lib/tests/xe_wa.c
> new file mode 100644
> index 000000000000..d03d3f4e2ca0
> --- /dev/null
> +++ b/lib/tests/xe_wa.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2026 Intel Corporation. All rights reserved.
> + */
> +
> +#include "igt_core.h"
> +
> +#include "tests/xe_wa.h"
> +
> +static char fake_dump[] =
> +	"Some workarounds\n"
> +	"	wa_a\n"
> +	"	wa_b\n"
> +	"Another set of workarounds\n"
> +	"	wa_c\n"
> +	"	wa_d\n"
> +	"Yet another, but empty section\n"
> +	"A final section\n"
> +	"	wa_x, wa_y, wa_z\n"
> +	"	wa_abcd\n";
> +
> +int igt_simple_main()
> +{
> +	/* Match workaround that is the single occurrence in the line. */
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "wa_c"), 1);
> +
> +	/* Match workarounds that are listed with others in the same line. */
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "wa_x"), 1);
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "wa_y"), 1);
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "wa_z"), 1);
> +
> +	/* Do not match non-existing workarounds. */
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "non-existing"), 0);
> +
> +	/* Match full workaround name and not substrings. */
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "wa_abc"), 0);
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "wa_abcd"), 1);
> +
> +	/* Don't match section titles. */
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "Some workarounds"), 0);
> +	igt_assert_eq(xe_wa_test_debugfs_dump_has_wa(fake_dump, "Another set of workarounds"), 0);
> +}
> diff --git a/lib/tests/xe_wa.h b/lib/tests/xe_wa.h
> new file mode 100644
> index 000000000000..3ca2a553fe68
> --- /dev/null
> +++ b/lib/tests/xe_wa.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef LIB_TESTS_XE_WA_H
> +#define LIB_TESTS_XE_WA_H
> +
> +int xe_wa_test_debugfs_dump_has_wa(char *dump, const char *wa);
> +
> +#endif /* LIB_TESTS_XE_WA_H */
> +
> diff --git a/lib/xe/xe_wa.c b/lib/xe/xe_wa.c
> index 7b8e54b2ad27..c0c97e244564 100644
> --- a/lib/xe/xe_wa.c
> +++ b/lib/xe/xe_wa.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <fcntl.h>
> +#include <regex.h>
>  #include <stdint.h>
>  #include <stdio.h>
>  
> @@ -13,6 +14,68 @@
>  #include "xe/xe_query.h"
>  #include "xe/xe_wa.h"
>  
> +#include "tests/xe_wa.h"
> +
> +static int debugfs_dump_has_wa(char *dump, const char *wa)
> +{
> +	size_t wa_len;
> +	regex_t regex;
> +	regmatch_t match[2];
> +	int ret;
> +
> +	if (regcomp(&regex, "[ \t,]*([^ \t,]+)", REG_EXTENDED | REG_NEWLINE)) {
> +		igt_critical("Failed to compile regex, workaround %s will not be checked.\n",
> +			     wa);
> +
> +		return -1;
> +	}
> +
> +	wa_len = strlen(wa);
> +	ret = 0;
> +
> +	while (*dump) {
> +		/*
> +		 * Each workaround name is indented by one tab
> +		 * character; unindented lines are used as "section
> +		 * names" identifying the type of workarounds that
> +		 * follow (e.g. "GT Workarounds", "Engine Workarounds"
> +		 * etc) and should be ignored.
> +		 */
> +		if (*dump != '\t')
> +			goto next_line;
> +
> +		while (*dump && regexec(&regex, dump, 2, match, 0) == 0) {
> +			char *other_wa = dump + match[1].rm_so;
> +			size_t other_wa_len = match[1].rm_eo - match[1].rm_so;
> +
> +			dump += match[0].rm_eo;
> +
> +			if (wa_len == other_wa_len && !strncmp(wa, other_wa, wa_len)) {
> +				ret = 1;
> +
> +				goto done;
> +			}
> +		}
> +
> +	next_line:
> +		while (*dump && *dump != '\n')
> +			dump++;
> +
> +		if (*dump)
> +			dump++;
> +	}
> +
> +done:
> +	regfree(&regex);
> +
> +	return ret;
> +}
> +
> +int xe_wa_test_debugfs_dump_has_wa(char *dump, const char *wa)
> +{
> +	return debugfs_dump_has_wa(dump, wa);
> +}
> +
>  static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
>  			       const char *debugfs_name, const char *wa)
>  {
> @@ -23,12 +86,12 @@ static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
>  
>  	debugfs_dump = igt_sysfs_get(debugfs_fd, debugfs_name);
>  	if (debugfs_dump) {
> -		char *has_wa = strstr(debugfs_dump, wa);
> +		int ret = debugfs_dump_has_wa(debugfs_dump, wa);
>  
>  		free(debugfs_dump);
>  
> -		if (has_wa)
> -			return 1;
> +		if (ret)
> +			return ret;
>  	}
>  
>  	return 0;
> 
> -- 
> 2.55.0
>