[PATCH] drm/qxl: remove dependency on DRM simple helpers

[PATCH] drm/qxl: remove dependency on DRM simple helpers

[Diogo Silva]

Simple KMS helper are deprecated since they only add an intermediate
layer between drivers and the atomic modesetting.
This patch removes the drm_simple_encoder_init() helper usage in the
qxl display driver by open coding it and using the encoder atomic
helpers directly. This is a step to eventually get rid of this simple
KMS helper, once all drivers that use it have been converted.

Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
 drivers/gpu/drm/qxl/qxl_display.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index a026bd35ef485..236d027fbc9fe 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -31,12 +31,12 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_vblank.h>
 #include <drm/drm_vblank_helper.h>
@@ -1095,6 +1095,10 @@ static const struct drm_connector_helper_funcs qxl_connector_helper_funcs = {
 	.best_encoder = qxl_best_encoder,
 };
 
+static const struct drm_connector_funcs qxl_connector_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static enum drm_connector_status qxl_conn_detect(
 			struct drm_connector *connector,
 			bool force)
@@ -1169,10 +1173,10 @@ static int qdev_output_init(struct drm_device *dev, int num_output)
 	drm_connector_init(dev, &qxl_output->base,
 			   &qxl_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL);
 
-	ret = drm_simple_encoder_init(dev, &qxl_output->enc,
-				      DRM_MODE_ENCODER_VIRTUAL);
+	ret = drm_encoder_init(dev, &qxl_output->enc, &qxl_connector_funcs,
+			       DRM_MODE_ENCODER_VIRTUAL, NULL);
 	if (ret) {
-		drm_err(dev, "drm_simple_encoder_init() failed, error %d\n",
+		drm_err(dev, "drm_encoder_init() failed, error %d\n",
 			ret);
 		goto err_drm_connector_cleanup;
 	}

---
base-commit: ee2867b79f9bac3a6fc3221139b09598ce79099c
change-id: 20260707-qxl-simple-c01aa1a5c4eb

Best regards,
-- 
Diogo Silva <diogompaissilva@gmail.com>

Re: [PATCH] drm/qxl: remove dependency on DRM simple helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] The patch mistakenly defines a `drm_connector_funcs` structure named `qxl_connector_funcs` instead of a `drm_encoder_funcs` structure for the new encoder, causing a redefinition build error.

Pre-existing issues:
- [High] Lockless reallocation of `client_monitors_config` leads to Use-After-Free during connector operations.
- [High] Missing `free_irq()` and `cancel_work_sync()` during device teardown leads to Use-After-Free on module unload.
--

--- Patch [1]: [PATCH] drm/qxl: remove dependency on DRM simple helpers ---
commit d182374fb0440bb0cb67a0c069afac8494f4bf85
Author: Diogo Silva <diogompaissilva@gmail.com>

drm/qxl: remove dependency on DRM simple helpers

This patch removes the drm_simple_encoder_init() helper usage in the qxl
display driver by open coding it and using the encoder atomic helpers
directly. This is a step to eventually get rid of the simple KMS helper.

> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index a026bd35ef485..236d027fbc9fe 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -1095,6 +1095,10 @@ static const struct drm_connector_helper_funcs qxl_connector_helper_funcs = {
>  	.best_encoder = qxl_best_encoder,
>  };
>  
> +static const struct drm_connector_funcs qxl_connector_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +

[Severity: Low]
Does this code inadvertently define struct drm_connector_funcs instead of
struct drm_encoder_funcs?

The struct name qxl_connector_funcs also conflicts with an existing definition
later in the file.

[ ... ]

> @@ -1169,10 +1173,10 @@ static int qdev_output_init(struct drm_device *dev, int num_output)
>  	drm_connector_init(dev, &qxl_output->base,
>  			   &qxl_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL);
>  
> -	ret = drm_simple_encoder_init(dev, &qxl_output->enc,
> -				      DRM_MODE_ENCODER_VIRTUAL);
> +	ret = drm_encoder_init(dev, &qxl_output->enc, &qxl_connector_funcs,
> +			       DRM_MODE_ENCODER_VIRTUAL, NULL);

[Severity: Low]
Is this passing a pointer to the newly added struct drm_connector_funcs where
drm_encoder_init() expects struct drm_encoder_funcs *?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
risk during connector operations?

In qxl_alloc_client_monitors_config() in drivers/gpu/drm/qxl/qxl_display.c, 
the shared client monitors config can be freed locklessly:

	if (qdev->client_monitors_config &&
	    count > qdev->client_monitors_config->count) {
		kfree(qdev->client_monitors_config);
		qdev->client_monitors_config = NULL;
	}

Concurrently, DRM ioctls like qxl_conn_detect() check and dereference this
pointer without synchronization:

	if (!qdev->client_monitors_config) {
		if (output->index == 0)
			connected = true;
	} else
		connected = qdev->client_monitors_config->count > output->index &&
		     qxl_head_enabled(&qdev->client_monitors_config->heads[output->index]);

Could a concurrent reallocation free the config while it is being accessed here?


[Severity: High]
This is a pre-existing issue, but is there a missing teardown of the IRQ
handler and workqueue on module unload?

In qxl_irq_init() in drivers/gpu/drm/qxl/qxl_irq.c, the IRQ is manually 
requested and the workqueue is initialized:

	INIT_WORK(&qdev->client_monitors_config_work,
		  qxl_client_monitors_config_work_func);
...
	ret = request_irq(pdev->irq, qxl_irq_handler, IRQF_SHARED, 
			  ddev->driver->name, ddev);

However, the teardown logic in qxl_pci_remove() in drivers/gpu/drm/qxl/qxl_drv.c 
appears to omit free_irq() and cancel_work_sync():

static void
qxl_pci_remove(struct pci_dev *pdev)
{
	struct drm_device *dev = pci_get_drvdata(pdev);

	drm_kms_helper_poll_fini(dev);
	drm_dev_unregister(dev);
	drm_atomic_helper_shutdown(dev);

Does this leak the IRQ handler registration, potentially leading to a
use-after-free if the shared IRQ fires after the device is unbound?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-qxl-simple-v1-1-524f6316b5d5@gmail.com?part=1