Re: [PATCH i-g-t 2/4] tests/intel/xe_fault_injection: Make setup_injection_fault() programmable.

[Francois Dugast <francois.dugast@intel.com>]

Hi,

On Fri, Feb 07, 2025 at 12:59:00PM +0530, Satyanarayana K V P wrote:
> The current setup_injection_fault() is always programming fixed fault
> injection parameters. Update this function to accept fault injection
> parameters for better injection capabilities.
> 
> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> Cc: Michał Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Francois Dugast <francois.dugast@intel.com>
> ---
>  tests/intel/xe_fault_injection.c | 42 +++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/intel/xe_fault_injection.c b/tests/intel/xe_fault_injection.c
> index 7ae941367..a4252d028 100644
> --- a/tests/intel/xe_fault_injection.c
> +++ b/tests/intel/xe_fault_injection.c
> @@ -31,6 +31,17 @@ enum injection_list_action {
>  	INJECTION_LIST_REMOVE,
>  };
>  
> +struct fault_injection_params {
> +	/* @probability: Likelihood of failure injection, in percent. */
> +	int probability;

It seems there is a type mismatch between here and the calls below with
igt_sysfs_set_u32() and igt_sysfs_set_s32(). Those 4 attributes could
be:

    uint32_t probability;
    uint32_t interval;
    int32_t times;
    uint32_t space;

> +	/* @interval: Specifies the interval between failures */
> +	int interval;
> +	/* @times: Specifies how many times failures may happen at most */
> +	int times;
> +	/* @space: Specifies an initial resource “budget”, decremented by “size” */
                                                 ^      ^                 ^    ^
Is there a reason for using “ and ” instead of " like in the rest of IGT?

> +	unsigned int space;
> +};
> +
>  static int fail_function_open(void)
>  {
>  	int debugfs_fail_function_dir_fd;
> @@ -117,21 +128,40 @@ static void injection_list_do(enum injection_list_action action, const char func
>  	close(dir);
>  }
>  
> +/**
> + * Default fault injection parameters which injects fault on first call to the
> + * configured fail_function.
> + */
> +static const struct fault_injection_params default_fault_params = {
> +	.probability = 100,
> +	.interval = 0,
> +	.times = -1,
> +	.space = 0
> +};
> +
>  /*
>   * See https://docs.kernel.org/fault-injection/fault-injection.html#application-examples
>   */
> -static void setup_injection_fault(void)
> +static void setup_injection_fault(const struct fault_injection_params *fault_params)
>  {
>  	int dir;
>  
> +	igt_assert(fault_params);
> +	igt_assert(fault_params->probability >= 0);
> +	igt_assert(fault_params->probability <= 100);
> +
>  	dir = fail_function_open();
>  	igt_assert_lte(0, dir);
>  
> +	igt_info("probability = %d, interval = %d, times = %d, space = %u\n",
> +			fault_params->probability, fault_params->interval,
> +			fault_params->times, fault_params->space);
> +

igt_debug() is probably sufficient but up to you.

Francois

>  	igt_assert_lte(0, igt_sysfs_printf(dir, "task-filter", "N"));
> -	igt_sysfs_set_u32(dir, "probability", 100);
> -	igt_sysfs_set_u32(dir, "interval", 0);
> -	igt_sysfs_set_s32(dir, "times", -1);
> -	igt_sysfs_set_u32(dir, "space", 0);
> +	igt_sysfs_set_u32(dir, "probability", fault_params->probability);
> +	igt_sysfs_set_u32(dir, "interval", fault_params->interval);
> +	igt_sysfs_set_s32(dir, "times", fault_params->times);
> +	igt_sysfs_set_u32(dir, "space", fault_params->space);
>  	igt_sysfs_set_u32(dir, "verbose", 1);
>  
>  	close(dir);
> @@ -322,7 +352,7 @@ igt_main
>  		igt_require(fail_function_injection_enabled());
>  		fd = drm_open_driver(DRIVER_XE);
>  		igt_device_get_pci_slot_name(fd, pci_slot);
> -		setup_injection_fault();
> +		setup_injection_fault(&default_fault_params);
>  	}
>  
>  	for (const struct section *s = vm_create_fail_functions; s->name; s++)
> -- 
> 2.35.3
>