[PATCH v2 1/2] tests/intel/sysfs: Restore sysfs values on test failure

[Kamil Konieczny]

Hi Jonathan,
On 2024-10-31 at 16:31:21 +0000, Jonathan Cavitt wrote:
> The tests xe_sysfs_preempt_timeout and xe_sysfs_timeslice_duration
> modify the values of preempt_timeout_us and timeslice_duration_us,
> respectively.  However, on a test failure, it is possible that these
> values may remain in their modified states, resulting in the values
> being used in future tests and causing unexpected behavior.
> 
> Save the respective modified values before starting the test and attempt
> to restore the values on test exit.
> 
> Suggested-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> CC: Vinay Belgaumkar <vinay.belgaumkar at intel.com>

Thank you for the patch, I have few nits, see below.

> ---
>  tests/intel/xe_sysfs_preempt_timeout.c    | 34 ++++++++++++++++-------
>  tests/intel/xe_sysfs_timeslice_duration.c | 33 ++++++++++++++++------
>  2 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/intel/xe_sysfs_preempt_timeout.c b/tests/intel/xe_sysfs_preempt_timeout.c
> index 7fa0dfcdf7..1b9c26bfdc 100644
> --- a/tests/intel/xe_sysfs_preempt_timeout.c
> +++ b/tests/intel/xe_sysfs_preempt_timeout.c
> @@ -183,8 +183,10 @@ igt_main
>  				       "preempt_timeout_min",
>  				       "preempt_timeout_max"}, };
>  	int count = sizeof(property) / sizeof(property[0]);
> +	int gt_count = 0;
>  	int fd = -1, sys_fd, gt;
> -	int engines_fd = -1, gt_fd = -1;
> +	int engines_fd[8], gt_fd[8];
> +	unsigned int pts[8];

Please use define here, for example
#define MAX_XE_ENGINE 8

	int engines_fd[MAX_XE_ENGINE], gt_fd[MAX_XE_ENGINE];
	unsigned int pts[MAX_XE_ENGINE];

or allocate/realloc/dealloc it (it could be then a struct).

>  
>  	igt_fixture {
>  		fd = drm_open_driver(DRIVER_XE);
> @@ -192,26 +194,38 @@ igt_main
>  		sys_fd = igt_sysfs_open(fd);
>  		igt_require(sys_fd != -1);
>  		close(sys_fd);
> +
> +		xe_for_each_gt(fd, gt) {

Check

			igt_require(gt_count < MAX_XE_ENGINE);

or reallocate.

> +			gt_fd[gt_count] = xe_sysfs_gt_open(fd, gt);
> +			igt_require(gt_fd[gt_count] != -1);
> +			engines_fd[gt_count] = openat(gt_fd[gt_count], "engines", O_RDONLY);
> +			igt_require(engines_fd[gt_count] != -1);
> +			igt_assert(igt_sysfs_scanf(engines_fd[gt_count],

Why assert? imho igt_require should be enough.

> +						   "preempt_timeout_us",
> +						   "%u", &pts[gt_count]) == 1);
> +			gt_count++;
> +		}
> +
>  	}
>  
>  	for (int i = 0; i < count; i++) {
>  		for (typeof(*tests) *t = tests; t->name; t++) {
>  			igt_subtest_with_dynamic_f("%s-%s", property[i][0], t->name) {
> +				int j = 0;
>  				xe_for_each_gt(fd, gt) {

Add
			igt_assert(gt_count < MAX_XE_ENGINE);

> -					gt_fd = xe_sysfs_gt_open(fd, gt);
> -					igt_require(gt_fd != -1);
> -					engines_fd = openat(gt_fd, "engines", O_RDONLY);
> -					igt_require(engines_fd != -1);
> -
> -					igt_sysfs_engines(fd, engines_fd, gt, 1, property[i],
> -									  t->fn);
> -					close(engines_fd);
> -					close(gt_fd);
> +					igt_sysfs_engines(fd, engines_fd[j], gt, 1, property[i],
> +										    t->fn);

I would prefere this in one line, or align to 'fd'.

> +					j++;
>  				}
>  			}
>  		}
>  	}
>  	igt_fixture {
> +		for (int i = gt_count - 1; i >= 0; i--) {
> +			set_preempt_timeout(engines_fd[i], pts[i]);
> +			close(engines_fd[i]);
> +			close(gt_fd[i]);
> +		}

Add newline.

>  		drm_close_driver(fd);
>  	}
>  }
> diff --git a/tests/intel/xe_sysfs_timeslice_duration.c b/tests/intel/xe_sysfs_timeslice_duration.c
> index cf95a3ac1c..914575a282 100644
> --- a/tests/intel/xe_sysfs_timeslice_duration.c
> +++ b/tests/intel/xe_sysfs_timeslice_duration.c
> @@ -155,8 +155,10 @@ igt_main
>  				       "timeslice_duration_min",
>  				       "timeslice_duration_max"}, };
>  	int count = sizeof(property) / sizeof(property[0]);
> +	int gt_count = 0;
>  	int fd = -1, sys_fd, gt;
> -	int engines_fd = -1, gt_fd = -1;
> +	int engines_fd[8], gt_fd[8];
> +	unsigned int tds[8];

Same here, use define instead of '8' or dynamic alloc.

>  
>  	igt_fixture {
>  		fd = drm_open_driver(DRIVER_XE);
> @@ -164,25 +166,38 @@ igt_main
>  		sys_fd = igt_sysfs_open(fd);
>  		igt_require(sys_fd != -1);
>  		close(sys_fd);
> +
> +		xe_for_each_gt(fd, gt) {

Same here, add guard for 'gt_count < MAX_XE_ENGINE'.

> +			gt_fd[gt_count] = xe_sysfs_gt_open(fd, gt);
> +			igt_require(gt_fd[gt_count] != -1);
> +			engines_fd[gt_count] = openat(gt_fd[gt_count],
> +					                  "engines", O_RDONLY);

One line or align to 'gt_fd[...]'
Btw use checkpatch.pl from linux kernel, here are too many spaces.

> +			igt_require(engines_fd[gt_count] != -1);
> +			igt_assert(igt_sysfs_scanf(engines_fd[gt_count],

Same here, igt_require.

> +						   "timeslice_duration_us",
> +						   "%u", &tds[gt_count]) == 1);
> +			gt_count++;
> +		}
>  	}
>  
>  	for (int i = 0; i < count; i++) {
>  		for (typeof(*tests) *t = tests; t->name; t++) {
>  			igt_subtest_with_dynamic_f("%s-%s", property[i][0], t->name) {
> +				int j = 0;
>  				xe_for_each_gt(fd, gt) {

Same here, guard 'j < MAX_XE_ENGINE'

> -					gt_fd = xe_sysfs_gt_open(fd, gt);
> -					igt_require(gt_fd != -1);
> -					engines_fd = openat(gt_fd, "engines", O_RDONLY);
> -					igt_require(engines_fd != -1);
> -					igt_sysfs_engines(fd, engines_fd, gt, 1, property[i],
> -										 t->fn);
> -					close(engines_fd);
> -					close(gt_fd);
> +					igt_sysfs_engines(fd, engines_fd[j], gt, 1, property[i],
> +										    t->fn);

Same here, one line or align to 'fd'.

> +					j++;
>  				}
>  			}
>  		}
>  	}
>  	igt_fixture {
> +		for (int i = gt_count - 1; i >= 0; i--) {
> +			set_timeslice_duration(engines_fd[i], tds[i]);
> +			close(engines_fd[i]);
> +			close(gt_fd[i]);
> +		}

Add newline.

With above fixed
Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

>  		drm_close_driver(fd);
>  	}
>  }
> -- 
> 2.43.0
>