[Intel-gfx] [PATCH i-g-t v6 09/24] tests/core_hotunplug: Prepare invariant data once per test run

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

Each subtest now calls a prepare() helper which opens a couple of files
required by that subtest.  Those files are then closed after use,
either directly from the subtest body, or indirectly from inside one of
helper functions called during the subtest execution.  That approach
not only makes life cycle of individual file descriptors difficult to
follow but also prevents us from re-running health checks on subtest
failures from follow up igt_fixture sections since we may need to retry
bus rescan or driver rebind operations.

Two of those files - device bus and driver sysfs nodes - are not
affected nor interfere with driver unbind / device unplug operations
performed by subtests.  Then, there is not much sense in closing and
reopening those nodes.  Open them once at the beginning of a test run,
then close them as late as on test completion.

The prepare() helper also populates a device bus address string used by
driver unbind / rebind operations.  Since the bus address of an
exercised device never changes, also prepare that string only once at
the beginning of a test run.  Note that it is the same as the last
component of a device filter string which is already resolved and
installed from an initial igt_fixture section of the test.  Then,
initialize the device bus address field of a hotunplug structure
instance with a pointer to the respective substring of that filter
rather than resolving it again from the device sysfs node pathname.

There is one more sysfs node - a DRM device node - now opened by the
prepare() helper for subtests which perform device remove operations.
That node can't be opened only once at the beginning of a test run
because its open file descriptor is no longer usable as soon as a
driver unbind operation is performed.  On the other hand, it can't be
opened easily from inside a device_remove() helper since some subtests
just don't open the device so its file descriptor used by
igt_sysfs_open() may just not be available.  However, note that only a
PCI sysfs node of the device, not necessarily the DRM one, is actually
required for a successful device remove operation, and that node can be
opened easily from a bus file descriptor using a device bus address
string, both already available.  Then, change the semantics of a
.fd.sysfs_dev field of the hotunplug structure from DRM to PCI device
sysfs file descriptor, then let the device_remove() helper open the
device PCI node by itself and store its file descriptor in that field.
Also, for still more easy access to the device PCI node, use a
'subsystem/devices' sub-node of the PCI device as its bus sysfs
location instead of just 'subsystem', then adjust a relative path to
the bus 'rescan' function accordingly.

A side benefit of using the PCI device sysfs node, not the DRM one,
while removing the device is that a future subtest may now easily
perform both driver unbind and device remove operations in a row.

v2: Rebase only.
v3: Refresh.
v4: Refresh,
    still assert a device dile descriptor closed cleanly on subtest
    start, a device sysfs file descriptor still before open.

Suggested-by: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> # v1
---
 tests/core_hotunplug.c | 83 +++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 50 deletions(-)

diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
index 51de942ba..7f5e800c6 100644
--- a/tests/core_hotunplug.c
+++ b/tests/core_hotunplug.c
@@ -87,45 +87,31 @@ static int close_sysfs(int fd_sysfs_dev)
 	return local_close(fd_sysfs_dev, "Device sysfs node close failed");
 }
 
-static void prepare_for_unbind(struct hotunplug *priv, char *buf, int buflen)
+static void prepare(struct hotunplug *priv)
 {
-	int len;
+	const char *filter = igt_device_filter_get(0), *sysfs_path;
 
-	igt_assert(buflen);
+	igt_assert(filter);
 
-	priv->fd.sysfs_drv = openat(priv->fd.sysfs_dev, "device/driver",
-				    O_DIRECTORY);
-	igt_assert_fd(priv->fd.sysfs_drv);
-
-	len = readlinkat(priv->fd.sysfs_dev, "device", buf, buflen - 1);
-	buf[len] = '\0';
-	priv->dev_bus_addr = strrchr(buf, '/');
+	priv->dev_bus_addr = strrchr(filter, '/');
 	igt_assert(priv->dev_bus_addr++);
 
-	/* sysfs_dev no longer needed */
-	priv->fd.sysfs_dev = close_sysfs(priv->fd.sysfs_dev);
-	igt_assert_eq(priv->fd.sysfs_dev, -1);
-}
+	sysfs_path = strchr(filter, ':');
+	igt_assert(sysfs_path++);
 
-static void prepare(struct hotunplug *priv, char *buf, int buflen)
-{
-	/* assert device file descriptors closed cleanly on subtest start */
-	igt_assert_eq(priv->fd.drm, -1);
 	igt_assert_eq(priv->fd.sysfs_dev, -1);
+	priv->fd.sysfs_dev = open(sysfs_path, O_DIRECTORY);
+	igt_assert_fd(priv->fd.sysfs_dev);
 
-	priv->fd.drm = local_drm_open_driver("", " for subtest");
+	priv->fd.sysfs_drv = openat(priv->fd.sysfs_dev, "driver", O_DIRECTORY);
+	igt_assert_fd(priv->fd.sysfs_drv);
 
-	priv->fd.sysfs_dev = igt_sysfs_open(priv->fd.drm);
-	igt_assert_fd(priv->fd.sysfs_dev);
+	priv->fd.sysfs_bus = openat(priv->fd.sysfs_dev, "subsystem/devices",
+				    O_DIRECTORY);
+	igt_assert_fd(priv->fd.sysfs_bus);
 
-	if (buf) {
-		prepare_for_unbind(priv, buf, buflen);
-	} else {
-		/* prepare for bus rescan */
-		priv->fd.sysfs_bus = openat(priv->fd.sysfs_dev,
-					    "device/subsystem", O_DIRECTORY);
-		igt_assert_fd(priv->fd.sysfs_bus);
-	}
+	priv->fd.sysfs_dev = close_sysfs(priv->fd.sysfs_dev);
+	igt_assert_eq(priv->fd.sysfs_dev, -1);
 }
 
 /* Unbind the driver from the device */
@@ -138,8 +124,6 @@ static void driver_unbind(struct hotunplug *priv, const char *prefix)
 	igt_sysfs_set(priv->fd.sysfs_drv, "unbind", priv->dev_bus_addr);
 	igt_reset_timeout();
 	priv->failure = NULL;
-
-	/* don't close fd.sysfs_drv, it will be used for driver rebinding */
 }
 
 /* Re-bind the driver to the device */
@@ -152,18 +136,21 @@ static void driver_bind(struct hotunplug *priv)
 	igt_sysfs_set(priv->fd.sysfs_drv, "bind", priv->dev_bus_addr);
 	igt_reset_timeout();
 	priv->failure = NULL;
-
-	close(priv->fd.sysfs_drv);
 }
 
 /* Remove (virtually unplug) the device from its bus */
 static void device_unplug(struct hotunplug *priv, const char *prefix)
 {
+	igt_assert_eq(priv->fd.sysfs_dev, -1);
+	priv->fd.sysfs_dev = openat(priv->fd.sysfs_bus, priv->dev_bus_addr,
+				    O_DIRECTORY);
+	igt_assert_fd(priv->fd.sysfs_dev);
+
 	igt_debug("%sunplugging the device\n", prefix);
 
 	priv->failure = "Device unplug timeout!";
 	igt_set_timeout(60, priv->failure);
-	igt_sysfs_set(priv->fd.sysfs_dev, "device/remove", "1");
+	igt_sysfs_set(priv->fd.sysfs_dev, "remove", "1");
 	igt_reset_timeout();
 	priv->failure = NULL;
 
@@ -177,11 +164,9 @@ static void bus_rescan(struct hotunplug *priv)
 
 	priv->failure = "Bus rescan timeout!";
 	igt_set_timeout(60, priv->failure);
-	igt_sysfs_set(priv->fd.sysfs_bus, "rescan", "1");
+	igt_sysfs_set(priv->fd.sysfs_bus, "../rescan", "1");
 	igt_reset_timeout();
 	priv->failure = NULL;
-
-	close(priv->fd.sysfs_bus);
 }
 
 static void healthcheck(struct hotunplug *priv)
@@ -237,11 +222,6 @@ static void set_filter_from_device(int fd)
 
 static void unbind_rebind(struct hotunplug *priv)
 {
-	char buf[PATH_MAX];
-
-	prepare(priv, buf, sizeof(buf));
-
-	priv->fd.drm = close_device(priv->fd.drm, "", "exercised ");
 	igt_assert_eq(priv->fd.drm, -1);
 
 	driver_unbind(priv, "");
@@ -253,9 +233,6 @@ static void unbind_rebind(struct hotunplug *priv)
 
 static void unplug_rescan(struct hotunplug *priv)
 {
-	prepare(priv, NULL, 0);
-
-	priv->fd.drm = close_device(priv->fd.drm, "", "exercised ");
 	igt_assert_eq(priv->fd.drm, -1);
 
 	device_unplug(priv, "");
@@ -267,9 +244,8 @@ static void unplug_rescan(struct hotunplug *priv)
 
 static void hotunbind_lateclose(struct hotunplug *priv)
 {
-	char buf[PATH_MAX];
-
-	prepare(priv, buf, sizeof(buf));
+	igt_assert_eq(priv->fd.drm, -1);
+	priv->fd.drm = local_drm_open_driver("", " for hot unbind");
 
 	driver_unbind(priv, "hot ");
 
@@ -282,7 +258,8 @@ static void hotunbind_lateclose(struct hotunplug *priv)
 
 static void hotunplug_lateclose(struct hotunplug *priv)
 {
-	prepare(priv, NULL, 0);
+	igt_assert_eq(priv->fd.drm, -1);
+	priv->fd.drm = local_drm_open_driver("", " for hot unplug");
 
 	device_unplug(priv, "hot ");
 
@@ -317,6 +294,8 @@ igt_main
 		set_filter_from_device(fd_drm);
 
 		igt_assert_eq(close_device(fd_drm, "", "selected "), -1);
+
+		prepare(&priv);
 	}
 
 	igt_describe("Check if the driver can be cleanly unbound from a device believed to be closed");
@@ -344,6 +323,10 @@ igt_main
 	igt_subtest("hotunplug-lateclose")
 		hotunplug_lateclose(&priv);
 
-	igt_fixture
+	igt_fixture {
 		post_healthcheck(&priv);
+
+		close(priv.fd.sysfs_bus);
+		close(priv.fd.sysfs_drv);
+	}
 }
-- 
2.21.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx