[PATCH i-g-t] tests/device_reset: move later skips into subtests
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Fri Apr 26 16:35:53 UTC 2024
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 at linux.intel.com>
> Cc: Sai Gowtham Ch <sai.gowtham.ch at intel.com>
> Cc: "Zbigniew Kempczyński" <zbigniew.kempczynski at intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny at 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 at intel.com>
--
Zbigniew
More information about the igt-dev
mailing list