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

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

On Tue, 12 May 2026 23:41:26 -0700, Dixit, Ashutosh wrote:
>
> On Tue, 12 May 2026 21:36:27 -0700, 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.
> >
> > Signed-off-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
> > ---
> >  lib/intel_wa.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/intel_wa.c b/lib/intel_wa.c
> > index 727dd6c98..5d50dcc28 100644
> > --- a/lib/intel_wa.c
> > +++ b/lib/intel_wa.c
> > @@ -7,6 +7,7 @@
> >  #include <stdint.h>
> >  #include <stdio.h>
> >
> > +#include "igt_core.h"
> >  #include "igt_debugfs.h"
> >  #include "igt_sysfs.h"
> >  #include "intel_wa.h"
> > @@ -17,9 +18,6 @@ static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
> >  {
> >	char *debugfs_dump;
> >
> > -	if (!igt_debugfs_exists(drm_fd, debugfs_name, O_RDONLY))
> > -		return -1;
> > -

This should also be an assert, since kernel is expected to create the
device level workarounds directory. So add assert here and remove the
condition added below.

Also for a single patch a cover letter is generally not needed.

Further, from what I am seeing, debugfs is always mounted when running
IGT's, so it is ok to change these to assert's. But maybe double check that
too. That is probably the reason the assert was not added here
originally. But from what I am seeing, assert's should be ok here.

> >	debugfs_dump = igt_sysfs_get(debugfs_fd, debugfs_name);
> >	if (debugfs_dump) {
> >		char *has_wa = strstr(debugfs_dump, wa);
> > @@ -38,7 +36,7 @@ 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:	1 if WA present, 0 otherwise
> >   */
> >  int igt_has_intel_wa(int drm_fd, const char *check_wa)
>
> I'll take a look tomorrow, but one thing I would like to change in these
> functions is:
>
> * If we want to return 1 if WA present, change return type to bool and
>   return true/false, instead of 1/0
>
> * If we want to retain the int return type, return 0 if WA present and
>   something else (maybe 1 is ok) if WA is not present
>
> Because either of these is a more standard convention to follow. So these
> are in addition to changes here, which I will review tomorrow. Thanks.
>
> >  {
> > @@ -48,8 +46,7 @@ int igt_has_intel_wa(int drm_fd, const char *check_wa)
> >	char name[256];
> >
> >	debugfs_fd = igt_debugfs_dir(drm_fd);
> > -	if (debugfs_fd == -1)
> > -		return -1;
> > +	igt_assert(debugfs_fd >= 0);
> >
> >	xe_for_each_gt(drm_fd, xe) {
> >		sprintf(name, "gt%d/workarounds", xe);
> > @@ -58,7 +55,7 @@ int igt_has_intel_wa(int drm_fd, const char *check_wa)
> >			break;
> >	}
> >
> > -	if (!ret)
> > +	if (!ret && igt_debugfs_exists(drm_fd, "workarounds", O_RDONLY))
> >		ret = debugfs_file_has_wa(drm_fd, debugfs_fd, "workarounds", check_wa);
> >
> >	close(debugfs_fd);
> > --
> > 2.53.0
> >