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

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

Kamil Konieczny <kamil.konieczny@linux.intel.com> writes:

> Hi Gustavo,
> On 2026-08-19 at 18:36:40 -0300, Gustavo Sousa wrote:
>> 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.
>
> imho it is better to use regex lib, even if numer of lines are
> the same or little bigger.
>
> +cc Ashutosh and Karthik
>

Thanks! I ended up replacing this patch with another one that does a
full parsing of the debugfs dump layout instead of simply matching full
lines.  That new patch is using regcomp()/regexec().

--
Gustavo Sousa

>
>
> Regards,
> Kamil
>
>> 
>> --
>> 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