[PATCH i-g-t] tests/device_reset: move later skips into subtests

[PATCH i-g-t] tests/device_reset: move later skips into subtests

[Kamil Konieczny]

It is safe to use skips just after igt_main() for skipping all
subtests but later on they produce logs and even if CI will
ignore that, such logs can mislead developers reading them.

Move a skip from middle of igt_main() code block into subtests
which needed them.

Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
Cc: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 tests/device_reset.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/tests/device_reset.c b/tests/device_reset.c
index 583a59039..9b3b8ee6c 100644
--- a/tests/device_reset.c
+++ b/tests/device_reset.c
@@ -424,22 +424,25 @@ igt_main
 		healthcheck(&dev);
 	}
 
-	igt_subtest_group {
-		igt_fixture {
-			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
-				      "pcie slot for cold reset\n");
-			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
-		}
+#define SKIP_IF_NO_COLD_RESET(slot_dir) \
+	do { \
+		igt_skip_on_f(slot_dir < 0, "Gfx Card does not support any " \
+			      "pcie slot for cold reset\n"); \
+		igt_skip_on(!is_sysfs_cold_reset_supported(slot_dir)); \
+	} while(0)
 
+	igt_subtest_group {
 		igt_describe("Unbinds driver from device, initiates cold reset"
 			     " then rebinds driver to device");
 		igt_subtest("unbind-cold-reset-rebind") {
+			SKIP_IF_NO_COLD_RESET(dev.fds.slot_dir);
 			unbind_reset_rebind(&dev, COLD_RESET);
 			healthcheck(&dev);
 		}
 
 		igt_describe("Cold Resets device with bound driver");
 		igt_subtest("cold-reset-bound") {
+			SKIP_IF_NO_COLD_RESET(dev.fds.slot_dir);
 			initiate_device_reset(&dev, COLD_RESET);
 			/*
 			 * Cold reset will initiate card boot sequence again,
-- 
2.42.0

Re: [PATCH i-g-t] tests/device_reset: move later skips into subtests

[Zbigniew Kempczyński]

On Thu, Apr 04, 2024 at 05:41:34PM +0200, Kamil Konieczny wrote:
> It is safe to use skips just after igt_main() for skipping all
> subtests but later on they produce logs and even if CI will
> ignore that, such logs can mislead developers reading them.
> 
> Move a skip from middle of igt_main() code block into subtests
> which needed them.
> 
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> Cc: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  tests/device_reset.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> index 583a59039..9b3b8ee6c 100644
> --- a/tests/device_reset.c
> +++ b/tests/device_reset.c
> @@ -424,22 +424,25 @@ igt_main
>  		healthcheck(&dev);
>  	}
>  
> -	igt_subtest_group {
> -		igt_fixture {
> -			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
> -				      "pcie slot for cold reset\n");
> -			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> -		}
> +#define SKIP_IF_NO_COLD_RESET(slot_dir) \
> +	do { \
> +		igt_skip_on_f(slot_dir < 0, "Gfx Card does not support any " \
> +			      "pcie slot for cold reset\n"); \
> +		igt_skip_on(!is_sysfs_cold_reset_supported(slot_dir)); \
> +	} while(0)
>  
> +	igt_subtest_group {
>  		igt_describe("Unbinds driver from device, initiates cold reset"
>  			     " then rebinds driver to device");
>  		igt_subtest("unbind-cold-reset-rebind") {
> +			SKIP_IF_NO_COLD_RESET(dev.fds.slot_dir);

Why not to introduce:

igt_require_cold_reset();

?

--
Zbigniew

>  			unbind_reset_rebind(&dev, COLD_RESET);
>  			healthcheck(&dev);
>  		}
>  
>  		igt_describe("Cold Resets device with bound driver");
>  		igt_subtest("cold-reset-bound") {
> +			SKIP_IF_NO_COLD_RESET(dev.fds.slot_dir);
>  			initiate_device_reset(&dev, COLD_RESET);
>  			/*
>  			 * Cold reset will initiate card boot sequence again,
> -- 
> 2.42.0
> 

[PATCH i-g-t] tests/device_reset: move later skips into subtests

[Kamil Konieczny]

It is safe to use skips just after igt_main() for skipping all
subtests but later on they produce logs and even if CI will
ignore that, such logs can mislead developers reading them.

Move a skip from middle of igt_main() code block into subtests
which needed them.

v2: add space after while (Kamil)

Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
Cc: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 tests/device_reset.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/tests/device_reset.c b/tests/device_reset.c
index 583a59039..de9e9d8c9 100644
--- a/tests/device_reset.c
+++ b/tests/device_reset.c
@@ -424,22 +424,25 @@ igt_main
 		healthcheck(&dev);
 	}
 
-	igt_subtest_group {
-		igt_fixture {
-			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
-				      "pcie slot for cold reset\n");
-			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
-		}
+#define SKIP_IF_NO_COLD_RESET(slot_dir) \
+	do { \
+		igt_skip_on_f(slot_dir < 0, "Gfx Card does not support any " \
+			      "pcie slot for cold reset\n"); \
+		igt_skip_on(!is_sysfs_cold_reset_supported(slot_dir)); \
+	} while (0)
 
+	igt_subtest_group {
 		igt_describe("Unbinds driver from device, initiates cold reset"
 			     " then rebinds driver to device");
 		igt_subtest("unbind-cold-reset-rebind") {
+			SKIP_IF_NO_COLD_RESET(dev.fds.slot_dir);
 			unbind_reset_rebind(&dev, COLD_RESET);
 			healthcheck(&dev);
 		}
 
 		igt_describe("Cold Resets device with bound driver");
 		igt_subtest("cold-reset-bound") {
+			SKIP_IF_NO_COLD_RESET(dev.fds.slot_dir);
 			initiate_device_reset(&dev, COLD_RESET);
 			/*
 			 * Cold reset will initiate card boot sequence again,
-- 
2.42.0

[PATCH i-g-t] tests/device_reset: move later skips into subtests

[Kamil Konieczny]

It is safe to use skips just after igt_main() for skipping all
subtests but later on they produce logs and even if CI will
ignore that, such logs can mislead developers reading them.

Move a skip from middle of igt_main() code block into subtests
which needed them.

v2: add space after while (Kamil)
v3: remove macro in favor of function (Zbigniew)

Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
Cc: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
Cc: "Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>
Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 tests/device_reset.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/tests/device_reset.c b/tests/device_reset.c
index 583a59039..09558ff20 100644
--- a/tests/device_reset.c
+++ b/tests/device_reset.c
@@ -299,6 +299,29 @@ static bool is_sysfs_cold_reset_supported(int slot_fd)
 	return true;
 }
 
+/**
+ * cold_reset_not_supported:
+ * @fd: opened sysfs pci slot descriptor
+ *
+ * Check if device supports cold reset based on slot dir or sysfs file presence.
+ *
+ * Returns:
+ * NULL if device supports cold reset, otherwise a string description
+ */
+static const char *cold_reset_not_supported(int slot_dir)
+{
+	static const char *no_slot = "Gfx Card does not support any pcie slot for cold reset";
+	static const char *not_supported = "Gfx Card does not support cold reset";
+
+	if (slot_dir < 0)
+		return no_slot;
+
+	if (!is_sysfs_cold_reset_supported(slot_dir))
+		return not_supported;
+
+	return NULL;
+}
+
 /* Unbind the driver from the device */
 static void driver_unbind(struct device_fds *dev)
 {
@@ -424,22 +447,23 @@ igt_main
 		healthcheck(&dev);
 	}
 
-	igt_subtest_group {
-		igt_fixture {
-			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
-				      "pcie slot for cold reset\n");
-			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
-		}
 
+	igt_subtest_group {
 		igt_describe("Unbinds driver from device, initiates cold reset"
 			     " then rebinds driver to device");
 		igt_subtest("unbind-cold-reset-rebind") {
+			const char *reason = cold_reset_not_supported(dev.fds.slot_dir);
+
+			igt_skip_on_f(reason, "%s\n", reason);
 			unbind_reset_rebind(&dev, COLD_RESET);
 			healthcheck(&dev);
 		}
 
 		igt_describe("Cold Resets device with bound driver");
 		igt_subtest("cold-reset-bound") {
+			const char *reason = cold_reset_not_supported(dev.fds.slot_dir);
+
+			igt_skip_on_f(reason, "%s\n", reason);
 			initiate_device_reset(&dev, COLD_RESET);
 			/*
 			 * Cold reset will initiate card boot sequence again,
-- 
2.42.0

Re: [PATCH i-g-t] tests/device_reset: move later skips into subtests

[Zbigniew Kempczyński]

On Thu, Apr 25, 2024 at 04:05:44PM +0200, Kamil Konieczny wrote:
> It is safe to use skips just after igt_main() for skipping all
> subtests but later on they produce logs and even if CI will
> ignore that, such logs can mislead developers reading them.
> 
> Move a skip from middle of igt_main() code block into subtests
> which needed them.
> 
> v2: add space after while (Kamil)
> v3: remove macro in favor of function (Zbigniew)
> 
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> Cc: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
> Cc: "Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  tests/device_reset.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> index 583a59039..09558ff20 100644
> --- a/tests/device_reset.c
> +++ b/tests/device_reset.c
> @@ -299,6 +299,29 @@ static bool is_sysfs_cold_reset_supported(int slot_fd)
>  	return true;
>  }
>  
> +/**
> + * cold_reset_not_supported:
> + * @fd: opened sysfs pci slot descriptor
> + *
> + * Check if device supports cold reset based on slot dir or sysfs file presence.
> + *
> + * Returns:
> + * NULL if device supports cold reset, otherwise a string description
> + */
> +static const char *cold_reset_not_supported(int slot_dir)
> +{
> +	static const char *no_slot = "Gfx Card does not support any pcie slot for cold reset";
> +	static const char *not_supported = "Gfx Card does not support cold reset";
> +
> +	if (slot_dir < 0)
> +		return no_slot;
> +
> +	if (!is_sysfs_cold_reset_supported(slot_dir))
> +		return not_supported;
> +
> +	return NULL;
> +}
> +
>  /* Unbind the driver from the device */
>  static void driver_unbind(struct device_fds *dev)
>  {
> @@ -424,22 +447,23 @@ igt_main
>  		healthcheck(&dev);
>  	}
>  
> -	igt_subtest_group {
> -		igt_fixture {
> -			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
> -				      "pcie slot for cold reset\n");
> -			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> -		}
>  
> +	igt_subtest_group {
>  		igt_describe("Unbinds driver from device, initiates cold reset"
>  			     " then rebinds driver to device");
>  		igt_subtest("unbind-cold-reset-rebind") {
> +			const char *reason = cold_reset_not_supported(dev.fds.slot_dir);
> +
> +			igt_skip_on_f(reason, "%s\n", reason);
>  			unbind_reset_rebind(&dev, COLD_RESET);
>  			healthcheck(&dev);
>  		}
>  
>  		igt_describe("Cold Resets device with bound driver");
>  		igt_subtest("cold-reset-bound") {
> +			const char *reason = cold_reset_not_supported(dev.fds.slot_dir);
> +
> +			igt_skip_on_f(reason, "%s\n", reason);
>  			initiate_device_reset(&dev, COLD_RESET);
>  			/*
>  			 * Cold reset will initiate card boot sequence again,
> -- 
> 2.42.0
> 

I understand you want to be verbose when cold reset is not supported
and what's the reason. At the first glance cold_reset_not_supported()
is a little bit confusing (returns NULL in case it is fine). But at
least assignment is descriptive (reason = ...).

So:

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
--
Zbigniew