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

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

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?

Thanks.
--
Ashutosh


> >	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
> >