[PATCH RESEND] drm/connector: check if destroy function exist

[PATCH RESEND] drm/connector: check if destroy function exist

[Andreas Kemnade]

Connectors might have not resources needed to be manually freed.
E.g. the drm_bridge_connector does not have such a callback.

Fixes: c12907be57b1 ("drm/bridge-connector: switch to using drmm allocations")
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/gpu/drm/drm_connector.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 47dc53c4a738..d2dfc6e8f7d0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -198,7 +198,9 @@ static void drm_connector_free(struct kref *kref)
 	struct drm_device *dev = connector->dev;
 
 	drm_mode_object_unregister(dev, &connector->base);
-	connector->funcs->destroy(connector);
+
+	if (connector->funcs->destroy)
+		connector->funcs->destroy(connector);
 }
 
 void drm_connector_free_work_fn(struct work_struct *work)
@@ -216,7 +218,9 @@ void drm_connector_free_work_fn(struct work_struct *work)
 
 	llist_for_each_entry_safe(connector, n, freed, free_node) {
 		drm_mode_object_unregister(dev, &connector->base);
-		connector->funcs->destroy(connector);
+
+		if (connector->funcs->destroy)
+			connector->funcs->destroy(connector);
 	}
 }
 
-- 
2.47.3

Re: [PATCH RESEND] drm/connector: check if destroy function exist

[sashiko-bot]

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

Pre-existing issues:
- [High] Manual invocation of `funcs->destroy()` causes a NULL pointer dereference on the error path in `msm_hdmi_modeset_init`.
--

commit aa404d4746f85dae926593fae989fac53f064b44
Author: Andreas Kemnade <andreas@kemnade.info>

drm/connector: check if destroy function exist

Connectors might have not resources needed to be manually freed.
E.g. the drm_bridge_connector does not have such a callback.

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8b4baed060f3a..288d9d6c75a8d 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -198,7 +198,9 @@ static void drm_connector_free(struct kref *kref)
>  	struct drm_device *dev = connector->dev;
>  
>  	drm_mode_object_unregister(dev, &connector->base);
> -	connector->funcs->destroy(connector);
> +
> +	if (connector->funcs->destroy)
> +		connector->funcs->destroy(connector);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but could the same NULL pointer
dereference happen in the error path of msm_hdmi_modeset_init() when it
manually cleans up the connector?

If devm_request_irq() fails after the drm_bridge_connector is successfully
initialized, it seems the failure path blindly calls the destroy callback:

drivers/gpu/drm/msm/hdmi/hdmi.c:msm_hdmi_modeset_init() {
	...
	hdmi->connector = drm_bridge_connector_init(hdmi->dev, encoder);
	if (IS_ERR(hdmi->connector)) {
		...
	}

	ret = devm_request_irq(dev->dev, hdmi->irq, ...);
	if (ret < 0) {
		DRM_DEV_ERROR(dev->dev, "failed to request IRQ%u: %d\n",
				hdmi->irq, ret);
		goto fail;
	}

	return 0;

fail:
	if (hdmi->connector) {
		hdmi->connector->funcs->destroy(hdmi->connector);
		hdmi->connector = NULL;
	}

	return ret;
}

Should this driver-specific cleanup path also be updated to check if
funcs->destroy is present, similar to the fix applied here in
drm_connector_free?

>  
>  void drm_connector_free_work_fn(struct work_struct *work)
[ ... ]
> @@ -216,7 +218,9 @@ void drm_connector_free_work_fn(struct work_struct *work)
>  
>  	llist_for_each_entry_safe(connector, n, freed, free_node) {
>  		drm_mode_object_unregister(dev, &connector->base);
> -		connector->funcs->destroy(connector);
> +
> +		if (connector->funcs->destroy)
> +			connector->funcs->destroy(connector);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702191453.611070-1-andreas@kemnade.info?part=1

Re: [PATCH RESEND] drm/connector: check if destroy function exist

[Thomas Zimmermann]

Hi

Am 02.07.26 um 21:14 schrieb Andreas Kemnade:
> Connectors might have not resources needed to be manually freed.
> E.g. the drm_bridge_connector does not have such a callback.
>
> Fixes: c12907be57b1 ("drm/bridge-connector: switch to using drmm allocations")

This patch uses drmm_connector_init() for the cleanup, which is a 
mis-design IMHO.  Rather than calling drm_connector_cleanup(), it should 
call connector->funcs->destroy, which then points to 
drm_connector_cleanup. This would make the whole thing more useful and 
not require fixups (like this one) elsewhere in the code. Just just my 2cts.

Best regards
Thomas


> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>   drivers/gpu/drm/drm_connector.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 47dc53c4a738..d2dfc6e8f7d0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -198,7 +198,9 @@ static void drm_connector_free(struct kref *kref)
>   	struct drm_device *dev = connector->dev;
>   
>   	drm_mode_object_unregister(dev, &connector->base);
> -	connector->funcs->destroy(connector);
> +
> +	if (connector->funcs->destroy)
> +		connector->funcs->destroy(connector);
>   }
>   
>   void drm_connector_free_work_fn(struct work_struct *work)
> @@ -216,7 +218,9 @@ void drm_connector_free_work_fn(struct work_struct *work)
>   
>   	llist_for_each_entry_safe(connector, n, freed, free_node) {
>   		drm_mode_object_unregister(dev, &connector->base);
> -		connector->funcs->destroy(connector);
> +
> +		if (connector->funcs->destroy)
> +			connector->funcs->destroy(connector);
>   	}
>   }
>   

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)