[PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[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, 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. Use a cleanup action as well to ensure it
is put on return.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
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).
---
Changes in v2:
- Use drmm_add_action() instead of hacking the .destroy connector func
- Removed patch 1 (where the hacking the .destroy connector func was)
- Link to v1: https://lore.kernel.org/r/20250925-drm-bridge-alloc-getput-bridge-connector-v1-0-f0736e1c73ee@bootlin.com
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 114 +++++++++++++++++--------
 1 file changed, 78 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..7b18be3ff9a32b362468351835bdab43c3f524f1 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -618,6 +618,20 @@ static const struct drm_connector_hdmi_cec_funcs drm_bridge_connector_hdmi_cec_f
  * Bridge Connector Initialisation
  */
 
+static void drm_bridge_connector_put_bridges(struct drm_device *dev, void *data)
+{
+	struct drm_bridge_connector *bridge_connector = (struct drm_bridge_connector *)data;
+
+	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);
+}
+
 /**
  * drm_bridge_connector_init - Initialise a connector for a chain of bridges
  * @drm: the DRM device
@@ -638,7 +652,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;
@@ -649,6 +671,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	if (!bridge_connector)
 		return ERR_PTR(-ENOMEM);
 
+	ret = drmm_add_action(drm, drm_bridge_connector_put_bridges, bridge_connector);
+	if (ret)
+		return ERR_PTR(ret);
+
 	bridge_connector->encoder = encoder;
 
 	/*
@@ -672,22 +698,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 +730,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 +744,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 +762,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 +773,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 +796,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 +805,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 +827,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 +848,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 +859,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 +874,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 +887,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);

---
base-commit: 063db451832b8849faf1b0b8404b3a6a39995b29
change-id: 20250925-drm-bridge-alloc-getput-bridge-connector-556f8dc14af4

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

Re: [PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[Dmitry Baryshkov]

On Fri, Sep 26, 2025 at 04:59:40PM +0200, Luca Ceresoli wrote:
> 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, 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. Use a cleanup action as well to ensure it
> is put on return.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> 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).
> ---
> Changes in v2:
> - Use drmm_add_action() instead of hacking the .destroy connector func
> - Removed patch 1 (where the hacking the .destroy connector func was)
> - Link to v1: https://lore.kernel.org/r/20250925-drm-bridge-alloc-getput-bridge-connector-v1-0-f0736e1c73ee@bootlin.com
> ---
>  drivers/gpu/drm/display/drm_bridge_connector.c | 114 +++++++++++++++++--------
>  1 file changed, 78 insertions(+), 36 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

On Fri, Sep 26, 2025 at 04:59:40PM +0200, Luca Ceresoli wrote:
> 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, 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. Use a cleanup action as well to ensure it
> is put on return.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> 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).
> ---
> Changes in v2:
> - Use drmm_add_action() instead of hacking the .destroy connector func
> - Removed patch 1 (where the hacking the .destroy connector func was)
> - Link to v1: https://lore.kernel.org/r/20250925-drm-bridge-alloc-getput-bridge-connector-v1-0-f0736e1c73ee@bootlin.com
> ---
>  drivers/gpu/drm/display/drm_bridge_connector.c | 114 +++++++++++++++++--------
>  1 file changed, 78 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..7b18be3ff9a32b362468351835bdab43c3f524f1 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -618,6 +618,20 @@ static const struct drm_connector_hdmi_cec_funcs drm_bridge_connector_hdmi_cec_f
>   * Bridge Connector Initialisation
>   */
>  
> +static void drm_bridge_connector_put_bridges(struct drm_device *dev, void *data)
> +{
> +	struct drm_bridge_connector *bridge_connector = (struct drm_bridge_connector *)data;
> +
> +	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);
> +}
> +

I'd rather have a drmm_bridge_get helper that register the action
itself, but that can be fixed later on.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[Luca Ceresoli]

On Fri, 26 Sep 2025 16:59:40 +0200, Luca Ceresoli wrote:
> 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).
> 
> [...]

Applied, thanks!

[1/1] drm/display: bridge_connector: get/put the stored bridges
      commit: 2be300f9a0b6f6b0ae2a90be97e558ec0535be54

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

Re: [PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[Marek Szyprowski]

Hi Luca,

On 26.09.2025 16:59, Luca Ceresoli wrote:
> 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, 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. Use a cleanup action as well to ensure it
> is put on return.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

This patch landed recently in linux-next as commit 2be300f9a0b6 
("drm/display: bridge_connector: get/put the stored bridges"). In my 
tests I found that it causes the following NULL pointer dereference on 
DragonBoard410c (arch/arm64/boot/dts/qcom/apq8016-sbc.dts):

--->8---

  msm_mdp 1a01000.display-controller: bound 1c00000.gpu (ops a3xx_ops [msm])
  msm_mdp 1a01000.display-controller: [drm:mdp5_kms_init [msm]] MDP5 
version v1.6
  msm_mdp 1a01000.display-controller: fall back to the other CTL 
category for INTF 1!
  Unable to handle kernel NULL pointer dereference at virtual address 
0000000000000110
  ...
  Internal error: Oops: 0000000096000004 [#1]  SMP
  Modules linked in: qcom_wcnss_pil(+) cpufreq_powersave 
cpufreq_conservative coresight_stm stm_core coresight_replicator 
coresight_cpu_debug coresight_funnel coresight_tmc coresight_cti 
coresight_tpiu adv7511 nfc coresight rfkill msm snd_soc_lpass_apq8016 
snd_soc_lpass_cpu snd_soc_apq8016_sbc snd_soc_msm8916_digital 
snd_soc_msm8916_analog snd_soc_lpass_platform snd_soc_qcom_common 
snd_soc_core qrtr snd_compress snd_pcm_dmaengine qcom_camss 
qcom_spmi_temp_alarm snd_pcm qcom_q6v5_mss qcom_pil_info ubwc_config 
qcom_q6v5 videobuf2_dma_sg llcc_qcom v4l2_fwnode rtc_pm8xxx ocmem 
venus_core v4l2_async qcom_sysmon qcom_common qcom_spmi_vadc drm_gpuvm 
qcom_vadc_common qcom_pon v4l2_mem2mem snd_timer drm_exec 
videobuf2_memops gpu_sched videobuf2_v4l2 snd qcom_glink_smem 
drm_dp_aux_bus videodev soundcore mdt_loader qmi_helpers 
drm_display_helper qnoc_msm8916 videobuf2_common qcom_stats mc qcom_rng 
rpmsg_ctrl rpmsg_char ramoops reed_solomon display_connector socinfo 
rmtfs_mem ax88796b asix usbnet phy_qcom_usb_hs ipv6
  CPU: 2 UID: 0 PID: 42 Comm: kworker/u16:1 Tainted: G W           
6.17.0-rc6+ #16051 PREEMPT
  Tainted: [W]=WARN
  Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
  Workqueue: events_unbound deferred_probe_work_func
  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : drm_bridge_connector_hdmi_cec_init+0x8/0x38 [drm_display_helper]
  lr : drmm_connector_hdmi_cec_register+0xf8/0x1c8 [drm_display_helper]
  sp : ffff8000843d3560
  x29: ffff8000843d3560 x28: 0000000000000000 x27: 0000000000000000
  x26: ffff000010c14400 x25: ffff00000c034b68 x24: ffff00003b82a020
  x23: ffff00000c030000 x22: ffff0000097bf7d0 x21: ffff0000042e6a60
  x20: ffff80007c162af8 x19: ffff00000c034080 x18: 00000000ffffffff
  x5 : 00000000000007ff x4 : ffff800083cc5c50 x3 : 0000000000000000
  x2 : 0000000000000000 x1 : ffff00000c034080 x0 : 0000000000000000
  Call trace:
   drm_bridge_connector_hdmi_cec_init+0x8/0x38 [drm_display_helper] (P)
   drm_bridge_connector_init+0x6b4/0x6d4 [drm_display_helper]
   msm_dsi_manager_connector_init+0x9c/0xf0 [msm]
   msm_dsi_modeset_init+0x60/0xe8 [msm]
   modeset_init+0x3c4/0x4c0 [msm]
   mdp5_kms_init+0x3cc/0x670 [msm]
   msm_drm_kms_init+0x40/0x33c [msm]
   msm_drm_init+0x1c4/0x284 [msm]
   msm_drm_bind+0x30/0x3c [msm]
   try_to_bring_up_aggregate_device+0x168/0x1d4
   __component_add+0xa8/0x170
   component_add+0x14/0x20
   dsi_dev_attach+0x20/0x2c [msm]
   dsi_host_attach+0x58/0x98 [msm]
   devm_mipi_dsi_attach+0x34/0x90
   adv7533_attach_dsi+0x8c/0x104 [adv7511]
   adv7511_probe+0x764/0x988 [adv7511]
   i2c_device_probe+0x154/0x350
   really_probe+0xbc/0x298
   __driver_probe_device+0x78/0x12c
   driver_probe_device+0xdc/0x164
   __device_attach_driver+0xb8/0x138
   bus_for_each_drv+0x80/0xdc
   __device_attach+0xa8/0x1b0
   device_initial_probe+0x14/0x20
   bus_probe_device+0xb0/0xb4
   deferred_probe_work_func+0x8c/0xc8
   process_one_work+0x208/0x60c
   worker_thread+0x244/0x388
   kthread+0x150/0x228
   ret_from_fork+0x10/0x20
  Code: d50323bf d65f03c0 aa0003e1 f945e400 (f9408802)
  ---[ end trace 0000000000000000 ]---

This can be easily fixed by adding the following check:

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c 
b/drivers/gpu/drm/display/drm_bridge_connector.c
index 7b18be3ff9a3..222ecbc98155 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -601,7 +601,7 @@ static int drm_bridge_connector_hdmi_cec_init(struct 
drm_connector *connector)

         bridge = bridge_connector->bridge_hdmi_cec;

-       if (!bridge->funcs->hdmi_cec_init)
+       if (!bridge || !bridge->funcs->hdmi_cec_init)
                 return 0;

         return bridge->funcs->hdmi_cec_init(bridge, connector);


However I don't know the internals of the related code to judge that 
such check is the proper way to fix this issue.


> ---
> 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).
> ---
> Changes in v2:
> - Use drmm_add_action() instead of hacking the .destroy connector func
> - Removed patch 1 (where the hacking the .destroy connector func was)
> - Link to v1: https://lore.kernel.org/r/20250925-drm-bridge-alloc-getput-bridge-connector-v1-0-f0736e1c73ee@bootlin.com
> ---
>   drivers/gpu/drm/display/drm_bridge_connector.c | 114 +++++++++++++++++--------
>   1 file changed, 78 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..7b18be3ff9a32b362468351835bdab43c3f524f1 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -618,6 +618,20 @@ static const struct drm_connector_hdmi_cec_funcs drm_bridge_connector_hdmi_cec_f
>    * Bridge Connector Initialisation
>    */
>   
> +static void drm_bridge_connector_put_bridges(struct drm_device *dev, void *data)
> +{
> +	struct drm_bridge_connector *bridge_connector = (struct drm_bridge_connector *)data;
> +
> +	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);
> +}
> +
>   /**
>    * drm_bridge_connector_init - Initialise a connector for a chain of bridges
>    * @drm: the DRM device
> @@ -638,7 +652,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;
> @@ -649,6 +671,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   	if (!bridge_connector)
>   		return ERR_PTR(-ENOMEM);
>   
> +	ret = drmm_add_action(drm, drm_bridge_connector_put_bridges, bridge_connector);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>   	bridge_connector->encoder = encoder;
>   
>   	/*
> @@ -672,22 +698,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 +730,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 +744,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 +762,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 +773,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 +796,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 +805,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 +827,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 +848,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 +859,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 +874,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 +887,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);
>
> ---
> base-commit: 063db451832b8849faf1b0b8404b3a6a39995b29
> change-id: 20250925-drm-bridge-alloc-getput-bridge-connector-556f8dc14af4
>
> Best regards,

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[Luca Ceresoli]

Hello Marek,

On Wed Oct 15, 2025 at 10:22 AM CEST, Marek Szyprowski wrote:
> Hi Luca,
>
> On 26.09.2025 16:59, Luca Ceresoli wrote:
>> 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, 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. Use a cleanup action as well to ensure it
>> is put on return.
>>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> This patch landed recently in linux-next as commit 2be300f9a0b6
> ("drm/display: bridge_connector: get/put the stored bridges"). In my
> tests I found that it causes the following NULL pointer dereference on
> DragonBoard410c (arch/arm64/boot/dts/qcom/apq8016-sbc.dts):
>
> --->8---
>
>   msm_mdp 1a01000.display-controller: bound 1c00000.gpu (ops a3xx_ops [msm])
>   msm_mdp 1a01000.display-controller: [drm:mdp5_kms_init [msm]] MDP5
> version v1.6
>   msm_mdp 1a01000.display-controller: fall back to the other CTL
> category for INTF 1!
>   Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000110
>   ...
>   Internal error: Oops: 0000000096000004 [#1]  SMP
>   Modules linked in: qcom_wcnss_pil(+) cpufreq_powersave
> cpufreq_conservative coresight_stm stm_core coresight_replicator
> coresight_cpu_debug coresight_funnel coresight_tmc coresight_cti
> coresight_tpiu adv7511 nfc coresight rfkill msm snd_soc_lpass_apq8016
> snd_soc_lpass_cpu snd_soc_apq8016_sbc snd_soc_msm8916_digital
> snd_soc_msm8916_analog snd_soc_lpass_platform snd_soc_qcom_common
> snd_soc_core qrtr snd_compress snd_pcm_dmaengine qcom_camss
> qcom_spmi_temp_alarm snd_pcm qcom_q6v5_mss qcom_pil_info ubwc_config
> qcom_q6v5 videobuf2_dma_sg llcc_qcom v4l2_fwnode rtc_pm8xxx ocmem
> venus_core v4l2_async qcom_sysmon qcom_common qcom_spmi_vadc drm_gpuvm
> qcom_vadc_common qcom_pon v4l2_mem2mem snd_timer drm_exec
> videobuf2_memops gpu_sched videobuf2_v4l2 snd qcom_glink_smem
> drm_dp_aux_bus videodev soundcore mdt_loader qmi_helpers
> drm_display_helper qnoc_msm8916 videobuf2_common qcom_stats mc qcom_rng
> rpmsg_ctrl rpmsg_char ramoops reed_solomon display_connector socinfo
> rmtfs_mem ax88796b asix usbnet phy_qcom_usb_hs ipv6
>   CPU: 2 UID: 0 PID: 42 Comm: kworker/u16:1 Tainted: G W
> 6.17.0-rc6+ #16051 PREEMPT
>   Tainted: [W]=WARN
>   Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
>   Workqueue: events_unbound deferred_probe_work_func
>   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : drm_bridge_connector_hdmi_cec_init+0x8/0x38 [drm_display_helper]
>   lr : drmm_connector_hdmi_cec_register+0xf8/0x1c8 [drm_display_helper]
>   sp : ffff8000843d3560
>   x29: ffff8000843d3560 x28: 0000000000000000 x27: 0000000000000000
>   x26: ffff000010c14400 x25: ffff00000c034b68 x24: ffff00003b82a020
>   x23: ffff00000c030000 x22: ffff0000097bf7d0 x21: ffff0000042e6a60
>   x20: ffff80007c162af8 x19: ffff00000c034080 x18: 00000000ffffffff
>   x5 : 00000000000007ff x4 : ffff800083cc5c50 x3 : 0000000000000000
>   x2 : 0000000000000000 x1 : ffff00000c034080 x0 : 0000000000000000
>   Call trace:
>    drm_bridge_connector_hdmi_cec_init+0x8/0x38 [drm_display_helper] (P)
>    drm_bridge_connector_init+0x6b4/0x6d4 [drm_display_helper]
>    msm_dsi_manager_connector_init+0x9c/0xf0 [msm]
>    msm_dsi_modeset_init+0x60/0xe8 [msm]
>    modeset_init+0x3c4/0x4c0 [msm]
>    mdp5_kms_init+0x3cc/0x670 [msm]
>    msm_drm_kms_init+0x40/0x33c [msm]
>    msm_drm_init+0x1c4/0x284 [msm]
>    msm_drm_bind+0x30/0x3c [msm]
>    try_to_bring_up_aggregate_device+0x168/0x1d4
>    __component_add+0xa8/0x170
>    component_add+0x14/0x20
>    dsi_dev_attach+0x20/0x2c [msm]
>    dsi_host_attach+0x58/0x98 [msm]
>    devm_mipi_dsi_attach+0x34/0x90
>    adv7533_attach_dsi+0x8c/0x104 [adv7511]
>    adv7511_probe+0x764/0x988 [adv7511]
>    i2c_device_probe+0x154/0x350
>    really_probe+0xbc/0x298
>    __driver_probe_device+0x78/0x12c
>    driver_probe_device+0xdc/0x164
>    __device_attach_driver+0xb8/0x138
>    bus_for_each_drv+0x80/0xdc
>    __device_attach+0xa8/0x1b0
>    device_initial_probe+0x14/0x20
>    bus_probe_device+0xb0/0xb4
>    deferred_probe_work_func+0x8c/0xc8
>    process_one_work+0x208/0x60c
>    worker_thread+0x244/0x388
>    kthread+0x150/0x228
>    ret_from_fork+0x10/0x20
>   Code: d50323bf d65f03c0 aa0003e1 f945e400 (f9408802)
>   ---[ end trace 0000000000000000 ]---

Thanks for testing and reporting.

I'm afraid I have no hardware where the same bug can be reproduced, but by
code inspection the root cause is clear given the call chain:

  drm_bridge_connector_init() [1]
   -> drmm_connector_hdmi_cec_register() [2]
       -> funcs->init() = drm_bridge_connector_hdmi_cec_init() [3]

[1] used to set bridge_connector->bridge_hdmi_cec before calling [2], now
it does it afterwards. But [3] expects it to be set already.

I have overlooked this when writing the patch. My apologies.

> This can be easily fixed by adding the following check:
>
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c
> b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 7b18be3ff9a3..222ecbc98155 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -601,7 +601,7 @@ static int drm_bridge_connector_hdmi_cec_init(struct
> drm_connector *connector)
>
>          bridge = bridge_connector->bridge_hdmi_cec;
>
> -       if (!bridge->funcs->hdmi_cec_init)
> +       if (!bridge || !bridge->funcs->hdmi_cec_init)
>                  return 0;
>
>          return bridge->funcs->hdmi_cec_init(bridge, connector);
>
>
> However I don't know the internals of the related code to judge that
> such check is the proper way to fix this issue.

I'm afraid this is not a correct solution. When this function is called,
the bridge_hdmi_cec is already known, but (after my patch) not yet set
where it is expected to be. So skipping the bridge->funcs->hdmi_cec_init()
call would make the code behave differently for no good reason.

I'm looking at how to properly fix this bug ASAP.

Best regards,
Luca

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

Re: [PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[Luca Ceresoli]

Hello,

On Wed Oct 15, 2025 at 10:08 PM CEST, Luca Ceresoli wrote:
> Hello Marek,
>
> On Wed Oct 15, 2025 at 10:22 AM CEST, Marek Szyprowski wrote:
>> Hi Luca,
>>
>> On 26.09.2025 16:59, Luca Ceresoli wrote:
>>> 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, 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. Use a cleanup action as well to ensure it
>>> is put on return.
>>>
>>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>>
>> This patch landed recently in linux-next as commit 2be300f9a0b6
>> ("drm/display: bridge_connector: get/put the stored bridges"). In my
>> tests I found that it causes the following NULL pointer dereference on
>> DragonBoard410c (arch/arm64/boot/dts/qcom/apq8016-sbc.dts):

...

> Thanks for testing and reporting.
>
> I'm afraid I have no hardware where the same bug can be reproduced, but by
> code inspection the root cause is clear given the call chain:
>
>   drm_bridge_connector_init() [1]
>    -> drmm_connector_hdmi_cec_register() [2]
>        -> funcs->init() = drm_bridge_connector_hdmi_cec_init() [3]
>
> [1] used to set bridge_connector->bridge_hdmi_cec before calling [2], now
> it does it afterwards. But [3] expects it to be set already.
>
> I have overlooked this when writing the patch. My apologies.

...

> I'm looking at how to properly fix this bug ASAP.

Here it is:
https://lore.kernel.org/lkml/20251017-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v2-0-667abf6d47c0@bootlin.com/

Luca

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

Re: [PATCH v2] drm/display: bridge_connector: get/put the stored bridges

[Geert Uytterhoeven]

Hi Luca,

On Sun, 28 Sept 2025 at 16:25, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 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, 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. Use a cleanup action as well to ensure it
> is put on return.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Thanks for your patch, which is now commit 2be300f9a0b6f6b0
("drm/display: bridge_connector: get/put the stored bridges")
in drm-misc/drm-misc-next.

FTR, this causes the following crash on Koelsch (R-Car M2-W):

    Unable to handle kernel NULL pointer dereference at virtual
address 00000050 when read
    [00000050] *pgd=00000000
    Internal error: Oops: 5 [#1] SMP ARM
    CPU: 1 UID: 0 PID: 12 Comm: kworker/u8:0 Not tainted
6.17.0-rc6-shmobile-01124-g2be300f9a0b6 #2283 NONE
    Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
    Workqueue: events_unbound deferred_probe_work_func
    PC is at drm_bridge_connector_hdmi_cec_init+0x8/0x24
    LR is at drmm_connector_hdmi_cec_register+0x104/0x1a8
    pc : [<c0507240>]    lr : [<c051460c>]    psr: 60000013
    sp : f0849c50  ip : 00000000  fp : 00000008
    r10: c1f865c8  r9 : c1f84820  r8 : c1d32ecc
    r7 : c1e4a980  r6 : c1f94000  r5 : c1d32840  r4 : c0a97930
    r3 : c0507238  r2 : c151e040  r1 : c1d32840  r0 : 00000000
    Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
    Control: 10c5387d  Table: 4000406a  DAC: 00000051
    Register r0 information: NULL pointer
    Register r1 information: slab kmalloc-2k start c1d32800 pointer
offset 64 size 2048
    Register r2 information: slab task_struct start c151e040 pointer
offset 0 size 2240
    Register r3 information: non-slab/vmalloc memory
    Register r4 information: non-slab/vmalloc memory
    Register r5 information: slab kmalloc-2k start c1d32800 pointer
offset 64 size 2048
    Register r6 information: slab kmalloc-1k start c1f94000 pointer
offset 0 size 1024
    Register r7 information: slab kmalloc-64 start c1e4a980 pointer
offset 0 size 64
    Register r8 information: slab kmalloc-2k start c1d32800 pointer
offset 1740 size 2048
    Register r9 information: slab kmalloc-1k start c1f84800 pointer
offset 32 size 1024
    Register r10 information: slab kmalloc-1k start c1f86400 pointer
offset 456 size 1024
    Register r11 information: non-paged memory
    Register r12 information: NULL pointer
    Process kworker/u8:0 (pid: 12, stack limit = 0x(ptrval))
    Stack: (0xf0849c50 to 0xf084a000)
    9c40:                                     00000003 00000011
00000001 00000000
    9c60: 00000049 00000000 00000000 00000000 00000000 00000000
00000000 00000000
    9c80: 00000000 00000000 00000000 00000000 00000000 00000000
00000000 a8bfa8e4
    9ca0: c1d32840 c1d32840 c1f865c8 c1f865c8 00000000 c1d32840
00000000 c0507ba0
    9cc0: c1f84820 00000011 00000001 00000002 00000001 00000008
c04fba80 c204800c
    9ce0: 00000dc0 00000000 c1f865c8 c1f865c8 c1f865c8 00000000
c1f865c8 c1f865c8
    9d00: 00000000 c1f865c8 00000000 a8bfa8e4 c1e42a00 00000000
00000000 c2048000
    9d20: c1f865c8 c1e42a00 c204800c c0c66958 ef7f7b44 c0517ab4
00000000 00000000
    9d40: c2048000 00000000 00000000 c204b000 ef7f0794 00000000
00000000 c0518ab8
    9d60: 00000000 00000000 01ffffff 00000000 c204800c ef7f0214
f0f40000 c204f000
    9d80: 00000000 00000000 ef7f0794 c2081f80 c0c7a60d c052d728
c1588c10 a0000013
    9da0: c2081fc0 c052d780 f0f40000 c2081fc0 00040000 c1588c10
f0f40000 c03f57f4
    9dc0: c0c4fe58 feb00000 c1588c10 c16683c0 00000000 a8bfa8e4
00000000 00000000
    9de0: c2048000 c204800c c1588c00 00000000 c1588c10 00000000
c0fe8e30 c0517818
    9e00: c1588c10 c0fe8670 c0fe8670 00000000 00000005 c140ed0d
61c88647 c052c148
    9e20: 00000000 c1588c10 c0fe8670 c052a118 c1588c10 c0fe8670
f0849ecc c1588c10
    9e40: 00000005 c052a3e8 c0fe8670 c1588c10 c10773d0 c10773d8
f0849ecc c1588c10
    9e60: 00000005 61c88647 c0fe8e30 c052a490 00000001 c0fe8670
f0849ecc c1588c10
    9e80: c102fc00 c052a568 00000000 c14ac400 f0849ecc c052a510
c102fc00 c05287fc
    9ea0: c140ed0d c14ac46c c1594db8 a8bfa8e4 c1588c10 c1588c10
c14ac400 00000001
    9ec0: c1588c54 c0529f80 c1588c10 c1588c10 00000001 a8bfa8e4
c14ac400 c1588c10
    9ee0: c14ac400 c1588c10 00000000 c05290f4 c1588c10 c0fe8e10
c0fe8e68 00000000
    9f00: c102fc00 c0529b3c c1496180 c140ed00 c1406600 c0fe8e2c
c102fc00 c01414ac
    9f20: 00000002 a8bfa8e4 c151e040 c151e040 c1406620 c1406600
c140665c c1496180
    9f40: c1406620 c1406600 c140665c c151e040 c14961ac c1030120
c0f03d00 c014173c
    9f60: 00000000 c151e040 c1496400 c1494140 00000001 00000000
c01415cc c1496180
    9f80: 00000000 c01498cc c1494140 a8bfa8e4 c1494140 c014976c
00000000 00000000
    9fa0: 00000000 00000000 00000000 c010014c 00000000 00000000
00000000 00000000
    9fc0: 00000000 00000000 00000000 00000000 00000000 00000000
00000000 00000000
    9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
00000000 00000000
    Call trace:
     drm_bridge_connector_hdmi_cec_init from
drmm_connector_hdmi_cec_register+0x104/0x1a8
     drmm_connector_hdmi_cec_register from drm_bridge_connector_init+0x4d8/0x5e8
     drm_bridge_connector_init from rcar_du_encoder_init+0x1e4/0x240
     rcar_du_encoder_init from rcar_du_modeset_init+0x4f0/0x640
     rcar_du_modeset_init from rcar_du_probe+0xe0/0x164
     rcar_du_probe from platform_probe+0x58/0x90
     platform_probe from really_probe+0x128/0x28c
     really_probe from __driver_probe_device+0x16c/0x18c
     __driver_probe_device from driver_probe_device+0x3c/0xbc
     driver_probe_device from __device_attach_driver+0x58/0xbc
     __device_attach_driver from bus_for_each_drv+0xc0/0xd4
     bus_for_each_drv from __device_attach+0xec/0x154
     __device_attach from bus_probe_device+0x2c/0x84
     bus_probe_device from deferred_probe_work_func+0x80/0x98
     deferred_probe_work_func from process_scheduled_works+0x1bc/0x2dc
     process_scheduled_works from worker_thread+0x170/0x208
     worker_thread from kthread+0x160/0x1fc
     kthread from ret_from_fork+0x14/0x28
    Exception stack(0xf0849fb0 to 0xf0849ff8)
    9fa0:                                     00000000 00000000
00000000 00000000
    9fc0: 00000000 00000000 00000000 00000000 00000000 00000000
00000000 00000000
    9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
    Code: e49de004 e12fff1c e1a01000 e59006c8 (e5903050)
    ---[ end trace 0000000000000000 ]---

Applying "[PATCH v2 0/3] drm/display: bridge_connector: get/put the
stored bridges: fix NULL pointer regression"[1] fixes the issue.

[1] https://lore.kernel.org/20251017-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v2-0-667abf6d47c0@bootlin.com/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds