Re: [igt-dev] [PATCH i-g-t] tests/core_hotunplug: Check sysfs reads after hotunplug

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 13 May 2022 02:31:28 -0700, Tvrtko Ursulin wrote:
>
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Leave some sysfs files open and attempt to read from them after hotunplug.

A few minor comments below but otherwise LGTM:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  tests/core_hotunplug.c | 62 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
> index 02eae19e1e16..c3831541202d 100644
> --- a/tests/core_hotunplug.c
> +++ b/tests/core_hotunplug.c
> @@ -602,6 +602,59 @@ static void hotreplug_lateclose(struct hotunplug *priv)
>	igt_assert_f(healthcheck(priv, false), "%s\n", priv->failure);
>  }
>
> +static void hotunplug_sysfs(struct hotunplug *priv)
> +{
> +	int i915, sysfs;
> +	struct {
> +		int fd;
> +		const char *path;
> +		char buf[256];
> +	} *item, items[] = {
> +		{ .fd = -1, .path = "error" },
> +		{ .fd = -1, .path = "gt_act_freq_mhz" },
> +		{ .fd = -1, .path = "gt/gt0/rps_act_freq_mhz" },
> +		{ .fd = -1, .path = "gt/gt0/rc6_residency_ms" },
> +		{ .fd = -1, .path = "engine/rcs0/name" },
> +		{ .fd = -1, .path = "engine/bcs0/name" },
> +		{ .path = NULL },
> +	};
> +	unsigned int cnt = 0;
> +
> +	pre_check(priv);
> +
> +	i915 = local_drm_open_driver(false, "", " for hot unplug");
> +	sysfs = igt_sysfs_open(i915);
> +	igt_require(sysfs >= 0);
> +	for (item = &items[0]; item->path; item++) {
> +		item->fd = openat(sysfs, item->path, O_RDONLY);
> +		cnt += item->fd >= 0;
> +	}
> +	close(sysfs);
> +	close_device(i915, " ", "sysfs open ");
> +
> +	igt_require(cnt >= 3);

igt_require(cnt) ?

> +
> +	device_unplug(priv, "hot ", 60);

Not sure what happens when there are child devices etc. Anyway it's being
used elsewhere too so probably ok.

Also maybe unbind (instead of remove) using driver_unbind() is sufficient?

> +
> +	sleep(2);

Device remove via sysfs:
	echo 1 > /sys/bus/pci/devices/0000\:03\:00.0/remove
is most likely synchronous but anyway...

> +
> +	for (item = &items[0]; item->path; item++) {
> +		ssize_t len;
> +		int err;
> +
> +		if (item->fd < 0)
> +			continue;
> +
> +		len = read(item->fd, item->buf, sizeof(item->buf));
> +		err = len < 0 ? -errno : 0;
> +		close(item->fd);
> +		igt_debug("%s='%s' (%d)\n", item->path, item->buf, err);
> +		igt_assert_eq(err, -ENODEV);
> +	}
> +
> +	igt_assert_f(healthcheck(priv, false), "%s\n", priv->failure);
> +}
> +
>  /* Main */
>
>  igt_main
> @@ -735,6 +788,15 @@ igt_main
>			recover(&priv);
>	}
>
> +	igt_subtest_group {
> +		igt_describe("Check if sysfs files left open can be safely accessed after hotunplug.");
> +		igt_subtest("hotunplug-sysfs")
> +			hotunplug_sysfs(&priv);
> +
> +		igt_fixture
> +			recover(&priv);
> +	}
> +
>	igt_fixture {
>		post_healthcheck(&priv);
>
> --
> 2.32.0
>