[PATCH v1] tests/intel: Enhance the wedged_mode sysfs support

[Cavitt, Jonathan]

-----Original Message-----
From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Pravalika Gurram
Sent: Friday, January 3, 2025 4:41 AM
To: igt-dev at lists.freedesktop.org
Cc: Gurram, Pravalika <pravalika.gurram at intel.com>
Subject: [PATCH v1] tests/intel: Enhance the wedged_mode sysfs support
> 
> 1. Added support to read the wedged_mode sysfs
> 2. Handle the -ETIMEDOUT return value during
> forcewake_all sysfs open
> 
> Signed-off-by: Pravalika Gurram <pravalika.gurram at intel.com>
> ---
>  tests/intel/xe_debugfs.c | 2 +-
>  tests/intel/xe_wedged.c  | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/intel/xe_debugfs.c b/tests/intel/xe_debugfs.c
> index bcbb5036a..3a83868ba 100644
> --- a/tests/intel/xe_debugfs.c
> +++ b/tests/intel/xe_debugfs.c
> @@ -268,7 +268,7 @@ test_forcewake(int fd)
>  {
>  	int handle = igt_debugfs_open(fd, "forcewake_all", O_WRONLY);
>  
> -	igt_assert_neq(handle, -1);
> +	igt_assert_lte(0, handle);
>  	close(handle);
>  }
>  
> diff --git a/tests/intel/xe_wedged.c b/tests/intel/xe_wedged.c
> index 613d571b4..d4acd5f7f 100644
> --- a/tests/intel/xe_wedged.c
> +++ b/tests/intel/xe_wedged.c
> @@ -208,6 +208,7 @@ igt_main
>  	struct drm_xe_engine_class_instance *hwe;
>  	int fd;
>  	char pci_slot[NAME_MAX];
> +	char str[150];
>  
>  	igt_fixture {
>  		fd = drm_open_driver(DRIVER_XE);
> @@ -228,6 +229,7 @@ igt_main
>  		igt_assert_eq(simple_ioctl(fd), 0);
>  		xe_for_each_engine(fd, hwe)
>  			simple_exec(fd, hwe);
> +		igt_debugfs_read(fd, "wedged_mode", str);

I don't think the system reports if this read fails.  AFAICT, while an error
value is reported and returned by igt_debugfs_simple_read in the
execution stack, the error value is not passed along any further than that,
and an error in this function otherwise does not trigger a test failure or
warning.

IMO, it would probably be for the best if we put some form of assertion here.
Maybe something like:

"""
		igt_assert_f(str[0] != '\0', "Failed to read wedged_mode from debugfs!\n");
"""

There's also an argument to be made that attempting to read wedged_mode
and asserting its existence should be handled by a separate test, but I'll leave
that up to your discretion.

Other than that, this change is acceptable.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

>  	}
>  
>  	igt_subtest_f("wedged-at-any-timeout") {
> -- 
> 2.34.1
> 
>