Re: [PATCH i-g-t 02/10] lib/xe: Use stricter line-equality check when checking for workarounds

[Gustavo Sousa <gustavo.sousa@intel.com>]

Matt Roper <matthew.d.roper@intel.com> writes:

> On Wed, Aug 19, 2026 at 04:32:04PM -0300, Gustavo Sousa wrote:
>> Gustavo Sousa <gustavo.sousa@intel.com> writes:
>> 
>> > Matt Roper <matthew.d.roper@intel.com> writes:
>> >
>> >> On Tue, Jul 21, 2026 at 03:59:55PM -0300, Gustavo Sousa wrote:
>> >>> Currently debugfs_file_has_wa() uses strstr() to check if a workaround
>> >>> name is present in the debugfs dump.  Using strstr() would match the
>> >>> workaround name anywhere in the dump buffer and with that we risk
>> >>> producing unexpected results if the checked workaround name happens to
>> >>> be a substring of another workaround present in the dump.
>> >>> 
>> >>> Fix that by making sure we match the workaround name with the full
>> >>> line from the dump.
>> >>
>> >> There are other parts of IGT that use regular expressions from either
>> >> POSIX (regcomp / regexec) or Glib (g_regex_new / g_regex_match).  Would
>> >> it be possible to use one of those here instead of opencoding a match
>> >> function?
>> >
>> > Yeah.  That could make this function more readable and easier to extend.
>> > I'll take a look at those options.
>> 
>> This new version of debugfs_file_has_wa() misses the fact that some
>> lines will contain multiple workarounds separated by comma.  So I
>> decided to implement a more robust version.  I have two versions, one
>> that is still not using regular expressions and another that uses POSIX
>> regcomp/regexec functions.  I have a slight preference for the former,
>> but if you prefer, we can go with the latter.  IMO, both versions are
>> more readable than the one currently proposed in this patch.
>> 
>> I would like your opinion before sending v2 of this series. :-)
>
> It might be best to ping the IGT maintainers and see what they think.
> Today IGT has a mix of open-coded string matching and two different
> styles of regular expressions.  It seems like the kind of thing they may
> want to set a standard direction for with new code.  I don't have much
> of a personal opinion.

Kamil, do you have a direction here? Feel free to add other maintainers
as well.

--
Gustavo Sousa

>
>
> Matt
>
>> 
>> Note: I also created a separate commit adding a simple test for this
>> function in lib/tests.
>> 
>> Version without regular expressions:
>> 
>>     | static bool is_eol(char c)
>>     | {
>>     |         return !c || c == '\n';
>>     | }
>>     | 
>>     | static bool is_wa_sep(char c)
>>     | {
>>     |         return isspace(c) || c == ',';
>>     | }
>>     | 
>>     | static bool debugfs_dump_has_wa(char *dump, const char *wa)
>>     | {
>>     |         size_t wa_len = strlen(wa);
>>     | 
>>     |         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).
>>     |                  */
>>     |                 if (*dump != '\t')
>>     |                         goto next_line;
>>     | 
>>     |                 while (!is_eol(*dump)) {
>>     |                         char *other_wa;
>>     | 
>>     |                         while (!is_eol(*dump) && is_wa_sep(*dump))
>>     |                                 dump++;
>>     | 
>>     |                         other_wa = dump;
>>     | 
>>     |                         while (!is_eol(*dump) && !is_wa_sep(*dump))
>>     |                                 dump++;
>>     | 
>>     |                         if (wa_len == dump - other_wa &&
>>     |                             !strncmp(wa, other_wa, wa_len))
>>     |                                 return true;
>>     |                 }
>>     | 
>>     |         next_line:
>>     |                 while (!is_eol(*dump))
>>     |                         dump++;
>>     | 
>>     |                 if (*dump)
>>     |                         dump++;
>>     |         }
>>     | 
>>     |         return false;
>>     | }
>> 
>> Version with POSIX regcomp/regexec:
>> 
>>     | 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).
>>     |                  */
>>     |                 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;
>>     | }
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> > --
>> > Gustavo Sousa
>> >
>> >>
>> >>
>> >> Matt
>> >>
>> >>> 
>> >>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> >>> ---
>> >>>  lib/xe/xe_wa.c | 39 ++++++++++++++++++++++++++++++++++++++-
>> >>>  1 file changed, 38 insertions(+), 1 deletion(-)
>> >>> 
>> >>> diff --git a/lib/xe/xe_wa.c b/lib/xe/xe_wa.c
>> >>> index ff5daf529831..d44431e7e61f 100644
>> >>> --- a/lib/xe/xe_wa.c
>> >>> +++ b/lib/xe/xe_wa.c
>> >>> @@ -13,6 +13,43 @@
>> >>>  #include "xe/xe_wa.h"
>> >>>  #include "xe/xe_query.h"
>> >>>  
>> >>> +static bool debugfs_dump_has_wa(char *dump, const char *wa)
>> >>> +{
>> >>> +	char *a = dump;
>> >>> +
>> >>> +	while (*a) {
>> >>> +		const char *b = wa;
>> >>> +
>> >>> +		/*
>> >>> +		 * 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).
>> >>> +		 */
>> >>> +		if (*a++ != '\t')
>> >>> +			goto next_line;
>> >>> +
>> >>> +		while (*a == *b && !(*a == '\0' || *a == '\n' || *b == '\0')) {
>> >>> +			a++;
>> >>> +			b++;
>> >>> +		}
>> >>> +
>> >>> +		if ((*a == '\0' || *a == '\n') && *b == '\0')
>> >>> +			return true;
>> >>> +
>> >>> +	next_line:
>> >>> +		/* No match for this line, advance to the next one. */
>> >>> +		while (*a != '\n' && *a != '\0')
>> >>> +			a++;
>> >>> +
>> >>> +		if (*a == '\n')
>> >>> +			a++;
>> >>> +	}
>> >>> +
>> >>> +	return false;
>> >>> +}
>> >>> +
>> >>>  static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
>> >>>  			       const char *debugfs_name, const char *wa)
>> >>>  {
>> >>> @@ -23,7 +60,7 @@ 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);
>> >>> +		bool has_wa = debugfs_dump_has_wa(debugfs_dump, wa);
>> >>>  
>> >>>  		free(debugfs_dump);
>> >>>  
>> >>> 
>> >>> -- 
>> >>> 2.55.0
>> >>> 
>> >>
>> >> -- 
>> >> Matt Roper
>> >> Graphics Software Engineer
>> >> Linux GPU Platform Enablement
>> >> Intel Corporation
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation