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

From: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
To: igt-dev@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org,
	"Michał Winiarski" <michal.winiarski@intel.com>
Subject: [Intel-gfx] [PATCH i-g-t v2 09/16] tests/core_hotunplug: Prepare invariant data once per test run
Date: Fri,  7 Aug 2020 11:19:55 +0200	[thread overview]
Message-ID: <20200807092002.32350-10-janusz.krzysztofik@linux.intel.com> (raw)
In-Reply-To: <20200807092002.32350-1-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 lifecycle 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' subnode 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 on current upstream master.

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>
---
 tests/core_hotunplug.c | 91 ++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 56 deletions(-)

diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
index ffba32568..71ac4d169 100644
--- a/tests/core_hotunplug.c
+++ b/tests/core_hotunplug.c
@@ -59,46 +59,33 @@ static int local_close(int fd)
 	return -1;	/* success - return 'closed' */
 }
 
-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);
+	priv->dev_bus_addr = strrchr(filter, '/');
+	igt_assert(priv->dev_bus_addr++);
+
+	sysfs_path = strchr(filter, ':');
+	igt_assert(sysfs_path++);
+
+	priv->fd.sysfs_dev = open(sysfs_path, O_DIRECTORY);
+	igt_assert_fd(priv->fd.sysfs_dev);
+
+	priv->fd.sysfs_drv = openat(priv->fd.sysfs_dev, "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, '/');
-	igt_assert(priv->dev_bus_addr++);
+	priv->fd.sysfs_bus = openat(priv->fd.sysfs_dev, "subsystem/devices",
+				    O_DIRECTORY);
+	igt_assert_fd(priv->fd.sysfs_bus);
 
-	/* sysfs_dev no longer needed */
 	priv->fd.sysfs_dev = local_close(priv->fd.sysfs_dev);
 	igt_assert_f(priv->fd.sysfs_dev == -1,
 		     "Device sysfs node close failed\n");
 }
 
-static void prepare(struct hotunplug *priv, char *buf, int buflen)
-{
-	igt_debug("opening device\n");
-	priv->fd.drm = __drm_open_driver(DRIVER_ANY);
-	igt_assert_fd(priv->fd.drm);
-
-	priv->fd.sysfs_dev = igt_sysfs_open(priv->fd.drm);
-	igt_assert_fd(priv->fd.sysfs_dev);
-
-	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);
-	}
-}
-
 /* Unbind the driver from the device */
 static void driver_unbind(struct hotunplug *priv, const char *prefix)
 {
@@ -109,8 +96,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 */
@@ -123,18 +108,20 @@ 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)
 {
+	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;
 
@@ -150,11 +137,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)
@@ -209,14 +194,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));
-
-	igt_debug("closing the device\n");
-	priv->fd.drm = local_close(priv->fd.drm);
-	igt_assert_f(priv->fd.drm == -1, "Device close failed\n");
-
 	driver_unbind(priv, "");
 
 	driver_bind(priv);
@@ -226,12 +203,6 @@ static void unbind_rebind(struct hotunplug *priv)
 
 static void unplug_rescan(struct hotunplug *priv)
 {
-	prepare(priv, NULL, 0);
-
-	igt_debug("closing the device\n");
-	priv->fd.drm = local_close(priv->fd.drm);
-	igt_assert_f(priv->fd.drm == -1, "Device close failed\n");
-
 	device_unplug(priv, "");
 
 	bus_rescan(priv);
@@ -241,9 +212,9 @@ static void unplug_rescan(struct hotunplug *priv)
 
 static void hotunbind_lateclose(struct hotunplug *priv)
 {
-	char buf[PATH_MAX];
-
-	prepare(priv, buf, sizeof(buf));
+	igt_debug("opening device\n");
+	priv->fd.drm = __drm_open_driver(DRIVER_ANY);
+	igt_assert_fd(priv->fd.drm);
 
 	driver_unbind(priv, "hot ");
 
@@ -258,7 +229,9 @@ static void hotunbind_lateclose(struct hotunplug *priv)
 
 static void hotunplug_lateclose(struct hotunplug *priv)
 {
-	prepare(priv, NULL, 0);
+	igt_debug("opening device\n");
+	priv->fd.drm = __drm_open_driver(DRIVER_ANY);
+	igt_assert_fd(priv->fd.drm);
 
 	device_unplug(priv, "hot ");
 
@@ -298,6 +271,8 @@ igt_main
 		set_filter_from_device(fd_drm);
 
 		igt_fail_on_f(close(fd_drm), "Device close failed\n");
+
+		prepare(&priv);
 	}
 
 	igt_describe("Check if the driver can be cleanly unbound from a device believed to be closed");
@@ -325,6 +300,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
