public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] drm/bridge: bridge-connector: get/put the stored bridges
@ 2025-09-25 17:19 Luca Ceresoli
  2025-09-25 17:19 ` [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors Luca Ceresoli
  2025-09-25 17:19 ` [PATCH 2/2] drm/display: bridge-connector: get/put the stored bridges Luca Ceresoli
  0 siblings, 2 replies; 6+ messages in thread
From: Luca Ceresoli @ 2025-09-25 17:19 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Dmitry Baryshkov
  Cc: Hui Pu, Thomas Petazzoni, dri-devel, linux-kernel, Luca Ceresoli

This series ensures the bridge-connector gets a reference to bridges when
storing a pointer to them, and releases them afterwards.

This is part of the work towards removal of bridges from a still existing
DRM pipeline without use-after-free. The grand plan was discussed in [1].
Here's the work breakdown (➜ marks the current series):

 1. ➜ add refcounting to DRM bridges (struct drm_bridge)
    (based on devm_drm_bridge_alloc() [0])
    A. ✔ add new alloc API and refcounting (v6.16)
    B. ✔ convert all bridge drivers to new API (v6.17)
    C. ✔ kunit tests (v6.17)
    D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
         and warn on old allocation pattern (v6.17)
    E. … add get/put on drm_bridge accessors
       1. ✔ drm_bridge_chain_get_first_bridge() + add a cleanup action
            (drm-misc-next)
       2. ✔ drm_bridge_get_prev_bridge() (drm-misc-next)
       3. ✔ drm_bridge_get_next_bridge() (drm-misc-next)
       4. ✔ drm_for_each_bridge_in_chain() (drm-misc-next)
       5. ➜ drm_bridge_connector_init
       6. protect encoder bridge chain with a mutex
       7. of_drm_find_bridge
       8. drm_of_find_panel_or_bridge, *_of_get_bridge
    F. ➜ debugfs improvements
       1. ✔ add top-level 'bridges' file (v6.16)
       2. ✔ show refcount and list removed bridges (drm-misc-next)
 2. … handle gracefully atomic updates during bridge removal
 3. … DSI host-device driver interaction
 4. removing the need for the "always-disconnected" connector
 5. finish the hotplug bridge work, moving code to the core and potentially
    removing the hotplug-bridge itself (this needs to be clarified as
    points 1-3 are developed)

This was tricky both because there is no central place in
drm_bridge_connector.c to put the references on disposal (handled by patch
1) and because of the complex code flow of drm_bridge_connector_init()
(handled by patch 2).

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (2):
      drm/connector: allow a .destroy callback for drmm-allocated connectors
      drm/display: bridge-connector: get/put the stored bridges

 drivers/gpu/drm/display/drm_bridge_connector.c | 112 +++++++++++++++++--------
 drivers/gpu/drm/drm_connector.c                |   6 +-
 2 files changed, 78 insertions(+), 40 deletions(-)
---
base-commit: 063db451832b8849faf1b0b8404b3a6a39995b29
change-id: 20250925-drm-bridge-alloc-getput-bridge-connector-556f8dc14af4

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors
  2025-09-25 17:19 [PATCH 0/2] drm/bridge: bridge-connector: get/put the stored bridges Luca Ceresoli
@ 2025-09-25 17:19 ` Luca Ceresoli
  2025-09-25 22:07   ` Dmitry Baryshkov
  2025-09-26  7:33   ` Jani Nikula
  2025-09-25 17:19 ` [PATCH 2/2] drm/display: bridge-connector: get/put the stored bridges Luca Ceresoli
  1 sibling, 2 replies; 6+ messages in thread
From: Luca Ceresoli @ 2025-09-25 17:19 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Dmitry Baryshkov
  Cc: Hui Pu, Thomas Petazzoni, dri-devel, linux-kernel, Luca Ceresoli

Some code is going to need connector-specific cleanup actions (namely
drm_bridge_connector will need to put refcounted bridges).

The .destroy callback is appropriate for this task but it is currently
forbidden by drmm_connector_init(). Relax this limitation and document it.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

The other obvious approach would be adding a separate .cleanup callback for
the cleanup-only actions. I tried both, they both apparently work, so any
arguments and opinions on which approach is best within the overall DRM
design would be very useful here.
---
 drivers/gpu/drm/drm_connector.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 272d6254ea4784e97ca894ec4d463beebf9fdbf0..bd0220513a23afcb096b0c4c4d2b957b81f21ee1 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -513,7 +513,8 @@ static void drm_connector_cleanup_action(struct drm_device *dev,
  *
  * The connector structure should be allocated with drmm_kzalloc().
  *
- * The @drm_connector_funcs.destroy hook must be NULL.
+ * The @drm_connector_funcs.destroy hook must only do connector-specific
+ * cleanups if any is needed, not dealloacte the connector.
  *
  * Returns:
  * Zero on success, error code on failure.
@@ -526,9 +527,6 @@ int drmm_connector_init(struct drm_device *dev,
 {
 	int ret;
 
-	if (drm_WARN_ON(dev, funcs && funcs->destroy))
-		return -EINVAL;
-
 	ret = drm_connector_init_and_add(dev, connector, funcs, connector_type, ddc);
 	if (ret)
 		return ret;

-- 
2.51.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] drm/display: bridge-connector: get/put the stored bridges
  2025-09-25 17:19 [PATCH 0/2] drm/bridge: bridge-connector: get/put the stored bridges Luca Ceresoli
  2025-09-25 17:19 ` [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors Luca Ceresoli
@ 2025-09-25 17:19 ` Luca Ceresoli
  1 sibling, 0 replies; 6+ messages in thread
From: Luca Ceresoli @ 2025-09-25 17:19 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Dmitry Baryshkov
  Cc: Hui Pu, Thomas Petazzoni, dri-devel, linux-kernel, Luca Ceresoli

drm_bridge_connector_init() takes eight pointers to various bridges, some
of which can be identical, and stores them in pointers inside struct
drm_bridge_connector. Get a reference to each of the taken bridges and put
it on cleanup.

This is tricky because the pointers are currently stored directly in the
drm_bridge_connector in the loop, but there is no nice and clean way to put
those pointers on error return paths. To overcome this, store all pointers
in temporary local variables with a cleanup action, and only on success
copy them into struct drm_bridge_connector (getting another ref while
copying).

Additionally four of these pointers (edid, hpd, detect and modes) can be
written in multiple loop iterations, in order to eventually store the last
matching bridge. However when one of those pointers is overwritten once we
need to put the reference that we got during the previous assignment. Add a
drm_bridge_put() before writing them to handle this.

Finally, there is also a function-local panel_bridge pointer taken inside
the loop and used after the loop. Due to the various return points, use a
__free() cleanup action to ensure it is put on return.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 112 +++++++++++++++++--------
 1 file changed, 76 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index a5bdd6c1064399ece6b19560f145b877c9e0680e..4bcebbb8c9935fa55010a9d317f3d9e70b2ff7a3 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -268,11 +268,27 @@ static void drm_bridge_connector_reset(struct drm_connector *connector)
 							 connector->state);
 }
 
+static void drm_bridge_connector_destroy(struct drm_connector *connector)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+
+	drm_bridge_put(bridge_connector->bridge_edid);
+	drm_bridge_put(bridge_connector->bridge_hpd);
+	drm_bridge_put(bridge_connector->bridge_detect);
+	drm_bridge_put(bridge_connector->bridge_modes);
+	drm_bridge_put(bridge_connector->bridge_hdmi);
+	drm_bridge_put(bridge_connector->bridge_hdmi_audio);
+	drm_bridge_put(bridge_connector->bridge_dp_audio);
+	drm_bridge_put(bridge_connector->bridge_hdmi_cec);
+}
+
 static const struct drm_connector_funcs drm_bridge_connector_funcs = {
 	.reset = drm_bridge_connector_reset,
 	.detect = drm_bridge_connector_detect,
 	.force = drm_bridge_connector_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_bridge_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 	.debugfs_init = drm_bridge_connector_debugfs_init,
@@ -638,7 +654,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	struct drm_bridge_connector *bridge_connector;
 	struct drm_connector *connector;
 	struct i2c_adapter *ddc = NULL;
-	struct drm_bridge *panel_bridge = NULL;
+	struct drm_bridge *panel_bridge      __free(drm_bridge_put) = NULL;
+	struct drm_bridge *bridge_edid       __free(drm_bridge_put) = NULL;
+	struct drm_bridge *bridge_hpd        __free(drm_bridge_put) = NULL;
+	struct drm_bridge *bridge_detect     __free(drm_bridge_put) = NULL;
+	struct drm_bridge *bridge_modes      __free(drm_bridge_put) = NULL;
+	struct drm_bridge *bridge_hdmi       __free(drm_bridge_put) = NULL;
+	struct drm_bridge *bridge_hdmi_audio __free(drm_bridge_put) = NULL;
+	struct drm_bridge *bridge_dp_audio   __free(drm_bridge_put) = NULL;
+	struct drm_bridge *bridge_hdmi_cec   __free(drm_bridge_put) = NULL;
 	unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
 	unsigned int max_bpc = 8;
 	bool support_hdcp = false;
@@ -672,22 +696,30 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		if (!bridge->ycbcr_420_allowed)
 			connector->ycbcr_420_allowed = false;
 
-		if (bridge->ops & DRM_BRIDGE_OP_EDID)
-			bridge_connector->bridge_edid = bridge;
-		if (bridge->ops & DRM_BRIDGE_OP_HPD)
-			bridge_connector->bridge_hpd = bridge;
-		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
-			bridge_connector->bridge_detect = bridge;
-		if (bridge->ops & DRM_BRIDGE_OP_MODES)
-			bridge_connector->bridge_modes = bridge;
+		if (bridge->ops & DRM_BRIDGE_OP_EDID) {
+			drm_bridge_put(bridge_edid);
+			bridge_edid = drm_bridge_get(bridge);
+		}
+		if (bridge->ops & DRM_BRIDGE_OP_HPD) {
+			drm_bridge_put(bridge_hpd);
+			bridge_hpd = drm_bridge_get(bridge);
+		}
+		if (bridge->ops & DRM_BRIDGE_OP_DETECT) {
+			drm_bridge_put(bridge_detect);
+			bridge_detect = drm_bridge_get(bridge);
+		}
+		if (bridge->ops & DRM_BRIDGE_OP_MODES) {
+			drm_bridge_put(bridge_modes);
+			bridge_modes = drm_bridge_get(bridge);
+		}
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
-			if (bridge_connector->bridge_hdmi)
+			if (bridge_hdmi)
 				return ERR_PTR(-EBUSY);
 			if (!bridge->funcs->hdmi_write_infoframe ||
 			    !bridge->funcs->hdmi_clear_infoframe)
 				return ERR_PTR(-EINVAL);
 
-			bridge_connector->bridge_hdmi = bridge;
+			bridge_hdmi = drm_bridge_get(bridge);
 
 			if (bridge->supported_formats)
 				supported_formats = bridge->supported_formats;
@@ -696,10 +728,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI_AUDIO) {
-			if (bridge_connector->bridge_hdmi_audio)
+			if (bridge_hdmi_audio)
 				return ERR_PTR(-EBUSY);
 
-			if (bridge_connector->bridge_dp_audio)
+			if (bridge_dp_audio)
 				return ERR_PTR(-EBUSY);
 
 			if (!bridge->hdmi_audio_max_i2s_playback_channels &&
@@ -710,14 +742,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			    !bridge->funcs->hdmi_audio_shutdown)
 				return ERR_PTR(-EINVAL);
 
-			bridge_connector->bridge_hdmi_audio = bridge;
+			bridge_hdmi_audio = drm_bridge_get(bridge);
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_DP_AUDIO) {
-			if (bridge_connector->bridge_dp_audio)
+			if (bridge_dp_audio)
 				return ERR_PTR(-EBUSY);
 
-			if (bridge_connector->bridge_hdmi_audio)
+			if (bridge_hdmi_audio)
 				return ERR_PTR(-EBUSY);
 
 			if (!bridge->hdmi_audio_max_i2s_playback_channels &&
@@ -728,7 +760,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			    !bridge->funcs->dp_audio_shutdown)
 				return ERR_PTR(-EINVAL);
 
-			bridge_connector->bridge_dp_audio = bridge;
+			bridge_dp_audio = drm_bridge_get(bridge);
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
@@ -739,10 +771,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
-			if (bridge_connector->bridge_hdmi_cec)
+			if (bridge_hdmi_cec)
 				return ERR_PTR(-EBUSY);
 
-			bridge_connector->bridge_hdmi_cec = bridge;
+			bridge_hdmi_cec = drm_bridge_get(bridge);
 
 			if (!bridge->funcs->hdmi_cec_enable ||
 			    !bridge->funcs->hdmi_cec_log_addr ||
@@ -762,7 +794,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			ddc = bridge->ddc;
 
 		if (drm_bridge_is_panel(bridge))
-			panel_bridge = bridge;
+			panel_bridge = drm_bridge_get(bridge);
 
 		if (bridge->support_hdcp)
 			support_hdcp = true;
@@ -771,13 +803,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	if (connector_type == DRM_MODE_CONNECTOR_Unknown)
 		return ERR_PTR(-EINVAL);
 
-	if (bridge_connector->bridge_hdmi) {
+	if (bridge_hdmi) {
 		if (!connector->ycbcr_420_allowed)
 			supported_formats &= ~BIT(HDMI_COLORSPACE_YUV420);
 
 		ret = drmm_connector_hdmi_init(drm, connector,
-					       bridge_connector->bridge_hdmi->vendor,
-					       bridge_connector->bridge_hdmi->product,
+					       bridge_hdmi->vendor,
+					       bridge_hdmi->product,
 					       &drm_bridge_connector_funcs,
 					       &drm_bridge_connector_hdmi_funcs,
 					       connector_type, ddc,
@@ -793,15 +825,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			return ERR_PTR(ret);
 	}
 
-	if (bridge_connector->bridge_hdmi_audio ||
-	    bridge_connector->bridge_dp_audio) {
+	if (bridge_hdmi_audio || bridge_dp_audio) {
 		struct device *dev;
 		struct drm_bridge *bridge;
 
-		if (bridge_connector->bridge_hdmi_audio)
-			bridge = bridge_connector->bridge_hdmi_audio;
+		if (bridge_hdmi_audio)
+			bridge = bridge_hdmi_audio;
 		else
-			bridge = bridge_connector->bridge_dp_audio;
+			bridge = bridge_dp_audio;
 
 		dev = bridge->hdmi_audio_dev;
 
@@ -815,9 +846,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			return ERR_PTR(ret);
 	}
 
-	if (bridge_connector->bridge_hdmi_cec &&
-	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
-		struct drm_bridge *bridge = bridge_connector->bridge_hdmi_cec;
+	if (bridge_hdmi_cec &&
+	    bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
+		struct drm_bridge *bridge = bridge_hdmi_cec;
 
 		ret = drmm_connector_hdmi_cec_notifier_register(connector,
 								NULL,
@@ -826,9 +857,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			return ERR_PTR(ret);
 	}
 
-	if (bridge_connector->bridge_hdmi_cec &&
-	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
-		struct drm_bridge *bridge = bridge_connector->bridge_hdmi_cec;
+	if (bridge_hdmi_cec &&
+	    bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
+		struct drm_bridge *bridge = bridge_hdmi_cec;
 
 		ret = drmm_connector_hdmi_cec_register(connector,
 						       &drm_bridge_connector_hdmi_cec_funcs,
@@ -841,9 +872,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 
 	drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
 
-	if (bridge_connector->bridge_hpd)
+	if (bridge_hpd)
 		connector->polled = DRM_CONNECTOR_POLL_HPD;
-	else if (bridge_connector->bridge_detect)
+	else if (bridge_detect)
 		connector->polled = DRM_CONNECTOR_POLL_CONNECT
 				  | DRM_CONNECTOR_POLL_DISCONNECT;
 
@@ -854,6 +885,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	    IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
 		drm_connector_attach_content_protection_property(connector, true);
 
+	bridge_connector->bridge_edid       = drm_bridge_get(bridge_edid);
+	bridge_connector->bridge_hpd        = drm_bridge_get(bridge_hpd);
+	bridge_connector->bridge_detect     = drm_bridge_get(bridge_detect);
+	bridge_connector->bridge_modes      = drm_bridge_get(bridge_modes);
+	bridge_connector->bridge_hdmi       = drm_bridge_get(bridge_hdmi);
+	bridge_connector->bridge_hdmi_audio = drm_bridge_get(bridge_hdmi_audio);
+	bridge_connector->bridge_dp_audio   = drm_bridge_get(bridge_dp_audio);
+	bridge_connector->bridge_hdmi_cec   = drm_bridge_get(bridge_hdmi_cec);
+
 	return connector;
 }
 EXPORT_SYMBOL_GPL(drm_bridge_connector_init);

-- 
2.51.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors
  2025-09-25 17:19 ` [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors Luca Ceresoli
@ 2025-09-25 22:07   ` Dmitry Baryshkov
  2025-09-26 14:58     ` Luca Ceresoli
  2025-09-26  7:33   ` Jani Nikula
  1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Baryshkov @ 2025-09-25 22:07 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Hui Pu,
	Thomas Petazzoni, dri-devel, linux-kernel

On Thu, Sep 25, 2025 at 07:19:49PM +0200, Luca Ceresoli wrote:
> Some code is going to need connector-specific cleanup actions (namely
> drm_bridge_connector will need to put refcounted bridges).
> 
> The .destroy callback is appropriate for this task but it is currently
> forbidden by drmm_connector_init(). Relax this limitation and document it.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> The other obvious approach would be adding a separate .cleanup callback for
> the cleanup-only actions. I tried both, they both apparently work, so any
> arguments and opinions on which approach is best within the overall DRM
> design would be very useful here.

Would it be better to use drmm-reset actions. I think the check here
makes much more help overall than harm in your case, so I'd suggest
leaving it in place.

> ---
>  drivers/gpu/drm/drm_connector.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 272d6254ea4784e97ca894ec4d463beebf9fdbf0..bd0220513a23afcb096b0c4c4d2b957b81f21ee1 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -513,7 +513,8 @@ static void drm_connector_cleanup_action(struct drm_device *dev,
>   *
>   * The connector structure should be allocated with drmm_kzalloc().
>   *
> - * The @drm_connector_funcs.destroy hook must be NULL.
> + * The @drm_connector_funcs.destroy hook must only do connector-specific
> + * cleanups if any is needed, not dealloacte the connector.
>   *
>   * Returns:
>   * Zero on success, error code on failure.
> @@ -526,9 +527,6 @@ int drmm_connector_init(struct drm_device *dev,
>  {
>  	int ret;
>  
> -	if (drm_WARN_ON(dev, funcs && funcs->destroy))
> -		return -EINVAL;
> -
>  	ret = drm_connector_init_and_add(dev, connector, funcs, connector_type, ddc);
>  	if (ret)
>  		return ret;
> 
> -- 
> 2.51.0
> 

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors
  2025-09-25 17:19 ` [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors Luca Ceresoli
  2025-09-25 22:07   ` Dmitry Baryshkov
@ 2025-09-26  7:33   ` Jani Nikula
  1 sibling, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2025-09-26  7:33 UTC (permalink / raw)
  To: Luca Ceresoli, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Andrzej Hajda,
	Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, Dmitry Baryshkov
  Cc: Hui Pu, Thomas Petazzoni, dri-devel, linux-kernel, Luca Ceresoli

On Thu, 25 Sep 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> Some code is going to need connector-specific cleanup actions (namely
> drm_bridge_connector will need to put refcounted bridges).
>
> The .destroy callback is appropriate for this task but it is currently
> forbidden by drmm_connector_init(). Relax this limitation and document it.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> ---
>
> The other obvious approach would be adding a separate .cleanup callback for
> the cleanup-only actions. I tried both, they both apparently work, so any
> arguments and opinions on which approach is best within the overall DRM
> design would be very useful here.
> ---
>  drivers/gpu/drm/drm_connector.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 272d6254ea4784e97ca894ec4d463beebf9fdbf0..bd0220513a23afcb096b0c4c4d2b957b81f21ee1 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -513,7 +513,8 @@ static void drm_connector_cleanup_action(struct drm_device *dev,
>   *
>   * The connector structure should be allocated with drmm_kzalloc().
>   *
> - * The @drm_connector_funcs.destroy hook must be NULL.
> + * The @drm_connector_funcs.destroy hook must only do connector-specific
> + * cleanups if any is needed, not dealloacte the connector.

It slightly feels like a trap to have different semantics for ->destroy
depending on how the connector was allocated.

BR,
Jani.


>   *
>   * Returns:
>   * Zero on success, error code on failure.
> @@ -526,9 +527,6 @@ int drmm_connector_init(struct drm_device *dev,
>  {
>  	int ret;
>  
> -	if (drm_WARN_ON(dev, funcs && funcs->destroy))
> -		return -EINVAL;
> -
>  	ret = drm_connector_init_and_add(dev, connector, funcs, connector_type, ddc);
>  	if (ret)
>  		return ret;

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors
  2025-09-25 22:07   ` Dmitry Baryshkov
@ 2025-09-26 14:58     ` Luca Ceresoli
  0 siblings, 0 replies; 6+ messages in thread
From: Luca Ceresoli @ 2025-09-26 14:58 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Hui Pu,
	Thomas Petazzoni, dri-devel, linux-kernel

Hi Dmitry,

On Fri, 26 Sep 2025 01:07:26 +0300
Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:

> On Thu, Sep 25, 2025 at 07:19:49PM +0200, Luca Ceresoli wrote:
> > Some code is going to need connector-specific cleanup actions (namely
> > drm_bridge_connector will need to put refcounted bridges).
> > 
> > The .destroy callback is appropriate for this task but it is currently
> > forbidden by drmm_connector_init(). Relax this limitation and document it.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > The other obvious approach would be adding a separate .cleanup callback for
> > the cleanup-only actions. I tried both, they both apparently work, so any
> > arguments and opinions on which approach is best within the overall DRM
> > design would be very useful here.  
> 
> Would it be better to use drmm-reset actions. I think the check here
> makes much more help overall than harm in your case, so I'd suggest
> leaving it in place.

Thanks for the feedback!

I think using drmm_add_action[_or_reset]() here makes sense indeed.

As I understand it, both .destroy and drmm_add_action[_or_reset]()
actions will trigger when the drm_device is removed. This is not ideal
for hotplugging because one would add/remove bridges while the
drm_device is persistent, so on multiple hot plug/unplug loops stale
resources would accumulate until the final card removal, perhaps at
system shutdown. However for now my goal is to have bridges refcount in
place to avoid use-after-free. Releasing resources for hot-unplugged
bridges for this case is a further step.

Bottom line: same drawback for both solutions, but the drmm action is
cleaner. v2 incoming with a drmm action.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-09-26 14:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-25 17:19 [PATCH 0/2] drm/bridge: bridge-connector: get/put the stored bridges Luca Ceresoli
2025-09-25 17:19 ` [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors Luca Ceresoli
2025-09-25 22:07   ` Dmitry Baryshkov
2025-09-26 14:58     ` Luca Ceresoli
2025-09-26  7:33   ` Jani Nikula
2025-09-25 17:19 ` [PATCH 2/2] drm/display: bridge-connector: get/put the stored bridges Luca Ceresoli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox