Re: [PATCH i-g-t v2 7/8] tests/core_hotunplug: Check health once after subtest

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

[-- Attachment #1: Type: text/plain, Size: 6545 bytes --]



On 05.08.2024 16:37, 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.
>
> Lets try to lower number of checks to those really needed and
> do them only once per fixture, which should make reading logs
> easier.
>
> v2: drop new var from test priv structure, update subject and
>    description, change second macro parameter name to 'y'
>
> Signed-off-by: Kamil Konieczny<kamil.konieczny@linux.intel.com>
> ---
>   tests/core_hotunplug.c | 108 +++++++++++++++++++++++++----------------
>   1 file changed, 67 insertions(+), 41 deletions(-)
>
> diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
> index 7f17f4423..51edee985 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)
> +

Not used anymore.

>   struct hotunplug {
>   	struct {
>   		int drm;
> @@ -515,7 +518,6 @@ static void recover(struct hotunplug *priv)
>   	bool late_close = priv->fd.drm >= 0;
>   
>   	cleanup(priv);
> -
>   	if (!priv->failure && late_close)
>   		igt_ignore_warn(healthcheck(priv, false));
>   
> @@ -671,6 +673,19 @@ static void hotreplug_lateclose(struct hotunplug *priv)
>   }
>   
>   /* Main */
> +#define RECOVER(x) igt_fixture { \
> +		if (x) { \
> +			x = false; \

Do we need this assignment?

> +			recover(&priv); \
> +		} \
> +	}
> +
> +#define POST_CHECK(y) igt_fixture { \
> +		if (y) { \
> +			y = false; \
ditto
> +			post_healthcheck(&priv); \
> +		} \
> +	}
>   
>   igt_main
>   {
> @@ -682,6 +697,8 @@ igt_main
>   		.snd_driver	= NULL,
>   		.chipset	= DRIVER_ANY,
>   	};
> +	bool needs_recover = false;
> +	bool needs_post_check = false;
>   
>   	igt_fixture {
>   		int fd_drm;
> @@ -717,100 +734,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") {
> +			needs_recover = true;
> +			needs_post_check = true;
>   			unbind_rebind(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER(needs_recover);
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK(needs_post_check);

Why recover is in subtest_group and post_check outside, here and below?
Why not call explicitly recover/post_check after every test, ex:
igt_subtest("unbind-rebind") {
      unbind_rebind(&priv);
      recover(..);
      post_check();
}
Or even call recover/post_check from inside unbind_rebind ?

This dance with extra vars and checks seems strange.

IIRC I have proposed previously to embed recover/post_check into 
cleanup, which should be called after every test.
And recover/post_check could be conditionally called based on some flags 
present in priv.

Regards
Andrzej

>   
>   	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") {
> +			needs_recover = true;
> +			needs_post_check = true;
>   			unplug_rescan(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER(needs_recover);
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK(needs_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") {
> +			needs_recover = true;
> +			needs_post_check = true;
>   			hotunbind_rebind(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER(needs_recover);
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK(needs_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") {
> +			needs_recover = true;
> +			needs_post_check = true;
>   			hotunplug_rescan(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER(needs_recover);
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK(needs_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") {
> +			needs_recover = true;
> +			needs_post_check = true;
>   			hotrebind(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER(needs_recover);
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK(needs_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") {
> +			needs_recover = true;
> +			needs_post_check = true;
>   			hotreplug(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER(needs_recover);
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK(needs_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") {
> +			needs_recover = true;
> +			needs_post_check = true;
>   			hotrebind_lateclose(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER(needs_recover);
>   	}
>   
> -	igt_fixture
> -		post_healthcheck(&priv);
> +	POST_CHECK(needs_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") {
> +			needs_recover = true;
> +			needs_post_check = true;
>   			hotreplug_lateclose(&priv);
> +		}
>   
> -		igt_fixture
> -			recover(&priv);
> +		RECOVER(needs_recover);
>   	}
>   
>   	igt_fixture {
> -		post_healthcheck(&priv);
> -
> +		if (needs_post_check)
> +			post_healthcheck(&priv);
>   		igt_ignore_warn(close(priv.fd.sysfs_bus));
>   		igt_ignore_warn(close(priv.fd.sysfs_drv));
>   	}

[-- Attachment #2: Type: text/html, Size: 7768 bytes --]