Re: [PATCH i-g-t 1/2] lib/igt_configfs: Add library for configfs

["José Expósito" <jose.exposito89@gmail.com>]

Hi Riana,

On Fri, Apr 11, 2025 at 04:28:00PM +0530, Riana Tauro wrote:
> Hi Louis
> 
> On 4/11/2025 1:23 PM, Louis Chauvet wrote:
> > Hi Riana,
> > 
> > Le 10/04/2025 à 08:07, Riana Tauro a écrit :
> > > Add library functions to open configfs and create
> > > and remove configfs directories.
> > 
> > Can you take a look at [1], something similar was proposed few weeks
> > ago? The main difference is that [1] checks that /sys/kernel/config is
> > actually a mount point.
> I hadn't looked at this. Thank you for the link
> 
> @Jose is this close to merge? If not, can i send [2] and [3] as part of this
> series?
> 
> [2]https://lore.kernel.org/all/20250218165011.9123-3-jose.exposito89@gmail.com/
> [3]https://lore.kernel.org/all/20250218165011.9123-4-jose.exposito89@gmail.com/>

Sure, feel free to send them as part of your series.

The VKMS changes required by my tests are not merged yet, so, most likely,
your series will be merged sooner.

Please CC me in v2 and I'll review it.

> > +Cc: José Exposito

Thanks for CCing me :)

> > [1]:https://lore.kernel.org/all/20250218165011.9123-4-
> > jose.exposito89@gmail.com/
> > 
> > > Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> > > ---
> > >   lib/igt_configfs.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++
> > >   lib/igt_configfs.h | 13 +++++++
> > >   lib/meson.build    |  1 +
> > >   3 files changed, 102 insertions(+)
> > >   create mode 100644 lib/igt_configfs.c
> > >   create mode 100644 lib/igt_configfs.h
> > > 
> > > diff --git a/lib/igt_configfs.c b/lib/igt_configfs.c
> > > new file mode 100644
> > > index 000000000..57d59f1c4
> > > --- /dev/null
> > > +++ b/lib/igt_configfs.c
> > > @@ -0,0 +1,88 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2025 Intel Corporation
> > > + */
> > > +#include <sys/mount.h>
> > > +#include <sys/stat.h>
> > > +
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <limits.h>
> > > +
> > > +#include "igt_configfs.h"
> > > +#include "igt_core.h"
> > > +
> > > +static const char *igt_configfs_mount(void)
> > > +{
> > > +    struct stat st;
> > > +
> > > +    if (stat("/sys/kernel/debug", &st) == 0)
> > > +        return "/sys/kernel/config";
> > > +
> > > +    if (mount("none", "/sys/kernel/config", "configfs", 0, 0))
> > > +        return NULL;
> > > +
> > > +    return "/sys/kernel/config";
> > > +}
> > > +
> > > +/**
> > > + * igt_configfs_open: open configfs path
> > > + * @name: name of the configfs directory
> > > + *
> > > + * Opens the configfs directory corresponding to the name
> > > + *
> > > + * Returns:
> > > + * The directory fd, or -1 on failure.
> > > + */
> > > +int igt_configfs_open(const char *name)
> > > +{
> > > +    char path[PATH_MAX];
> > > +    const char *configfs_path;
> > > +
> > > +    configfs_path = igt_configfs_mount();
> > > +    igt_assert(configfs_path);
> > > +
> > > +    snprintf(path, sizeof(path), "%s/%s", configfs_path, name);
> > > +
> > > +    return open(path, O_RDONLY);
> > > +}
> > 
> > The usage of file descriptors for folders is nice here!
> > 
> > @José, I think this may reduce the complexity of the vkms tests. At
> > least it will avoid having snprintf(%s/%s, vkms_root, vkms_item). What
> > do you think?

In v2, I wrapped all path related code in gettiers like
igt_vkms_get_device_enabled_path() or igt_vkms_get_plane_path(), and
all of those getters are just calling common code.

I don't think it'll reduce *test* code complexity, but if we end up using
these methods, I'll have to adapt lib/igt_vkms.c to use them.

> > > +
> > > +/**
> > > + * igt_configfs_create_directory: creates configfs group
> > > + * @fd: fd of configfs parent directory
> > > + * @name: name of the directory to create
> > > + *
> > > + * creates a directory under configfs parent directory
> > > + *
> > > + * Returns: 0 on success, -errno otherwise
> > 
> > This seems wrong, as the function returns a file descriptor, did I miss
> > something?
> > Missed this. Will fix it>> + */
> > > +int igt_configfs_create_directory(int fd, const char *name)
> > > +{
> > > +    int ret;
> > > +    int dirfd;
> > > +
> > > +    ret = mkdirat(fd, name, 0755);
> > > +    if (ret)
> > > +        return -errno;
> > > +
> > > +    dirfd = openat(fd, name, O_DIRECTORY);
> > > +    if (dirfd < 0)
> > > +        return -errno;
> > > +
> > > +    return dirfd;
> > > +}
> > > +
> > > +/**
> > > + * igt_configfs_remove_directory: removes configfs group
> > > + * @fd: fd of configfs parent directory
> > > + * @name: name of directory to create
> > > + *
> > > + * removes directory under configfs parent directory
> > > + */
> > > +void igt_configfs_remove_directory(int fd, const char *name)
> > > +{
> > > +    int ret = unlinkat(fd, name, AT_REMOVEDIR);
> > > +
> > > +    if (ret)
> > > +        igt_warn("Unable to remove %s directory: %s\n", name,
> > > strerror(errno));
> > > +}
> > 
> > I completely understand the point of those helpers, but I don't think
> > they are configfs-specific. The file descriptor is passed by argument,
> > so you could use it for any interface (configfs, debugfs, sysfs...).
> > 
> > I don't see any igt_fs.c, do you think it beneficial to create such file?
> 
> I thought of calling the igt_configfs_open here but that would not work for
> nested configfs directories.
> 
> This could be moved to igt_io and that can be renamed to igt_fs as that has
> only file read and write calls
> 
> Thank you
> Riana Tauro>
> > @José, do you think it make sense to also reuse igt_sysfs helpers for
> > the VKMS tests? (igt_sysfs_get/set_s32/u32/boolean only use a directory
> > fd + path, if you decide to use fd for vkms, it will avoid code
> > duplication)

Definetely, if we are adding equivalents for configfs, I have my own
write/read_int() and write/read_bool() functions that could be replaced by
them.

Thanks
Jose

> > Thanks a lot Riana for this implementation and new ideas.
> > 
> > Louis Chauvet
> > 
> > > diff --git a/lib/igt_configfs.h b/lib/igt_configfs.h
> > > new file mode 100644
> > > index 000000000..d839db49a
> > > --- /dev/null
> > > +++ b/lib/igt_configfs.h
> > > @@ -0,0 +1,13 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2025 Intel Corporation
> > > + */
> > > +
> > > +#ifndef IGT_CONFIGFS_H
> > > +#define IGT_CONFIGFS_H
> > > +
> > > +int igt_configfs_open(const char *name);
> > > +int igt_configfs_create_directory(int fd, const char *name);
> > > +void igt_configfs_remove_directory(int fd, const char *name);
> > > +
> > > +#endif /* IGT_CONFIGFS_H */
> > > diff --git a/lib/meson.build b/lib/meson.build
> > > index d7bb72c57..f087947e7 100644
> > > --- a/lib/meson.build
> > > +++ b/lib/meson.build
> > > @@ -19,6 +19,7 @@ lib_sources = [
> > >       'igt_collection.c',
> > >       'igt_color_encoding.c',
> > >       'igt_facts.c',
> > > +    'igt_configfs.c',
> > >       'igt_crc.c',
> > >       'igt_debugfs.c',
> > >       'igt_device.c',
> > 
>