| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aP-NH2IWRl_juhaI@fedora>
Date: Mon, 27 Oct 2025 16:17:51 +0100
From: José Expósito <jose.exposito89@...il.com>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: Haneen Mohammed <hamohammed.sa@...il.com>,
Simona Vetter <simona@...ll.ch>,
Melissa Wen <melissa.srw@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Jonathan Corbet <corbet@....net>,
victoria@...tem76.com, sebastian.wick@...hat.com,
thomas.petazzoni@...tlin.com, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 22/22] drm/vkms: Allows the creation of connector at
runtime
On Sat, Oct 18, 2025 at 04:01:22AM +0200, Louis Chauvet wrote:
> DRM allows the connector to be created after the device. To allows
> emulating this, add two configfs attributes to connector to allows this.
>
> Using the dynamic attribute you can set if a connector will be dynamic or
> not.
> Using the enabled attribute, you can set at runtime if a dynamic connector
> is present or not.
>
> Co-developed-by: José Expósito <jose.exposito89@...il.com>
> Signed-off-by: José Expósito <jose.exposito89@...il.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> ---
> Documentation/gpu/vkms.rst | 6 +-
> drivers/gpu/drm/vkms/vkms_configfs.c | 146 ++++++++++++++++++++++++++++++++---
> 2 files changed, 139 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index bbd03f61e61c..8b17aaa28eeb 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -135,7 +135,7 @@ Last but not least, create one or more connectors::
>
> sudo mkdir /config/vkms/my-vkms/connectors/connector0
>
> -Connectors have 5 configurable attribute:
> +Connectors have 7 configurable attribute:
>
> - status: Connection status: 1 connected, 2 disconnected, 3 unknown (same values
> as those exposed by the "status" property of a connector)
> @@ -147,7 +147,9 @@ Connectors have 5 configurable attribute:
> - edid_enabled: Enable or not EDID for this connector. Some connectors may not have an
> EDID but just a list of modes, this attribute allows to disable EDID property.
> - edid: Content of the EDID. Ignored if edid_enabled is not set
> -
> +- dynamic: Set to 1 while configuring the device to create a dynamic connector. A dynamic
> + connector can be used to emulate DP MST connectors.
> +- enabled: For dynamic connector, set it to 1 to create the connector, 0 to remove it.
>
> To finish the configuration, link the different pipeline items::
>
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index a977c0842cd6..937b749142ad 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -1115,8 +1115,12 @@ static ssize_t connector_status_store(struct config_item *item,
> scoped_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);
> + if (connector->dev->enabled) {
> + if (connector->config->dynamic && connector->config->enabled)
> + vkms_trigger_connector_hotplug(connector->dev->config->dev);
> + if (!connector->config->dynamic)
> + vkms_trigger_connector_hotplug(connector->dev->config->dev);
To avoid duplicating code, we could do something like:
bool hotplug = !connector->config->dynamic ||
(connector->config->dynamic && connector->config->enabled);
if (hotplug)
vkms_trigger_connector_hotplug(connector->dev->config->dev);
> + }
> }
>
> return (ssize_t)count;
> @@ -1176,8 +1180,12 @@ static ssize_t connector_type_store(struct config_item *item,
> }
>
> scoped_guard(mutex, &connector->dev->lock) {
> - if (connector->dev->enabled)
> - return -EINVAL;
> + if (connector->dev->enabled) {
> + if (connector->config->dynamic && connector->config->enabled)
> + return -EBUSY;
> + if (!connector->config->dynamic)
> + return -EBUSY;
> + }
And something similar here. Also, instead of returning -EINVAL
in "drm/vkms: Introduce configfs for connector type", you could return
-EBUSY and avoid changing the return type here.
> vkms_config_connector_set_type(connector->config, val);
> }
>
> @@ -1293,6 +1301,102 @@ static ssize_t connector_edid_store(struct config_item *item,
> connector_status_disconnected)
> vkms_trigger_connector_hotplug(connector->dev->config->dev);
> }
> + return count;
> +}
> +
> +static ssize_t connector_enabled_show(struct config_item *item, char *page)
> +{
> + struct vkms_configfs_connector *connector;
> + bool enabled;
> +
> + connector = connector_item_to_vkms_configfs_connector(item);
> +
> + scoped_guard(mutex, &connector->dev->lock)
> + enabled = vkms_config_connector_is_enabled(connector->config);
> +
> + return sprintf(page, "%d\n", enabled);
> +}
> +
> +static ssize_t connector_enabled_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct vkms_configfs_connector *connector;
> + struct vkms_config_connector *connector_cfg;
> + bool enabled, was_enabled;
> +
> + connector = connector_item_to_vkms_configfs_connector(item);
> + connector_cfg = connector->config;
> +
> + if (kstrtobool(page, &enabled))
> + return -EINVAL;
> +
> + if (!connector->dev->enabled) {
> + vkms_config_connector_set_enabled(connector_cfg, enabled);
> + } else {
> + // Only dynamic connector can be enabled/disabled at runtime
> + if (!connector_cfg->dynamic)
> + return -EBUSY;
> +
> + was_enabled = vkms_config_connector_is_enabled(connector_cfg);
> + vkms_config_connector_set_enabled(connector_cfg, enabled);
> +
> + // Resulting configuration is invalid (missing encoder for example)
> + // Early return to avoid drm core issue
> + if (!vkms_config_is_valid(connector->dev->config)) {
> + vkms_config_connector_set_enabled(connector_cfg, was_enabled);
> + return -EINVAL;
> + }
> +
> + if (!was_enabled && enabled) {
> + // Adding the connector
> + connector_cfg->connector = vkms_connector_hot_add(connector->dev->config->dev,
> + connector_cfg);
> + if (IS_ERR(connector_cfg->connector)) {
> + vkms_config_connector_set_enabled(connector_cfg, was_enabled);
> + return PTR_ERR(connector_cfg->connector);
> + }
> + } else if (was_enabled && !enabled) {
> + vkms_connector_hot_remove(connector->dev->config->dev,
> + connector_cfg->connector);
> + }
> + }
> + return count;
As a suggestion, we could add a "goto rollback;" and avoid duplicating error
handling:
rollback:
vkms_config_connector_set_enabled(connector_cfg, was_enabled);
return ret;
> +}
> +
> +static ssize_t connector_dynamic_show(struct config_item *item, char *page)
> +{
> + struct vkms_configfs_connector *connector;
> + bool enabled;
> +
> + connector = connector_item_to_vkms_configfs_connector(item);
> +
> + scoped_guard(mutex, &connector->dev->lock) {
> + enabled = vkms_config_connector_is_dynamic(connector->config);
> + }
> +
> + return sprintf(page, "%d\n", enabled);
> +}
> +
> +static ssize_t connector_dynamic_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct vkms_configfs_connector *connector;
> + struct vkms_config_connector *connector_cfg;
> + bool dynamic;
> +
> + connector = connector_item_to_vkms_configfs_connector(item);
> + connector_cfg = connector->config;
> +
> + if (kstrtobool(page, &dynamic))
> + return -EINVAL;
> +
> + scoped_guard(mutex, &connector->dev->lock) {
> + // Can't change the dynamic status when the device is activated
> + if (connector->dev->enabled)
> + return -EBUSY;
> +
> + vkms_config_connector_set_dynamic(connector_cfg, dynamic);
> + }
>
> return count;
> }
> @@ -1302,6 +1406,8 @@ CONFIGFS_ATTR(connector_, type);
> CONFIGFS_ATTR(connector_, supported_colorspaces);
> CONFIGFS_ATTR(connector_, edid_enabled);
> CONFIGFS_ATTR(connector_, edid);
> +CONFIGFS_ATTR(connector_, dynamic);
> +CONFIGFS_ATTR(connector_, enabled);
>
> static struct configfs_attribute *connector_item_attrs[] = {
> &connector_attr_status,
> @@ -1309,19 +1415,28 @@ static struct configfs_attribute *connector_item_attrs[] = {
> &connector_attr_supported_colorspaces,
> &connector_attr_edid_enabled,
> &connector_attr_edid,
> + &connector_attr_dynamic,
> + &connector_attr_enabled,
> NULL,
> };
>
> static void connector_release(struct config_item *item)
> {
> struct vkms_configfs_connector *connector;
> + struct vkms_config_connector *connector_cfg;
> struct mutex *lock;
>
> connector = connector_item_to_vkms_configfs_connector(item);
> + connector_cfg = connector->config;
> lock = &connector->dev->lock;
>
> scoped_guard(mutex, lock) {
> + if (connector->dev->enabled && connector_cfg->dynamic && connector_cfg->enabled)
> + vkms_connector_hot_remove(connector->dev->config->dev,
> + connector_cfg->connector);
> +
> vkms_config_destroy_connector(connector->config);
> +
> kfree(connector);
> }
> }
> @@ -1340,6 +1455,7 @@ static int connector_possible_encoders_allow_link(struct config_item *src,
> struct config_item *target)
> {
> struct vkms_configfs_connector *connector;
> + struct vkms_config_connector *connector_cfg;
> struct vkms_configfs_encoder *encoder;
> int ret;
>
> @@ -1347,16 +1463,25 @@ static int connector_possible_encoders_allow_link(struct config_item *src,
> return -EINVAL;
>
> connector = connector_possible_encoders_item_to_vkms_configfs_connector(src);
> + connector_cfg = connector->config;
> encoder = encoder_item_to_vkms_configfs_encoder(target);
>
> scoped_guard(mutex, &connector->dev->lock) {
> - if (connector->dev->enabled)
> - return -EBUSY;
> + if (connector->dev->enabled && connector_cfg->enabled) {
> + if (!connector_cfg->dynamic)
> + return -EBUSY;
> + ret = vkms_connector_hot_attach_encoder(connector->dev->config->dev,
> + connector->config->connector,
> + encoder->config->encoder);
> + if (ret)
> + return ret;
> + }
>
> ret = vkms_config_connector_attach_encoder(connector->config,
> encoder->config);
> + if (ret)
> + return ret;
> }
> -
> return ret;
> }
>
> @@ -1394,9 +1519,6 @@ static struct config_group *make_connector_group(struct config_group *group,
> dev = child_group_to_vkms_configfs_device(group);
>
> scoped_guard(mutex, &dev->lock) {
> - if (dev->enabled)
> - return ERR_PTR(-EBUSY);
> -
> connector = kzalloc(sizeof(*connector), GFP_KERNEL);
> if (!connector)
> return ERR_PTR(-ENOMEM);
> @@ -1409,9 +1531,11 @@ static struct config_group *make_connector_group(struct config_group *group,
> return ERR_CAST(connector->config);
> }
>
> + vkms_config_connector_set_dynamic(connector->config, connector->dev->enabled);
> + vkms_config_connector_set_enabled(connector->config, !connector->dev->enabled);
> +
> 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);
>
> --
> 2.51.0
>
Powered by blists - more mailing lists