From: Jayesh Choudhary <j-choudhary@ti.com>

In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
NULL pointer while trying to access the mutex. We need the connector to
be set before that. Unlike in legacy !(DBANC) cases, we do not have
connector initialised in bridge_attach(). Now that we have the connector
pointer in mhdp bridge structure, so set the mhdp->connector in
atomic_enable() earlier to avoid possible NULL pointer dereference
in recovery paths like modeset_retry_fn() with the DBANC flag set.

Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index c2ce3d6e5a88..b2f5a48cac2d 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -1759,12 +1759,21 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
 	struct drm_bridge_state *new_state;
 	const struct drm_display_mode *mode;
 	u32 resp;
-	int ret;
+	int ret = 0;
 
 	dev_dbg(mhdp->dev, "bridge enable\n");
 
 	mutex_lock(&mhdp->link_mutex);
 
+	mhdp->connector = drm_atomic_get_new_connector_for_encoder(state,
+								   bridge->encoder);
+	if (WARN_ON(!mhdp->connector))
+		goto out;
+
+	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector);
+	if (WARN_ON(!conn_state))
+		goto out;
+
 	if (mhdp->plugged && !mhdp->link_up) {
 		ret = cdns_mhdp_link_up(mhdp);
 		if (ret < 0)
@@ -1784,15 +1793,6 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
 	cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
 			    resp | CDNS_VIF_CLK_EN | CDNS_VIF_CLK_RSTN);
 
-	mhdp->connector = drm_atomic_get_new_connector_for_encoder(state,
-								   bridge->encoder);
-	if (WARN_ON(!mhdp->connector))
-		goto out;
-
-	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector);
-	if (WARN_ON(!conn_state))
-		goto out;
-
 	if (mhdp->hdcp_supported &&
 	    mhdp->hw_state == MHDP_HW_READY &&
 	    conn_state->content_protection ==
-- 
2.34.1

On Mon, Sep 29, 2025 at 02:09:33PM +0530, Harikrishna Shenoy wrote:
> From: Jayesh Choudhary <j-choudhary@ti.com>
> 
> In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
> in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
> NULL pointer while trying to access the mutex. We need the connector to
> be set before that. Unlike in legacy !(DBANC) cases, we do not have
> connector initialised in bridge_attach(). Now that we have the connector
> pointer in mhdp bridge structure, so set the mhdp->connector in
> atomic_enable() earlier to avoid possible NULL pointer dereference
> in recovery paths like modeset_retry_fn() with the DBANC flag set.
> 
> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")

This Fixes tag means that this patch can be attempted to be backported
back to v6.5 (even w/o cc:stable, etc). I know that it is a pain, but
please move all Fixes to the top of the series. Yes, you want to drop
non-DBANC case first and then fix everything. It doesn't look like it is
a correct approach for the sake of maintaing the -stable branches.

> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 20 +++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 

-- 
With best wishes
Dmitry

On 06/10/25 04:42, Dmitry Baryshkov wrote:
> On Mon, Sep 29, 2025 at 02:09:33PM +0530, Harikrishna Shenoy wrote:
>> From: Jayesh Choudhary <j-choudhary@ti.com>
>>
>> In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
>> in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
>> NULL pointer while trying to access the mutex. We need the connector to
>> be set before that. Unlike in legacy !(DBANC) cases, we do not have
>> connector initialised in bridge_attach(). Now that we have the connector
>> pointer in mhdp bridge structure, so set the mhdp->connector in
>> atomic_enable() earlier to avoid possible NULL pointer dereference
>> in recovery paths like modeset_retry_fn() with the DBANC flag set.
>>
>> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
> 
> This Fixes tag means that this patch can be attempted to be backported
> back to v6.5 (even w/o cc:stable, etc). I know that it is a pain, but
> please move all Fixes to the top of the series. Yes, you want to drop
> non-DBANC case first and then fix everything. It doesn't look like it is
> a correct approach for the sake of maintaing the -stable branches.
> 
Hi Dmitry,

The patch which drops non-DBANC case can be moved after fixes, but for
making fixes i need the pointer structure so adding fixes tag to PATCH 
2/6 "drm/bridge: cadence: cdns-mhdp8546*: Change
  drm_connector from structure to pointer"
Let me know your thoughts on this, will re-spin accordingly.

Regards.

>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
>> ---
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 20 +++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>

On Fri, Oct 10, 2025 at 02:48:10PM +0530, Harikrishna shenoy wrote:
> 
> 
> On 06/10/25 04:42, Dmitry Baryshkov wrote:
> > On Mon, Sep 29, 2025 at 02:09:33PM +0530, Harikrishna Shenoy wrote:
> > > From: Jayesh Choudhary <j-choudhary@ti.com>
> > > 
> > > In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
> > > in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
> > > NULL pointer while trying to access the mutex. We need the connector to
> > > be set before that. Unlike in legacy !(DBANC) cases, we do not have
> > > connector initialised in bridge_attach(). Now that we have the connector
> > > pointer in mhdp bridge structure, so set the mhdp->connector in
> > > atomic_enable() earlier to avoid possible NULL pointer dereference
> > > in recovery paths like modeset_retry_fn() with the DBANC flag set.
> > > 
> > > Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
> > 
> > This Fixes tag means that this patch can be attempted to be backported
> > back to v6.5 (even w/o cc:stable, etc). I know that it is a pain, but
> > please move all Fixes to the top of the series. Yes, you want to drop
> > non-DBANC case first and then fix everything. It doesn't look like it is
> > a correct approach for the sake of maintaing the -stable branches.
> > 
> Hi Dmitry,
> 
> The patch which drops non-DBANC case can be moved after fixes, but for
> making fixes i need the pointer structure so adding fixes tag to PATCH 2/6
> "drm/bridge: cadence: cdns-mhdp8546*: Change
>  drm_connector from structure to pointer"
> Let me know your thoughts on this, will re-spin accordingly.

You still can have a pointer to drm_connector. Just make sure that it
points to the internally-created one if the driver did create it and to
the external one if DBANC flag has been passed.

The easiest way to ensure correctness is by reordering patches: move the
fixes to the top of the series and make sure that they are correct
first. Then drop the old code dropping drm_connector creation.

> 
> Regards.
> 
> > > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> > > Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
> > > ---
> > >   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 20 +++++++++----------
> > >   1 file changed, 10 insertions(+), 10 deletions(-)
> > > 
> > 

-- 
With best wishes
Dmitry