Re: [PATCH] tests/intel: Trigger configfs attr callbacks

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 16 Apr 2026, Smitha Balasubramanyam <smitha.balasubramanyam@intel.com> wrote:
> Coverage improvement achieved by adding new subtests
> for show and store functionality of enable_psmi and sriov_max_vfs
> Enhanced existing subtests like survivability_mode, gt_types_allowed,
> engines_allowed, to cover show and store functions.
>
> Add a test vector with a partially invalid second command in the
> ctx_restore_*_bb invalid-case suite to ensure a malformed multi-line
> BB is rejected and the stored BB remains empty (rollback).
>
> Replaced the hardcoded "ctx_restore_post_bb" write with the computed
> file name ("ctx_restore_%s_bb") in test_ctx_restore_invalid so the
> invalid-case tests work for both "post" and "mid" variants.

There's too many things going on at once. One thing per patch, please.

>
> Signed-off-by: Smitha Balasubramanyam <smitha.balasubramanyam@intel.com>
> ---
>  tests/intel/xe_configfs.c | 102 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 3 deletions(-)
>
> diff --git a/tests/intel/xe_configfs.c b/tests/intel/xe_configfs.c
> index 755524e7c..bde52e6ef 100644
> --- a/tests/intel/xe_configfs.c
> +++ b/tests/intel/xe_configfs.c
> @@ -85,6 +85,13 @@ static void test_survivability_mode(int configfs_device_fd)
>  	char path[PATH_MAX];
>  	int fd;
>  
> +	/* Buffer for reading attribute values */

I think that's a pretty useless comment.

> +	char buf[256];
> +
> +	/* survivability_mode_show */

Ditto. It's unclear what you're doing here.

> +	igt_assert(igt_sysfs_read(configfs_device_fd, "survivability_mode",
> +				buf, sizeof(buf) - 1));

So this checks that you can read from survivability_mode, but ignores
the value. The sizeof(buf) - 1 seems like cargo culted copy-paste. The
whole igt_sysfs_read() function looks difficult to use, maybe there
should be a function to read a string that's ensured to be NUL
terminated.

> +
>  	/* Enable survivability mode */
>  	set_survivability_mode(configfs_device_fd, true);
>  
> @@ -138,6 +145,13 @@ static void test_engines_allowed(int configfs_device_fd)
>  		"rcs000",
>  	};
>  
> +	/* Buffer for reading attribute values */
> +	char buf[256];
> +
> +	/* engines_allowed_show */
> +	igt_assert(igt_sysfs_read(configfs_device_fd, "engines_allowed",
> +				buf, sizeof(buf) - 1));

Same comments here.

> +
>  	/*
>  	 * These only test if engine parsing is correct, so just make sure
>  	 * there's no device bound
> @@ -166,8 +180,25 @@ static void test_gt_types_allowed(int configfs_device_fd)
>  	};
>  
>  	static const char *invalid_values[] = {
> -                "check",
> -        };
> +		"check",
> +	};
> +
> +	/* Buffer for reading attribute values */
> +	char buf[256];
> +
> +	/* gt_types_allowed_show */
> +	igt_assert(igt_sysfs_read(configfs_device_fd, "gt_types_allowed",
> +				buf, sizeof(buf) - 1));

And here.

> +
> +	/* gt_types_allowed_store
> +	 * Store may succeed or fail depending on platform/device state.
> +	 * Accept success (errno == 0) or expected policy failures.
> +	 */
> +	errno = 0;
> +	igt_sysfs_set(configfs_device_fd, "gt_types_allowed", "0");
> +	igt_assert_f(errno == 0 || errno == EBUSY || errno == EINVAL,
> +			"Unexpected errno from gt_types_allowed store: %d\n", errno);

igt_sysfs_write() will return -errno on failure, number of bytes
otherwise. Maybe you should use that instead of relying on errno being
still set. There's the close() function being called which I presume
will trash errno.

> +
>  
>  	/*
>  	 * These only test if gt type parsing is correct, so just make sure
> @@ -225,12 +256,16 @@ static void test_ctx_restore_invalid(int configfs_device_fd, const char *type)
>  		{ .test = "invalid-engine-instance",
>  		  .in = "rcs0 reg 4F100 DEADBEEF",
>  		},
> +		{ .test = "invalid-second-command",
> +			.in = "rcs cmd 11000001 4F100 DEADBEEF\n"
> +				"rcs cmd 11000001 4F10G DEADBEEF",
> +		},
>  	};
>  	char buf[4096] = { };
>  	char file[64] = { };
>  
>  	snprintf(file, sizeof(file), "ctx_restore_%s_bb", type);
> -	igt_sysfs_set(configfs_device_fd, "ctx_restore_post_bb", "");
> +	igt_sysfs_set(configfs_device_fd, file, "");
>  
>  	/*
>  	 * These only test if command parsing is correct,
> @@ -335,6 +370,53 @@ static void test_ctx_restore(int configfs_device_fd, const char *type)
>  	}
>  }
>  
> +/**
> + * SUBTEST: psmi_store_show
> + * Description: Validate PSMI Store and Show functionality
> + */
> +static void test_psmi_store_show(int configfs_device_fd)
> +{
> +	/* Buffer for reading attribute values */
> +	char buf[256];
> +
> +	/* enable_psmi_show */
> +	igt_assert(igt_sysfs_read(configfs_device_fd, "enable_psmi",
> +				buf, sizeof(buf) - 1));

Again this just checks the file exists. But if you're writing to it
later, you could just check for that error there.

And the comments are also useless.

> +
> +	/* enable_psmi_store
> +	 * Enabling PSMI on an active device is expected to fail with EBUSY.
> +	 */
> +	errno = 0;
> +	igt_sysfs_set(configfs_device_fd, "enable_psmi", "1");
> +	igt_assert_f(errno == EBUSY,
> +			"Expected EBUSY from enable_psmi_store, got %d\n", errno);

errno is likely trashed.

> +
> +}
> +
> +/**
> + * SUBTEST: sriov_max_vfs_store_show
> + * Description: Validate SRIOV Max VFs store and show functionality
> + */
> +static void test_sriov_max_vfs_store_show(int configfs_device_fd)
> +{
> +	/* Buffer for reading attribute values */
> +	char buf[256];
> +
> +	/* sriov_max_vfs_show */
> +	igt_assert(igt_sysfs_read(configfs_device_fd, "sriov/max_vfs",
> +				buf, sizeof(buf) - 1));
> +
> +	/* sriov_max_vfs_store
> +	 * Changing max_vfs usually fails once device is active.
> +	 * Accept expected policy failures while exercising store path.
> +	 */
> +	errno = 0;
> +	igt_sysfs_set(configfs_device_fd, "sriov/max_vfs", "1");
> +	igt_assert_f(errno == EBUSY,
> +			"Expected EBUSY from sriov_max_vfs_store, got %d\n", errno);

Ditto about errno and everything.

> +
> +}
> +
>  static void set_bus_addr(int fd)
>  {
>  	pci_dev = igt_device_get_pci_device(fd);
> @@ -439,6 +521,20 @@ int igt_main()
>  		close_configfs_group(configfs_fd, configfs_device_fd);
>  	}
>  
> +	igt_describe("Validate PSMI Store and Show functionality");
> +	igt_subtest("psmi_store_show") {
> +		configfs_device_fd = create_device_configfs_group(configfs_fd);
> +		test_psmi_store_show(configfs_device_fd);
> +		close_configfs_group(configfs_fd, configfs_device_fd);
> +	}
> +
> +	igt_describe("Validate SRIOV Max Vfs Store and Show functionality");
> +	igt_subtest("sriov_max_vfs_store_show") {
> +		configfs_device_fd = create_device_configfs_group(configfs_fd);
> +		test_sriov_max_vfs_store_show(configfs_device_fd);
> +		close_configfs_group(configfs_fd, configfs_device_fd);
> +	}
> +
>  	igt_fixture() {
>  		close(configfs_fd);
>  	}

-- 
Jani Nikula, Intel