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