[PATCH v2 0/3] drm/display: bridge_connector: get/put the stored bridges: fix NULL pointer regression

[PATCH v2 0/3] drm/display: bridge_connector: get/put the stored bridges: fix NULL pointer regression

[Luca Ceresoli]

A patch of mine recently merged in drm-misc-next [1] has a NULL pointer
deref regression (reported here [2] and here [3]). Being in lack of a quick
fix, I sent a revert proposal [4].

The revert proposal has no answers currenty, and in the meanwhile I have a
patch that implements the original idea but without the same bug. So here's
a v2 series with:

 - the same revert patch
 - the original patch but rewritten without the same bug (and even simpler)

Also the re-written patch is now split in two for clarity because it was
doing two somewhat different things.

[1] https://lore.kernel.org/all/20250926-drm-bridge-alloc-getput-bridge-connector-v2-1-138b4bb70576@bootlin.com/
[2] https://lore.kernel.org/lkml/336fbfdd-c424-490e-b5d1-8ee84043dc80@samsung.com/
[3] https://lore.kernel.org/lkml/CA+G9fYuKHp3QgPKjgFY3TfkDdh5Vf=Ae5pCW+eU41Bu=D7th2g@mail.gmail.com/
[4] https://lore.kernel.org/lkml/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com/

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Changes in v2:
- No changes to the revert patch
- Added the (corrected) patch introducing the same feature as the original
  buggy patch, and also split it in two fir clarity
- Link to v1: https://lore.kernel.org/r/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com

---
Luca Ceresoli (3):
      Revert "drm/display: bridge_connector: get/put the stored bridges"
      drm/display: bridge_connector: get/put the stored bridges
      drm/display: bridge_connector: get/put the panel_bridge

 drivers/gpu/drm/display/drm_bridge_connector.c | 92 +++++++++++---------------
 1 file changed, 39 insertions(+), 53 deletions(-)
---
base-commit: 84a0a3f014cda68ff10b8517d09e9f0c1cd942a2
change-id: 20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-9a429ddb48e2

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

[PATCH v2 1/3] Revert "drm/display: bridge_connector: get/put the stored bridges"

[Luca Ceresoli]

This reverts commit 2be300f9a0b6f6b0ae2a90be97e558ec0535be54.

The commit being reverted moved all the bridge_connector->bridge_*
assignments to just before the final successful return in order to handle
the bridge refcounting in a clean way.

This introduced a bug, because a bit before the successful return
drmm_connector_hdmi_cec_register() is called, which calls funcs->init()
which is drm_bridge_connector_hdmi_cec_init() which needs
bridge_connector->bridge_hdmi_cec to be set.

The reported bug may be fixed in a relatively simple way, but other similar
patterns are potentially present, so just revert the offending commit. A
different approach will be implemented.

Fixes: 2be300f9a0b6 ("drm/display: bridge_connector: get/put the stored bridges")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Closes: https://lore.kernel.org/all/336fbfdd-c424-490e-b5d1-8ee84043dc80@samsung.com/
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Closes: https://lore.kernel.org/r/CA+G9fYuKHp3QgPKjgFY3TfkDdh5Vf=Ae5pCW+eU41Bu=D7th2g@mail.gmail.com
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changes in v2: none
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 114 ++++++++-----------------
 1 file changed, 36 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 7b18be3ff9a32b362468351835bdab43c3f524f1..a5bdd6c1064399ece6b19560f145b877c9e0680e 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -618,20 +618,6 @@ 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
@@ -652,15 +638,7 @@ 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      __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;
+	struct drm_bridge *panel_bridge = NULL;
 	unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
 	unsigned int max_bpc = 8;
 	bool support_hdcp = false;
@@ -671,10 +649,6 @@ 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;
 
 	/*
@@ -698,30 +672,22 @@ 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) {
-			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_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_HDMI) {
-			if (bridge_hdmi)
+			if (bridge_connector->bridge_hdmi)
 				return ERR_PTR(-EBUSY);
 			if (!bridge->funcs->hdmi_write_infoframe ||
 			    !bridge->funcs->hdmi_clear_infoframe)
 				return ERR_PTR(-EINVAL);
 
-			bridge_hdmi = drm_bridge_get(bridge);
+			bridge_connector->bridge_hdmi = bridge;
 
 			if (bridge->supported_formats)
 				supported_formats = bridge->supported_formats;
@@ -730,10 +696,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI_AUDIO) {
-			if (bridge_hdmi_audio)
+			if (bridge_connector->bridge_hdmi_audio)
 				return ERR_PTR(-EBUSY);
 
-			if (bridge_dp_audio)
+			if (bridge_connector->bridge_dp_audio)
 				return ERR_PTR(-EBUSY);
 
 			if (!bridge->hdmi_audio_max_i2s_playback_channels &&
@@ -744,14 +710,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			    !bridge->funcs->hdmi_audio_shutdown)
 				return ERR_PTR(-EINVAL);
 
-			bridge_hdmi_audio = drm_bridge_get(bridge);
+			bridge_connector->bridge_hdmi_audio = bridge;
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_DP_AUDIO) {
-			if (bridge_dp_audio)
+			if (bridge_connector->bridge_dp_audio)
 				return ERR_PTR(-EBUSY);
 
-			if (bridge_hdmi_audio)
+			if (bridge_connector->bridge_hdmi_audio)
 				return ERR_PTR(-EBUSY);
 
 			if (!bridge->hdmi_audio_max_i2s_playback_channels &&
@@ -762,7 +728,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			    !bridge->funcs->dp_audio_shutdown)
 				return ERR_PTR(-EINVAL);
 
-			bridge_dp_audio = drm_bridge_get(bridge);
+			bridge_connector->bridge_dp_audio = bridge;
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
@@ -773,10 +739,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
-			if (bridge_hdmi_cec)
+			if (bridge_connector->bridge_hdmi_cec)
 				return ERR_PTR(-EBUSY);
 
-			bridge_hdmi_cec = drm_bridge_get(bridge);
+			bridge_connector->bridge_hdmi_cec = bridge;
 
 			if (!bridge->funcs->hdmi_cec_enable ||
 			    !bridge->funcs->hdmi_cec_log_addr ||
@@ -796,7 +762,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			ddc = bridge->ddc;
 
 		if (drm_bridge_is_panel(bridge))
-			panel_bridge = drm_bridge_get(bridge);
+			panel_bridge = bridge;
 
 		if (bridge->support_hdcp)
 			support_hdcp = true;
@@ -805,13 +771,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_hdmi) {
+	if (bridge_connector->bridge_hdmi) {
 		if (!connector->ycbcr_420_allowed)
 			supported_formats &= ~BIT(HDMI_COLORSPACE_YUV420);
 
 		ret = drmm_connector_hdmi_init(drm, connector,
-					       bridge_hdmi->vendor,
-					       bridge_hdmi->product,
+					       bridge_connector->bridge_hdmi->vendor,
+					       bridge_connector->bridge_hdmi->product,
 					       &drm_bridge_connector_funcs,
 					       &drm_bridge_connector_hdmi_funcs,
 					       connector_type, ddc,
@@ -827,14 +793,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			return ERR_PTR(ret);
 	}
 
-	if (bridge_hdmi_audio || bridge_dp_audio) {
+	if (bridge_connector->bridge_hdmi_audio ||
+	    bridge_connector->bridge_dp_audio) {
 		struct device *dev;
 		struct drm_bridge *bridge;
 
-		if (bridge_hdmi_audio)
-			bridge = bridge_hdmi_audio;
+		if (bridge_connector->bridge_hdmi_audio)
+			bridge = bridge_connector->bridge_hdmi_audio;
 		else
-			bridge = bridge_dp_audio;
+			bridge = bridge_connector->bridge_dp_audio;
 
 		dev = bridge->hdmi_audio_dev;
 
@@ -848,9 +815,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			return ERR_PTR(ret);
 	}
 
-	if (bridge_hdmi_cec &&
-	    bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
-		struct drm_bridge *bridge = bridge_hdmi_cec;
+	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;
 
 		ret = drmm_connector_hdmi_cec_notifier_register(connector,
 								NULL,
@@ -859,9 +826,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			return ERR_PTR(ret);
 	}
 
-	if (bridge_hdmi_cec &&
-	    bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
-		struct drm_bridge *bridge = bridge_hdmi_cec;
+	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;
 
 		ret = drmm_connector_hdmi_cec_register(connector,
 						       &drm_bridge_connector_hdmi_cec_funcs,
@@ -874,9 +841,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 
 	drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
 
-	if (bridge_hpd)
+	if (bridge_connector->bridge_hpd)
 		connector->polled = DRM_CONNECTOR_POLL_HPD;
-	else if (bridge_detect)
+	else if (bridge_connector->bridge_detect)
 		connector->polled = DRM_CONNECTOR_POLL_CONNECT
 				  | DRM_CONNECTOR_POLL_DISCONNECT;
 
@@ -887,15 +854,6 @@ 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

[PATCH v2 2/3] 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.

Achieve this by adding a drmm cleanup callback whic puts all the non-NULL
bridges. Using drmm ensures the cleanup happens on drm_device teardown,
whichever is the return value of this function.

Four of these pointers (edid, hpd, detect and modes) can be written
multiple times (up to once per loop iterations), in order to eventually
store the last matching bridge. So 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.

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

---

This patch was added in v2:
- it is a rewrite of the original (buggy) patch, but simpler after
  realizing thanks to drmm we don't need the temporary pointers as the
  stored pointers will be put just before deallocating
- compared to the original patch, moved the panel_bridge changes to a
  separate patch for clarity
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 52 +++++++++++++++++++-------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index a5bdd6c1064399ece6b19560f145b877c9e0680e..95ccf86527129edaa6fcc75c6202985e73c46da8 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
@@ -649,6 +663,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,14 +690,22 @@ 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_connector->bridge_edid);
+			bridge_connector->bridge_edid = drm_bridge_get(bridge);
+		}
+		if (bridge->ops & DRM_BRIDGE_OP_HPD) {
+			drm_bridge_put(bridge_connector->bridge_hpd);
+			bridge_connector->bridge_hpd = drm_bridge_get(bridge);
+		}
+		if (bridge->ops & DRM_BRIDGE_OP_DETECT) {
+			drm_bridge_put(bridge_connector->bridge_detect);
+			bridge_connector->bridge_detect = drm_bridge_get(bridge);
+		}
+		if (bridge->ops & DRM_BRIDGE_OP_MODES) {
+			drm_bridge_put(bridge_connector->bridge_modes);
+			bridge_connector->bridge_modes = drm_bridge_get(bridge);
+		}
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
 			if (bridge_connector->bridge_hdmi)
 				return ERR_PTR(-EBUSY);
@@ -687,7 +713,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			    !bridge->funcs->hdmi_clear_infoframe)
 				return ERR_PTR(-EINVAL);
 
-			bridge_connector->bridge_hdmi = bridge;
+			bridge_connector->bridge_hdmi = drm_bridge_get(bridge);
 
 			if (bridge->supported_formats)
 				supported_formats = bridge->supported_formats;
@@ -710,7 +736,7 @@ 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_connector->bridge_hdmi_audio = drm_bridge_get(bridge);
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_DP_AUDIO) {
@@ -728,21 +754,21 @@ 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_connector->bridge_dp_audio = drm_bridge_get(bridge);
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
 			if (bridge_connector->bridge_hdmi_cec)
 				return ERR_PTR(-EBUSY);
 
-			bridge_connector->bridge_hdmi_cec = bridge;
+			bridge_connector->bridge_hdmi_cec = drm_bridge_get(bridge);
 		}
 
 		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
 			if (bridge_connector->bridge_hdmi_cec)
 				return ERR_PTR(-EBUSY);
 
-			bridge_connector->bridge_hdmi_cec = bridge;
+			bridge_connector->bridge_hdmi_cec = drm_bridge_get(bridge);
 
 			if (!bridge->funcs->hdmi_cec_enable ||
 			    !bridge->funcs->hdmi_cec_log_addr ||

-- 
2.51.0

[PATCH v2 3/3] drm/display: bridge_connector: get/put the panel_bridge

[Luca Ceresoli]

The panel_bridge pointer is taken inside the loop and used after the
loop. Being a local variable, use a cleanup action to ensure it is put on
return.

Based on the code structure the panel_bridge pointer might be assigned
during multiple loop iterations. Even though this is probably not possible
in the practice, ensure there is no reference leak by putting the reference
to the old value before overwriting with the new value.

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

---

This patch was added in v2:
- taking the panel_bridge specific code from the original (buggy) patch,
  and split it for clarity from the larger patch covering stored bridge
  pointers
- comapred to the original code, added drm_bridge_put() for extra safety
  even though likely not necessary
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 95ccf86527129edaa6fcc75c6202985e73c46da8..a2d30cf9e06df44b89456b5aba8198ee1e5d5601 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -652,7 +652,7 @@ 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;
 	unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
 	unsigned int max_bpc = 8;
 	bool support_hdcp = false;
@@ -787,8 +787,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		if (bridge->ddc)
 			ddc = bridge->ddc;
 
-		if (drm_bridge_is_panel(bridge))
-			panel_bridge = bridge;
+		if (drm_bridge_is_panel(bridge)) {
+			drm_bridge_put(panel_bridge);
+			panel_bridge = drm_bridge_get(bridge);
+		}
 
 		if (bridge->support_hdcp)
 			support_hdcp = true;

-- 
2.51.0

Re: [PATCH v2 0/3] drm/display: bridge_connector: get/put the stored bridges: fix NULL pointer regression

[Marek Szyprowski]

On 17.10.2025 18:15, Luca Ceresoli wrote:
> A patch of mine recently merged in drm-misc-next [1] has a NULL pointer
> deref regression (reported here [2] and here [3]). Being in lack of a quick
> fix, I sent a revert proposal [4].
>
> The revert proposal has no answers currenty, and in the meanwhile I have a
> patch that implements the original idea but without the same bug. So here's
> a v2 series with:
>
>   - the same revert patch
>   - the original patch but rewritten without the same bug (and even simpler)
>
> Also the re-written patch is now split in two for clarity because it was
> doing two somewhat different things.
>
> [1] https://lore.kernel.org/all/20250926-drm-bridge-alloc-getput-bridge-connector-v2-1-138b4bb70576@bootlin.com/
> [2] https://lore.kernel.org/lkml/336fbfdd-c424-490e-b5d1-8ee84043dc80@samsung.com/
> [3] https://lore.kernel.org/lkml/CA+G9fYuKHp3QgPKjgFY3TfkDdh5Vf=Ae5pCW+eU41Bu=D7th2g@mail.gmail.com/
> [4] https://lore.kernel.org/lkml/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com/
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Works fine on all boards in my test lab.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> Changes in v2:
> - No changes to the revert patch
> - Added the (corrected) patch introducing the same feature as the original
>    buggy patch, and also split it in two fir clarity
> - Link to v1: https://lore.kernel.org/r/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com
>
> ---
> Luca Ceresoli (3):
>        Revert "drm/display: bridge_connector: get/put the stored bridges"
>        drm/display: bridge_connector: get/put the stored bridges
>        drm/display: bridge_connector: get/put the panel_bridge
>
>   drivers/gpu/drm/display/drm_bridge_connector.c | 92 +++++++++++---------------
>   1 file changed, 39 insertions(+), 53 deletions(-)
> ---
> base-commit: 84a0a3f014cda68ff10b8517d09e9f0c1cd942a2
> change-id: 20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-9a429ddb48e2
>
> Best regards,

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

Re: [PATCH v2 0/3] drm/display: bridge_connector: get/put the stored bridges: fix NULL pointer regression

[Tommaso Merciai]

Hi Luca,

Thank you for your patch!

On Fri, Oct 17, 2025 at 06:15:03PM +0200, Luca Ceresoli wrote:
> A patch of mine recently merged in drm-misc-next [1] has a NULL pointer
> deref regression (reported here [2] and here [3]). Being in lack of a quick
> fix, I sent a revert proposal [4].
> 
> The revert proposal has no answers currenty, and in the meanwhile I have a
> patch that implements the original idea but without the same bug. So here's
> a v2 series with:
> 
>  - the same revert patch
>  - the original patch but rewritten without the same bug (and even simpler)
> 
> Also the re-written patch is now split in two for clarity because it was
> doing two somewhat different things.
> 
> [1] https://lore.kernel.org/all/20250926-drm-bridge-alloc-getput-bridge-connector-v2-1-138b4bb70576@bootlin.com/
> [2] https://lore.kernel.org/lkml/336fbfdd-c424-490e-b5d1-8ee84043dc80@samsung.com/
> [3] https://lore.kernel.org/lkml/CA+G9fYuKHp3QgPKjgFY3TfkDdh5Vf=Ae5pCW+eU41Bu=D7th2g@mail.gmail.com/
> [4] https://lore.kernel.org/lkml/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com/
> 

Tested on RZ/G3E board using DSI output.
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>

Thanks & Regards,
Tommaso

> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> Changes in v2:
> - No changes to the revert patch
> - Added the (corrected) patch introducing the same feature as the original
>   buggy patch, and also split it in two fir clarity
> - Link to v1: https://lore.kernel.org/r/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com
> 
> ---
> Luca Ceresoli (3):
>       Revert "drm/display: bridge_connector: get/put the stored bridges"
>       drm/display: bridge_connector: get/put the stored bridges
>       drm/display: bridge_connector: get/put the panel_bridge
> 
>  drivers/gpu/drm/display/drm_bridge_connector.c | 92 +++++++++++---------------
>  1 file changed, 39 insertions(+), 53 deletions(-)
> ---
> base-commit: 84a0a3f014cda68ff10b8517d09e9f0c1cd942a2
> change-id: 20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-9a429ddb48e2
> 
> Best regards,
> -- 
> Luca Ceresoli <luca.ceresoli@bootlin.com>
> 

Re: [PATCH v2 0/3] drm/display: bridge_connector: get/put the stored bridges: fix NULL pointer regression

[Nicolas Frattaroli]

On Friday, 17 October 2025 18:15:03 Central European Standard Time Luca Ceresoli wrote:
> A patch of mine recently merged in drm-misc-next [1] has a NULL pointer
> deref regression (reported here [2] and here [3]). Being in lack of a quick
> fix, I sent a revert proposal [4].
> 
> The revert proposal has no answers currenty, and in the meanwhile I have a
> patch that implements the original idea but without the same bug. So here's
> a v2 series with:
> 
>  - the same revert patch
>  - the original patch but rewritten without the same bug (and even simpler)
> 
> Also the re-written patch is now split in two for clarity because it was
> doing two somewhat different things.
> 
> [1] https://lore.kernel.org/all/20250926-drm-bridge-alloc-getput-bridge-connector-v2-1-138b4bb70576@bootlin.com/
> [2] https://lore.kernel.org/lkml/336fbfdd-c424-490e-b5d1-8ee84043dc80@samsung.com/
> [3] https://lore.kernel.org/lkml/CA+G9fYuKHp3QgPKjgFY3TfkDdh5Vf=Ae5pCW+eU41Bu=D7th2g@mail.gmail.com/
> [4] https://lore.kernel.org/lkml/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com/
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> Changes in v2:
> - No changes to the revert patch
> - Added the (corrected) patch introducing the same feature as the original
>   buggy patch, and also split it in two fir clarity
> - Link to v1: https://lore.kernel.org/r/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com
> 
> ---
> Luca Ceresoli (3):
>       Revert "drm/display: bridge_connector: get/put the stored bridges"
>       drm/display: bridge_connector: get/put the stored bridges
>       drm/display: bridge_connector: get/put the panel_bridge
> 
>  drivers/gpu/drm/display/drm_bridge_connector.c | 92 +++++++++++---------------
>  1 file changed, 39 insertions(+), 53 deletions(-)
> ---
> base-commit: 84a0a3f014cda68ff10b8517d09e9f0c1cd942a2
> change-id: 20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-9a429ddb48e2
> 
> Best regards,
> 

Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Fixes a null pointer dereference on boot on my Radxa ROCK 5T
(RK3588) that's present in next-20251027.

Kind regards,
Nicolas Frattaroli

Re: [PATCH v2 0/3] drm/display: bridge_connector: get/put the stored bridges: fix NULL pointer regression

[Geert Uytterhoeven]

Hi Luca,

On Fri, 17 Oct 2025 at 18:58, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> A patch of mine recently merged in drm-misc-next [1] has a NULL pointer
> deref regression (reported here [2] and here [3]). Being in lack of a quick
> fix, I sent a revert proposal [4].
>
> The revert proposal has no answers currenty, and in the meanwhile I have a
> patch that implements the original idea but without the same bug. So here's
> a v2 series with:
>
>  - the same revert patch
>  - the original patch but rewritten without the same bug (and even simpler)
>
> Also the re-written patch is now split in two for clarity because it was
> doing two somewhat different things.
>
> [1] https://lore.kernel.org/all/20250926-drm-bridge-alloc-getput-bridge-connector-v2-1-138b4bb70576@bootlin.com/
> [2] https://lore.kernel.org/lkml/336fbfdd-c424-490e-b5d1-8ee84043dc80@samsung.com/
> [3] https://lore.kernel.org/lkml/CA+G9fYuKHp3QgPKjgFY3TfkDdh5Vf=Ae5pCW+eU41Bu=D7th2g@mail.gmail.com/
> [4] https://lore.kernel.org/lkml/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com/
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> Changes in v2:
> - No changes to the revert patch
> - Added the (corrected) patch introducing the same feature as the original
>   buggy patch, and also split it in two fir clarity
> - Link to v1: https://lore.kernel.org/r/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com

Thanks, this fixes the crash I experienced on Koelsch (R-Car M2-W)
(https://lore.kernel.org/CAMuHMdW=zEi4XjG2Qrvj=jCa9LPBRU7HBTwEQVbe0zoz5mV_XA@mail.gmail.com)

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v2 1/3] Revert "drm/display: bridge_connector: get/put the stored bridges"

[Mark Brown]

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

On Fri, Oct 17, 2025 at 06:15:04PM +0200, Luca Ceresoli wrote:
> This reverts commit 2be300f9a0b6f6b0ae2a90be97e558ec0535be54.
> 
> The commit being reverted moved all the bridge_connector->bridge_*
> assignments to just before the final successful return in order to handle
> the bridge refcounting in a clean way.

Is there any news on getting this series merged - the currently broken
code in -next is causing boot issues on several affected platforms (eg,
Rock5B) which is disrupting other testing?  If the other patches are
somehow causing issues could we perhaps get the revert in to fix the
boot issue while those issues are resolved?

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

Re: [PATCH v2 1/3] Revert "drm/display: bridge_connector: get/put the stored bridges"

[Louis Chauvet]

Le 17/10/2025 à 18:15, Luca Ceresoli a écrit :
> This reverts commit 2be300f9a0b6f6b0ae2a90be97e558ec0535be54.
> 
> The commit being reverted moved all the bridge_connector->bridge_*
> assignments to just before the final successful return in order to handle
> the bridge refcounting in a clean way.
> 
> This introduced a bug, because a bit before the successful return
> drmm_connector_hdmi_cec_register() is called, which calls funcs->init()
> which is drm_bridge_connector_hdmi_cec_init() which needs
> bridge_connector->bridge_hdmi_cec to be set.
> 
> The reported bug may be fixed in a relatively simple way, but other similar
> patterns are potentially present, so just revert the offending commit. A
> different approach will be implemented.
> 
> Fixes: 2be300f9a0b6 ("drm/display: bridge_connector: get/put the stored bridges")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/all/336fbfdd-c424-490e-b5d1-8ee84043dc80@samsung.com/
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Closes: https://lore.kernel.org/r/CA+G9fYuKHp3QgPKjgFY3TfkDdh5Vf=Ae5pCW+eU41Bu=D7th2g@mail.gmail.com
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
> 
> Changes in v2: none
> ---
>   drivers/gpu/drm/display/drm_bridge_connector.c | 114 ++++++++-----------------
>   1 file changed, 36 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 7b18be3ff9a32b362468351835bdab43c3f524f1..a5bdd6c1064399ece6b19560f145b877c9e0680e 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -618,20 +618,6 @@ 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
> @@ -652,15 +638,7 @@ 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      __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;
> +	struct drm_bridge *panel_bridge = NULL;
>   	unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
>   	unsigned int max_bpc = 8;
>   	bool support_hdcp = false;
> @@ -671,10 +649,6 @@ 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;
>   
>   	/*
> @@ -698,30 +672,22 @@ 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) {
> -			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_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_HDMI) {
> -			if (bridge_hdmi)
> +			if (bridge_connector->bridge_hdmi)
>   				return ERR_PTR(-EBUSY);
>   			if (!bridge->funcs->hdmi_write_infoframe ||
>   			    !bridge->funcs->hdmi_clear_infoframe)
>   				return ERR_PTR(-EINVAL);
>   
> -			bridge_hdmi = drm_bridge_get(bridge);
> +			bridge_connector->bridge_hdmi = bridge;
>   
>   			if (bridge->supported_formats)
>   				supported_formats = bridge->supported_formats;
> @@ -730,10 +696,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   		}
>   
>   		if (bridge->ops & DRM_BRIDGE_OP_HDMI_AUDIO) {
> -			if (bridge_hdmi_audio)
> +			if (bridge_connector->bridge_hdmi_audio)
>   				return ERR_PTR(-EBUSY);
>   
> -			if (bridge_dp_audio)
> +			if (bridge_connector->bridge_dp_audio)
>   				return ERR_PTR(-EBUSY);
>   
>   			if (!bridge->hdmi_audio_max_i2s_playback_channels &&
> @@ -744,14 +710,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   			    !bridge->funcs->hdmi_audio_shutdown)
>   				return ERR_PTR(-EINVAL);
>   
> -			bridge_hdmi_audio = drm_bridge_get(bridge);
> +			bridge_connector->bridge_hdmi_audio = bridge;
>   		}
>   
>   		if (bridge->ops & DRM_BRIDGE_OP_DP_AUDIO) {
> -			if (bridge_dp_audio)
> +			if (bridge_connector->bridge_dp_audio)
>   				return ERR_PTR(-EBUSY);
>   
> -			if (bridge_hdmi_audio)
> +			if (bridge_connector->bridge_hdmi_audio)
>   				return ERR_PTR(-EBUSY);
>   
>   			if (!bridge->hdmi_audio_max_i2s_playback_channels &&
> @@ -762,7 +728,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   			    !bridge->funcs->dp_audio_shutdown)
>   				return ERR_PTR(-EINVAL);
>   
> -			bridge_dp_audio = drm_bridge_get(bridge);
> +			bridge_connector->bridge_dp_audio = bridge;
>   		}
>   
>   		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
> @@ -773,10 +739,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   		}
>   
>   		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
> -			if (bridge_hdmi_cec)
> +			if (bridge_connector->bridge_hdmi_cec)
>   				return ERR_PTR(-EBUSY);
>   
> -			bridge_hdmi_cec = drm_bridge_get(bridge);
> +			bridge_connector->bridge_hdmi_cec = bridge;
>   
>   			if (!bridge->funcs->hdmi_cec_enable ||
>   			    !bridge->funcs->hdmi_cec_log_addr ||
> @@ -796,7 +762,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   			ddc = bridge->ddc;
>   
>   		if (drm_bridge_is_panel(bridge))
> -			panel_bridge = drm_bridge_get(bridge);
> +			panel_bridge = bridge;
>   
>   		if (bridge->support_hdcp)
>   			support_hdcp = true;
> @@ -805,13 +771,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_hdmi) {
> +	if (bridge_connector->bridge_hdmi) {
>   		if (!connector->ycbcr_420_allowed)
>   			supported_formats &= ~BIT(HDMI_COLORSPACE_YUV420);
>   
>   		ret = drmm_connector_hdmi_init(drm, connector,
> -					       bridge_hdmi->vendor,
> -					       bridge_hdmi->product,
> +					       bridge_connector->bridge_hdmi->vendor,
> +					       bridge_connector->bridge_hdmi->product,
>   					       &drm_bridge_connector_funcs,
>   					       &drm_bridge_connector_hdmi_funcs,
>   					       connector_type, ddc,
> @@ -827,14 +793,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   			return ERR_PTR(ret);
>   	}
>   
> -	if (bridge_hdmi_audio || bridge_dp_audio) {
> +	if (bridge_connector->bridge_hdmi_audio ||
> +	    bridge_connector->bridge_dp_audio) {
>   		struct device *dev;
>   		struct drm_bridge *bridge;
>   
> -		if (bridge_hdmi_audio)
> -			bridge = bridge_hdmi_audio;
> +		if (bridge_connector->bridge_hdmi_audio)
> +			bridge = bridge_connector->bridge_hdmi_audio;
>   		else
> -			bridge = bridge_dp_audio;
> +			bridge = bridge_connector->bridge_dp_audio;
>   
>   		dev = bridge->hdmi_audio_dev;
>   
> @@ -848,9 +815,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   			return ERR_PTR(ret);
>   	}
>   
> -	if (bridge_hdmi_cec &&
> -	    bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
> -		struct drm_bridge *bridge = bridge_hdmi_cec;
> +	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;
>   
>   		ret = drmm_connector_hdmi_cec_notifier_register(connector,
>   								NULL,
> @@ -859,9 +826,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   			return ERR_PTR(ret);
>   	}
>   
> -	if (bridge_hdmi_cec &&
> -	    bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
> -		struct drm_bridge *bridge = bridge_hdmi_cec;
> +	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;
>   
>   		ret = drmm_connector_hdmi_cec_register(connector,
>   						       &drm_bridge_connector_hdmi_cec_funcs,
> @@ -874,9 +841,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   
>   	drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
>   
> -	if (bridge_hpd)
> +	if (bridge_connector->bridge_hpd)
>   		connector->polled = DRM_CONNECTOR_POLL_HPD;
> -	else if (bridge_detect)
> +	else if (bridge_connector->bridge_detect)
>   		connector->polled = DRM_CONNECTOR_POLL_CONNECT
>   				  | DRM_CONNECTOR_POLL_DISCONNECT;
>   
> @@ -887,15 +854,6 @@ 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);
> 

-- 
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Louis Chauvet]

Le 17/10/2025 à 18:15, Luca Ceresoli a écrit :
> 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.
> 
> Achieve this by adding a drmm cleanup callback whic puts all the non-NULL
> bridges. Using drmm ensures the cleanup happens on drm_device teardown,
> whichever is the return value of this function.
> 
> Four of these pointers (edid, hpd, detect and modes) can be written
> multiple times (up to once per loop iterations), in order to eventually
> store the last matching bridge. So 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.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
> 
> This patch was added in v2:
> - it is a rewrite of the original (buggy) patch, but simpler after
>    realizing thanks to drmm we don't need the temporary pointers as the
>    stored pointers will be put just before deallocating
> - compared to the original patch, moved the panel_bridge changes to a
>    separate patch for clarity
> ---
>   drivers/gpu/drm/display/drm_bridge_connector.c | 52 +++++++++++++++++++-------
>   1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index a5bdd6c1064399ece6b19560f145b877c9e0680e..95ccf86527129edaa6fcc75c6202985e73c46da8 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
> @@ -649,6 +663,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,14 +690,22 @@ 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_connector->bridge_edid);
> +			bridge_connector->bridge_edid = drm_bridge_get(bridge);
> +		}
> +		if (bridge->ops & DRM_BRIDGE_OP_HPD) {
> +			drm_bridge_put(bridge_connector->bridge_hpd);
> +			bridge_connector->bridge_hpd = drm_bridge_get(bridge);
> +		}
> +		if (bridge->ops & DRM_BRIDGE_OP_DETECT) {
> +			drm_bridge_put(bridge_connector->bridge_detect);
> +			bridge_connector->bridge_detect = drm_bridge_get(bridge);
> +		}
> +		if (bridge->ops & DRM_BRIDGE_OP_MODES) {
> +			drm_bridge_put(bridge_connector->bridge_modes);
> +			bridge_connector->bridge_modes = drm_bridge_get(bridge);
> +		}
>   		if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
>   			if (bridge_connector->bridge_hdmi)
>   				return ERR_PTR(-EBUSY);
> @@ -687,7 +713,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   			    !bridge->funcs->hdmi_clear_infoframe)
>   				return ERR_PTR(-EINVAL);
>   
> -			bridge_connector->bridge_hdmi = bridge;
> +			bridge_connector->bridge_hdmi = drm_bridge_get(bridge);
>   
>   			if (bridge->supported_formats)
>   				supported_formats = bridge->supported_formats;
> @@ -710,7 +736,7 @@ 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_connector->bridge_hdmi_audio = drm_bridge_get(bridge);
>   		}
>   
>   		if (bridge->ops & DRM_BRIDGE_OP_DP_AUDIO) {
> @@ -728,21 +754,21 @@ 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_connector->bridge_dp_audio = drm_bridge_get(bridge);
>   		}
>   
>   		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
>   			if (bridge_connector->bridge_hdmi_cec)
>   				return ERR_PTR(-EBUSY);
>   
> -			bridge_connector->bridge_hdmi_cec = bridge;
> +			bridge_connector->bridge_hdmi_cec = drm_bridge_get(bridge);
>   		}
>   
>   		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
>   			if (bridge_connector->bridge_hdmi_cec)
>   				return ERR_PTR(-EBUSY);
>   
> -			bridge_connector->bridge_hdmi_cec = bridge;
> +			bridge_connector->bridge_hdmi_cec = drm_bridge_get(bridge);
>   
>   			if (!bridge->funcs->hdmi_cec_enable ||
>   			    !bridge->funcs->hdmi_cec_log_addr ||
> 

-- 
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 3/3] drm/display: bridge_connector: get/put the panel_bridge

[Louis Chauvet]

Le 17/10/2025 à 18:15, Luca Ceresoli a écrit :
> The panel_bridge pointer is taken inside the loop and used after the
> loop. Being a local variable, use a cleanup action to ensure it is put on
> return.
> 
> Based on the code structure the panel_bridge pointer might be assigned
> during multiple loop iterations. Even though this is probably not possible
> in the practice, ensure there is no reference leak by putting the reference
> to the old value before overwriting with the new value.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
> 
> This patch was added in v2:
> - taking the panel_bridge specific code from the original (buggy) patch,
>    and split it for clarity from the larger patch covering stored bridge
>    pointers
> - comapred to the original code, added drm_bridge_put() for extra safety
>    even though likely not necessary
> ---
>   drivers/gpu/drm/display/drm_bridge_connector.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 95ccf86527129edaa6fcc75c6202985e73c46da8..a2d30cf9e06df44b89456b5aba8198ee1e5d5601 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -652,7 +652,7 @@ 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;
>   	unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
>   	unsigned int max_bpc = 8;
>   	bool support_hdcp = false;
> @@ -787,8 +787,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   		if (bridge->ddc)
>   			ddc = bridge->ddc;
>   
> -		if (drm_bridge_is_panel(bridge))
> -			panel_bridge = bridge;
> +		if (drm_bridge_is_panel(bridge)) {
> +			drm_bridge_put(panel_bridge);
> +			panel_bridge = drm_bridge_get(bridge);
> +		}
>   
>   		if (bridge->support_hdcp)
>   			support_hdcp = true;
> 

-- 
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 1/3] Revert "drm/display: bridge_connector: get/put the stored bridges"

[Luca Ceresoli]

Hello Mark,

On Thu Oct 30, 2025 at 2:11 PM CET, Mark Brown wrote:
> On Fri, Oct 17, 2025 at 06:15:04PM +0200, Luca Ceresoli wrote:
>> This reverts commit 2be300f9a0b6f6b0ae2a90be97e558ec0535be54.
>>
>> The commit being reverted moved all the bridge_connector->bridge_*
>> assignments to just before the final successful return in order to handle
>> the bridge refcounting in a clean way.
>
> Is there any news on getting this series merged - the currently broken
> code in -next is causing boot issues on several affected platforms (eg,
> Rock5B) which is disrupting other testing?  If the other patches are
> somehow causing issues could we perhaps get the revert in to fix the
> boot issue while those issues are resolved?

Thanks for pinging, I must agree the regression is out since quite a few
weeks. Despite having four Tested-by, this series was lacking Reviewed- and
Tested-by entirely.

Now Louis reviewed the whole series (thanks!), so my understanding of the
drm-misc policy is that I can apply the series, which I plan to do in a few
days to let anybody else comment.

Best regards,
Luca

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

Re: [PATCH v2 0/3] drm/display: bridge_connector: get/put the stored bridges: fix NULL pointer regression

[Dmitry Baryshkov]

On Fri, Oct 17, 2025 at 06:15:03PM +0200, Luca Ceresoli wrote:
> A patch of mine recently merged in drm-misc-next [1] has a NULL pointer
> deref regression (reported here [2] and here [3]). Being in lack of a quick
> fix, I sent a revert proposal [4].
> 
> The revert proposal has no answers currenty, and in the meanwhile I have a
> patch that implements the original idea but without the same bug. So here's
> a v2 series with:
> 
>  - the same revert patch
>  - the original patch but rewritten without the same bug (and even simpler)
> 
> Also the re-written patch is now split in two for clarity because it was
> doing two somewhat different things.
> 
> [1] https://lore.kernel.org/all/20250926-drm-bridge-alloc-getput-bridge-connector-v2-1-138b4bb70576@bootlin.com/
> [2] https://lore.kernel.org/lkml/336fbfdd-c424-490e-b5d1-8ee84043dc80@samsung.com/
> [3] https://lore.kernel.org/lkml/CA+G9fYuKHp3QgPKjgFY3TfkDdh5Vf=Ae5pCW+eU41Bu=D7th2g@mail.gmail.com/
> [4] https://lore.kernel.org/lkml/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com/
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> Changes in v2:
> - No changes to the revert patch
> - Added the (corrected) patch introducing the same feature as the original
>   buggy patch, and also split it in two fir clarity
> - Link to v1: https://lore.kernel.org/r/20251016-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v1-1-81d6984c5361@bootlin.com
> 

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

Tested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> # db410c



-- 
With best wishes
Dmitry

Re: [PATCH v2 0/3] drm/display: bridge_connector: get/put the stored bridges: fix NULL pointer regression

[Luca Ceresoli]

On Fri, 17 Oct 2025 18:15:03 +0200, Luca Ceresoli wrote:
> A patch of mine recently merged in drm-misc-next [1] has a NULL pointer
> deref regression (reported here [2] and here [3]). Being in lack of a quick
> fix, I sent a revert proposal [4].
> 
> The revert proposal has no answers currenty, and in the meanwhile I have a
> patch that implements the original idea but without the same bug. So here's
> a v2 series with:
> 
> [...]

Applied, thanks!

[1/3] Revert "drm/display: bridge_connector: get/put the stored bridges"
      commit: b4027536933f813e51cc53be0b7542012f09aa38
[2/3] drm/display: bridge_connector: get/put the stored bridges
      commit: 13adb8c97846603efc7bfc7663dfdc0ba2f34b8f
[3/3] drm/display: bridge_connector: get/put the panel_bridge
      commit: a3f433c57c46776f95fdf4cfaad1ab27dbca7311

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