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

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

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

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev