diff mbox series

[RFC,i-g-t,v2,2/8] tests/core_hotunplug: Use PCI device sysfs entry, not DRM

Message ID 20200622164415.30352-3-janusz.krzysztofik@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series tests/core_hotunplug: New subtests and enhancements | expand

Commit Message

Janusz Krzysztofik June 22, 2020, 4:44 p.m. UTC 
Future subtests may want to access PCI attributes of the device after
driver unbind.  Refactor prepare() helper.

v2: rebase on upstream

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
---
 tests/core_hotunplug.c | 68 +++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 28 deletions(-)

Comments

Michał Winiarski June 25, 2020, 7:23 p.m. UTC | #1 
Quoting Janusz Krzysztofik (2020-06-22 18:44:09)
> Future subtests may want to access PCI attributes of the device after
> driver unbind.  Refactor prepare() helper.
> 
> v2: rebase on upstream
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> ---
>  tests/core_hotunplug.c | 68 +++++++++++++++++++++++++-----------------
>  1 file changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
> index 826645b1f..35eba9b8a 100644
> --- a/tests/core_hotunplug.c
> +++ b/tests/core_hotunplug.c
> @@ -55,42 +55,54 @@ struct hotunplug {
>         igt_kmsg(KMSG_DEBUG "%s: %s: %s\n", igt_test_name(), __func__, msg); \
>  })
>  
> -static void prepare_for_unbind(struct hotunplug *priv, char *buf, int buflen)
> +static inline int prepare_common(struct hotunplug *priv)
>  {
> -       int len;
> +       int fd_sysfs_drm;
> +
> +       local_debug("opening device");
> +       priv->fd.drm = __drm_open_driver(DRIVER_ANY);
> +       igt_assert(priv->fd.drm >= 0);
> +
> +       fd_sysfs_drm = igt_sysfs_open(priv->fd.drm);
> +       igt_assert(fd_sysfs_drm >= 0);
> +
> +       return fd_sysfs_drm;
> +}
> +
> +static inline void prepare_for_rebind(struct hotunplug *priv,
> +                                     char *buf, int buflen)
> +{
> +       int fd_sysfs_drm, len;
>  
>         igt_assert(buflen);
>  
> -       priv->fd.sysfs_drv = openat(priv->fd.sysfs_dev, "device/driver",
> -                                   O_DIRECTORY);
> -       igt_assert(priv->fd.sysfs_drv >= 0);
> +       fd_sysfs_drm = prepare_common(priv);
> +
> +       priv->fd.sysfs_drv = openat(fd_sysfs_drm, "device/driver", O_DIRECTORY);
>  
> -       len = readlinkat(priv->fd.sysfs_dev, "device", buf, buflen - 1);
> +       len = readlinkat(fd_sysfs_drm, "device", buf, buflen - 1);
>         buf[len] = '\0';
>         priv->dev_bus_addr = strrchr(buf, '/');
> -       igt_assert(priv->dev_bus_addr++);
>  
> -       /* sysfs_dev no longer needed */
> -       close(priv->fd.sysfs_dev);
> +       close(fd_sysfs_drm);
> +
> +       igt_assert(priv->fd.sysfs_drv >= 0);
> +       igt_assert(priv->dev_bus_addr++);
>  }
>  
> -static inline void prepare(struct hotunplug *priv, char *buf, int buflen)
> +static inline void prepare_for_rescan(struct hotunplug *priv)
>  {
> -       local_debug("opening device");
> -       priv->fd.drm = __drm_open_driver(DRIVER_ANY);
> -       igt_assert(priv->fd.drm >= 0);
> +       int fd_sysfs_drm = prepare_common(priv);
>  
> -       priv->fd.sysfs_dev = igt_sysfs_open(priv->fd.drm);
> -       igt_assert(priv->fd.sysfs_dev >= 0);
> +       priv->fd.sysfs_dev = openat(fd_sysfs_drm, "device", O_DIRECTORY);
>  
> -       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(priv->fd.sysfs_bus >= 0);
> -       }
> +       priv->fd.sysfs_bus = openat(fd_sysfs_drm, "device/subsystem",
> +                                   O_DIRECTORY);
> +
> +       close(fd_sysfs_drm);
> +
> +       igt_assert(priv->fd.sysfs_dev >= 0);
> +       igt_assert(priv->fd.sysfs_bus >= 0);
>  }

I find the lifecycle of hotunplug.fd.sysfs_* difficult to follow now...
Would it be possible to keep the "prepare" step simpler and just open everything
everytime? (perhaps closing and opening new ones when called multiple times?)
Or do we need to have drv separate from bus/dev?

-Michał

>  
>  static const char *failure;
> @@ -124,7 +136,7 @@ static void device_unplug(int fd_sysfs_dev)
>  {
>         failure = "Device unplug timeout!";
>         igt_set_timeout(60, failure);
> -       igt_sysfs_set(fd_sysfs_dev, "device/remove", "1");
> +       igt_sysfs_set(fd_sysfs_dev, "remove", "1");
>         igt_reset_timeout();
>         failure = NULL;
>  
> @@ -185,7 +197,7 @@ static void unbind_rebind(void)
>         struct hotunplug priv;
>         char buf[PATH_MAX];
>  
> -       prepare(&priv, buf, sizeof(buf));
> +       prepare_for_rebind(&priv, buf, sizeof(buf));
>  
>         local_debug("closing the device");
>         close(priv.fd.drm);
> @@ -203,7 +215,7 @@ static void unplug_rescan(void)
>  {
>         struct hotunplug priv;
>  
> -       prepare(&priv, NULL, 0);
> +       prepare_for_rescan(&priv);
>  
>         local_debug("closing the device");
>         close(priv.fd.drm);
> @@ -222,7 +234,7 @@ static void hotunbind_lateclose(void)
>         struct hotunplug priv;
>         char buf[PATH_MAX];
>  
> -       prepare(&priv, buf, sizeof(buf));
> +       prepare_for_rebind(&priv, buf, sizeof(buf));
>  
>         local_debug("hot unbinding the driver from the device");
>         driver_unbind(priv.fd.sysfs_drv, priv.dev_bus_addr);
> @@ -240,7 +252,7 @@ static void hotunplug_lateclose(void)
>  {
>         struct hotunplug priv;
>  
> -       prepare(&priv, NULL, 0);
> +       prepare_for_rescan(&priv);
>  
>         local_debug("hot unplugging the device");
>         device_unplug(priv.fd.sysfs_dev);
> -- 
> 2.21.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
Janusz Krzysztofik June 26, 2020, 10:20 a.m. UTC | #2 
Hi Michał,

On Thu, 2020-06-25 at 21:23 +0200, Michał Winiarski wrote:
> Quoting Janusz Krzysztofik (2020-06-22 18:44:09)
> > Future subtests may want to access PCI attributes of the device after
> > driver unbind.  Refactor prepare() helper.
> > 
> > v2: rebase on upstream
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > ---
> >  tests/core_hotunplug.c | 68 +++++++++++++++++++++++++-----------------
> >  1 file changed, 40 insertions(+), 28 deletions(-)
> > 
> > diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
> > index 826645b1f..35eba9b8a 100644
> > --- a/tests/core_hotunplug.c
> > +++ b/tests/core_hotunplug.c
> > @@ -55,42 +55,54 @@ struct hotunplug {
> >         igt_kmsg(KMSG_DEBUG "%s: %s: %s\n", igt_test_name(), __func__, msg); \
> >  })
> >  
> > -static void prepare_for_unbind(struct hotunplug *priv, char *buf, int buflen)
> > +static inline int prepare_common(struct hotunplug *priv)
> >  {
> > -       int len;
> > +       int fd_sysfs_drm;
> > +
> > +       local_debug("opening device");
> > +       priv->fd.drm = __drm_open_driver(DRIVER_ANY);
> > +       igt_assert(priv->fd.drm >= 0);
> > +
> > +       fd_sysfs_drm = igt_sysfs_open(priv->fd.drm);
> > +       igt_assert(fd_sysfs_drm >= 0);
> > +
> > +       return fd_sysfs_drm;
> > +}
> > +
> > +static inline void prepare_for_rebind(struct hotunplug *priv,
> > +                                     char *buf, int buflen)
> > +{
> > +       int fd_sysfs_drm, len;
> >  
> >         igt_assert(buflen);
> >  
> > -       priv->fd.sysfs_drv = openat(priv->fd.sysfs_dev, "device/driver",
> > -                                   O_DIRECTORY);
> > -       igt_assert(priv->fd.sysfs_drv >= 0);
> > +       fd_sysfs_drm = prepare_common(priv);
> > +
> > +       priv->fd.sysfs_drv = openat(fd_sysfs_drm, "device/driver", O_DIRECTORY);
> >  
> > -       len = readlinkat(priv->fd.sysfs_dev, "device", buf, buflen - 1);
> > +       len = readlinkat(fd_sysfs_drm, "device", buf, buflen - 1);
> >         buf[len] = '\0';
> >         priv->dev_bus_addr = strrchr(buf, '/');
> > -       igt_assert(priv->dev_bus_addr++);
> >  
> > -       /* sysfs_dev no longer needed */
> > -       close(priv->fd.sysfs_dev);
> > +       close(fd_sysfs_drm);
> > +
> > +       igt_assert(priv->fd.sysfs_drv >= 0);
> > +       igt_assert(priv->dev_bus_addr++);
> >  }
> >  
> > -static inline void prepare(struct hotunplug *priv, char *buf, int buflen)
> > +static inline void prepare_for_rescan(struct hotunplug *priv)
> >  {
> > -       local_debug("opening device");
> > -       priv->fd.drm = __drm_open_driver(DRIVER_ANY);
> > -       igt_assert(priv->fd.drm >= 0);
> > +       int fd_sysfs_drm = prepare_common(priv);
> >  
> > -       priv->fd.sysfs_dev = igt_sysfs_open(priv->fd.drm);
> > -       igt_assert(priv->fd.sysfs_dev >= 0);
> > +       priv->fd.sysfs_dev = openat(fd_sysfs_drm, "device", O_DIRECTORY);
> >  
> > -       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(priv->fd.sysfs_bus >= 0);
> > -       }
> > +       priv->fd.sysfs_bus = openat(fd_sysfs_drm, "device/subsystem",
> > +                                   O_DIRECTORY);
> > +
> > +       close(fd_sysfs_drm);
> > +
> > +       igt_assert(priv->fd.sysfs_dev >= 0);
> > +       igt_assert(priv->fd.sysfs_bus >= 0);
> >  }
> 
> I find the lifecycle of hotunplug.fd.sysfs_* difficult to follow now...
> Would it be possible to keep the "prepare" step simpler and just open everything
> everytime? (perhaps closing and opening new ones when called multiple times?)

OK.

Thanks,
Janusz

> Or do we need to have drv separate from bus/dev?
> 
> -Michał
> 
> >  
> >  static const char *failure;
> > @@ -124,7 +136,7 @@ static void device_unplug(int fd_sysfs_dev)
> >  {
> >         failure = "Device unplug timeout!";
> >         igt_set_timeout(60, failure);
> > -       igt_sysfs_set(fd_sysfs_dev, "device/remove", "1");
> > +       igt_sysfs_set(fd_sysfs_dev, "remove", "1");
> >         igt_reset_timeout();
> >         failure = NULL;
> >  
> > @@ -185,7 +197,7 @@ static void unbind_rebind(void)
> >         struct hotunplug priv;
> >         char buf[PATH_MAX];
> >  
> > -       prepare(&priv, buf, sizeof(buf));
> > +       prepare_for_rebind(&priv, buf, sizeof(buf));
> >  
> >         local_debug("closing the device");
> >         close(priv.fd.drm);
> > @@ -203,7 +215,7 @@ static void unplug_rescan(void)
> >  {
> >         struct hotunplug priv;
> >  
> > -       prepare(&priv, NULL, 0);
> > +       prepare_for_rescan(&priv);
> >  
> >         local_debug("closing the device");
> >         close(priv.fd.drm);
> > @@ -222,7 +234,7 @@ static void hotunbind_lateclose(void)
> >         struct hotunplug priv;
> >         char buf[PATH_MAX];
> >  
> > -       prepare(&priv, buf, sizeof(buf));
> > +       prepare_for_rebind(&priv, buf, sizeof(buf));
> >  
> >         local_debug("hot unbinding the driver from the device");
> >         driver_unbind(priv.fd.sysfs_drv, priv.dev_bus_addr);
> > @@ -240,7 +252,7 @@ static void hotunplug_lateclose(void)
> >  {
> >         struct hotunplug priv;
> >  
> > -       prepare(&priv, NULL, 0);
> > +       prepare_for_rescan(&priv);
> >  
> >         local_debug("hot unplugging the device");
> >         device_unplug(priv.fd.sysfs_dev);
> > -- 
> > 2.21.1
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
diff mbox series

Patch

diff --git a/tests/core_hotunplug.c b/tests/core_hotunplug.c
index 826645b1f..35eba9b8a 100644
--- a/tests/core_hotunplug.c
+++ b/tests/core_hotunplug.c
@@ -55,42 +55,54 @@  struct hotunplug {
 	igt_kmsg(KMSG_DEBUG "%s: %s: %s\n", igt_test_name(), __func__, msg); \
 })
 
-static void prepare_for_unbind(struct hotunplug *priv, char *buf, int buflen)
+static inline int prepare_common(struct hotunplug *priv)
 {
-	int len;
+	int fd_sysfs_drm;
+
+	local_debug("opening device");
+	priv->fd.drm = __drm_open_driver(DRIVER_ANY);
+	igt_assert(priv->fd.drm >= 0);
+
+	fd_sysfs_drm = igt_sysfs_open(priv->fd.drm);
+	igt_assert(fd_sysfs_drm >= 0);
+
+	return fd_sysfs_drm;
+}
+
+static inline void prepare_for_rebind(struct hotunplug *priv,
+				      char *buf, int buflen)
+{
+	int fd_sysfs_drm, len;
 
 	igt_assert(buflen);
 
-	priv->fd.sysfs_drv = openat(priv->fd.sysfs_dev, "device/driver",
-				    O_DIRECTORY);
-	igt_assert(priv->fd.sysfs_drv >= 0);
+	fd_sysfs_drm = prepare_common(priv);
+
+	priv->fd.sysfs_drv = openat(fd_sysfs_drm, "device/driver", O_DIRECTORY);
 
-	len = readlinkat(priv->fd.sysfs_dev, "device", buf, buflen - 1);
+	len = readlinkat(fd_sysfs_drm, "device", buf, buflen - 1);
 	buf[len] = '\0';
 	priv->dev_bus_addr = strrchr(buf, '/');
-	igt_assert(priv->dev_bus_addr++);
 
-	/* sysfs_dev no longer needed */
-	close(priv->fd.sysfs_dev);
+	close(fd_sysfs_drm);
+
+	igt_assert(priv->fd.sysfs_drv >= 0);
+	igt_assert(priv->dev_bus_addr++);
 }
 
-static inline void prepare(struct hotunplug *priv, char *buf, int buflen)
+static inline void prepare_for_rescan(struct hotunplug *priv)
 {
-	local_debug("opening device");
-	priv->fd.drm = __drm_open_driver(DRIVER_ANY);
-	igt_assert(priv->fd.drm >= 0);
+	int fd_sysfs_drm = prepare_common(priv);
 
-	priv->fd.sysfs_dev = igt_sysfs_open(priv->fd.drm);
-	igt_assert(priv->fd.sysfs_dev >= 0);
+	priv->fd.sysfs_dev = openat(fd_sysfs_drm, "device", O_DIRECTORY);
 
-	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(priv->fd.sysfs_bus >= 0);
-	}
+	priv->fd.sysfs_bus = openat(fd_sysfs_drm, "device/subsystem",
+				    O_DIRECTORY);
+
+	close(fd_sysfs_drm);
+
+	igt_assert(priv->fd.sysfs_dev >= 0);
+	igt_assert(priv->fd.sysfs_bus >= 0);
 }
 
 static const char *failure;
@@ -124,7 +136,7 @@  static void device_unplug(int fd_sysfs_dev)
 {
 	failure = "Device unplug timeout!";
 	igt_set_timeout(60, failure);
-	igt_sysfs_set(fd_sysfs_dev, "device/remove", "1");
+	igt_sysfs_set(fd_sysfs_dev, "remove", "1");
 	igt_reset_timeout();
 	failure = NULL;
 
@@ -185,7 +197,7 @@  static void unbind_rebind(void)
 	struct hotunplug priv;
 	char buf[PATH_MAX];
 
-	prepare(&priv, buf, sizeof(buf));
+	prepare_for_rebind(&priv, buf, sizeof(buf));
 
 	local_debug("closing the device");
 	close(priv.fd.drm);
@@ -203,7 +215,7 @@  static void unplug_rescan(void)
 {
 	struct hotunplug priv;
 
-	prepare(&priv, NULL, 0);
+	prepare_for_rescan(&priv);
 
 	local_debug("closing the device");
 	close(priv.fd.drm);
@@ -222,7 +234,7 @@  static void hotunbind_lateclose(void)
 	struct hotunplug priv;
 	char buf[PATH_MAX];
 
-	prepare(&priv, buf, sizeof(buf));
+	prepare_for_rebind(&priv, buf, sizeof(buf));
 
 	local_debug("hot unbinding the driver from the device");
 	driver_unbind(priv.fd.sysfs_drv, priv.dev_bus_addr);
@@ -240,7 +252,7 @@  static void hotunplug_lateclose(void)
 {
 	struct hotunplug priv;
 
-	prepare(&priv, NULL, 0);
+	prepare_for_rescan(&priv);
 
 	local_debug("hot unplugging the device");
 	device_unplug(priv.fd.sysfs_dev);