[PATCH v2 00/16] drm/vkms: Add configfs support

[PATCH v2 00/16] drm/vkms: Add configfs support

[José Expósito]

Hi everyone,

This series, to be applied on top of [1], allow to configure one or more VKMS
instances without having to reload the driver using configfs.

The series is structured in 3 blocks:

  - Patches 1..11: Basic device configuration. For simplicity, I kept the
    available options as minimal as possible.

  - Patches 12 and 13: New option to skip the default device creation and to-do
    cleanup.

  - Patches 14, 15 and 16: Allow to hot-plug and unplug connectors. This is not
    part of the minimal set of options, but I included in this series so it can
    be used as a template/example of how new configurations can be added.

The process of configuring a VKMS device is documented in "vkms.rst".

Finally, the code is thoroughly tested by a collection of IGT tests [2].

Best wishes,
José Expósito

[1] https://lore.kernel.org/all/20250218101214.5790-1-jose.exposito89@gmail.com/
[2] https://lists.freedesktop.org/archives/igt-dev/2025-February/086071.html

Changes in v2:

  - Applied review comments by Louis Chauvet: (thanks!!)
    - Use guard(mutex)(...) instead of lock/unlock
    - Return -EBUSY when trying to modify a enabled device
    - Move the connector hot-plug related patches to the end
  - Rebased on top of drm-misc-next
  - Link to v1: https://lore.kernel.org/dri-devel/20250218170808.9507-1-jose.exposito89@gmail.com/T/

José Expósito (16):
  drm/vkms: Expose device creation and destruction
  drm/vkms: Add and remove VKMS instances via configfs
  drm/vkms: Allow to configure multiple planes via configfs
  drm/vkms: Allow to configure the plane type via configfs
  drm/vkms: Allow to configure multiple CRTCs via configfs
  drm/vkms: Allow to configure CRTC writeback support via configfs
  drm/vkms: Allow to attach planes and CRTCs via configfs
  drm/vkms: Allow to configure multiple encoders via configfs
  drm/vkms: Allow to attach encoders and CRTCs via configfs
  drm/vkms: Allow to configure multiple connectors via configfs
  drm/vkms: Allow to attach connectors and encoders via configfs
  drm/vkms: Allow to configure the default device creation
  drm/vkms: Remove completed task from the TODO list
  drm/vkms: Allow to configure connector status
  drm/vkms: Allow to update the connector status
  drm/vkms: Allow to configure connector status via configfs

 Documentation/gpu/vkms.rst                    | 100 ++-
 drivers/gpu/drm/vkms/Kconfig                  |   1 +
 drivers/gpu/drm/vkms/Makefile                 |   3 +-
 drivers/gpu/drm/vkms/tests/vkms_config_test.c |  24 +
 drivers/gpu/drm/vkms/vkms_config.c            |   8 +-
 drivers/gpu/drm/vkms/vkms_config.h            |  26 +
 drivers/gpu/drm/vkms/vkms_configfs.c          | 807 ++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_configfs.h          |   8 +
 drivers/gpu/drm/vkms/vkms_connector.c         |  26 +-
 drivers/gpu/drm/vkms/vkms_connector.h         |  18 +-
 drivers/gpu/drm/vkms/vkms_drv.c               |  18 +-
 drivers/gpu/drm/vkms/vkms_drv.h               |  20 +
 drivers/gpu/drm/vkms/vkms_output.c            |   2 +-
 13 files changed, 1045 insertions(+), 16 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.h


base-commit: 130377304ed09e54ff35a8974372498aad7059f3
prerequisite-patch-id: 1bff7bbc4ef0e29266265ac3dc009011c046f745
prerequisite-patch-id: 74a284d40a426a0038a7054068192238f7658187
prerequisite-patch-id: c3e34e88ad6a0acf7d9ded0cdb4745a87cf6fd82
prerequisite-patch-id: 9cd0dfaf8e21a811edbe5a2da7185b6f9055d42d
prerequisite-patch-id: f50c41578b639370a5d610af6f25c2077321a886
prerequisite-patch-id: 5a7219a51e42de002b8dbf94ec8af96320043489
prerequisite-patch-id: 67ea5d4e21b4ce4acbd6fc3ce83017f55811c49b
prerequisite-patch-id: 37a7fab113a32581f053c09f45efb137afd75a1b
prerequisite-patch-id: 475bcdc6267f4b02fb1bb2379145529c33684e4f
prerequisite-patch-id: d3114f0b3da3d8b5ad64692df761f1cf42fbdf12
prerequisite-patch-id: d1d9280fb056130df2050a09b7ea7e7ddde007c5
prerequisite-patch-id: 2c370f3de6d227fa8881212207978cce7bbb18ba
prerequisite-patch-id: 938b8fe5437e5f7bc22bffc55ae249a27d399d66
prerequisite-patch-id: ab0a510994fbe9985dc46a3d35e6d0574ddbb633
-- 
2.48.1

[PATCH v2 01/16] drm/vkms: Expose device creation and destruction

[José Expósito]

In preparation for configfs support, expose vkms_create() and
vkms_destroy().

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>
---
 drivers/gpu/drm/vkms/vkms_drv.c |  4 ++--
 drivers/gpu/drm/vkms/vkms_drv.h | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index a24d1655f7b8..23817c7b997e 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -146,7 +146,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
 	return vkms_output_init(vkmsdev);
 }
 
-static int vkms_create(struct vkms_config *config)
+int vkms_create(struct vkms_config *config)
 {
 	int ret;
 	struct platform_device *pdev;
@@ -229,7 +229,7 @@ static int __init vkms_init(void)
 	return 0;
 }
 
-static void vkms_destroy(struct vkms_config *config)
+void vkms_destroy(struct vkms_config *config)
 {
 	struct platform_device *pdev;
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index a74a7fc3a056..2ad8e3da3b7b 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -223,6 +223,26 @@ struct vkms_device {
 #define to_vkms_plane_state(target)\
 	container_of(target, struct vkms_plane_state, base.base)
 
+/**
+ * vkms_create() - Create a device from a configuration
+ * @config: Config used to configure the new device
+ *
+ * A pointer to the created vkms_device is stored in @config
+ *
+ * Returns:
+ * 0 on success or an error.
+ */
+int vkms_create(struct vkms_config *config);
+
+/**
+ * vkms_destroy() - Destroy a device
+ * @config: Config from which the device was created
+ *
+ * The device is completely removed, but the @config is not freed. It can be
+ * reused or destroyed with vkms_config_destroy().
+ */
+void vkms_destroy(struct vkms_config *config);
+
 /**
  * vkms_crtc_init() - Initialize a CRTC for VKMS
  * @dev: DRM device associated with the VKMS buffer
-- 
2.48.1

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

[José Expósito]

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;
+
+	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;
 
-- 
2.48.1

[PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[José Expósito]

Create a default subgroup at /config/vkms/planes to allow to create as
many planes as required.

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           | 16 ++++-
 drivers/gpu/drm/vkms/vkms_configfs.c | 87 ++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 423bdf86b5b1..bf23d0da33fe 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -71,6 +71,19 @@ By default, the instance is disabled::
   cat /config/vkms/my-vkms/enabled
   0
 
+And directories are created for each configurable item of the display pipeline::
+
+  tree /config/vkms/my-vkms
+  ├── enabled
+  └── planes
+
+To add items to the display pipeline, create one or more directories under the
+available paths.
+
+Start by creating one or more planes::
+
+  sudo mkdir /config/vkms/my-vkms/planes/plane0
+
 Once you are done configuring the VKMS instance, enable it::
 
   echo "1" | sudo tee /config/vkms/my-vkms/enabled
@@ -79,8 +92,9 @@ Finally, you can remove the VKMS instance disabling it::
 
   echo "0" | sudo tee /config/vkms/my-vkms/enabled
 
-And removing the top level directory::
+And removing the top level directory and its subdirectories::
 
+  sudo rmdir /config/vkms/my-vkms/planes/*
   sudo rmdir /config/vkms/my-vkms
 
 Testing With IGT
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 92512d52ddae..4f9d3341e6c0 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -16,6 +16,7 @@ static bool is_configfs_registered;
  *
  * @group: Top level configuration group that represents a VKMS device.
  * Initialized when a new directory is created under "/config/vkms/"
+ * @planes_group: Default subgroup of @group at "/config/vkms/planes"
  * @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
@@ -23,16 +24,98 @@ static bool is_configfs_registered;
  */
 struct vkms_configfs_device {
 	struct config_group group;
+	struct config_group planes_group;
 
 	struct mutex lock;
 	struct vkms_config *config;
 	bool enabled;
 };
 
+/**
+ * struct vkms_configfs_plane - Configfs representation of a plane
+ *
+ * @group: Top level configuration group that represents a plane.
+ * Initialized when a new directory is created under "/config/vkms/planes"
+ * @dev: The vkms_configfs_device this plane belongs to
+ * @config: Configuration of the VKMS plane
+ */
+struct vkms_configfs_plane {
+	struct config_group group;
+	struct vkms_configfs_device *dev;
+	struct vkms_config_plane *config;
+};
+
 #define device_item_to_vkms_configfs_device(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_device, \
 		     group)
 
+#define child_group_to_vkms_configfs_device(group) \
+	device_item_to_vkms_configfs_device((&(group)->cg_item)->ci_parent)
+
+#define plane_item_to_vkms_configfs_plane(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_plane, group)
+
+static void plane_release(struct config_item *item)
+{
+	struct vkms_configfs_plane *plane;
+	struct mutex *lock;
+
+	plane = plane_item_to_vkms_configfs_plane(item);
+	lock = &plane->dev->lock;
+
+	guard(mutex)(lock);
+	vkms_config_destroy_plane(plane->config);
+	kfree(plane);
+}
+
+static struct configfs_item_operations plane_item_operations = {
+	.release	= &plane_release,
+};
+
+static const struct config_item_type plane_item_type = {
+	.ct_item_ops	= &plane_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *make_plane_group(struct config_group *group,
+					     const char *name)
+{
+	struct vkms_configfs_device *dev;
+	struct vkms_configfs_plane *plane;
+
+	dev = child_group_to_vkms_configfs_device(group);
+
+	guard(mutex)(&dev->lock);
+
+	if (dev->enabled)
+		return ERR_PTR(-EBUSY);
+
+	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
+	if (!plane)
+		return ERR_PTR(-ENOMEM);
+
+	plane->dev = dev;
+
+	plane->config = vkms_config_create_plane(dev->config);
+	if (IS_ERR(plane->config)) {
+		kfree(plane);
+		return ERR_CAST(plane->config);
+	}
+
+	config_group_init_type_name(&plane->group, name, &plane_item_type);
+
+	return &plane->group;
+}
+
+static struct configfs_group_operations planes_group_operations = {
+	.make_group	= &make_plane_group,
+};
+
+static const struct config_item_type plane_group_type = {
+	.ct_group_ops	= &planes_group_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
 static ssize_t device_enabled_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_device *dev;
@@ -125,6 +208,10 @@ static struct config_group *make_device_group(struct config_group *group,
 	config_group_init_type_name(&dev->group, name, &device_item_type);
 	mutex_init(&dev->lock);
 
+	config_group_init_type_name(&dev->planes_group, "planes",
+				    &plane_group_type);
+	configfs_add_default_group(&dev->planes_group, &dev->group);
+
 	return &dev->group;
 }
 
-- 
2.48.1

[PATCH v2 04/16] drm/vkms: Allow to configure the plane type via configfs

[José Expósito]

When a plane is created, add a `type` file to allow to set the type:

 - 0 overlay
 - 1 primary
 - 2 cursor

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           |  5 ++++
 drivers/gpu/drm/vkms/vkms_configfs.c | 43 ++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index bf23d0da33fe..a87e0925bebb 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -84,6 +84,11 @@ Start by creating one or more planes::
 
   sudo mkdir /config/vkms/my-vkms/planes/plane0
 
+Planes have 1 configurable attribute:
+
+- type: Plane type: 0 overlay, 1 primary, 2 cursor (same values as those
+  exposed by the "type" property of a plane)
+
 Once you are done configuring the VKMS instance, enable it::
 
   echo "1" | sudo tee /config/vkms/my-vkms/enabled
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 4f9d3341e6c0..c2e1eef75906 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -55,6 +55,48 @@ struct vkms_configfs_plane {
 #define plane_item_to_vkms_configfs_plane(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_plane, group)
 
+static ssize_t plane_type_show(struct config_item *item, char *page)
+{
+	struct vkms_configfs_plane *plane;
+
+	plane = plane_item_to_vkms_configfs_plane(item);
+
+	guard(mutex)(&plane->dev->lock);
+	return sprintf(page, "%u", vkms_config_plane_get_type(plane->config));
+}
+
+static ssize_t plane_type_store(struct config_item *item, const char *page,
+				size_t count)
+{
+	struct vkms_configfs_plane *plane;
+	enum drm_plane_type type;
+
+	plane = plane_item_to_vkms_configfs_plane(item);
+
+	if (kstrtouint(page, 10, &type))
+		return -EINVAL;
+
+	if (type != DRM_PLANE_TYPE_OVERLAY && type != DRM_PLANE_TYPE_PRIMARY &&
+	    type != DRM_PLANE_TYPE_CURSOR)
+		return -EINVAL;
+
+	guard(mutex)(&plane->dev->lock);
+
+	if (plane->dev->enabled)
+		return -EBUSY;
+
+	vkms_config_plane_set_type(plane->config, type);
+
+	return (ssize_t)count;
+}
+
+CONFIGFS_ATTR(plane_, type);
+
+static struct configfs_attribute *plane_item_attrs[] = {
+	&plane_attr_type,
+	NULL,
+};
+
 static void plane_release(struct config_item *item)
 {
 	struct vkms_configfs_plane *plane;
@@ -73,6 +115,7 @@ static struct configfs_item_operations plane_item_operations = {
 };
 
 static const struct config_item_type plane_item_type = {
+	.ct_attrs	= plane_item_attrs,
 	.ct_item_ops	= &plane_item_operations,
 	.ct_owner	= THIS_MODULE,
 };
-- 
2.48.1

[PATCH v2 05/16] drm/vkms: Allow to configure multiple CRTCs via configfs

[José Expósito]

Create a default subgroup at /config/vkms/crtcs to allow to create as
many CRTCs as required.

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           |  6 ++
 drivers/gpu/drm/vkms/vkms_configfs.c | 84 ++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index a87e0925bebb..e0699603ef53 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -74,6 +74,7 @@ By default, the instance is disabled::
 And directories are created for each configurable item of the display pipeline::
 
   tree /config/vkms/my-vkms
+  ├── crtcs
   ├── enabled
   └── planes
 
@@ -89,6 +90,10 @@ Planes have 1 configurable attribute:
 - type: Plane type: 0 overlay, 1 primary, 2 cursor (same values as those
   exposed by the "type" property of a plane)
 
+Continue by creating one or more CRTCs::
+
+  sudo mkdir /config/vkms/my-vkms/crtcs/crtc0
+
 Once you are done configuring the VKMS instance, enable it::
 
   echo "1" | sudo tee /config/vkms/my-vkms/enabled
@@ -100,6 +105,7 @@ Finally, you can remove the VKMS instance disabling it::
 And removing the top level directory and its subdirectories::
 
   sudo rmdir /config/vkms/my-vkms/planes/*
+  sudo rmdir /config/vkms/my-vkms/crtcs/*
   sudo rmdir /config/vkms/my-vkms
 
 Testing With IGT
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index c2e1eef75906..1483f47244e6 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -17,6 +17,7 @@ static bool is_configfs_registered;
  * @group: Top level configuration group that represents a VKMS device.
  * Initialized when a new directory is created under "/config/vkms/"
  * @planes_group: Default subgroup of @group at "/config/vkms/planes"
+ * @crtcs_group: Default subgroup of @group at "/config/vkms/crtcs"
  * @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
@@ -25,6 +26,7 @@ static bool is_configfs_registered;
 struct vkms_configfs_device {
 	struct config_group group;
 	struct config_group planes_group;
+	struct config_group crtcs_group;
 
 	struct mutex lock;
 	struct vkms_config *config;
@@ -45,6 +47,20 @@ struct vkms_configfs_plane {
 	struct vkms_config_plane *config;
 };
 
+/**
+ * struct vkms_configfs_crtc - Configfs representation of a CRTC
+ *
+ * @group: Top level configuration group that represents a CRTC.
+ * Initialized when a new directory is created under "/config/vkms/crtcs"
+ * @dev: The vkms_configfs_device this CRTC belongs to
+ * @config: Configuration of the VKMS CRTC
+ */
+struct vkms_configfs_crtc {
+	struct config_group group;
+	struct vkms_configfs_device *dev;
+	struct vkms_config_crtc *config;
+};
+
 #define device_item_to_vkms_configfs_device(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_device, \
 		     group)
@@ -55,6 +71,70 @@ struct vkms_configfs_plane {
 #define plane_item_to_vkms_configfs_plane(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_plane, group)
 
+#define crtc_item_to_vkms_configfs_crtc(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_crtc, group)
+
+static void crtc_release(struct config_item *item)
+{
+	struct vkms_configfs_crtc *crtc;
+	struct mutex *lock;
+
+	crtc = crtc_item_to_vkms_configfs_crtc(item);
+	lock = &crtc->dev->lock;
+
+	guard(mutex)(lock);
+	vkms_config_destroy_crtc(crtc->dev->config, crtc->config);
+	kfree(crtc);
+}
+
+static struct configfs_item_operations crtc_item_operations = {
+	.release	= &crtc_release,
+};
+
+static const struct config_item_type crtc_item_type = {
+	.ct_item_ops	= &crtc_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *make_crtc_group(struct config_group *group,
+					    const char *name)
+{
+	struct vkms_configfs_device *dev;
+	struct vkms_configfs_crtc *crtc;
+
+	dev = child_group_to_vkms_configfs_device(group);
+
+	guard(mutex)(&dev->lock);
+
+	if (dev->enabled)
+		return ERR_PTR(-EBUSY);
+
+	crtc = kzalloc(sizeof(*crtc), GFP_KERNEL);
+	if (!crtc)
+		return ERR_PTR(-ENOMEM);
+
+	crtc->dev = dev;
+
+	crtc->config = vkms_config_create_crtc(dev->config);
+	if (IS_ERR(crtc->config)) {
+		kfree(crtc);
+		return ERR_CAST(crtc->config);
+	}
+
+	config_group_init_type_name(&crtc->group, name, &crtc_item_type);
+
+	return &crtc->group;
+}
+
+static struct configfs_group_operations crtcs_group_operations = {
+	.make_group	= &make_crtc_group,
+};
+
+static const struct config_item_type crtc_group_type = {
+	.ct_group_ops	= &crtcs_group_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
 static ssize_t plane_type_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_plane *plane;
@@ -255,6 +335,10 @@ static struct config_group *make_device_group(struct config_group *group,
 				    &plane_group_type);
 	configfs_add_default_group(&dev->planes_group, &dev->group);
 
+	config_group_init_type_name(&dev->crtcs_group, "crtcs",
+				    &crtc_group_type);
+	configfs_add_default_group(&dev->crtcs_group, &dev->group);
+
 	return &dev->group;
 }
 
-- 
2.48.1

[PATCH v2 06/16] drm/vkms: Allow to configure CRTC writeback support via configfs

[José Expósito]

When a CRTC is created, add a `writeback` file to allow to enable or
disable writeback connector support

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           |  4 +++
 drivers/gpu/drm/vkms/vkms_configfs.c | 40 ++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index e0699603ef53..abe7a0f5a4ab 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -94,6 +94,10 @@ Continue by creating one or more CRTCs::
 
   sudo mkdir /config/vkms/my-vkms/crtcs/crtc0
 
+CRTCs have 1 configurable attribute:
+
+- writeback: Enable or disable writeback connector support by writing 1 or 0
+
 Once you are done configuring the VKMS instance, enable it::
 
   echo "1" | sudo tee /config/vkms/my-vkms/enabled
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 1483f47244e6..233a3d4ad75c 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -74,6 +74,45 @@ struct vkms_configfs_crtc {
 #define crtc_item_to_vkms_configfs_crtc(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_crtc, group)
 
+static ssize_t crtc_writeback_show(struct config_item *item, char *page)
+{
+	struct vkms_configfs_crtc *crtc;
+
+	crtc = crtc_item_to_vkms_configfs_crtc(item);
+
+	guard(mutex)(&crtc->dev->lock);
+	return sprintf(page, "%d\n",
+		       vkms_config_crtc_get_writeback(crtc->config));
+}
+
+static ssize_t crtc_writeback_store(struct config_item *item, const char *page,
+				    size_t count)
+{
+	struct vkms_configfs_crtc *crtc;
+	bool writeback;
+
+	crtc = crtc_item_to_vkms_configfs_crtc(item);
+
+	if (kstrtobool(page, &writeback))
+		return -EINVAL;
+
+	guard(mutex)(&crtc->dev->lock);
+
+	if (crtc->dev->enabled)
+		return -EBUSY;
+
+	vkms_config_crtc_set_writeback(crtc->config, writeback);
+
+	return (ssize_t)count;
+}
+
+CONFIGFS_ATTR(crtc_, writeback);
+
+static struct configfs_attribute *crtc_item_attrs[] = {
+	&crtc_attr_writeback,
+	NULL,
+};
+
 static void crtc_release(struct config_item *item)
 {
 	struct vkms_configfs_crtc *crtc;
@@ -92,6 +131,7 @@ static struct configfs_item_operations crtc_item_operations = {
 };
 
 static const struct config_item_type crtc_item_type = {
+	.ct_attrs	= crtc_item_attrs,
 	.ct_item_ops	= &crtc_item_operations,
 	.ct_owner	= THIS_MODULE,
 };
-- 
2.48.1

[PATCH v2 07/16] drm/vkms: Allow to attach planes and CRTCs via configfs

[José Expósito]

Create a default subgroup at /config/vkms/planes/plane/possible_crtcs
that will contain symbolic links to the possible CRTCs for the plane.

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           |  9 +++++
 drivers/gpu/drm/vkms/vkms_configfs.c | 54 ++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index abe7a0f5a4ab..13b96837a266 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -98,6 +98,14 @@ CRTCs have 1 configurable attribute:
 
 - writeback: Enable or disable writeback connector support by writing 1 or 0
 
+To finish the configuration, link the different pipeline items::
+
+  sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/planes/plane0/possible_crtcs
+
+Since at least one primary plane is required, make sure to set the right type::
+
+  echo "1" | sudo tee /config/vkms/my-vkms/planes/plane0/type
+
 Once you are done configuring the VKMS instance, enable it::
 
   echo "1" | sudo tee /config/vkms/my-vkms/enabled
@@ -108,6 +116,7 @@ Finally, you can remove the VKMS instance disabling it::
 
 And removing the top level directory and its subdirectories::
 
+  sudo rm /config/vkms/my-vkms/planes/*/possible_crtcs/*
   sudo rmdir /config/vkms/my-vkms/planes/*
   sudo rmdir /config/vkms/my-vkms/crtcs/*
   sudo rmdir /config/vkms/my-vkms
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 233a3d4ad75c..09d1f2224d01 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -38,11 +38,13 @@ struct vkms_configfs_device {
  *
  * @group: Top level configuration group that represents a plane.
  * Initialized when a new directory is created under "/config/vkms/planes"
+ * @possible_crtcs_group: Default subgroup of @group at "plane/possible_crtcs"
  * @dev: The vkms_configfs_device this plane belongs to
  * @config: Configuration of the VKMS plane
  */
 struct vkms_configfs_plane {
 	struct config_group group;
+	struct config_group possible_crtcs_group;
 	struct vkms_configfs_device *dev;
 	struct vkms_config_plane *config;
 };
@@ -71,6 +73,10 @@ struct vkms_configfs_crtc {
 #define plane_item_to_vkms_configfs_plane(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_plane, group)
 
+#define plane_possible_crtcs_item_to_vkms_configfs_plane(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_plane, \
+		     possible_crtcs_group)
+
 #define crtc_item_to_vkms_configfs_crtc(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_crtc, group)
 
@@ -175,6 +181,49 @@ static const struct config_item_type crtc_group_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static int plane_possible_crtcs_allow_link(struct config_item *src,
+					   struct config_item *target)
+{
+	struct vkms_configfs_plane *plane;
+	struct vkms_configfs_crtc *crtc;
+
+	if (target->ci_type != &crtc_item_type)
+		return -EINVAL;
+
+	plane = plane_possible_crtcs_item_to_vkms_configfs_plane(src);
+	crtc = crtc_item_to_vkms_configfs_crtc(target);
+
+	guard(mutex)(&plane->dev->lock);
+
+	if (plane->dev->enabled)
+		return -EBUSY;
+
+	return vkms_config_plane_attach_crtc(plane->config, crtc->config);
+}
+
+static void plane_possible_crtcs_drop_link(struct config_item *src,
+					   struct config_item *target)
+{
+	struct vkms_configfs_plane *plane;
+	struct vkms_configfs_crtc *crtc;
+
+	plane = plane_possible_crtcs_item_to_vkms_configfs_plane(src);
+	crtc = crtc_item_to_vkms_configfs_crtc(target);
+
+	guard(mutex)(&plane->dev->lock);
+	vkms_config_plane_detach_crtc(plane->config, crtc->config);
+}
+
+static struct configfs_item_operations plane_possible_crtcs_item_operations = {
+	.allow_link	= plane_possible_crtcs_allow_link,
+	.drop_link	= plane_possible_crtcs_drop_link,
+};
+
+static const struct config_item_type plane_possible_crtcs_group_type = {
+	.ct_item_ops	= &plane_possible_crtcs_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
 static ssize_t plane_type_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_plane *plane;
@@ -267,6 +316,11 @@ static struct config_group *make_plane_group(struct config_group *group,
 
 	config_group_init_type_name(&plane->group, name, &plane_item_type);
 
+	config_group_init_type_name(&plane->possible_crtcs_group,
+				    "possible_crtcs",
+				    &plane_possible_crtcs_group_type);
+	configfs_add_default_group(&plane->possible_crtcs_group, &plane->group);
+
 	return &plane->group;
 }
 
-- 
2.48.1

[PATCH v2 08/16] drm/vkms: Allow to configure multiple encoders via configfs

[José Expósito]

Create a default subgroup at /config/vkms/encoders to allow to create as
many encoders as required.

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           |  6 ++
 drivers/gpu/drm/vkms/vkms_configfs.c | 85 ++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 13b96837a266..e24767448775 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -76,6 +76,7 @@ And directories are created for each configurable item of the display pipeline::
   tree /config/vkms/my-vkms
   ├── crtcs
   ├── enabled
+  ├── encoders
   └── planes
 
 To add items to the display pipeline, create one or more directories under the
@@ -98,6 +99,10 @@ CRTCs have 1 configurable attribute:
 
 - writeback: Enable or disable writeback connector support by writing 1 or 0
 
+Next, create one or more encoders::
+
+  sudo mkdir /config/vkms/my-vkms/encoders/encoder0
+
 To finish the configuration, link the different pipeline items::
 
   sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/planes/plane0/possible_crtcs
@@ -119,6 +124,7 @@ And removing the top level directory and its subdirectories::
   sudo rm /config/vkms/my-vkms/planes/*/possible_crtcs/*
   sudo rmdir /config/vkms/my-vkms/planes/*
   sudo rmdir /config/vkms/my-vkms/crtcs/*
+  sudo rmdir /config/vkms/my-vkms/encoders/*
   sudo rmdir /config/vkms/my-vkms
 
 Testing With IGT
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 09d1f2224d01..cf865728abc2 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -18,6 +18,7 @@ static bool is_configfs_registered;
  * Initialized when a new directory is created under "/config/vkms/"
  * @planes_group: Default subgroup of @group at "/config/vkms/planes"
  * @crtcs_group: Default subgroup of @group at "/config/vkms/crtcs"
+ * @encoders_group: Default subgroup of @group at "/config/vkms/encoders"
  * @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
@@ -27,6 +28,7 @@ struct vkms_configfs_device {
 	struct config_group group;
 	struct config_group planes_group;
 	struct config_group crtcs_group;
+	struct config_group encoders_group;
 
 	struct mutex lock;
 	struct vkms_config *config;
@@ -63,6 +65,20 @@ struct vkms_configfs_crtc {
 	struct vkms_config_crtc *config;
 };
 
+/**
+ * struct vkms_configfs_encoder - Configfs representation of a encoder
+ *
+ * @group: Top level configuration group that represents a encoder.
+ * Initialized when a new directory is created under "/config/vkms/encoders"
+ * @dev: The vkms_configfs_device this encoder belongs to
+ * @config: Configuration of the VKMS encoder
+ */
+struct vkms_configfs_encoder {
+	struct config_group group;
+	struct vkms_configfs_device *dev;
+	struct vkms_config_encoder *config;
+};
+
 #define device_item_to_vkms_configfs_device(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_device, \
 		     group)
@@ -80,6 +96,10 @@ struct vkms_configfs_crtc {
 #define crtc_item_to_vkms_configfs_crtc(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_crtc, group)
 
+#define encoder_item_to_vkms_configfs_encoder(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_encoder, \
+		     group)
+
 static ssize_t crtc_writeback_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_crtc *crtc;
@@ -333,6 +353,67 @@ static const struct config_item_type plane_group_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static void encoder_release(struct config_item *item)
+{
+	struct vkms_configfs_encoder *encoder;
+	struct mutex *lock;
+
+	encoder = encoder_item_to_vkms_configfs_encoder(item);
+	lock = &encoder->dev->lock;
+
+	guard(mutex)(lock);
+	vkms_config_destroy_encoder(encoder->dev->config, encoder->config);
+	kfree(encoder);
+}
+
+static struct configfs_item_operations encoder_item_operations = {
+	.release	= &encoder_release,
+};
+
+static const struct config_item_type encoder_item_type = {
+	.ct_item_ops	= &encoder_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *make_encoder_group(struct config_group *group,
+					       const char *name)
+{
+	struct vkms_configfs_device *dev;
+	struct vkms_configfs_encoder *encoder;
+
+	dev = child_group_to_vkms_configfs_device(group);
+
+	guard(mutex)(&dev->lock);
+
+	if (dev->enabled)
+		return ERR_PTR(-EBUSY);
+
+	encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
+	if (!encoder)
+		return ERR_PTR(-ENOMEM);
+
+	encoder->dev = dev;
+
+	encoder->config = vkms_config_create_encoder(dev->config);
+	if (IS_ERR(encoder->config)) {
+		kfree(encoder);
+		return ERR_CAST(encoder->config);
+	}
+
+	config_group_init_type_name(&encoder->group, name, &encoder_item_type);
+
+	return &encoder->group;
+}
+
+static struct configfs_group_operations encoders_group_operations = {
+	.make_group	= &make_encoder_group,
+};
+
+static const struct config_item_type encoder_group_type = {
+	.ct_group_ops	= &encoders_group_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
 static ssize_t device_enabled_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_device *dev;
@@ -433,6 +514,10 @@ static struct config_group *make_device_group(struct config_group *group,
 				    &crtc_group_type);
 	configfs_add_default_group(&dev->crtcs_group, &dev->group);
 
+	config_group_init_type_name(&dev->encoders_group, "encoders",
+				    &encoder_group_type);
+	configfs_add_default_group(&dev->encoders_group, &dev->group);
+
 	return &dev->group;
 }
 
-- 
2.48.1

[PATCH v2 09/16] drm/vkms: Allow to attach encoders and CRTCs via configfs

[José Expósito]

Create a default subgroup at
/config/vkms/encoders/encoder/possible_crtcs that will contain symbolic
links to the possible CRTCs for the encoder.

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           |  2 +
 drivers/gpu/drm/vkms/vkms_configfs.c | 55 ++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index e24767448775..650dbfa76f59 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -106,6 +106,7 @@ Next, create one or more encoders::
 To finish the configuration, link the different pipeline items::
 
   sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/planes/plane0/possible_crtcs
+  sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/encoders/encoder0/possible_crtcs
 
 Since at least one primary plane is required, make sure to set the right type::
 
@@ -122,6 +123,7 @@ Finally, you can remove the VKMS instance disabling it::
 And removing the top level directory and its subdirectories::
 
   sudo rm /config/vkms/my-vkms/planes/*/possible_crtcs/*
+  sudo rm /config/vkms/my-vkms/encoders/*/possible_crtcs/*
   sudo rmdir /config/vkms/my-vkms/planes/*
   sudo rmdir /config/vkms/my-vkms/crtcs/*
   sudo rmdir /config/vkms/my-vkms/encoders/*
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index cf865728abc2..64aa10cd3156 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -70,11 +70,13 @@ struct vkms_configfs_crtc {
  *
  * @group: Top level configuration group that represents a encoder.
  * Initialized when a new directory is created under "/config/vkms/encoders"
+ * @possible_crtcs_group: Default subgroup of @group at "encoder/possible_crtcs"
  * @dev: The vkms_configfs_device this encoder belongs to
  * @config: Configuration of the VKMS encoder
  */
 struct vkms_configfs_encoder {
 	struct config_group group;
+	struct config_group possible_crtcs_group;
 	struct vkms_configfs_device *dev;
 	struct vkms_config_encoder *config;
 };
@@ -100,6 +102,10 @@ struct vkms_configfs_encoder {
 	container_of(to_config_group((item)), struct vkms_configfs_encoder, \
 		     group)
 
+#define encoder_possible_crtcs_item_to_vkms_configfs_encoder(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_encoder, \
+		     possible_crtcs_group)
+
 static ssize_t crtc_writeback_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_crtc *crtc;
@@ -353,6 +359,49 @@ static const struct config_item_type plane_group_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static int encoder_possible_crtcs_allow_link(struct config_item *src,
+					     struct config_item *target)
+{
+	struct vkms_configfs_encoder *encoder;
+	struct vkms_configfs_crtc *crtc;
+
+	if (target->ci_type != &crtc_item_type)
+		return -EINVAL;
+
+	encoder = encoder_possible_crtcs_item_to_vkms_configfs_encoder(src);
+	crtc = crtc_item_to_vkms_configfs_crtc(target);
+
+	guard(mutex)(&encoder->dev->lock);
+
+	if (encoder->dev->enabled)
+		return -EBUSY;
+
+	return vkms_config_encoder_attach_crtc(encoder->config, crtc->config);
+}
+
+static void encoder_possible_crtcs_drop_link(struct config_item *src,
+					     struct config_item *target)
+{
+	struct vkms_configfs_encoder *encoder;
+	struct vkms_configfs_crtc *crtc;
+
+	encoder = encoder_possible_crtcs_item_to_vkms_configfs_encoder(src);
+	crtc = crtc_item_to_vkms_configfs_crtc(target);
+
+	guard(mutex)(&encoder->dev->lock);
+	vkms_config_encoder_detach_crtc(encoder->config, crtc->config);
+}
+
+static struct configfs_item_operations encoder_possible_crtcs_item_operations = {
+	.allow_link	= encoder_possible_crtcs_allow_link,
+	.drop_link	= encoder_possible_crtcs_drop_link,
+};
+
+static const struct config_item_type encoder_possible_crtcs_group_type = {
+	.ct_item_ops	= &encoder_possible_crtcs_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
 static void encoder_release(struct config_item *item)
 {
 	struct vkms_configfs_encoder *encoder;
@@ -402,6 +451,12 @@ static struct config_group *make_encoder_group(struct config_group *group,
 
 	config_group_init_type_name(&encoder->group, name, &encoder_item_type);
 
+	config_group_init_type_name(&encoder->possible_crtcs_group,
+				    "possible_crtcs",
+				    &encoder_possible_crtcs_group_type);
+	configfs_add_default_group(&encoder->possible_crtcs_group,
+				   &encoder->group);
+
 	return &encoder->group;
 }
 
-- 
2.48.1

[PATCH v2 10/16] drm/vkms: Allow to configure multiple connectors via configfs

[José Expósito]

Create a default subgroup at
/config/vkms/connectors to allow to create as many connectors as
required.

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           |  6 ++
 drivers/gpu/drm/vkms/vkms_configfs.c | 86 ++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 650dbfa76f59..744e2355db23 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -74,6 +74,7 @@ By default, the instance is disabled::
 And directories are created for each configurable item of the display pipeline::
 
   tree /config/vkms/my-vkms
+  ├── connectors
   ├── crtcs
   ├── enabled
   ├── encoders
@@ -103,6 +104,10 @@ Next, create one or more encoders::
 
   sudo mkdir /config/vkms/my-vkms/encoders/encoder0
 
+Last but not least, create one or more connectors::
+
+  sudo mkdir /config/vkms/my-vkms/connectors/connector0
+
 To finish the configuration, link the different pipeline items::
 
   sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/planes/plane0/possible_crtcs
@@ -127,6 +132,7 @@ And removing the top level directory and its subdirectories::
   sudo rmdir /config/vkms/my-vkms/planes/*
   sudo rmdir /config/vkms/my-vkms/crtcs/*
   sudo rmdir /config/vkms/my-vkms/encoders/*
+  sudo rmdir /config/vkms/my-vkms/connectors/*
   sudo rmdir /config/vkms/my-vkms
 
 Testing With IGT
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 64aa10cd3156..bcbf91fbcd44 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -19,6 +19,7 @@ static bool is_configfs_registered;
  * @planes_group: Default subgroup of @group at "/config/vkms/planes"
  * @crtcs_group: Default subgroup of @group at "/config/vkms/crtcs"
  * @encoders_group: Default subgroup of @group at "/config/vkms/encoders"
+ * @connectors_group: Default subgroup of @group at "/config/vkms/connectors"
  * @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
@@ -29,6 +30,7 @@ struct vkms_configfs_device {
 	struct config_group planes_group;
 	struct config_group crtcs_group;
 	struct config_group encoders_group;
+	struct config_group connectors_group;
 
 	struct mutex lock;
 	struct vkms_config *config;
@@ -81,6 +83,20 @@ struct vkms_configfs_encoder {
 	struct vkms_config_encoder *config;
 };
 
+/**
+ * struct vkms_configfs_connector - Configfs representation of a connector
+ *
+ * @group: Top level configuration group that represents a connector.
+ * Initialized when a new directory is created under "/config/vkms/connectors"
+ * @dev: The vkms_configfs_device this connector belongs to
+ * @config: Configuration of the VKMS connector
+ */
+struct vkms_configfs_connector {
+	struct config_group group;
+	struct vkms_configfs_device *dev;
+	struct vkms_config_connector *config;
+};
+
 #define device_item_to_vkms_configfs_device(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_device, \
 		     group)
@@ -106,6 +122,10 @@ struct vkms_configfs_encoder {
 	container_of(to_config_group((item)), struct vkms_configfs_encoder, \
 		     possible_crtcs_group)
 
+#define connector_item_to_vkms_configfs_connector(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_connector, \
+		     group)
+
 static ssize_t crtc_writeback_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_crtc *crtc;
@@ -469,6 +489,68 @@ static const struct config_item_type encoder_group_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static void connector_release(struct config_item *item)
+{
+	struct vkms_configfs_connector *connector;
+	struct mutex *lock;
+
+	connector = connector_item_to_vkms_configfs_connector(item);
+	lock = &connector->dev->lock;
+
+	guard(mutex)(lock);
+	vkms_config_destroy_connector(connector->config);
+	kfree(connector);
+}
+
+static struct configfs_item_operations connector_item_operations = {
+	.release	= &connector_release,
+};
+
+static const struct config_item_type connector_item_type = {
+	.ct_item_ops	= &connector_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *make_connector_group(struct config_group *group,
+						 const char *name)
+{
+	struct vkms_configfs_device *dev;
+	struct vkms_configfs_connector *connector;
+
+	dev = child_group_to_vkms_configfs_device(group);
+
+	guard(mutex)(&dev->lock);
+
+	if (dev->enabled)
+		return ERR_PTR(-EBUSY);
+
+	connector = kzalloc(sizeof(*connector), GFP_KERNEL);
+	if (!connector)
+		return ERR_PTR(-ENOMEM);
+
+	connector->dev = dev;
+
+	connector->config = vkms_config_create_connector(dev->config);
+	if (IS_ERR(connector->config)) {
+		kfree(connector);
+		return ERR_CAST(connector->config);
+	}
+
+	config_group_init_type_name(&connector->group, name,
+				    &connector_item_type);
+
+	return &connector->group;
+}
+
+static struct configfs_group_operations connectors_group_operations = {
+	.make_group	= &make_connector_group,
+};
+
+static const struct config_item_type connector_group_type = {
+	.ct_group_ops	= &connectors_group_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
 static ssize_t device_enabled_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_device *dev;
@@ -573,6 +655,10 @@ static struct config_group *make_device_group(struct config_group *group,
 				    &encoder_group_type);
 	configfs_add_default_group(&dev->encoders_group, &dev->group);
 
+	config_group_init_type_name(&dev->connectors_group, "connectors",
+				    &connector_group_type);
+	configfs_add_default_group(&dev->connectors_group, &dev->group);
+
 	return &dev->group;
 }
 
-- 
2.48.1

[PATCH v2 11/16] drm/vkms: Allow to attach connectors and encoders via configfs

[José Expósito]

Create a default subgroup at
/config/vkms/connectors/connector/possible_encoders that will contain
symbolic links to the possible encoders for the connector.

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           |  2 +
 drivers/gpu/drm/vkms/vkms_configfs.c | 58 ++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 744e2355db23..74126d2e32e4 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -112,6 +112,7 @@ To finish the configuration, link the different pipeline items::
 
   sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/planes/plane0/possible_crtcs
   sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/encoders/encoder0/possible_crtcs
+  sudo ln -s /config/vkms/my-vkms/encoders/encoder0 /config/vkms/my-vkms/connectors/connector0/possible_encoders
 
 Since at least one primary plane is required, make sure to set the right type::
 
@@ -129,6 +130,7 @@ And removing the top level directory and its subdirectories::
 
   sudo rm /config/vkms/my-vkms/planes/*/possible_crtcs/*
   sudo rm /config/vkms/my-vkms/encoders/*/possible_crtcs/*
+  sudo rm /config/vkms/my-vkms/connectors/*/possible_encoders/*
   sudo rmdir /config/vkms/my-vkms/planes/*
   sudo rmdir /config/vkms/my-vkms/crtcs/*
   sudo rmdir /config/vkms/my-vkms/encoders/*
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index bcbf91fbcd44..d4d36d2c8f3d 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -88,11 +88,14 @@ struct vkms_configfs_encoder {
  *
  * @group: Top level configuration group that represents a connector.
  * Initialized when a new directory is created under "/config/vkms/connectors"
+ * @possible_encoders_group: Default subgroup of @group at
+ * "connector/possible_encoders"
  * @dev: The vkms_configfs_device this connector belongs to
  * @config: Configuration of the VKMS connector
  */
 struct vkms_configfs_connector {
 	struct config_group group;
+	struct config_group possible_encoders_group;
 	struct vkms_configfs_device *dev;
 	struct vkms_config_connector *config;
 };
@@ -126,6 +129,10 @@ struct vkms_configfs_connector {
 	container_of(to_config_group((item)), struct vkms_configfs_connector, \
 		     group)
 
+#define connector_possible_encoders_item_to_vkms_configfs_connector(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_connector, \
+		     possible_encoders_group)
+
 static ssize_t crtc_writeback_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_crtc *crtc;
@@ -511,6 +518,51 @@ static const struct config_item_type connector_item_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static int connector_possible_encoders_allow_link(struct config_item *src,
+						  struct config_item *target)
+{
+	struct vkms_configfs_connector *connector;
+	struct vkms_configfs_encoder *encoder;
+
+	if (target->ci_type != &encoder_item_type)
+		return -EINVAL;
+
+	connector = connector_possible_encoders_item_to_vkms_configfs_connector(src);
+	encoder = encoder_item_to_vkms_configfs_encoder(target);
+
+	guard(mutex)(&connector->dev->lock);
+
+	if (connector->dev->enabled)
+		return -EBUSY;
+
+	return vkms_config_connector_attach_encoder(connector->config,
+						    encoder->config);
+}
+
+static void connector_possible_encoders_drop_link(struct config_item *src,
+						  struct config_item *target)
+{
+	struct vkms_configfs_connector *connector;
+	struct vkms_configfs_encoder *encoder;
+
+	connector = connector_possible_encoders_item_to_vkms_configfs_connector(src);
+	encoder = encoder_item_to_vkms_configfs_encoder(target);
+
+	guard(mutex)(&connector->dev->lock);
+	vkms_config_connector_detach_encoder(connector->config,
+					     encoder->config);
+}
+
+static struct configfs_item_operations connector_possible_encoders_item_operations = {
+	.allow_link	= connector_possible_encoders_allow_link,
+	.drop_link	= connector_possible_encoders_drop_link,
+};
+
+static const struct config_item_type connector_possible_encoders_group_type = {
+	.ct_item_ops	= &connector_possible_encoders_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
 static struct config_group *make_connector_group(struct config_group *group,
 						 const char *name)
 {
@@ -539,6 +591,12 @@ static struct config_group *make_connector_group(struct config_group *group,
 	config_group_init_type_name(&connector->group, name,
 				    &connector_item_type);
 
+	config_group_init_type_name(&connector->possible_encoders_group,
+				    "possible_encoders",
+				    &connector_possible_encoders_group_type);
+	configfs_add_default_group(&connector->possible_encoders_group,
+				   &connector->group);
+
 	return &connector->group;
 }
 
-- 
2.48.1

[PATCH v2 12/16] drm/vkms: Allow to configure the default device creation

[José Expósito]

Add a new module param to allow to create or not the default VKMS
instance. Useful when combined with configfs to avoid having additional
VKMS instances.

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 5bcfbcb6c0c5..b4ed19c97576 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -50,6 +50,10 @@ static bool enable_overlay;
 module_param_named(enable_overlay, enable_overlay, bool, 0444);
 MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
 
+static bool create_default_dev = true;
+module_param_named(create_default_dev, create_default_dev, bool, 0444);
+MODULE_PARM_DESC(create_default_dev, "Create or not the default VKMS device");
+
 DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
 
 static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
@@ -219,6 +223,9 @@ static int __init vkms_init(void)
 	if (ret)
 		return ret;
 
+	if (!create_default_dev)
+		return 0;
+
 	config = vkms_config_default_create(enable_cursor, enable_writeback, enable_overlay);
 	if (IS_ERR(config))
 		return PTR_ERR(config);
-- 
2.48.1

[PATCH v2 13/16] drm/vkms: Remove completed task from the TODO list

[José Expósito]

Remove the configfs related TODO items from the "Runtime Configuration"
section.

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 Documentation/gpu/vkms.rst | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 74126d2e32e4..c551241fe873 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -222,21 +222,14 @@ Runtime Configuration
 ---------------------
 
 We want to be able to reconfigure vkms instance without having to reload the
-module. Use/Test-cases:
+module through configfs. Use/Test-cases:
 
 - Hotplug/hotremove connectors on the fly (to be able to test DP MST handling
   of compositors).
 
-- Configure planes/crtcs/connectors (we'd need some code to have more than 1 of
-  them first).
-
 - Change output configuration: Plug/unplug screens, change EDID, allow changing
   the refresh rate.
 
-The currently proposed solution is to expose vkms configuration through
-configfs. All existing module options should be supported through configfs
-too.
-
 Writeback support
 -----------------
 
-- 
2.48.1

[PATCH v2 14/16] drm/vkms: Allow to configure connector status

[José Expósito]

Allow to store the connector status in vkms_config_connector and add a
getter and a setter functions as well a KUnit test.

This change only adds the configuration, the connector status is not
used yet.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/vkms/tests/vkms_config_test.c | 24 +++++++++++++++++
 drivers/gpu/drm/vkms/vkms_config.c            |  8 ++++--
 drivers/gpu/drm/vkms/vkms_config.h            | 26 +++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
index ff4566cf9925..3574a829a6ed 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
@@ -916,6 +916,29 @@ static void vkms_config_test_connector_get_possible_encoders(struct kunit *test)
 	vkms_config_destroy(config);
 }
 
+static void vkms_config_test_connector_status(struct kunit *test)
+{
+	struct vkms_config *config;
+	struct vkms_config_connector *connector_cfg;
+	enum drm_connector_status status;
+
+	config = vkms_config_create("test");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
+
+	connector_cfg = vkms_config_create_connector(config);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, connector_cfg);
+
+	status = vkms_config_connector_get_status(connector_cfg);
+	KUNIT_EXPECT_EQ(test, status, connector_status_connected);
+
+	vkms_config_connector_set_status(connector_cfg,
+					 connector_status_disconnected);
+	status = vkms_config_connector_get_status(connector_cfg);
+	KUNIT_EXPECT_EQ(test, status, connector_status_disconnected);
+
+	vkms_config_destroy(config);
+}
+
 static struct kunit_case vkms_config_test_cases[] = {
 	KUNIT_CASE(vkms_config_test_empty_config),
 	KUNIT_CASE_PARAM(vkms_config_test_default_config,
@@ -937,6 +960,7 @@ static struct kunit_case vkms_config_test_cases[] = {
 	KUNIT_CASE(vkms_config_test_plane_get_possible_crtcs),
 	KUNIT_CASE(vkms_config_test_encoder_get_possible_crtcs),
 	KUNIT_CASE(vkms_config_test_connector_get_possible_encoders),
+	KUNIT_CASE(vkms_config_test_connector_status),
 	{}
 };
 
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index a1df5659b0fb..f8394a063ecf 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -361,8 +361,11 @@ static int vkms_config_show(struct seq_file *m, void *data)
 	vkms_config_for_each_encoder(vkmsdev->config, encoder_cfg)
 		seq_puts(m, "encoder\n");
 
-	vkms_config_for_each_connector(vkmsdev->config, connector_cfg)
-		seq_puts(m, "connector\n");
+	vkms_config_for_each_connector(vkmsdev->config, connector_cfg) {
+		seq_puts(m, "connector:\n");
+		seq_printf(m, "\tstatus=%d\n",
+			   vkms_config_connector_get_status(connector_cfg));
+	}
 
 	return 0;
 }
@@ -588,6 +591,7 @@ struct vkms_config_connector *vkms_config_create_connector(struct vkms_config *c
 		return ERR_PTR(-ENOMEM);
 
 	connector_cfg->config = config;
+	connector_cfg->status = connector_status_connected;
 	xa_init_flags(&connector_cfg->possible_encoders, XA_FLAGS_ALLOC);
 
 	list_add_tail(&connector_cfg->link, &config->connectors);
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 0118e3f99706..e202b5a84ddd 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -7,6 +7,8 @@
 #include <linux/types.h>
 #include <linux/xarray.h>
 
+#include <drm/drm_connector.h>
+
 #include "vkms_drv.h"
 
 /**
@@ -99,6 +101,7 @@ struct vkms_config_encoder {
  *
  * @link: Link to the others connector in vkms_config
  * @config: The vkms_config this connector belongs to
+ * @status: Status (connected, disconnected...) of the connector
  * @possible_encoders: Array of encoders that can be used with this connector
  * @connector: Internal usage. This pointer should never be considered as valid.
  *             It can be used to store a temporary reference to a VKMS connector
@@ -109,6 +112,7 @@ struct vkms_config_connector {
 	struct list_head link;
 	struct vkms_config *config;
 
+	enum drm_connector_status status;
 	struct xarray possible_encoders;
 
 	/* Internal usage */
@@ -434,4 +438,26 @@ int __must_check vkms_config_connector_attach_encoder(struct vkms_config_connect
 void vkms_config_connector_detach_encoder(struct vkms_config_connector *connector_cfg,
 					  struct vkms_config_encoder *encoder_cfg);
 
+/**
+ * vkms_config_connector_get_status() - Return the status of the connector
+ * @connector_cfg: Connector to get the status from
+ */
+static inline enum drm_connector_status
+vkms_config_connector_get_status(struct vkms_config_connector *connector_cfg)
+{
+	return connector_cfg->status;
+}
+
+/**
+ * vkms_config_crtc_set_writeback() - If a writeback connector will be created
+ * @crtc_cfg: Target CRTC
+ * @writeback: Enable or disable the writeback connector
+ */
+static inline void
+vkms_config_connector_set_status(struct vkms_config_connector *connector_cfg,
+				 enum drm_connector_status status)
+{
+	connector_cfg->status = status;
+}
+
 #endif /* _VKMS_CONFIG_H_ */
-- 
2.48.1

[PATCH v2 15/16] drm/vkms: Allow to update the connector status

[José Expósito]

Implement the drm_connector_funcs.detect() callback to update the
connector status by returning the status stored in the configuration.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_connector.c | 19 ++++++++++++++++++-
 drivers/gpu/drm/vkms/vkms_connector.h | 12 +++++++++++-
 drivers/gpu/drm/vkms/vkms_output.c    |  2 +-
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
index 48b10cba322a..b03a00b5803a 100644
--- a/drivers/gpu/drm/vkms/vkms_connector.c
+++ b/drivers/gpu/drm/vkms/vkms_connector.c
@@ -5,9 +5,23 @@
 #include <drm/drm_managed.h>
 #include <drm/drm_probe_helper.h>
 
+#include "vkms_config.h"
 #include "vkms_connector.h"
 
+static enum drm_connector_status vkms_connector_detect(struct drm_connector *connector,
+						       bool force)
+{
+	struct vkms_connector *vkms_connector;
+	enum drm_connector_status status;
+
+	vkms_connector = drm_connector_to_vkms_connector(connector);
+	status = vkms_config_connector_get_status(vkms_connector->connector_cfg);
+
+	return status;
+}
+
 static const struct drm_connector_funcs vkms_connector_funcs = {
+	.detect = vkms_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
@@ -40,7 +54,8 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.best_encoder = vkms_conn_best_encoder,
 };
 
-struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev)
+struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev,
+					   struct vkms_config_connector *connector_cfg)
 {
 	struct drm_device *dev = &vkmsdev->drm;
 	struct vkms_connector *connector;
@@ -50,6 +65,8 @@ struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev)
 	if (!connector)
 		return ERR_PTR(-ENOMEM);
 
+	connector->connector_cfg = connector_cfg;
+
 	ret = drmm_connector_init(dev, &connector->base, &vkms_connector_funcs,
 				  DRM_MODE_CONNECTOR_VIRTUAL, NULL);
 	if (ret)
diff --git a/drivers/gpu/drm/vkms/vkms_connector.h b/drivers/gpu/drm/vkms/vkms_connector.h
index c9149c1b7af0..5ab8a6d65182 100644
--- a/drivers/gpu/drm/vkms/vkms_connector.h
+++ b/drivers/gpu/drm/vkms/vkms_connector.h
@@ -5,22 +5,32 @@
 
 #include "vkms_drv.h"
 
+struct vkms_config_connector;
+
+#define drm_connector_to_vkms_connector(target) \
+	container_of(target, struct vkms_connector, base)
+
 /**
  * struct vkms_connector - VKMS custom type wrapping around the DRM connector
  *
  * @drm: Base DRM connector
+ * @connector_cfg: Connector configuration
  */
 struct vkms_connector {
 	struct drm_connector base;
+
+	struct vkms_config_connector *connector_cfg;
 };
 
 /**
  * vkms_connector_init() - Initialize a connector
  * @vkmsdev: VKMS device containing the connector
+ * @connector_cfg: Configuration for the connector
  *
  * Returns:
  * The connector or an error on failure.
  */
-struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev);
+struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev,
+					   struct vkms_config_connector *connector_cfg);
 
 #endif /* _VKMS_CONNECTOR_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 8d7ca0cdd79f..3af95983026e 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -87,7 +87,7 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 		struct vkms_config_encoder *possible_encoder;
 		unsigned long idx = 0;
 
-		connector_cfg->connector = vkms_connector_init(vkmsdev);
+		connector_cfg->connector = vkms_connector_init(vkmsdev, connector_cfg);
 		if (IS_ERR(connector_cfg->connector)) {
 			DRM_ERROR("Failed to init connector\n");
 			return PTR_ERR(connector_cfg->connector);
-- 
2.48.1

[PATCH v2 16/16] drm/vkms: Allow to configure connector status via configfs

[José Expósito]

When a connector is created, add a `status` file to allow to update the
connector status to:

 - 1 connector_status_connected
 - 2 connector_status_disconnected
 - 3 connector_status_unknown

If the device is enabled, updating the status hot-plug or unplugs the
connector.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 Documentation/gpu/vkms.rst            |  5 +++
 drivers/gpu/drm/vkms/vkms_configfs.c  | 46 +++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_connector.c |  7 ++++
 drivers/gpu/drm/vkms/vkms_connector.h |  6 ++++
 4 files changed, 64 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index c551241fe873..7c54099b1dc6 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -108,6 +108,11 @@ Last but not least, create one or more connectors::
 
   sudo mkdir /config/vkms/my-vkms/connectors/connector0
 
+Connectors have 1 configurable attribute:
+
+- status: Connection status: 1 connected, 2 disconnected, 3 unknown (same values
+  as those exposed by the "status" property of a connector)
+
 To finish the configuration, link the different pipeline items::
 
   sudo ln -s /config/vkms/my-vkms/crtcs/crtc0 /config/vkms/my-vkms/planes/plane0/possible_crtcs
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index d4d36d2c8f3d..76afb0245388 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -7,6 +7,7 @@
 #include "vkms_drv.h"
 #include "vkms_config.h"
 #include "vkms_configfs.h"
+#include "vkms_connector.h"
 
 /* To avoid registering configfs more than once or unregistering on error */
 static bool is_configfs_registered;
@@ -496,6 +497,50 @@ static const struct config_item_type encoder_group_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static ssize_t connector_status_show(struct config_item *item, char *page)
+{
+	struct vkms_configfs_connector *connector;
+
+	connector = connector_item_to_vkms_configfs_connector(item);
+
+	guard(mutex)(&connector->dev->lock);
+	return sprintf(page, "%u",
+		       vkms_config_connector_get_status(connector->config));
+}
+
+static ssize_t connector_status_store(struct config_item *item,
+				      const char *page, size_t count)
+{
+	struct vkms_configfs_connector *connector;
+	enum drm_connector_status status;
+
+	connector = connector_item_to_vkms_configfs_connector(item);
+
+	if (kstrtouint(page, 10, &status))
+		return -EINVAL;
+
+	if (status != connector_status_connected &&
+	    status != connector_status_disconnected &&
+	    status != connector_status_unknown)
+		return -EINVAL;
+
+	guard(mutex)(&connector->dev->lock);
+
+	vkms_config_connector_set_status(connector->config, status);
+
+	if (connector->dev->enabled)
+		vkms_trigger_connector_hotplug(connector->dev->config->dev);
+
+	return (ssize_t)count;
+}
+
+CONFIGFS_ATTR(connector_, status);
+
+static struct configfs_attribute *connector_item_attrs[] = {
+	&connector_attr_status,
+	NULL,
+};
+
 static void connector_release(struct config_item *item)
 {
 	struct vkms_configfs_connector *connector;
@@ -514,6 +559,7 @@ static struct configfs_item_operations connector_item_operations = {
 };
 
 static const struct config_item_type connector_item_type = {
+	.ct_attrs	= connector_item_attrs,
 	.ct_item_ops	= &connector_item_operations,
 	.ct_owner	= THIS_MODULE,
 };
diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
index b03a00b5803a..ed99270c590f 100644
--- a/drivers/gpu/drm/vkms/vkms_connector.c
+++ b/drivers/gpu/drm/vkms/vkms_connector.c
@@ -76,3 +76,10 @@ struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev,
 
 	return connector;
 }
+
+void vkms_trigger_connector_hotplug(struct vkms_device *vkmsdev)
+{
+	struct drm_device *dev = &vkmsdev->drm;
+
+	drm_kms_helper_hotplug_event(dev);
+}
diff --git a/drivers/gpu/drm/vkms/vkms_connector.h b/drivers/gpu/drm/vkms/vkms_connector.h
index 5ab8a6d65182..7865f54a313c 100644
--- a/drivers/gpu/drm/vkms/vkms_connector.h
+++ b/drivers/gpu/drm/vkms/vkms_connector.h
@@ -33,4 +33,10 @@ struct vkms_connector {
 struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev,
 					   struct vkms_config_connector *connector_cfg);
 
+/**
+ * struct vkms_device *vkmsdev() - Update the device's connectors status
+ * @vkmsdev: VKMS device to update
+ */
+void vkms_trigger_connector_hotplug(struct vkms_device *vkmsdev);
+
 #endif /* _VKMS_CONNECTOR_H_ */
-- 
2.48.1

Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[Louis Chauvet]

Le 25/02/2025 à 18:59, José Expósito a écrit :
> Create a default subgroup at /config/vkms/planes to allow to create as
> many planes as required.
> 
> 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           | 16 ++++-
>   drivers/gpu/drm/vkms/vkms_configfs.c | 87 ++++++++++++++++++++++++++++
>   2 files changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index 423bdf86b5b1..bf23d0da33fe 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -71,6 +71,19 @@ By default, the instance is disabled::
>     cat /config/vkms/my-vkms/enabled
>     0
>   
> +And directories are created for each configurable item of the display pipeline::
> +
> +  tree /config/vkms/my-vkms
> +  ├── enabled
> +  └── planes
> +
> +To add items to the display pipeline, create one or more directories under the
> +available paths.
> +
> +Start by creating one or more planes::
> +
> +  sudo mkdir /config/vkms/my-vkms/planes/plane0
> +
>   Once you are done configuring the VKMS instance, enable it::
>   
>     echo "1" | sudo tee /config/vkms/my-vkms/enabled
> @@ -79,8 +92,9 @@ Finally, you can remove the VKMS instance disabling it::
>   
>     echo "0" | sudo tee /config/vkms/my-vkms/enabled
>   
> -And removing the top level directory::
> +And removing the top level directory and its subdirectories::
>   
> +  sudo rmdir /config/vkms/my-vkms/planes/*
>     sudo rmdir /config/vkms/my-vkms
>   
>   Testing With IGT
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index 92512d52ddae..4f9d3341e6c0 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -16,6 +16,7 @@ static bool is_configfs_registered;
>    *
>    * @group: Top level configuration group that represents a VKMS device.
>    * Initialized when a new directory is created under "/config/vkms/"
> + * @planes_group: Default subgroup of @group at "/config/vkms/planes"
>    * @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
> @@ -23,16 +24,98 @@ static bool is_configfs_registered;
>    */
>   struct vkms_configfs_device {
>   	struct config_group group;
> +	struct config_group planes_group;
>   
>   	struct mutex lock;
>   	struct vkms_config *config;
>   	bool enabled;
>   };
>   
> +/**
> + * struct vkms_configfs_plane - Configfs representation of a plane
> + *
> + * @group: Top level configuration group that represents a plane.
> + * Initialized when a new directory is created under "/config/vkms/planes"
> + * @dev: The vkms_configfs_device this plane belongs to
> + * @config: Configuration of the VKMS plane
> + */
> +struct vkms_configfs_plane {
> +	struct config_group group;
> +	struct vkms_configfs_device *dev;
> +	struct vkms_config_plane *config;
> +};
> +
>   #define device_item_to_vkms_configfs_device(item) \
>   	container_of(to_config_group((item)), struct vkms_configfs_device, \
>   		     group)
>   
> +#define child_group_to_vkms_configfs_device(group) \
> +	device_item_to_vkms_configfs_device((&(group)->cg_item)->ci_parent)
> +
> +#define plane_item_to_vkms_configfs_plane(item) \
> +	container_of(to_config_group((item)), struct vkms_configfs_plane, group)
> +
> +static void plane_release(struct config_item *item)
> +{
> +	struct vkms_configfs_plane *plane;
> +	struct mutex *lock;
> +
> +	plane = plane_item_to_vkms_configfs_plane(item);
> +	lock = &plane->dev->lock;
> +
> +	guard(mutex)(lock);
> +	vkms_config_destroy_plane(plane->config);
> +	kfree(plane);
> +}

I just found a flaw in our work: there is currently no way to forbid the 
deletion of item/symlinks...

If you do:

modprobe vkms
cd /sys/kernel/config/vkms/
mkdir DEV
mkdir DEV/connectors/CON
mkdir DEV/planes/PLA
mkdir DEV/crtcs/CRT
mkdir DEV/encoders/ENC
ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
echo 1 > DEV/planes/PLA/type
tree
echo 1 > DEV/enabled
modetest -M vkms
=> everything fine

rm DEV/connectors/CON/possible_encoders/ENC
rmdir DEV/connectors/CON
modetest -M vkms
=> BUG: KASAN: slab-use-after-free


I see two solutions:
- we don't care and keep as is: if the device is enabled, and you delete 
link/groups, it is your fault. As shown above: it can crash the kernel, 
so it is a no-go.

- we care and we don't want to touch configfs: we need to implement a 
kind of refcount for all vkms_config elements. Issue: non-trivial work, 
may allow memory leaks/use after free...

- we care and we want to touch configfs: see my two patches (they apply 
on the v1 of this series). This solution allows adding a check before 
removing configfs item/group/link. I found it cleaner and way easier to 
understand.

What do you think about my proposition? Do you have another idea?

> +static struct configfs_item_operations plane_item_operations = {
> +	.release	= &plane_release,
> +};
> +
> +static const struct config_item_type plane_item_type = {
> +	.ct_item_ops	= &plane_item_operations,
> +	.ct_owner	= THIS_MODULE,
> +};
> +
> +static struct config_group *make_plane_group(struct config_group *group,
> +					     const char *name)
> +{
> +	struct vkms_configfs_device *dev;
> +	struct vkms_configfs_plane *plane;
> +
> +	dev = child_group_to_vkms_configfs_device(group);
> +
> +	guard(mutex)(&dev->lock);
> +
> +	if (dev->enabled)
> +		return ERR_PTR(-EBUSY);
> +
> +	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> +	if (!plane)
> +		return ERR_PTR(-ENOMEM);
> +
> +	plane->dev = dev;
> +
> +	plane->config = vkms_config_create_plane(dev->config);
> +	if (IS_ERR(plane->config)) {
> +		kfree(plane);
> +		return ERR_CAST(plane->config);
> +	}
> +
> +	config_group_init_type_name(&plane->group, name, &plane_item_type);
> +
> +	return &plane->group;
> +}
> +
> +static struct configfs_group_operations planes_group_operations = {
> +	.make_group	= &make_plane_group,
> +};
> +
> +static const struct config_item_type plane_group_type = {
> +	.ct_group_ops	= &planes_group_operations,
> +	.ct_owner	= THIS_MODULE,
> +};
> +
>   static ssize_t device_enabled_show(struct config_item *item, char *page)
>   {
>   	struct vkms_configfs_device *dev;
> @@ -125,6 +208,10 @@ static struct config_group *make_device_group(struct config_group *group,
>   	config_group_init_type_name(&dev->group, name, &device_item_type);
>   	mutex_init(&dev->lock);
>   
> +	config_group_init_type_name(&dev->planes_group, "planes",
> +				    &plane_group_type);
> +	configfs_add_default_group(&dev->planes_group, &dev->group);
> +
>   	return &dev->group;
>   }
>   

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

[PATCH 2/2] configfs: Add mechanism to prevent item/group deletion

[Louis Chauvet]

Introduce a new mechanism in configfs to prevent the deletion of certain 
item/group.
This is particularly useful in scenarios where userspace should not be 
allowed
to modify the configfs structure under some conditions, such as in VKMS.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
  drivers/gpu/drm/vkms/vkms_configfs.c | 21 +++++++++++++++++++++
  fs/configfs/dir.c                    |  8 +++++++-
  include/linux/configfs.h             |  1 +
  3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c 
b/drivers/gpu/drm/vkms/vkms_configfs.c
index 8a7d954399e9..0a196a28ed4a 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -393,9 +393,30 @@ static struct configfs_item_operations 
plane_item_operations = {
  	.release	= &plane_release,
  };

+int allow_drop_plane_group(struct config_group *group, struct 
config_item *item)
+{
+	struct vkms_configfs_plane *plane;
+	bool enabled;
+
+	plane = plane_item_to_vkms_configfs_plane(item);
+
+	mutex_lock(&plane->dev->lock);
+	enabled = plane->dev->enabled;
+	mutex_unlock(&plane->dev->lock);
+
+	if (enabled)
+		return -EBUSY;
+	return 0;
+}
+
+static struct configfs_group_operations plane_group_operation ={
+	.allow_drop_item = &allow_drop_plane_group,
+};
+
  static const struct config_item_type plane_item_type = {
  	.ct_attrs	= plane_item_attrs,
  	.ct_item_ops	= &plane_item_operations,
+	.ct_group_ops   = &plane_group_operation,
  	.ct_owner	= THIS_MODULE,
  };

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 7d10278db30d..a103196af0f9 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1544,7 +1544,13 @@ static int configfs_rmdir(struct inode *dir, 
struct dentry *dentry)

  	/* Drop reference from above, item already holds one. */
  	config_item_put(parent_item);
-
+	if (item->ci_type && item->ci_type->ct_group_ops && 
item->ci_type->ct_group_ops->allow_drop_item) {
+		ret = 
item->ci_type->ct_group_ops->allow_drop_item(to_config_group(parent_item), 
item);
+		if (ret) {
+			config_item_put(item);
+			return ret;
+		}
+	}
  	if (item->ci_type)
  		dead_item_owner = item->ci_type->ct_owner;

diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 7fc52a78d6cd..92933397590d 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -216,6 +216,7 @@ struct configfs_group_operations {
  	struct config_item *(*make_item)(struct config_group *group, const 
char *name);
  	struct config_group *(*make_group)(struct config_group *group, const 
char *name);
  	void (*disconnect_notify)(struct config_group *group, struct 
config_item *item);
+	int (*allow_drop_item)(struct config_group *group, struct config_item 
*item);
  	void (*drop_item)(struct config_group *group, struct config_item *item);
  	bool (*is_visible)(struct config_item *item, struct 
configfs_attribute *attr, int n);
  	bool (*is_bin_visible)(struct config_item *item, struct 
configfs_bin_attribute *attr,
-- 
2.48.1

[PATCH 1/2] configfs: Add mechanism to prevent symlink deletion

[Louis Chauvet]

Introduce a new mechanism in configfs to prevent the deletion of certain 
symlink.
This is particularly useful in scenarios where userspace should not be 
allowed
to modify the configfs structure under some conditions, such as in VKMS.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
  drivers/gpu/drm/vkms/vkms_configfs.c | 20 ++++++++++++++++++++
  fs/configfs/symlink.c                | 12 +++++++++---
  include/linux/configfs.h             |  1 +
  3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c 
b/drivers/gpu/drm/vkms/vkms_configfs.c
index f0813536be12..8a7d954399e9 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -295,8 +295,28 @@ static void plane_possible_crtcs_drop_link(struct 
config_item *src,
  	mutex_unlock(&plane->dev->lock);
  }

+static int plane_possible_crtcs_allow_drop_link(struct config_item *src,
+					   struct config_item *target)
+{
+	struct vkms_configfs_plane *plane;
+	struct vkms_configfs_crtc *crtc;
+	bool enabled;
+
+	plane = plane_possible_crtcs_item_to_vkms_configfs_plane(src);
+	crtc = crtc_item_to_vkms_configfs_crtc(target);
+
+	mutex_lock(&plane->dev->lock);
+	enabled = plane->dev->enabled;
+	mutex_unlock(&plane->dev->lock);
+
+	if (enabled)
+		return -EBUSY;
+	return 0;
+}
+
  static struct configfs_item_operations 
plane_possible_crtcs_item_operations = {
  	.allow_link	= plane_possible_crtcs_allow_link,
+	.allow_drop_link = plane_possible_crtcs_allow_drop_link,
  	.drop_link	= plane_possible_crtcs_drop_link,
  };

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 69133ec1fac2..925e2e15eb9b 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -233,6 +233,13 @@ int configfs_unlink(struct inode *dir, struct 
dentry *dentry)
  	parent_item = configfs_get_config_item(dentry->d_parent);
  	type = parent_item->ci_type;

+	if (type && type->ct_item_ops &&
+	    type->ct_item_ops->allow_drop_link) {
+		ret = type->ct_item_ops->allow_drop_link(parent_item, 
target_sd->s_element);
+		if (ret)
+			goto out_put;
+	}
+
  	spin_lock(&configfs_dirent_lock);
  	list_del_init(&sd->s_sibling);
  	spin_unlock(&configfs_dirent_lock);
@@ -255,10 +262,9 @@ int configfs_unlink(struct inode *dir, struct 
dentry *dentry)
  	spin_unlock(&configfs_dirent_lock);
  	configfs_put(target_sd);

-	config_item_put(parent_item);
-
  	ret = 0;
-
+out_put:
+	config_item_put(parent_item);
  out:
  	return ret;
  }
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index c771e9d0d0b9..7fc52a78d6cd 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -208,6 +208,7 @@ static struct configfs_bin_attribute 
_pfx##attr_##_name = {	\
  struct configfs_item_operations {
  	void (*release)(struct config_item *);
  	int (*allow_link)(struct config_item *src, struct config_item *target);
+	int (*allow_drop_link)(struct config_item *src, struct config_item 
*target);
  	void (*drop_link)(struct config_item *src, struct config_item *target);
  };

-- 
2.48.1

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

[Louis Chauvet]

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

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

[José Expósito]

Hi Louis,

On Fri, Feb 28, 2025 at 04:19:13PM +0100, Louis Chauvet wrote:
> 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>
> > [...]
> > 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 @@
> > [...]
> > +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

It is fine, don't worry :) I'll send v3 using scoped_guard() and addressing
other review comments.

Thanks for your reviews,
Jose

Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[José Expósito]

Hi Louis,

On Fri, Feb 28, 2025 at 03:43:25PM +0100, Louis Chauvet wrote:
> 
> 
> Le 25/02/2025 à 18:59, José Expósito a écrit :
> > Create a default subgroup at /config/vkms/planes to allow to create as
> > many planes as required.
> > 
> > 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>
> > [...]
> > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > index 92512d52ddae..4f9d3341e6c0 100644
> > --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > [...]
> > +static void plane_release(struct config_item *item)
> > +{
> > +	struct vkms_configfs_plane *plane;
> > +	struct mutex *lock;
> > +
> > +	plane = plane_item_to_vkms_configfs_plane(item);
> > +	lock = &plane->dev->lock;
> > +
> > +	guard(mutex)(lock);
> > +	vkms_config_destroy_plane(plane->config);
> > +	kfree(plane);
> > +}
> 
> I just found a flaw in our work: there is currently no way to forbid the
> deletion of item/symlinks...
> 
> If you do:
> 
> modprobe vkms
> cd /sys/kernel/config/vkms/
> mkdir DEV
> mkdir DEV/connectors/CON
> mkdir DEV/planes/PLA
> mkdir DEV/crtcs/CRT
> mkdir DEV/encoders/ENC
> ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> echo 1 > DEV/planes/PLA/type
> tree
> echo 1 > DEV/enabled
> modetest -M vkms
> => everything fine
> 
> rm DEV/connectors/CON/possible_encoders/ENC
> rmdir DEV/connectors/CON
> modetest -M vkms
> => BUG: KASAN: slab-use-after-free
> 
> 
> I see two solutions:
> - we don't care and keep as is: if the device is enabled, and you delete
> link/groups, it is your fault. As shown above: it can crash the kernel, so
> it is a no-go.

I was aware of this limitation and, since the configfs is clear about
deleting items: [1]

    Important:
    drop_item() is void, and as such cannot fail. When rmdir(2) is called,
    configfs WILL remove the item from the filesystem tree (assuming that
    it has no children to keep it busy).
    The subsystem is responsible for responding to this. [...]

I decided to follow this approach, i.e., allowing the user to delete the items.

However, that use-after-free is a bug I need to fix. I was wondering how I didn't
catch it with IGT... Turns out, I didn't enable Kasan in my QEMU .config (ops!).

Do you agree on folowing this solution? If so, I'll send v3 fixing the memory
issues.

Best wishes,
Jose

[1] https://docs.kernel.org/filesystems/configfs.html

> - we care and we don't want to touch configfs: we need to implement a kind
> of refcount for all vkms_config elements. Issue: non-trivial work, may allow
> memory leaks/use after free...
> 
> - we care and we want to touch configfs: see my two patches (they apply on
> the v1 of this series). This solution allows adding a check before removing
> configfs item/group/link. I found it cleaner and way easier to understand.
> 
> What do you think about my proposition? Do you have another idea?
> 
> > +static struct configfs_item_operations plane_item_operations = {
> > +	.release	= &plane_release,
> > +};
> > +
> > +static const struct config_item_type plane_item_type = {
> > +	.ct_item_ops	= &plane_item_operations,
> > +	.ct_owner	= THIS_MODULE,
> > +};
> > +
> > +static struct config_group *make_plane_group(struct config_group *group,
> > +					     const char *name)
> > +{
> > +	struct vkms_configfs_device *dev;
> > +	struct vkms_configfs_plane *plane;
> > +
> > +	dev = child_group_to_vkms_configfs_device(group);
> > +
> > +	guard(mutex)(&dev->lock);
> > +
> > +	if (dev->enabled)
> > +		return ERR_PTR(-EBUSY);
> > +
> > +	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> > +	if (!plane)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	plane->dev = dev;
> > +
> > +	plane->config = vkms_config_create_plane(dev->config);
> > +	if (IS_ERR(plane->config)) {
> > +		kfree(plane);
> > +		return ERR_CAST(plane->config);
> > +	}
> > +
> > +	config_group_init_type_name(&plane->group, name, &plane_item_type);
> > +
> > +	return &plane->group;
> > +}
> > +
> > +static struct configfs_group_operations planes_group_operations = {
> > +	.make_group	= &make_plane_group,
> > +};
> > +
> > +static const struct config_item_type plane_group_type = {
> > +	.ct_group_ops	= &planes_group_operations,
> > +	.ct_owner	= THIS_MODULE,
> > +};
> > +
> >   static ssize_t device_enabled_show(struct config_item *item, char *page)
> >   {
> >   	struct vkms_configfs_device *dev;
> > @@ -125,6 +208,10 @@ static struct config_group *make_device_group(struct config_group *group,
> >   	config_group_init_type_name(&dev->group, name, &device_item_type);
> >   	mutex_init(&dev->lock);
> > +	config_group_init_type_name(&dev->planes_group, "planes",
> > +				    &plane_group_type);
> > +	configfs_add_default_group(&dev->planes_group, &dev->group);
> > +
> >   	return &dev->group;
> >   }
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[Louis Chauvet]

Le 03/03/2025 à 09:50, José Expósito a écrit :
> Hi Louis,
> 
> On Fri, Feb 28, 2025 at 03:43:25PM +0100, Louis Chauvet wrote:
>>
>>
>> Le 25/02/2025 à 18:59, José Expósito a écrit :
>>> Create a default subgroup at /config/vkms/planes to allow to create as
>>> many planes as required.
>>>
>>> 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>
>>> [...]
>>> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
>>> index 92512d52ddae..4f9d3341e6c0 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
>>> [...]
>>> +static void plane_release(struct config_item *item)
>>> +{
>>> +	struct vkms_configfs_plane *plane;
>>> +	struct mutex *lock;
>>> +
>>> +	plane = plane_item_to_vkms_configfs_plane(item);
>>> +	lock = &plane->dev->lock;
>>> +
>>> +	guard(mutex)(lock);
>>> +	vkms_config_destroy_plane(plane->config);
>>> +	kfree(plane);
>>> +}
>>
>> I just found a flaw in our work: there is currently no way to forbid the
>> deletion of item/symlinks...
>>
>> If you do:
>>
>> modprobe vkms
>> cd /sys/kernel/config/vkms/
>> mkdir DEV
>> mkdir DEV/connectors/CON
>> mkdir DEV/planes/PLA
>> mkdir DEV/crtcs/CRT
>> mkdir DEV/encoders/ENC
>> ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
>> ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
>> ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
>> echo 1 > DEV/planes/PLA/type
>> tree
>> echo 1 > DEV/enabled
>> modetest -M vkms
>> => everything fine
>>
>> rm DEV/connectors/CON/possible_encoders/ENC
>> rmdir DEV/connectors/CON
>> modetest -M vkms
>> => BUG: KASAN: slab-use-after-free
>>
>>
>> I see two solutions:
>> - we don't care and keep as is: if the device is enabled, and you delete
>> link/groups, it is your fault. As shown above: it can crash the kernel, so
>> it is a no-go.
> 
> I was aware of this limitation and, since the configfs is clear about
> deleting items: [1]
> 
>      Important:
>      drop_item() is void, and as such cannot fail. When rmdir(2) is called,
>      configfs WILL remove the item from the filesystem tree (assuming that
>      it has no children to keep it busy).
>      The subsystem is responsible for responding to this. [...]

Thanks for pointing out. I read it and I think you're right, they 
clearly want the user space to be able to delete any item at any time.

> I decided to follow this approach, i.e., allowing the user to delete the items.

I think a simple fix would be to have something like that:

int device_enabled_store(...) {
	if enabled == True:
		for item in configfs_items:
			configfs_get_item(item);
		vkms_device_enable(...)
	else:
		vkms_device_disable(...)
		for item in configfs_items:
			configfs_put_item(item)
}

void device_release(...) {
	if enable == True:
		vkms_device_disable(...)
		for item in configfs_items:
			configfs_put_item(item)
}

This way:
- no change in VKMS code
- no ConfigFS change
- we never have use-after-free (at least in my head)
- we never "lose" a DRM device

I did not test it, there is maybe some issue in this implementation (the 
"for item in configfs_items" may be a bit complex to write for example).

> However, that use-after-free is a bug I need to fix. I was wondering how I didn't
> catch it with IGT... Turns out, I didn't enable Kasan in my QEMU .config (ops!).
> 
> Do you agree on folowing this solution? If so, I'll send v3 fixing the memory
> issues.

Indeed yes! If possible, I would like to avoid "complex" refcount/mutex 
in vkms_config structure, but if my proposition does not work, feel free 
to add whatever you think relevant!

Thanks a lot,
Louis Chauvet

> Best wishes,
> Jose
> 
> [1] https://docs.kernel.org/filesystems/configfs.html
> 
>> - we care and we don't want to touch configfs: we need to implement a kind
>> of refcount for all vkms_config elements. Issue: non-trivial work, may allow
>> memory leaks/use after free...
>>
>> - we care and we want to touch configfs: see my two patches (they apply on
>> the v1 of this series). This solution allows adding a check before removing
>> configfs item/group/link. I found it cleaner and way easier to understand.
>>
>> What do you think about my proposition? Do you have another idea?
>>
>>> +static struct configfs_item_operations plane_item_operations = {
>>> +	.release	= &plane_release,
>>> +};
>>> +
>>> +static const struct config_item_type plane_item_type = {
>>> +	.ct_item_ops	= &plane_item_operations,
>>> +	.ct_owner	= THIS_MODULE,
>>> +};
>>> +
>>> +static struct config_group *make_plane_group(struct config_group *group,
>>> +					     const char *name)
>>> +{
>>> +	struct vkms_configfs_device *dev;
>>> +	struct vkms_configfs_plane *plane;
>>> +
>>> +	dev = child_group_to_vkms_configfs_device(group);
>>> +
>>> +	guard(mutex)(&dev->lock);
>>> +
>>> +	if (dev->enabled)
>>> +		return ERR_PTR(-EBUSY);
>>> +
>>> +	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
>>> +	if (!plane)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	plane->dev = dev;
>>> +
>>> +	plane->config = vkms_config_create_plane(dev->config);
>>> +	if (IS_ERR(plane->config)) {
>>> +		kfree(plane);
>>> +		return ERR_CAST(plane->config);
>>> +	}
>>> +
>>> +	config_group_init_type_name(&plane->group, name, &plane_item_type);
>>> +
>>> +	return &plane->group;
>>> +}
>>> +
>>> +static struct configfs_group_operations planes_group_operations = {
>>> +	.make_group	= &make_plane_group,
>>> +};
>>> +
>>> +static const struct config_item_type plane_group_type = {
>>> +	.ct_group_ops	= &planes_group_operations,
>>> +	.ct_owner	= THIS_MODULE,
>>> +};
>>> +
>>>    static ssize_t device_enabled_show(struct config_item *item, char *page)
>>>    {
>>>    	struct vkms_configfs_device *dev;
>>> @@ -125,6 +208,10 @@ static struct config_group *make_device_group(struct config_group *group,
>>>    	config_group_init_type_name(&dev->group, name, &device_item_type);
>>>    	mutex_init(&dev->lock);
>>> +	config_group_init_type_name(&dev->planes_group, "planes",
>>> +				    &plane_group_type);
>>> +	configfs_add_default_group(&dev->planes_group, &dev->group);
>>> +
>>>    	return &dev->group;
>>>    }
>>
>> -- 
>> Louis Chauvet, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
>>

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

Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[José Expósito]

Hi Louis,

On Mon, Mar 03, 2025 at 11:34:50AM +0100, Louis Chauvet wrote:
> 
> 
> Le 03/03/2025 à 09:50, José Expósito a écrit :
> > Hi Louis,
> > 
> > On Fri, Feb 28, 2025 at 03:43:25PM +0100, Louis Chauvet wrote:
> > > 
> > > 
> > > Le 25/02/2025 à 18:59, José Expósito a écrit :
> > > > Create a default subgroup at /config/vkms/planes to allow to create as
> > > > many planes as required.
> > > > 
> > > > 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>
> > > > [...]
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > index 92512d52ddae..4f9d3341e6c0 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > [...]
> > > > +static void plane_release(struct config_item *item)
> > > > +{
> > > > +	struct vkms_configfs_plane *plane;
> > > > +	struct mutex *lock;
> > > > +
> > > > +	plane = plane_item_to_vkms_configfs_plane(item);
> > > > +	lock = &plane->dev->lock;
> > > > +
> > > > +	guard(mutex)(lock);
> > > > +	vkms_config_destroy_plane(plane->config);
> > > > +	kfree(plane);
> > > > +}
> > > 
> > > I just found a flaw in our work: there is currently no way to forbid the
> > > deletion of item/symlinks...
> > > 
> > > If you do:
> > > 
> > > modprobe vkms
> > > cd /sys/kernel/config/vkms/
> > > mkdir DEV
> > > mkdir DEV/connectors/CON
> > > mkdir DEV/planes/PLA
> > > mkdir DEV/crtcs/CRT
> > > mkdir DEV/encoders/ENC
> > > ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> > > ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> > > ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> > > echo 1 > DEV/planes/PLA/type
> > > tree
> > > echo 1 > DEV/enabled
> > > modetest -M vkms
> > > => everything fine
> > > 
> > > rm DEV/connectors/CON/possible_encoders/ENC
> > > rmdir DEV/connectors/CON
> > > modetest -M vkms
> > > => BUG: KASAN: slab-use-after-free

I'm trying to reproduce this issue, but those commands don't show any BUG
in dmesg. This is my Kasan .config:

    CONFIG_HAVE_ARCH_KASAN=y
    CONFIG_HAVE_ARCH_KASAN_VMALLOC=y
    CONFIG_CC_HAS_KASAN_GENERIC=y
    CONFIG_CC_HAS_KASAN_SW_TAGS=y
    CONFIG_CC_HAS_WORKING_NOSANITIZE_ADDRESS=y
    CONFIG_KASAN=y
    CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX=y
    CONFIG_KASAN_GENERIC=y
    # CONFIG_KASAN_OUTLINE is not set
    CONFIG_KASAN_INLINE=y
    CONFIG_KASAN_STACK=y
    CONFIG_KASAN_VMALLOC=y
    # CONFIG_KASAN_KUNIT_TEST is not set
    CONFIG_KASAN_EXTRA_INFO=y

I tryed to delete even more items:

    root@kernel-dev:/sys/kernel/config/vkms# tree
    .
    └── DEV
        ├── connectors
        ├── crtcs
        ├── enabled
        ├── encoders
        └── planes

    root@kernel-dev:/sys/kernel/config/vkms# cat DEV/enabled 
    1

And I still don't see any errors. Is it possible that we are running different
branches? Asking because of the failing IGT tests you reported. There seems to
be a difference in our code or setup that is creating these differences.

Best wishes,
Jose

> > > I see two solutions:
> > > - we don't care and keep as is: if the device is enabled, and you delete
> > > link/groups, it is your fault. As shown above: it can crash the kernel, so
> > > it is a no-go.
> > 
> > I was aware of this limitation and, since the configfs is clear about
> > deleting items: [1]
> > 
> >      Important:
> >      drop_item() is void, and as such cannot fail. When rmdir(2) is called,
> >      configfs WILL remove the item from the filesystem tree (assuming that
> >      it has no children to keep it busy).
> >      The subsystem is responsible for responding to this. [...]
> 
> Thanks for pointing out. I read it and I think you're right, they clearly
> want the user space to be able to delete any item at any time.
> 
> > I decided to follow this approach, i.e., allowing the user to delete the items.
> 
> I think a simple fix would be to have something like that:
> 
> int device_enabled_store(...) {
> 	if enabled == True:
> 		for item in configfs_items:
> 			configfs_get_item(item);
> 		vkms_device_enable(...)
> 	else:
> 		vkms_device_disable(...)
> 		for item in configfs_items:
> 			configfs_put_item(item)
> }
> 
> void device_release(...) {
> 	if enable == True:
> 		vkms_device_disable(...)
> 		for item in configfs_items:
> 			configfs_put_item(item)
> }
> 
> This way:
> - no change in VKMS code
> - no ConfigFS change
> - we never have use-after-free (at least in my head)
> - we never "lose" a DRM device
> 
> I did not test it, there is maybe some issue in this implementation (the
> "for item in configfs_items" may be a bit complex to write for example).
> 
> > However, that use-after-free is a bug I need to fix. I was wondering how I didn't
> > catch it with IGT... Turns out, I didn't enable Kasan in my QEMU .config (ops!).
> > 
> > Do you agree on folowing this solution? If so, I'll send v3 fixing the memory
> > issues.
> 
> Indeed yes! If possible, I would like to avoid "complex" refcount/mutex in
> vkms_config structure, but if my proposition does not work, feel free to add
> whatever you think relevant!
> 
> Thanks a lot,
> Louis Chauvet
> 
> > Best wishes,
> > Jose
> > 
> > [1] https://docs.kernel.org/filesystems/configfs.html
> > 
> > > - we care and we don't want to touch configfs: we need to implement a kind
> > > of refcount for all vkms_config elements. Issue: non-trivial work, may allow
> > > memory leaks/use after free...
> > > 
> > > - we care and we want to touch configfs: see my two patches (they apply on
> > > the v1 of this series). This solution allows adding a check before removing
> > > configfs item/group/link. I found it cleaner and way easier to understand.
> > > 
> > > What do you think about my proposition? Do you have another idea?
> > > 
> > > > +static struct configfs_item_operations plane_item_operations = {
> > > > +	.release	= &plane_release,
> > > > +};
> > > > +
> > > > +static const struct config_item_type plane_item_type = {
> > > > +	.ct_item_ops	= &plane_item_operations,
> > > > +	.ct_owner	= THIS_MODULE,
> > > > +};
> > > > +
> > > > +static struct config_group *make_plane_group(struct config_group *group,
> > > > +					     const char *name)
> > > > +{
> > > > +	struct vkms_configfs_device *dev;
> > > > +	struct vkms_configfs_plane *plane;
> > > > +
> > > > +	dev = child_group_to_vkms_configfs_device(group);
> > > > +
> > > > +	guard(mutex)(&dev->lock);
> > > > +
> > > > +	if (dev->enabled)
> > > > +		return ERR_PTR(-EBUSY);
> > > > +
> > > > +	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> > > > +	if (!plane)
> > > > +		return ERR_PTR(-ENOMEM);
> > > > +
> > > > +	plane->dev = dev;
> > > > +
> > > > +	plane->config = vkms_config_create_plane(dev->config);
> > > > +	if (IS_ERR(plane->config)) {
> > > > +		kfree(plane);
> > > > +		return ERR_CAST(plane->config);
> > > > +	}
> > > > +
> > > > +	config_group_init_type_name(&plane->group, name, &plane_item_type);
> > > > +
> > > > +	return &plane->group;
> > > > +}
> > > > +
> > > > +static struct configfs_group_operations planes_group_operations = {
> > > > +	.make_group	= &make_plane_group,
> > > > +};
> > > > +
> > > > +static const struct config_item_type plane_group_type = {
> > > > +	.ct_group_ops	= &planes_group_operations,
> > > > +	.ct_owner	= THIS_MODULE,
> > > > +};
> > > > +
> > > >    static ssize_t device_enabled_show(struct config_item *item, char *page)
> > > >    {
> > > >    	struct vkms_configfs_device *dev;
> > > > @@ -125,6 +208,10 @@ static struct config_group *make_device_group(struct config_group *group,
> > > >    	config_group_init_type_name(&dev->group, name, &device_item_type);
> > > >    	mutex_init(&dev->lock);
> > > > +	config_group_init_type_name(&dev->planes_group, "planes",
> > > > +				    &plane_group_type);
> > > > +	configfs_add_default_group(&dev->planes_group, &dev->group);
> > > > +
> > > >    	return &dev->group;
> > > >    }
> > > 
> > > -- 
> > > Louis Chauvet, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com
> > > 
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[Louis Chauvet]

Le 04/03/2025 à 15:54, José Expósito a écrit :
> Hi Louis,
> 
> On Mon, Mar 03, 2025 at 11:34:50AM +0100, Louis Chauvet wrote:
>>
>>
>> Le 03/03/2025 à 09:50, José Expósito a écrit :
>>> Hi Louis,
>>>
>>> On Fri, Feb 28, 2025 at 03:43:25PM +0100, Louis Chauvet wrote:
>>>>
>>>>
>>>> Le 25/02/2025 à 18:59, José Expósito a écrit :
>>>>> Create a default subgroup at /config/vkms/planes to allow to create as
>>>>> many planes as required.
>>>>>
>>>>> 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>
>>>>> [...]
>>>>> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
>>>>> index 92512d52ddae..4f9d3341e6c0 100644
>>>>> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
>>>>> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
>>>>> [...]
>>>>> +static void plane_release(struct config_item *item)
>>>>> +{
>>>>> +	struct vkms_configfs_plane *plane;
>>>>> +	struct mutex *lock;
>>>>> +
>>>>> +	plane = plane_item_to_vkms_configfs_plane(item);
>>>>> +	lock = &plane->dev->lock;
>>>>> +
>>>>> +	guard(mutex)(lock);
>>>>> +	vkms_config_destroy_plane(plane->config);
>>>>> +	kfree(plane);
>>>>> +}
>>>>
>>>> I just found a flaw in our work: there is currently no way to forbid the
>>>> deletion of item/symlinks...
>>>>
>>>> If you do:
>>>>
>>>> modprobe vkms
>>>> cd /sys/kernel/config/vkms/
>>>> mkdir DEV
>>>> mkdir DEV/connectors/CON
>>>> mkdir DEV/planes/PLA
>>>> mkdir DEV/crtcs/CRT
>>>> mkdir DEV/encoders/ENC
>>>> ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
>>>> ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
>>>> ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
>>>> echo 1 > DEV/planes/PLA/type
>>>> tree
>>>> echo 1 > DEV/enabled
>>>> modetest -M vkms
>>>> => everything fine
>>>>
>>>> rm DEV/connectors/CON/possible_encoders/ENC
>>>> rmdir DEV/connectors/CON
>>>> modetest -M vkms
>>>> => BUG: KASAN: slab-use-after-free
> 
> I'm trying to reproduce this issue, but those commands don't show any BUG
> in dmesg. This is my Kasan .config:
> 
>      CONFIG_HAVE_ARCH_KASAN=y
>      CONFIG_HAVE_ARCH_KASAN_VMALLOC=y
>      CONFIG_CC_HAS_KASAN_GENERIC=y
>      CONFIG_CC_HAS_KASAN_SW_TAGS=y
>      CONFIG_CC_HAS_WORKING_NOSANITIZE_ADDRESS=y
>      CONFIG_KASAN=y
>      CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX=y
>      CONFIG_KASAN_GENERIC=y
>      # CONFIG_KASAN_OUTLINE is not set
>      CONFIG_KASAN_INLINE=y
>      CONFIG_KASAN_STACK=y
>      CONFIG_KASAN_VMALLOC=y
>      # CONFIG_KASAN_KUNIT_TEST is not set
>      CONFIG_KASAN_EXTRA_INFO=y
> 
> I tryed to delete even more items:
> 
>      root@kernel-dev:/sys/kernel/config/vkms# tree
>      .
>      └── DEV
>          ├── connectors
>          ├── crtcs
>          ├── enabled
>          ├── encoders
>          └── planes
> 
>      root@kernel-dev:/sys/kernel/config/vkms# cat DEV/enabled
>      1
> 
> And I still don't see any errors. Is it possible that we are running different
> branches? Asking because of the failing IGT tests you reported. There seems to
> be a difference in our code or setup that is creating these differences.

I just re-applied your last vkms-config version and this series on top 
of drm-misc-next. See [1] for the exact commits.

Argg sorry, I just noticed something: you need to disable the default 
vkms device (I had this option in my kernel command line...), otherwise 
modetest only use the first vkms gpu...

I will check again the igt tests, but I don't think this is the same 
issue (it should not use the default device to test)

So, with [1] and the defconfig below, I have this:


     1  modprobe vkms create_default_dev=0
     2  cd /sys/kernel/config/vkms/
     3  mkdir DEV
     4  mkdir DEV/connectors/CON
     5  mkdir DEV/planes/PLA
     6  mkdir DEV/crtcs/CRT
     7  mkdir DEV/encoders/ENC
     8  ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
     9  ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
    10  ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
    11  echo 1 > DEV/planes/PLA/type
    12  tree
    13  echo 1 > DEV/enabled
    14  modetest -M vkms
    15  rm DEV/connectors/CON/possible_encoders/ENC
    16  rmdir DEV/connectors/CON
    17  modetest -M vkms
KASAN: slab-use-after-free


[1]:https://github.com/Fomys/linux/tree/20250225175936.7223-1-jose.exposito89%40gmail.com


===== defconfig =====

CONFIG_SYSVIPC=y
CONFIG_CGROUPS=y
CONFIG_KALLSYMS_ALL=y
CONFIG_SMP=y
CONFIG_HYPERVISOR_GUEST=y
CONFIG_PARAVIRT=y
# CONFIG_VIRTUALIZATION is not set
CONFIG_JUMP_LABEL=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_NET=y
CONFIG_PACKET=y
# CONFIG_WIRELESS is not set
CONFIG_NET_9P=y
CONFIG_NET_9P_VIRTIO=y
CONFIG_PCI=y
CONFIG_DEVTMPFS=y
CONFIG_DEVTMPFS_MOUNT=y
CONFIG_VIRTIO_BLK=y
# CONFIG_INTEL_MEI is not set
CONFIG_NETDEVICES=y
CONFIG_VIRTIO_NET=y
# CONFIG_ETHERNET is not set
# CONFIG_WLAN is not set
CONFIG_INPUT_EVDEV=y
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_VIRTIO_CONSOLE=y
CONFIG_HW_RANDOM_VIRTIO=m
CONFIG_PTP_1588_CLOCK=y
# CONFIG_HWMON is not set
CONFIG_THERMAL_GOV_USER_SPACE=y
CONFIG_DRM=y
CONFIG_DRM_KUNIT_TEST=m
CONFIG_DRM_VKMS=m
CONFIG_DRM_VKMS_KUNIT_TEST=m
# CONFIG_USB_SUPPORT is not set
CONFIG_VIRTIO_PCI=y
CONFIG_VIRTIO_BALLOON=y
CONFIG_VIRTIO_INPUT=y
CONFIG_VIRTIO_MMIO=y
CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES=y
# CONFIG_SURFACE_PLATFORMS is not set
CONFIG_EXT4_FS=y
CONFIG_FUSE_FS=y
CONFIG_VIRTIO_FS=y
CONFIG_OVERLAY_FS=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_CONFIGFS_FS=y
CONFIG_9P_FS=y
CONFIG_CRYPTO=y
CONFIG_CRYPTO_CRC32C=y
CONFIG_DYNAMIC_DEBUG=y
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_INFO_DWARF5=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_FS=y
CONFIG_DEBUG_PAGEALLOC=y
CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y
CONFIG_PAGE_POISONING=y
CONFIG_DEBUG_OBJECTS=y
CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
CONFIG_SCHED_STACK_END_CHECK=y
CONFIG_KASAN=y
CONFIG_KASAN_VMALLOC=y
CONFIG_KASAN_EXTRA_INFO=y
CONFIG_KFENCE=y
# CONFIG_FTRACE is not set
CONFIG_UNWINDER_FRAME_POINTER=y
CONFIG_KUNIT=y
CONFIG_TEST_DYNAMIC_DEBUG=m

Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[José Expósito]

On Tue, Mar 04, 2025 at 04:35:50PM +0100, Louis Chauvet wrote:
> 
> 
> Le 04/03/2025 à 15:54, José Expósito a écrit :
> > Hi Louis,
> > 
> > On Mon, Mar 03, 2025 at 11:34:50AM +0100, Louis Chauvet wrote:
> > > 
> > > 
> > > Le 03/03/2025 à 09:50, José Expósito a écrit :
> > > > Hi Louis,
> > > > 
> > > > On Fri, Feb 28, 2025 at 03:43:25PM +0100, Louis Chauvet wrote:
> > > > > 
> > > > > 
> > > > > Le 25/02/2025 à 18:59, José Expósito a écrit :
> > > > > > Create a default subgroup at /config/vkms/planes to allow to create as
> > > > > > many planes as required.
> > > > > > 
> > > > > > 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>
> > > > > > [...]
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > index 92512d52ddae..4f9d3341e6c0 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > [...]
> > > > > > +static void plane_release(struct config_item *item)
> > > > > > +{
> > > > > > +	struct vkms_configfs_plane *plane;
> > > > > > +	struct mutex *lock;
> > > > > > +
> > > > > > +	plane = plane_item_to_vkms_configfs_plane(item);
> > > > > > +	lock = &plane->dev->lock;
> > > > > > +
> > > > > > +	guard(mutex)(lock);
> > > > > > +	vkms_config_destroy_plane(plane->config);
> > > > > > +	kfree(plane);
> > > > > > +}
> > > > > 
> > > > > I just found a flaw in our work: there is currently no way to forbid the
> > > > > deletion of item/symlinks...
> > > > > 
> > > > > If you do:
> > > > > 
> > > > > modprobe vkms
> > > > > cd /sys/kernel/config/vkms/
> > > > > mkdir DEV
> > > > > mkdir DEV/connectors/CON
> > > > > mkdir DEV/planes/PLA
> > > > > mkdir DEV/crtcs/CRT
> > > > > mkdir DEV/encoders/ENC
> > > > > ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> > > > > ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> > > > > ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> > > > > echo 1 > DEV/planes/PLA/type
> > > > > tree
> > > > > echo 1 > DEV/enabled
> > > > > modetest -M vkms
> > > > > => everything fine
> > > > > 
> > > > > rm DEV/connectors/CON/possible_encoders/ENC
> > > > > rmdir DEV/connectors/CON
> > > > > modetest -M vkms
> > > > > => BUG: KASAN: slab-use-after-free
> > 
> > I'm trying to reproduce this issue, but those commands don't show any BUG
> > in dmesg. This is my Kasan .config:
> > 
> >      CONFIG_HAVE_ARCH_KASAN=y
> >      CONFIG_HAVE_ARCH_KASAN_VMALLOC=y
> >      CONFIG_CC_HAS_KASAN_GENERIC=y
> >      CONFIG_CC_HAS_KASAN_SW_TAGS=y
> >      CONFIG_CC_HAS_WORKING_NOSANITIZE_ADDRESS=y
> >      CONFIG_KASAN=y
> >      CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX=y
> >      CONFIG_KASAN_GENERIC=y
> >      # CONFIG_KASAN_OUTLINE is not set
> >      CONFIG_KASAN_INLINE=y
> >      CONFIG_KASAN_STACK=y
> >      CONFIG_KASAN_VMALLOC=y
> >      # CONFIG_KASAN_KUNIT_TEST is not set
> >      CONFIG_KASAN_EXTRA_INFO=y
> > 
> > I tryed to delete even more items:
> > 
> >      root@kernel-dev:/sys/kernel/config/vkms# tree
> >      .
> >      └── DEV
> >          ├── connectors
> >          ├── crtcs
> >          ├── enabled
> >          ├── encoders
> >          └── planes
> > 
> >      root@kernel-dev:/sys/kernel/config/vkms# cat DEV/enabled
> >      1
> > 
> > And I still don't see any errors. Is it possible that we are running different
> > branches? Asking because of the failing IGT tests you reported. There seems to
> > be a difference in our code or setup that is creating these differences.
> 
> I just re-applied your last vkms-config version and this series on top of
> drm-misc-next. See [1] for the exact commits.
> 
> Argg sorry, I just noticed something: you need to disable the default vkms
> device (I had this option in my kernel command line...), otherwise modetest
> only use the first vkms gpu...
> 
> I will check again the igt tests, but I don't think this is the same issue
> (it should not use the default device to test)
> 
> So, with [1] and the defconfig below, I have this:
> 
> 
>     1  modprobe vkms create_default_dev=0
>     2  cd /sys/kernel/config/vkms/
>     3  mkdir DEV
>     4  mkdir DEV/connectors/CON
>     5  mkdir DEV/planes/PLA
>     6  mkdir DEV/crtcs/CRT
>     7  mkdir DEV/encoders/ENC
>     8  ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
>     9  ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
>    10  ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
>    11  echo 1 > DEV/planes/PLA/type
>    12  tree
>    13  echo 1 > DEV/enabled
>    14  modetest -M vkms
>    15  rm DEV/connectors/CON/possible_encoders/ENC
>    16  rmdir DEV/connectors/CON
>    17  modetest -M vkms
> KASAN: slab-use-after-free
> 
> 
> [1]:https://github.com/Fomys/linux/tree/20250225175936.7223-1-jose.exposito89%40gmail.com

Aha! Are you testing without a desktop environment running?

    $ sudo systemctl isolate multi-user.target

Running that (^) command before yours gives me this use after free:

    BUG: KASAN: slab-use-after-free in vkms_connector_detect+0x61/0x70 [vkms]

Is the same one you are seeing? 

Looking at the connector_release() function in vkms_configfs.c, I see
that I'm freeing the configuration:

    vkms_config_destroy_connector(connector->config);

And I don't think there is a reason to do it. vkms_config_destroy() in
device_release() will free everything once we are done.

I need to do more testing, but the solution might be that simple.

Good catch, I didn't test like that and I overlooked this issue :D

Jose

> 
> 
> ===== defconfig =====
> 
> CONFIG_SYSVIPC=y
> CONFIG_CGROUPS=y
> CONFIG_KALLSYMS_ALL=y
> CONFIG_SMP=y
> CONFIG_HYPERVISOR_GUEST=y
> CONFIG_PARAVIRT=y
> # CONFIG_VIRTUALIZATION is not set
> CONFIG_JUMP_LABEL=y
> CONFIG_MODULES=y
> CONFIG_MODULE_UNLOAD=y
> CONFIG_NET=y
> CONFIG_PACKET=y
> # CONFIG_WIRELESS is not set
> CONFIG_NET_9P=y
> CONFIG_NET_9P_VIRTIO=y
> CONFIG_PCI=y
> CONFIG_DEVTMPFS=y
> CONFIG_DEVTMPFS_MOUNT=y
> CONFIG_VIRTIO_BLK=y
> # CONFIG_INTEL_MEI is not set
> CONFIG_NETDEVICES=y
> CONFIG_VIRTIO_NET=y
> # CONFIG_ETHERNET is not set
> # CONFIG_WLAN is not set
> CONFIG_INPUT_EVDEV=y
> CONFIG_SERIAL_8250=y
> CONFIG_SERIAL_8250_CONSOLE=y
> CONFIG_VIRTIO_CONSOLE=y
> CONFIG_HW_RANDOM_VIRTIO=m
> CONFIG_PTP_1588_CLOCK=y
> # CONFIG_HWMON is not set
> CONFIG_THERMAL_GOV_USER_SPACE=y
> CONFIG_DRM=y
> CONFIG_DRM_KUNIT_TEST=m
> CONFIG_DRM_VKMS=m
> CONFIG_DRM_VKMS_KUNIT_TEST=m
> # CONFIG_USB_SUPPORT is not set
> CONFIG_VIRTIO_PCI=y
> CONFIG_VIRTIO_BALLOON=y
> CONFIG_VIRTIO_INPUT=y
> CONFIG_VIRTIO_MMIO=y
> CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES=y
> # CONFIG_SURFACE_PLATFORMS is not set
> CONFIG_EXT4_FS=y
> CONFIG_FUSE_FS=y
> CONFIG_VIRTIO_FS=y
> CONFIG_OVERLAY_FS=y
> CONFIG_TMPFS=y
> CONFIG_TMPFS_POSIX_ACL=y
> CONFIG_CONFIGFS_FS=y
> CONFIG_9P_FS=y
> CONFIG_CRYPTO=y
> CONFIG_CRYPTO_CRC32C=y
> CONFIG_DYNAMIC_DEBUG=y
> CONFIG_DEBUG_KERNEL=y
> CONFIG_DEBUG_INFO_DWARF5=y
> CONFIG_MAGIC_SYSRQ=y
> CONFIG_DEBUG_FS=y
> CONFIG_DEBUG_PAGEALLOC=y
> CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y
> CONFIG_PAGE_POISONING=y
> CONFIG_DEBUG_OBJECTS=y
> CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
> CONFIG_SCHED_STACK_END_CHECK=y
> CONFIG_KASAN=y
> CONFIG_KASAN_VMALLOC=y
> CONFIG_KASAN_EXTRA_INFO=y
> CONFIG_KFENCE=y
> # CONFIG_FTRACE is not set
> CONFIG_UNWINDER_FRAME_POINTER=y
> CONFIG_KUNIT=y
> CONFIG_TEST_DYNAMIC_DEBUG=m
> 

Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[Louis Chauvet]

Le 04/03/2025 à 17:23, José Expósito a écrit :
> On Tue, Mar 04, 2025 at 04:35:50PM +0100, Louis Chauvet wrote:
>>
>>
>> Le 04/03/2025 à 15:54, José Expósito a écrit :
>>> Hi Louis,
>>>
>>> On Mon, Mar 03, 2025 at 11:34:50AM +0100, Louis Chauvet wrote:
>>>>
>>>>
>>>> Le 03/03/2025 à 09:50, José Expósito a écrit :
>>>>> Hi Louis,
>>>>>
>>>>> On Fri, Feb 28, 2025 at 03:43:25PM +0100, Louis Chauvet wrote:
>>>>>>
>>>>>>
>>>>>> Le 25/02/2025 à 18:59, José Expósito a écrit :
>>>>>>> Create a default subgroup at /config/vkms/planes to allow to create as
>>>>>>> many planes as required.
>>>>>>>
>>>>>>> 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>
>>>>>>> [...]
>>>>>>> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
>>>>>>> index 92512d52ddae..4f9d3341e6c0 100644
>>>>>>> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
>>>>>>> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
>>>>>>> [...]
>>>>>>> +static void plane_release(struct config_item *item)
>>>>>>> +{
>>>>>>> +	struct vkms_configfs_plane *plane;
>>>>>>> +	struct mutex *lock;
>>>>>>> +
>>>>>>> +	plane = plane_item_to_vkms_configfs_plane(item);
>>>>>>> +	lock = &plane->dev->lock;
>>>>>>> +
>>>>>>> +	guard(mutex)(lock);
>>>>>>> +	vkms_config_destroy_plane(plane->config);
>>>>>>> +	kfree(plane);
>>>>>>> +}
>>>>>>
>>>>>> I just found a flaw in our work: there is currently no way to forbid the
>>>>>> deletion of item/symlinks...
>>>>>>
>>>>>> If you do:
>>>>>>
>>>>>> modprobe vkms
>>>>>> cd /sys/kernel/config/vkms/
>>>>>> mkdir DEV
>>>>>> mkdir DEV/connectors/CON
>>>>>> mkdir DEV/planes/PLA
>>>>>> mkdir DEV/crtcs/CRT
>>>>>> mkdir DEV/encoders/ENC
>>>>>> ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
>>>>>> ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
>>>>>> ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
>>>>>> echo 1 > DEV/planes/PLA/type
>>>>>> tree
>>>>>> echo 1 > DEV/enabled
>>>>>> modetest -M vkms
>>>>>> => everything fine
>>>>>>
>>>>>> rm DEV/connectors/CON/possible_encoders/ENC
>>>>>> rmdir DEV/connectors/CON
>>>>>> modetest -M vkms
>>>>>> => BUG: KASAN: slab-use-after-free
>>>
>>> I'm trying to reproduce this issue, but those commands don't show any BUG
>>> in dmesg. This is my Kasan .config:
>>>
>>>       CONFIG_HAVE_ARCH_KASAN=y
>>>       CONFIG_HAVE_ARCH_KASAN_VMALLOC=y
>>>       CONFIG_CC_HAS_KASAN_GENERIC=y
>>>       CONFIG_CC_HAS_KASAN_SW_TAGS=y
>>>       CONFIG_CC_HAS_WORKING_NOSANITIZE_ADDRESS=y
>>>       CONFIG_KASAN=y
>>>       CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX=y
>>>       CONFIG_KASAN_GENERIC=y
>>>       # CONFIG_KASAN_OUTLINE is not set
>>>       CONFIG_KASAN_INLINE=y
>>>       CONFIG_KASAN_STACK=y
>>>       CONFIG_KASAN_VMALLOC=y
>>>       # CONFIG_KASAN_KUNIT_TEST is not set
>>>       CONFIG_KASAN_EXTRA_INFO=y
>>>
>>> I tryed to delete even more items:
>>>
>>>       root@kernel-dev:/sys/kernel/config/vkms# tree
>>>       .
>>>       └── DEV
>>>           ├── connectors
>>>           ├── crtcs
>>>           ├── enabled
>>>           ├── encoders
>>>           └── planes
>>>
>>>       root@kernel-dev:/sys/kernel/config/vkms# cat DEV/enabled
>>>       1
>>>
>>> And I still don't see any errors. Is it possible that we are running different
>>> branches? Asking because of the failing IGT tests you reported. There seems to
>>> be a difference in our code or setup that is creating these differences.
>>
>> I just re-applied your last vkms-config version and this series on top of
>> drm-misc-next. See [1] for the exact commits.
>>
>> Argg sorry, I just noticed something: you need to disable the default vkms
>> device (I had this option in my kernel command line...), otherwise modetest
>> only use the first vkms gpu...
>>
>> I will check again the igt tests, but I don't think this is the same issue
>> (it should not use the default device to test)
>>
>> So, with [1] and the defconfig below, I have this:
>>
>>
>>      1  modprobe vkms create_default_dev=0
>>      2  cd /sys/kernel/config/vkms/
>>      3  mkdir DEV
>>      4  mkdir DEV/connectors/CON
>>      5  mkdir DEV/planes/PLA
>>      6  mkdir DEV/crtcs/CRT
>>      7  mkdir DEV/encoders/ENC
>>      8  ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
>>      9  ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
>>     10  ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
>>     11  echo 1 > DEV/planes/PLA/type
>>     12  tree
>>     13  echo 1 > DEV/enabled
>>     14  modetest -M vkms
>>     15  rm DEV/connectors/CON/possible_encoders/ENC
>>     16  rmdir DEV/connectors/CON
>>     17  modetest -M vkms
>> KASAN: slab-use-after-free
>>
>>
>> [1]:https://github.com/Fomys/linux/tree/20250225175936.7223-1-jose.exposito89%40gmail.com
> 
> Aha! Are you testing without a desktop environment running?

Yes! I run a minimal VM (virtme-ng), so no services are started.
> 
>      $ sudo systemctl isolate multi-user.target
> 
> Running that (^) command before yours gives me this use after free:
> 
>      BUG: KASAN: slab-use-after-free in vkms_connector_detect+0x61/0x70 [vkms]
> 
> Is the same one you are seeing?

Yes!

> Looking at the connector_release() function in vkms_configfs.c, I see
> that I'm freeing the configuration:
> 
>      vkms_config_destroy_connector(connector->config);
> 
> And I don't think there is a reason to do it. vkms_config_destroy() in
> device_release() will free everything once we are done.

Yes, but if you don't free it will always remain in the config, which 
means that:

modprobe vkms create_default_dev=0
cd /sys/kernel/config/vkms/
mkdir DEV
mkdir DEV/connectors/CON
mkdir DEV/crtcs/CRT
mkdir DEV/planes/PLA
mkdir DEV/encoders/ENC
ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
echo 1 > DEV/planes/PLA/type
echo 1 > DEV/enabled
echo 0 > DEV/enabled
rm DEV/connectors/CON/possible_encoders/ENC
rmdir DEV/connectors/CON
echo 1 > DEV/enabled

Expected (and current) result:

(NULL device *): [drm] The number of connectors must be between 1 and 31

Result with the diff:
- vkms_config_destroy_connector(connector->config);
+ //vkms_config_destroy_connector(connector->config);

(NULL device *): [drm] All connectors must have at least one possible 
encoder

This is because the connector list in vkms_config contains the deleted 
CON connector. If you also remove the connector from the list, it will 
be a memory leak.

The solution I proposed with get/put should solve this: once the device 
is disabled, all the release functions (and the corresponding 
vkms_config_destroy) will be called, so no issue there.

I attempted to do it, but it looks a bit more complex than I expected:
- config_item_get works as expected, if you get all the items in 
device_enabled_store they are not released, even if the directory is 
deleted;
- but as the directory is deleted, you can't use cg_children to put your 
last reference on it.

I think a solution could be to add refcounting in vkms_config, it seems 
to work, and it is even better, the refcounting is done in the vkms 
driver, not in configfs (I did it only for connector, see below). It 
seems to work as expected and doesn't increase the complexity of the code.

Do you think this is sufficient/clear enough?

Have a nice day,
Louis Chauvet

diff --git a/drivers/gpu/drm/vkms/vkms_config.c 
b/drivers/gpu/drm/vkms/vkms_config.c
index f8394a063ecf..4dc65ab15517 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -1,5 +1,6 @@
  // SPDX-License-Identifier: GPL-2.0+

+#include <linux/kref.h>
  #include <linux/slab.h>

  #include <drm/drm_print.h>
@@ -123,7 +124,7 @@ void vkms_config_destroy(struct vkms_config *config)
  		vkms_config_destroy_encoder(config, encoder_cfg);

  	list_for_each_entry_safe(connector_cfg, connector_tmp, 
&config->connectors, link)
-		vkms_config_destroy_connector(connector_cfg);
+		vkms_config_connector_put(connector_cfg);

  	kfree_const(config->dev_name);
  	kfree(config);
@@ -596,17 +597,32 @@ struct vkms_config_connector 
*vkms_config_create_connector(struct vkms_config *c

  	list_add_tail(&connector_cfg->link, &config->connectors);

+	kref_init(&connector_cfg->ref);
  	return connector_cfg;
  }
  EXPORT_SYMBOL_IF_KUNIT(vkms_config_create_connector);

-void vkms_config_destroy_connector(struct vkms_config_connector 
*connector_cfg)
+static void vkms_config_destroy_connector(struct vkms_config_connector 
*connector_cfg)
  {
  	xa_destroy(&connector_cfg->possible_encoders);
  	list_del(&connector_cfg->link);
  	kfree(connector_cfg);
  }
-EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
+// EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
+
+static void vkms_config_destroy_connector_kref(struct kref *kref)
+{
+	struct vkms_config_connector *connector = container_of(kref, struct 
vkms_config_connector, ref);
+
+	vkms_config_destroy_connector(connector);
+}
+
+void vkms_config_connector_get(struct vkms_config_connector *connector) {
+	kref_get(&connector->ref);
+}
+void vkms_config_connector_put(struct vkms_config_connector *connector) {
+	kref_put(&connector->ref, vkms_config_destroy_connector_kref);
+}

  int __must_check vkms_config_connector_attach_encoder(struct 
vkms_config_connector *connector_cfg,
  						      struct vkms_config_encoder *encoder_cfg)
diff --git a/drivers/gpu/drm/vkms/vkms_config.h 
b/drivers/gpu/drm/vkms/vkms_config.h
index e202b5a84ddd..30e6c6bf34f4 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -3,6 +3,7 @@
  #ifndef _VKMS_CONFIG_H_
  #define _VKMS_CONFIG_H_

+#include <linux/kref.h>
  #include <linux/list.h>
  #include <linux/types.h>
  #include <linux/xarray.h>
@@ -109,6 +110,7 @@ struct vkms_config_encoder {
   *             configuration and must be managed by other means.
   */
  struct vkms_config_connector {
+	struct kref ref;
  	struct list_head link;
  	struct vkms_config *config;

@@ -416,11 +418,8 @@ void vkms_config_encoder_detach_crtc(struct 
vkms_config_encoder *encoder_cfg,
   */
  struct vkms_config_connector *vkms_config_create_connector(struct 
vkms_config *config);

-/**
- * vkms_config_destroy_connector() - Remove and free a connector 
configuration
- * @connector_cfg: Connector configuration to destroy
- */
-void vkms_config_destroy_connector(struct vkms_config_connector 
*connector_cfg);
+void vkms_config_connector_get(struct vkms_config_connector *connector);
+void vkms_config_connector_put(struct vkms_config_connector *connector);

  /**
   * vkms_config_connector_attach_encoder - Attach a connector to an encoder
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c 
b/drivers/gpu/drm/vkms/vkms_configfs.c
index 76afb0245388..ae929a084feb 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -550,7 +550,7 @@ static void connector_release(struct config_item *item)
  	lock = &connector->dev->lock;

  	guard(mutex)(lock);
-	vkms_config_destroy_connector(connector->config);
+	vkms_config_connector_put(connector->config);
  	kfree(connector);
  }

diff --git a/drivers/gpu/drm/vkms/vkms_connector.c 
b/drivers/gpu/drm/vkms/vkms_connector.c
index ed99270c590f..c15451b50440 100644
--- a/drivers/gpu/drm/vkms/vkms_connector.c
+++ b/drivers/gpu/drm/vkms/vkms_connector.c
@@ -20,6 +20,15 @@ static enum drm_connector_status 
vkms_connector_detect(struct drm_connector *con
  	return status;
  }

+static void vkms_connector_destroy(struct drm_device *dev, void *raw)
+{
+	struct vkms_connector *vkms_connector = raw;
+
+	vkms_config_connector_put(vkms_connector->connector_cfg);
+
+	return;
+}
+
  static const struct drm_connector_funcs vkms_connector_funcs = {
  	.detect = vkms_connector_detect,
  	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -65,8 +74,13 @@ struct vkms_connector *vkms_connector_init(struct 
vkms_device *vkmsdev,
  	if (!connector)
  		return ERR_PTR(-ENOMEM);

+	vkms_config_connector_get(connector->connector_cfg);
  	connector->connector_cfg = connector_cfg;

+	ret = drmm_add_action_or_reset(dev, &vkms_connector_destroy, connector);
+	if (ret)
+		return ERR_PTR(ret);
+
  	ret = drmm_connector_init(dev, &connector->base, &vkms_connector_funcs,
  				  DRM_MODE_CONNECTOR_VIRTUAL, NULL);
  	if (ret)

Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[José Expósito]

Hi Louis,

On Tue, Mar 04, 2025 at 07:17:53PM +0100, Louis Chauvet wrote:
> 
> 
> Le 04/03/2025 à 17:23, José Expósito a écrit :
> > On Tue, Mar 04, 2025 at 04:35:50PM +0100, Louis Chauvet wrote:
> > > 
> > > 
> > > Le 04/03/2025 à 15:54, José Expósito a écrit :
> > > > Hi Louis,
> > > > 
> > > > On Mon, Mar 03, 2025 at 11:34:50AM +0100, Louis Chauvet wrote:
> > > > > 
> > > > > 
> > > > > Le 03/03/2025 à 09:50, José Expósito a écrit :
> > > > > > Hi Louis,
> > > > > > 
> > > > > > On Fri, Feb 28, 2025 at 03:43:25PM +0100, Louis Chauvet wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > Le 25/02/2025 à 18:59, José Expósito a écrit :
> > > > > > > > Create a default subgroup at /config/vkms/planes to allow to create as
> > > > > > > > many planes as required.
> > > > > > > > 
> > > > > > > > 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>
> > > > > > > > [...]
> > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > > > index 92512d52ddae..4f9d3341e6c0 100644
> > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > > > [...]
> > > > > > > > +static void plane_release(struct config_item *item)
> > > > > > > > +{
> > > > > > > > +	struct vkms_configfs_plane *plane;
> > > > > > > > +	struct mutex *lock;
> > > > > > > > +
> > > > > > > > +	plane = plane_item_to_vkms_configfs_plane(item);
> > > > > > > > +	lock = &plane->dev->lock;
> > > > > > > > +
> > > > > > > > +	guard(mutex)(lock);
> > > > > > > > +	vkms_config_destroy_plane(plane->config);
> > > > > > > > +	kfree(plane);
> > > > > > > > +}
> > > > > > > 
> > > > > > > I just found a flaw in our work: there is currently no way to forbid the
> > > > > > > deletion of item/symlinks...
> > > > > > > 
> > > > > > > If you do:
> > > > > > > 
> > > > > > > modprobe vkms
> > > > > > > cd /sys/kernel/config/vkms/
> > > > > > > mkdir DEV
> > > > > > > mkdir DEV/connectors/CON
> > > > > > > mkdir DEV/planes/PLA
> > > > > > > mkdir DEV/crtcs/CRT
> > > > > > > mkdir DEV/encoders/ENC
> > > > > > > ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> > > > > > > ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> > > > > > > ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> > > > > > > echo 1 > DEV/planes/PLA/type
> > > > > > > tree
> > > > > > > echo 1 > DEV/enabled
> > > > > > > modetest -M vkms
> > > > > > > => everything fine
> > > > > > > 
> > > > > > > rm DEV/connectors/CON/possible_encoders/ENC
> > > > > > > rmdir DEV/connectors/CON
> > > > > > > modetest -M vkms
> > > > > > > => BUG: KASAN: slab-use-after-free
> > > > 
> > > > I'm trying to reproduce this issue, but those commands don't show any BUG
> > > > in dmesg. This is my Kasan .config:
> > > > 
> > > >       CONFIG_HAVE_ARCH_KASAN=y
> > > >       CONFIG_HAVE_ARCH_KASAN_VMALLOC=y
> > > >       CONFIG_CC_HAS_KASAN_GENERIC=y
> > > >       CONFIG_CC_HAS_KASAN_SW_TAGS=y
> > > >       CONFIG_CC_HAS_WORKING_NOSANITIZE_ADDRESS=y
> > > >       CONFIG_KASAN=y
> > > >       CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX=y
> > > >       CONFIG_KASAN_GENERIC=y
> > > >       # CONFIG_KASAN_OUTLINE is not set
> > > >       CONFIG_KASAN_INLINE=y
> > > >       CONFIG_KASAN_STACK=y
> > > >       CONFIG_KASAN_VMALLOC=y
> > > >       # CONFIG_KASAN_KUNIT_TEST is not set
> > > >       CONFIG_KASAN_EXTRA_INFO=y
> > > > 
> > > > I tryed to delete even more items:
> > > > 
> > > >       root@kernel-dev:/sys/kernel/config/vkms# tree
> > > >       .
> > > >       └── DEV
> > > >           ├── connectors
> > > >           ├── crtcs
> > > >           ├── enabled
> > > >           ├── encoders
> > > >           └── planes
> > > > 
> > > >       root@kernel-dev:/sys/kernel/config/vkms# cat DEV/enabled
> > > >       1
> > > > 
> > > > And I still don't see any errors. Is it possible that we are running different
> > > > branches? Asking because of the failing IGT tests you reported. There seems to
> > > > be a difference in our code or setup that is creating these differences.
> > > 
> > > I just re-applied your last vkms-config version and this series on top of
> > > drm-misc-next. See [1] for the exact commits.
> > > 
> > > Argg sorry, I just noticed something: you need to disable the default vkms
> > > device (I had this option in my kernel command line...), otherwise modetest
> > > only use the first vkms gpu...
> > > 
> > > I will check again the igt tests, but I don't think this is the same issue
> > > (it should not use the default device to test)
> > > 
> > > So, with [1] and the defconfig below, I have this:
> > > 
> > > 
> > >      1  modprobe vkms create_default_dev=0
> > >      2  cd /sys/kernel/config/vkms/
> > >      3  mkdir DEV
> > >      4  mkdir DEV/connectors/CON
> > >      5  mkdir DEV/planes/PLA
> > >      6  mkdir DEV/crtcs/CRT
> > >      7  mkdir DEV/encoders/ENC
> > >      8  ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> > >      9  ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> > >     10  ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> > >     11  echo 1 > DEV/planes/PLA/type
> > >     12  tree
> > >     13  echo 1 > DEV/enabled
> > >     14  modetest -M vkms
> > >     15  rm DEV/connectors/CON/possible_encoders/ENC
> > >     16  rmdir DEV/connectors/CON
> > >     17  modetest -M vkms
> > > KASAN: slab-use-after-free
> > > 
> > > 
> > > [1]:https://github.com/Fomys/linux/tree/20250225175936.7223-1-jose.exposito89%40gmail.com
> > 
> > Aha! Are you testing without a desktop environment running?
> 
> Yes! I run a minimal VM (virtme-ng), so no services are started.
> > 
> >      $ sudo systemctl isolate multi-user.target
> > 
> > Running that (^) command before yours gives me this use after free:
> > 
> >      BUG: KASAN: slab-use-after-free in vkms_connector_detect+0x61/0x70 [vkms]
> > 
> > Is the same one you are seeing?
> 
> Yes!
> 
> > Looking at the connector_release() function in vkms_configfs.c, I see
> > that I'm freeing the configuration:
> > 
> >      vkms_config_destroy_connector(connector->config);
> > 
> > And I don't think there is a reason to do it. vkms_config_destroy() in
> > device_release() will free everything once we are done.
> 
> Yes, but if you don't free it will always remain in the config, which means
> that:

Busy week, but I finally have a couple of hours to sit and look into this
with detail.

The problem is in my patch to implement connector hot-plug ("drm/vkms: Allow
to update the connector status").

I was storing a pointer to the connector configuration in vkms_connector (OK)
and using it to get the connector status (wrong).
The configuration is mean to be used during device initialization, but after
that, it can change at any point responding to configfs changes.

I even documented that accessing vkms_config_connector->connector is not
safe... Just to access it a few patches later (sigh).
In my opinion, the solution is to avoid this behavior. This is how the fix
looks like [1]. The code is even simpler than the  previous version.

[1] https://github.com/JoseExposito/linux/commit/da116085590d644575e9d78fb5c3a665aa7848f5
 
> modprobe vkms create_default_dev=0
> cd /sys/kernel/config/vkms/
> mkdir DEV
> mkdir DEV/connectors/CON
> mkdir DEV/crtcs/CRT
> mkdir DEV/planes/PLA
> mkdir DEV/encoders/ENC
> ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> echo 1 > DEV/planes/PLA/type
> echo 1 > DEV/enabled
> echo 0 > DEV/enabled
> rm DEV/connectors/CON/possible_encoders/ENC
> rmdir DEV/connectors/CON
> echo 1 > DEV/enabled
> 
> Expected (and current) result:
> 
> (NULL device *): [drm] The number of connectors must be between 1 and 31
> 
> Result with the diff:
> - vkms_config_destroy_connector(connector->config);
> + //vkms_config_destroy_connector(connector->config);
> 
> (NULL device *): [drm] All connectors must have at least one possible
> encoder
> 
> This is because the connector list in vkms_config contains the deleted CON
> connector. If you also remove the connector from the list, it will be a
> memory leak.
> 
> The solution I proposed with get/put should solve this: once the device is
> disabled, all the release functions (and the corresponding
> vkms_config_destroy) will be called, so no issue there.
> 
> I attempted to do it, but it looks a bit more complex than I expected:
> - config_item_get works as expected, if you get all the items in
> device_enabled_store they are not released, even if the directory is
> deleted;
> - but as the directory is deleted, you can't use cg_children to put your
> last reference on it.
> 
> I think a solution could be to add refcounting in vkms_config, it seems to
> work, and it is even better, the refcounting is done in the vkms driver, not
> in configfs (I did it only for connector, see below). It seems to work as
> expected and doesn't increase the complexity of the code.
> 
> Do you think this is sufficient/clear enough?


This would also work. But, is it worth to include this complexity now?
I think that the configuration is handled correctly as it is now, we just
need to make sure that we don't rely on invalid pointers (as documented).

Enjoy your day,
Jose
 
> Have a nice day,
> Louis Chauvet
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c
> b/drivers/gpu/drm/vkms/vkms_config.c
> index f8394a063ecf..4dc65ab15517 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0+
> 
> +#include <linux/kref.h>
>  #include <linux/slab.h>
> 
>  #include <drm/drm_print.h>
> @@ -123,7 +124,7 @@ void vkms_config_destroy(struct vkms_config *config)
>  		vkms_config_destroy_encoder(config, encoder_cfg);
> 
>  	list_for_each_entry_safe(connector_cfg, connector_tmp,
> &config->connectors, link)
> -		vkms_config_destroy_connector(connector_cfg);
> +		vkms_config_connector_put(connector_cfg);
> 
>  	kfree_const(config->dev_name);
>  	kfree(config);
> @@ -596,17 +597,32 @@ struct vkms_config_connector
> *vkms_config_create_connector(struct vkms_config *c
> 
>  	list_add_tail(&connector_cfg->link, &config->connectors);
> 
> +	kref_init(&connector_cfg->ref);
>  	return connector_cfg;
>  }
>  EXPORT_SYMBOL_IF_KUNIT(vkms_config_create_connector);
> 
> -void vkms_config_destroy_connector(struct vkms_config_connector
> *connector_cfg)
> +static void vkms_config_destroy_connector(struct vkms_config_connector
> *connector_cfg)
>  {
>  	xa_destroy(&connector_cfg->possible_encoders);
>  	list_del(&connector_cfg->link);
>  	kfree(connector_cfg);
>  }
> -EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
> +// EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
> +
> +static void vkms_config_destroy_connector_kref(struct kref *kref)
> +{
> +	struct vkms_config_connector *connector = container_of(kref, struct
> vkms_config_connector, ref);
> +
> +	vkms_config_destroy_connector(connector);
> +}
> +
> +void vkms_config_connector_get(struct vkms_config_connector *connector) {
> +	kref_get(&connector->ref);
> +}
> +void vkms_config_connector_put(struct vkms_config_connector *connector) {
> +	kref_put(&connector->ref, vkms_config_destroy_connector_kref);
> +}
> 
>  int __must_check vkms_config_connector_attach_encoder(struct
> vkms_config_connector *connector_cfg,
>  						      struct vkms_config_encoder *encoder_cfg)
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h
> b/drivers/gpu/drm/vkms/vkms_config.h
> index e202b5a84ddd..30e6c6bf34f4 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -3,6 +3,7 @@
>  #ifndef _VKMS_CONFIG_H_
>  #define _VKMS_CONFIG_H_
> 
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/types.h>
>  #include <linux/xarray.h>
> @@ -109,6 +110,7 @@ struct vkms_config_encoder {
>   *             configuration and must be managed by other means.
>   */
>  struct vkms_config_connector {
> +	struct kref ref;
>  	struct list_head link;
>  	struct vkms_config *config;
> 
> @@ -416,11 +418,8 @@ void vkms_config_encoder_detach_crtc(struct
> vkms_config_encoder *encoder_cfg,
>   */
>  struct vkms_config_connector *vkms_config_create_connector(struct
> vkms_config *config);
> 
> -/**
> - * vkms_config_destroy_connector() - Remove and free a connector
> configuration
> - * @connector_cfg: Connector configuration to destroy
> - */
> -void vkms_config_destroy_connector(struct vkms_config_connector
> *connector_cfg);
> +void vkms_config_connector_get(struct vkms_config_connector *connector);
> +void vkms_config_connector_put(struct vkms_config_connector *connector);
> 
>  /**
>   * vkms_config_connector_attach_encoder - Attach a connector to an encoder
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c
> b/drivers/gpu/drm/vkms/vkms_configfs.c
> index 76afb0245388..ae929a084feb 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -550,7 +550,7 @@ static void connector_release(struct config_item *item)
>  	lock = &connector->dev->lock;
> 
>  	guard(mutex)(lock);
> -	vkms_config_destroy_connector(connector->config);
> +	vkms_config_connector_put(connector->config);
>  	kfree(connector);
>  }
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_connector.c
> b/drivers/gpu/drm/vkms/vkms_connector.c
> index ed99270c590f..c15451b50440 100644
> --- a/drivers/gpu/drm/vkms/vkms_connector.c
> +++ b/drivers/gpu/drm/vkms/vkms_connector.c
> @@ -20,6 +20,15 @@ static enum drm_connector_status
> vkms_connector_detect(struct drm_connector *con
>  	return status;
>  }
> 
> +static void vkms_connector_destroy(struct drm_device *dev, void *raw)
> +{
> +	struct vkms_connector *vkms_connector = raw;
> +
> +	vkms_config_connector_put(vkms_connector->connector_cfg);
> +
> +	return;
> +}
> +
>  static const struct drm_connector_funcs vkms_connector_funcs = {
>  	.detect = vkms_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> @@ -65,8 +74,13 @@ struct vkms_connector *vkms_connector_init(struct
> vkms_device *vkmsdev,
>  	if (!connector)
>  		return ERR_PTR(-ENOMEM);
> 
> +	vkms_config_connector_get(connector->connector_cfg);
>  	connector->connector_cfg = connector_cfg;
> 
> +	ret = drmm_add_action_or_reset(dev, &vkms_connector_destroy, connector);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	ret = drmm_connector_init(dev, &connector->base, &vkms_connector_funcs,
>  				  DRM_MODE_CONNECTOR_VIRTUAL, NULL);
>  	if (ret)
> 
>