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

[Michał Winiarski]

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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev