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

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

On Tue, 19 May 2026 02:52:26 -0700, Kamil Konieczny wrote:
>
> Hi all,
> On 2026-05-18 at 16:53:04 -0700, Dixit, Ashutosh wrote:
> > 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.
>
> Main reason was a tools asserting in middle of operation, where
> it should just report an error, lack of permission or some
> other reason why it cannot proceed. Another example would be
> implemetation of malloc() where lib will assert instead of
> returning a NULL pointer.
>
> > >
> > > 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?
>
> Definitly not, there are systems without debugfs for security
> reasons, and there is sysfs so for example intel_gpu_freq should
> work perfectly fine.
>
> >
> > 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.
> >
>
> Well, as it is only used in intel perf tests/tools, it is ok
> so I will not block it, you can merge it.

Well intel_wa functions are meant to be used generally to check if any
workarounds in the kernel, so they are not just for intel perf
tests/tools. However, as I mentioned, because this workaround information
is exposed through debugfs, IMO it is ok to assert if debugfs is not
mounted.

Therefore I have gone ahead and merged this. We can of course revisit if
needed in the future, but for now this patch seems correct to me.

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