Re: [PATCH i-g-t v1 3/7] tests/core_hotplug: Add checks to subtests

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 07.06.2024 17:36, Kamil Konieczny wrote:
> Currently health checks and recovery are performed in fixtures
> outside of subtest and even without any subtest running. Whats
> more, after one subtest fail, all other checks will fail and
> will print quite a few logs. Additionally they are also meant
> to check health of device after a test and make a subtest fail.
>
> Lets try to fail early inside subtest and lower number of checks
> to those really needed and do them only once per fixture, which
> should make reading logs easier.
>
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>   tests/core_hotunplug.c | 118 +++++++++++++++++++++++++++--------------
>   1 file changed, 78 insertions(+), 40 deletions(-)
>
> diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
> index 3971b94ff..7ed9abd37 100644
> --- a/tests/core_hotunplug.c
> +++ b/tests/core_hotunplug.c
> @@ -84,6 +84,9 @@
>   
>   IGT_TEST_DESCRIPTION("Examine behavior of a driver on device hot unplug");
>   
> +#define ACTION_RECOVER		(1 << 0)
> +#define ACTION_POST_CHECK	(1 << 1)
> +
>   struct hotunplug {
>   	struct {
>   		int drm;
> @@ -98,6 +101,7 @@ struct hotunplug {
>   	bool has_intel_perf;
>    	char *snd_driver;
>   	int chipset;
> +	int checks;
>   };
>   
>   /* Helpers */
> @@ -509,8 +513,11 @@ static void pre_check(struct hotunplug *priv)
>   static void recover(struct hotunplug *priv)
>   {
>   	bool late_close = priv->fd.drm >= 0;
> +	int needs_recover = priv->checks & ACTION_RECOVER;
>   
>   	cleanup(priv);
> +	if (!needs_recover)
> +		return;
>   
>   	if (!priv->failure && late_close)
>   		igt_ignore_warn(healthcheck(priv, false));
> @@ -528,10 +535,18 @@ static void recover(struct hotunplug *priv)
>   
>   	if (priv->failure)
>   		igt_assert_f(healthcheck(priv, true), "%s\n", priv->failure);
> +
> +	priv->checks &= ~ACTION_RECOVER;
>   }

With this change recover becomes kind of recover_if_requested.

>   
>   static void post_healthcheck(struct hotunplug *priv)
>   {
> +	bool needs_post_check = priv->checks & ACTION_POST_CHECK;
> +
> +	priv->checks = 0;
> +	if (!needs_post_check)
> +		return;
> +

Wouldn't be safer to clean only appropriate flag:

+	if (!needs_post_check)
+		return;
+	priv->checks &= ~ACTION_POST_CHECK;

>   	igt_abort_on_f(priv->failure, "%s\n", priv->failure);
>   
>   	cleanup(priv);
> @@ -667,6 +682,19 @@ static void hotreplug_lateclose(struct hotunplug *priv)
>   }
>   
>   /* Main */
> +#define RECOVER	igt_fixture { \
> +		recover(&priv); \
> +	}
> +
> +#define POST_CHECK igt_fixture { \
> +		post_healthcheck(&priv); \
> +	}
> +
> +static void recover_and_post_healthcheck(struct hotunplug *priv)
> +{
> +	recover(priv);
> +	post_healthcheck(priv);
> +}

Since you are proposing flags to trigger recover and check with cleanup, 
why do not
do it in one function, for example post_healthcheck:

static void post_healthcheck(struct hotunplug *priv)

{
     if (priv->checks & ACTION_RECOVER)
         recover(priv);
     priv->checks &= ~ACTION_RECOVER;

     if (!(priv->checks & ACTION_POST_CHECK))
         return;
     ...
}

and use post_healthcheck everywhere, and drop:
- recover_and_post_healthcheck,
- RECOVER

Regards
Andrzej

>   
>   igt_main
>   {
> @@ -677,6 +705,7 @@ igt_main
>   		.has_intel_perf = false,
>   		.snd_driver	= NULL,
>   		.chipset	= DRIVER_ANY,
> +		.checks		= 0,
>   	};
>   
>   	igt_fixture {
> @@ -704,100 +733,109 @@ igt_main
>   
>   	igt_subtest_group {
>   		igt_describe("Check if the driver can be cleanly unbound from a device believed to be closed, then rebound");
> -		igt_subtest("unbind-rebind")
> +		igt_subtest("unbind-rebind") {
> +			priv.checks = ACTION_RECOVER | ACTION_POST_CHECK;
>   			unbind_rebind(&priv);
> +			recover_and_post_healthcheck(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER;
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK;
>   
>   	igt_subtest_group {
>   		igt_describe("Check if a device believed to be closed can be cleanly unplugged, then restored");
> -		igt_subtest("unplug-rescan")
> +		igt_subtest("unplug-rescan") {
> +			priv.checks = ACTION_RECOVER | ACTION_POST_CHECK;
>   			unplug_rescan(&priv);
> +			recover_and_post_healthcheck(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER;
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK;
>   
>   	igt_subtest_group {
>   		igt_describe("Check if the driver can be cleanly unbound from an open device, then released and rebound");
> -		igt_subtest("hotunbind-rebind")
> +		igt_subtest("hotunbind-rebind") {
> +			priv.checks = ACTION_RECOVER | ACTION_POST_CHECK;
>   			hotunbind_rebind(&priv);
> +			recover_and_post_healthcheck(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER;
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK;
>   
>   	igt_subtest_group {
>   		igt_describe("Check if an open device can be cleanly unplugged, then released and restored");
> -		igt_subtest("hotunplug-rescan")
> +		igt_subtest("hotunplug-rescan") {
> +			priv.checks = ACTION_RECOVER | ACTION_POST_CHECK;
>   			hotunplug_rescan(&priv);
> +			recover_and_post_healthcheck(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER;
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK;
>   
>   	igt_subtest_group {
>   		igt_describe("Check if the driver can be cleanly rebound to a device with a still open hot unbound driver instance");
> -		igt_subtest("hotrebind")
> +		igt_subtest("hotrebind") {
> +			priv.checks = ACTION_RECOVER | ACTION_POST_CHECK;
>   			hotrebind(&priv);
> +			recover_and_post_healthcheck(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER;
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK;
>   
>   	igt_subtest_group {
>   		igt_describe("Check if a hot unplugged and still open device can be cleanly restored");
> -		igt_subtest("hotreplug")
> +		igt_subtest("hotreplug") {
> +			priv.checks = ACTION_RECOVER | ACTION_POST_CHECK;
>   			hotreplug(&priv);
> +			recover_and_post_healthcheck(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER;
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK;
>   
>   	igt_subtest_group {
>   		igt_describe("Check if a hot unbound driver instance still open after hot rebind can be cleanly released");
> -		igt_subtest("hotrebind-lateclose")
> +		igt_subtest("hotrebind-lateclose") {
> +			priv.checks = ACTION_RECOVER | ACTION_POST_CHECK;
>   			hotrebind_lateclose(&priv);
> +			recover_and_post_healthcheck(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER;
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK;
>   
>   	igt_subtest_group {
>   		igt_describe("Check if an instance of a still open while hot replugged device can be cleanly released");
> -		igt_subtest("hotreplug-lateclose")
> +		igt_subtest("hotreplug-lateclose") {
> +			priv.checks = ACTION_RECOVER | ACTION_POST_CHECK;
>   			hotreplug_lateclose(&priv);
> +			recover_and_post_healthcheck(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER;
>   	}
>   
> -	igt_fixture {
> -		post_healthcheck(&priv);
> +	POST_CHECK;
>   
> +	igt_fixture {
>   		igt_ignore_warn(close(priv.fd.sysfs_bus));
>   		igt_ignore_warn(close(priv.fd.sysfs_drv));
>   	}