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

Hi Gustavo,
On 2026-08-19 at 18:36:40 -0300, Gustavo Sousa wrote:
> Matt Roper <[email protected]> writes:
> 
> > On Wed, Aug 19, 2026 at 04:32:04PM -0300, Gustavo Sousa wrote:
> >> Gustavo Sousa <[email protected]> writes:
> >> 
> >> > Matt Roper <[email protected]> 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



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 <[email protected]>
> >> >>> ---
> >> >>>  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