Re: [PATCH v2 02/16] drm/vkms: Add and remove VKMS instances via configfs

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 25/02/2025 à 18:59, José Expósito a écrit :
> Allow to create, enable, disable and destroy VKMS instances using
> configfs.
> 
> For the moment, it is not possible to add pipeline items, so trying to
> enable the device will fail printing an informative error to the log.
> 
> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
> Co-developed-by: Louis Chauvet <louis.chauvet@bootlin.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>   Documentation/gpu/vkms.rst           |  32 +++++
>   drivers/gpu/drm/vkms/Kconfig         |   1 +
>   drivers/gpu/drm/vkms/Makefile        |   3 +-
>   drivers/gpu/drm/vkms/vkms_configfs.c | 169 +++++++++++++++++++++++++++
>   drivers/gpu/drm/vkms/vkms_configfs.h |   8 ++
>   drivers/gpu/drm/vkms/vkms_drv.c      |   7 ++
>   6 files changed, 219 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
>   create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.h
> 
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index ba04ac7c2167..423bdf86b5b1 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -51,6 +51,38 @@ To disable the driver, use ::
>   
>     sudo modprobe -r vkms
>   
> +Configuring With Configfs
> +=========================
> +
> +It is possible to create and configure multiple VKMS instances via configfs.
> +
> +Start by mounting configfs and loading VKMS::
> +
> +  sudo mount -t configfs none /config
> +  sudo modprobe vkms
> +
> +Once VKMS is loaded, ``/config/vkms`` is created automatically. Each directory
> +under ``/config/vkms`` represents a VKMS instance, create a new one::
> +
> +  sudo mkdir /config/vkms/my-vkms
> +
> +By default, the instance is disabled::
> +
> +  cat /config/vkms/my-vkms/enabled
> +  0
> +
> +Once you are done configuring the VKMS instance, enable it::
> +
> +  echo "1" | sudo tee /config/vkms/my-vkms/enabled
> +
> +Finally, you can remove the VKMS instance disabling it::
> +
> +  echo "0" | sudo tee /config/vkms/my-vkms/enabled
> +
> +And removing the top level directory::
> +
> +  sudo rmdir /config/vkms/my-vkms
> +
>   Testing With IGT
>   ================
>   
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index 3c02f928ffe6..3977bbb99f7d 100644
> --- a/drivers/gpu/drm/vkms/Kconfig
> +++ b/drivers/gpu/drm/vkms/Kconfig
> @@ -7,6 +7,7 @@ config DRM_VKMS
>   	select DRM_KMS_HELPER
>   	select DRM_GEM_SHMEM_HELPER
>   	select CRC32
> +	select CONFIGFS_FS
>   	default n
>   	help
>   	  Virtual Kernel Mode-Setting (VKMS) is used for testing or for
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index d657865e573f..939991fc8233 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -8,7 +8,8 @@ vkms-y := \
>   	vkms_composer.o \
>   	vkms_writeback.o \
>   	vkms_connector.o \
> -	vkms_config.o
> +	vkms_config.o \
> +	vkms_configfs.o
>   
>   obj-$(CONFIG_DRM_VKMS) += vkms.o
>   obj-$(CONFIG_DRM_VKMS_KUNIT_TEST) += tests/
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> new file mode 100644
> index 000000000000..92512d52ddae
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/cleanup.h>
> +#include <linux/configfs.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +#include "vkms_drv.h"
> +#include "vkms_config.h"
> +#include "vkms_configfs.h"
> +
> +/* To avoid registering configfs more than once or unregistering on error */
> +static bool is_configfs_registered;
> +
> +/**
> + * struct vkms_configfs_device - Configfs representation of a VKMS device
> + *
> + * @group: Top level configuration group that represents a VKMS device.
> + * Initialized when a new directory is created under "/config/vkms/"
> + * @lock: Lock used to project concurrent access to the configuration attributes
> + * @config: Protected by @lock. Configuration of the VKMS device
> + * @enabled: Protected by @lock. The device is created or destroyed when this
> + * option changes
> + */
> +struct vkms_configfs_device {
> +	struct config_group group;
> +
> +	struct mutex lock;
> +	struct vkms_config *config;
> +	bool enabled;
> +};
> +
> +#define device_item_to_vkms_configfs_device(item) \
> +	container_of(to_config_group((item)), struct vkms_configfs_device, \
> +		     group)
> +
> +static ssize_t device_enabled_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_device *dev;
> +
> +	dev = device_item_to_vkms_configfs_device(item);
> +
> +	guard(mutex)(&dev->lock);
> +	return sprintf(page, "%d\n", dev->enabled);
> +}
> +
> +static ssize_t device_enabled_store(struct config_item *item, const char *page,
> +				    size_t count)
> +{
> +	struct vkms_configfs_device *dev;
> +	bool enabled;
> +	int ret = 0;
> +
> +	dev = device_item_to_vkms_configfs_device(item);
> +
> +	if (kstrtobool(page, &enabled))
> +		return -EINVAL;
> +
> +	guard(mutex)(&dev->lock);
> +
> +	if (!dev->enabled && enabled) {
> +		if (!vkms_config_is_valid(dev->config))
> +			return -EINVAL;
> +
> +		ret = vkms_create(dev->config);
> +		if (ret)
> +			return ret;
> +	} else if (dev->enabled && !enabled) {
> +		vkms_destroy(dev->config);
> +	}
> +
> +	dev->enabled = enabled;

Sorry, I was maybe not clear enough, and you may hate me: I don't like 
`guard(mutex)` :‑(. I proposed scoped_guard because it makes very clear 
when the mutex is taken/released.

For me guard(mutex) is almost the same as mutex_lock/unlock. Yes, your 
mutex is always released, but:
- without reading the code carefully, you don't know you have a mutex 
(even worse than a mutex_lock because you don't have a bunch of 
mutex_unlock to remind you)
- you keep it until the end of the function, which may lock your mutex 
for too long

The scoped_guard solves the two issues:
- you are in a dedicated block + indentation, so it is very easy to see 
that you currently have a mutex
- you know exactly when the mutex is released: leaving the block

I am very sorry to make you work twice on this

> +
> +	return (ssize_t)count;
> +}
> +
> +CONFIGFS_ATTR(device_, enabled);
> +
> +static struct configfs_attribute *device_item_attrs[] = {
> +	&device_attr_enabled,
> +	NULL,
> +};
> +
> +static void device_release(struct config_item *item)
> +{
> +	struct vkms_configfs_device *dev;
> +
> +	dev = device_item_to_vkms_configfs_device(item);
> +
> +	if (dev->enabled)
> +		vkms_destroy(dev->config);
> +
> +	mutex_destroy(&dev->lock);
> +	vkms_config_destroy(dev->config);
> +	kfree(dev);
> +}
> +
> +static struct configfs_item_operations device_item_operations = {
> +	.release	= &device_release,
> +};
> +
> +static const struct config_item_type device_item_type = {
> +	.ct_attrs	= device_item_attrs,
> +	.ct_item_ops	= &device_item_operations,
> +	.ct_owner	= THIS_MODULE,
> +};
> +
> +static struct config_group *make_device_group(struct config_group *group,
> +					      const char *name)
> +{
> +	struct vkms_configfs_device *dev;
> +
> +	if (strcmp(name, DEFAULT_DEVICE_NAME) == 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dev->config = vkms_config_create(name);
> +	if (IS_ERR(dev->config)) {
> +		kfree(dev);
> +		return ERR_CAST(dev->config);
> +	}
> +
> +	config_group_init_type_name(&dev->group, name, &device_item_type);
> +	mutex_init(&dev->lock);
> +
> +	return &dev->group;
> +}
> +
> +static struct configfs_group_operations device_group_ops = {
> +	.make_group = &make_device_group,
> +};
> +
> +static const struct config_item_type device_group_type = {
> +	.ct_group_ops	= &device_group_ops,
> +	.ct_owner	= THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem vkms_subsys = {
> +	.su_group = {
> +		.cg_item = {
> +			.ci_name = "vkms",
> +			.ci_type = &device_group_type,
> +		},
> +	},
> +	.su_mutex = __MUTEX_INITIALIZER(vkms_subsys.su_mutex),
> +};
> +
> +int vkms_configfs_register(void)
> +{
> +	int ret;
> +
> +	if (is_configfs_registered)
> +		return 0;
> +
> +	config_group_init(&vkms_subsys.su_group);
> +	ret = configfs_register_subsystem(&vkms_subsys);
> +
> +	is_configfs_registered = ret == 0;
> +
> +	return ret;
> +}
> +
> +void vkms_configfs_unregister(void)
> +{
> +	if (is_configfs_registered)
> +		configfs_unregister_subsystem(&vkms_subsys);
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.h b/drivers/gpu/drm/vkms/vkms_configfs.h
> new file mode 100644
> index 000000000000..e9020b0043db
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +#ifndef _VKMS_CONFIGFS_H_
> +#define _VKMS_CONFIGFS_H_
> +
> +int vkms_configfs_register(void);
> +void vkms_configfs_unregister(void);
> +
> +#endif /* _VKMS_CONFIGFS_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 23817c7b997e..5bcfbcb6c0c5 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -28,6 +28,7 @@
>   #include <drm/drm_vblank.h>
>   
>   #include "vkms_config.h"
> +#include "vkms_configfs.h"
>   #include "vkms_drv.h"
>   
>   #define DRIVER_NAME	"vkms"
> @@ -214,6 +215,10 @@ static int __init vkms_init(void)
>   	int ret;
>   	struct vkms_config *config;
>   
> +	ret = vkms_configfs_register();
> +	if (ret)
> +		return ret;
> +
>   	config = vkms_config_default_create(enable_cursor, enable_writeback, enable_overlay);
>   	if (IS_ERR(config))
>   		return PTR_ERR(config);
> @@ -250,6 +255,8 @@ void vkms_destroy(struct vkms_config *config)
>   
>   static void __exit vkms_exit(void)
>   {
> +	vkms_configfs_unregister();
> +
>   	if (!default_config)
>   		return;
>   

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com