Re: [PATCH v2] lib/intel_wa: Assert on error instead of returning -1

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Sat, 16 May 2026 12:11:13 -0700, Dixit, Ashutosh wrote:
>
> On Fri, 15 May 2026 05:24:08 -0700, Kamil Konieczny wrote:
> >
>
> Hi Kamil,
>
> > Hi Shekhar,
> > On 2026-05-14 at 12:06:55 +0530, Shekhar Chauhan wrote:
> > > igt_has_intel_wa() returns 0, 1, or -1, but there is no point
> > > distinguishing the error and no-workaround cases for callers. Replace
> > > the error return with igt_assert() and simplify debugfs_file_has_wa() by
> > > dropping the igt_debugfs_exists() check, since igt_sysfs_get() returns
> > > NULL for missing files anyway.
> > >
> > > v2: Make the func as bool instead of int.
> > >
> > > Signed-off-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
> > > ---
> > >  lib/intel_wa.c | 19 +++++++++----------
> > >  lib/intel_wa.h |  2 +-
> > >  2 files changed, 10 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/lib/intel_wa.c b/lib/intel_wa.c
> > > index 727dd6c98..e4fe7b0ee 100644
> > > --- a/lib/intel_wa.c
> > > +++ b/lib/intel_wa.c
> > > @@ -7,18 +7,18 @@
> > >  #include <stdint.h>
> > >  #include <stdio.h>
> > >
> > > +#include "igt_core.h"
> > >  #include "igt_debugfs.h"
> > >  #include "igt_sysfs.h"
> > >  #include "intel_wa.h"
> > >  #include "xe/xe_query.h"
> > >
> > > -static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
> > > +static bool debugfs_file_has_wa(int drm_fd, int debugfs_fd,
> > >			       const char *debugfs_name, const char *wa)
> > >  {
> > >	char *debugfs_dump;
> > >
> > > -	if (!igt_debugfs_exists(drm_fd, debugfs_name, O_RDONLY))
> > > -		return -1;
> > > +	igt_assert(igt_debugfs_exists(drm_fd, debugfs_name, O_RDONLY));
> > >
> > >	debugfs_dump = igt_sysfs_get(debugfs_fd, debugfs_name);
> > >	if (debugfs_dump) {
> > > @@ -27,10 +27,10 @@ static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
> > >		free(debugfs_dump);
> > >
> > >		if (has_wa)
> > > -			return 1;
> > > +			return true;
> > >	}
> > >
> > > -	return 0;
> > > +	return false;
> > >  }
> > >
> > >  /**
> > > @@ -38,18 +38,17 @@ static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
> > >   * @drm_fd:	A drm file descriptor
> > >   * @check_wa:	Workaround to be checked
> > >   *
> > > - * Returns:	0 if no WA, 1 if WA present, -1 on error
> > > + * Returns:	true if WA present, false otherwise
> > >   */
> > > -int igt_has_intel_wa(int drm_fd, const char *check_wa)
> > > +bool igt_has_intel_wa(int drm_fd, const char *check_wa)
> > >  {
> > > -	int ret = 0;
> > > +	bool ret = 0;
> > >	int debugfs_fd;
> > >	unsigned int xe;
> > >	char name[256];
> > >
> > >	debugfs_fd = igt_debugfs_dir(drm_fd);
> > > -	if (debugfs_fd == -1)
> > > -		return -1;
> > > +	igt_assert(debugfs_fd >= 0);
> >
> > This will mean that all other users needs to be updated.
>
> To date there are no other users of this function, except one in OA.
>
> > The way of adding any igt_assert/igt_require into lib/
> > make it impossible to reuse them in tools/ and require
> > to write same functionality twice, one in lib/ and one
> > for any tools which will want to use them. Or to write
> > __function() without igt_assert/igt_require.
>
> Well it was I who suggested doing this. Can you explain your point a bit?
> Any tools which use a lib function which uses e.g. igt_assert/igt_require
> can also link with lib_igt (which provides igt_assert/igt_require). See
> e.g. tools/xe-perf/meson.build. It is not unreasonable for IGT tools to
> link with lib_igt. I thought not linking with igt_lib was an unreasonabe
> requirement and I removed it for tools/xe-perf.
>
> If you are saying tools should be able to run without debugfs mounted,
> maybe you have a point. In that case we can drop this patch. But even here
> maybe IGT tools should run only with debugfs mounted?

Also the tool should realize it is calling a function which requires
debugfs, and separately check whether or not debugfs is mounted and skip
calling the function if it isn't. So I am not sure why we should do
anything different, since wa list is exposed through debugfs.

>
>
> > >	xe_for_each_gt(drm_fd, xe) {
> > >		sprintf(name, "gt%d/workarounds", xe);
> > > diff --git a/lib/intel_wa.h b/lib/intel_wa.h
> > > index 765a5948e..34cafecc4 100644
> > > --- a/lib/intel_wa.h
> > > +++ b/lib/intel_wa.h
> > > @@ -6,6 +6,6 @@
> > >  #ifndef __INTEL_WA_H__
> > >  #define __INTEL_WA_H__
> > >
> > > -int igt_has_intel_wa(int drm_fd, const char *check_wa);
> > > +bool igt_has_intel_wa(int drm_fd, const char *check_wa);
> > >
> > >  #endif /* __INTEL_WA_H__ */
> > > --
> > > 2.53.0
> > >