Hi,
On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> From: Jayesh Choudhary <[email protected]>
>
> 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 cases with flag
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, we do not have connector initialised
> in bridge_attach(), so add the mhdp->connector_ptr in device structure
> to handle both cases with DRM_BRIDGE_ATTACH_NO_CONNECTOR and
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, set it in atomic_enable() earlier to
> avoid possible NULL pointer dereference in recovery paths like
> modeset_retry_fn() with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set.
>
> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
> Signed-off-by: Jayesh Choudhary <[email protected]>
> Signed-off-by: Harikrishna Shenoy <[email protected]>
> ---
> .../drm/bridge/cadence/cdns-mhdp8546-core.c | 29 ++++++++++---------
> .../drm/bridge/cadence/cdns-mhdp8546-core.h | 1 +
> .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c | 26 ++++++++++++-----
> 3 files changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 38726ae1bf150..f3076e9cdabbe 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -740,7 +740,7 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw,
> void *context)
> bridge_attached = mhdp->bridge_attached;
> spin_unlock(&mhdp->start_lock);
> if (bridge_attached) {
> - if (mhdp->connector.dev)
> + if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
> drm_kms_helper_hotplug_event(mhdp->bridge.dev);
> else
> drm_bridge_hpd_notify(&mhdp->bridge,
> cdns_mhdp_detect(mhdp));
> @@ -1636,6 +1636,7 @@ static int cdns_mhdp_connector_init(struct
> cdns_mhdp_device *mhdp)
> return ret;
> }
>
> + mhdp->connector_ptr = conn;
> drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
>
> ret = drm_display_info_set_bus_formats(&conn->display_info,
> @@ -1915,17 +1916,25 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge
> *bridge,
> struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> struct cdns_mhdp_bridge_state *mhdp_state;
> struct drm_crtc_state *crtc_state;
> - struct drm_connector *connector;
> struct drm_connector_state *conn_state;
> 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_ptr = drm_atomic_get_new_connector_for_encoder(state,
> +
> bridge->encoder);
> + if (WARN_ON(!mhdp->connector_ptr))
> + goto out;
> +
> + conn_state = drm_atomic_get_new_connector_state(state,
> mhdp->connector_ptr);
> + if (WARN_ON(!conn_state))
> + goto out;
> +
> if (mhdp->plugged && !mhdp->link_up) {
> ret = cdns_mhdp_link_up(mhdp);
> if (ret < 0)
> @@ -1945,15 +1954,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);
>
> - connector = drm_atomic_get_new_connector_for_encoder(state,
> - bridge->encoder);
> - if (WARN_ON(!connector))
> - goto out;
> -
> - conn_state = drm_atomic_get_new_connector_state(state, connector);
> - if (WARN_ON(!conn_state))
> - goto out;
> -
> if (mhdp->hdcp_supported &&
> mhdp->hw_state == MHDP_HW_READY &&
> conn_state->content_protection ==
> @@ -2030,6 +2030,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge
> *bridge,
> if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
> mhdp->info->ops->disable(mhdp);
>
> + mhdp->connector_ptr = NULL;
> mutex_unlock(&mhdp->link_mutex);
> }
>
> @@ -2296,7 +2297,7 @@ static void cdns_mhdp_modeset_retry_fn(struct
> work_struct *work)
>
> mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
>
> - conn = &mhdp->connector;
> + conn = mhdp->connector_ptr;
>
> /* Grab the locks before changing connector property */
> mutex_lock(&conn->dev->mode_config.mutex);
> @@ -2373,7 +2374,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
> int ret;
>
> ret = cdns_mhdp_update_link_status(mhdp);
> - if (mhdp->connector.dev) {
> + if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
> if (ret < 0)
> schedule_work(&mhdp->modeset_retry_work);
> else
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> index bad2fc0c73066..a76775c768956 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -376,6 +376,7 @@ struct cdns_mhdp_device {
> struct mutex link_mutex;
>
> struct drm_connector connector;
> + struct drm_connector *connector_ptr;
> struct drm_bridge bridge;
>
> struct cdns_mhdp_link link;
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> index 42248f179b69d..5ac2fad2f0078 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> @@ -393,8 +393,10 @@ static int _cdns_mhdp_hdcp_disable(struct
> cdns_mhdp_device *mhdp)
> {
> int ret;
>
> - dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> - mhdp->connector.name, mhdp->connector.base.id);
> + if (mhdp->connector_ptr) {
> + dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> + mhdp->connector_ptr->name,
> mhdp->connector_ptr->base.id);
> + }
>
> ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>
> @@ -443,9 +445,11 @@ static int cdns_mhdp_hdcp_check_link(struct
> cdns_mhdp_device *mhdp)
> if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
> goto out;
>
> - dev_err(mhdp->dev,
> - "[%s:%d] HDCP link failed, retrying authentication\n",
> - mhdp->connector.name, mhdp->connector.base.id);
> + if (mhdp->connector_ptr) {
> + dev_err(mhdp->dev,
> + "[%s:%d] HDCP link failed, retrying authentication\n",
> + mhdp->connector_ptr->name,
> mhdp->connector_ptr->base.id);
> + }
This looks hackish... What's the point of printing the connector name
and id anyway? For MST? And if we don't have a connector, is there any
point in doing anything in cdns_mhdp_hdcp_check_link() or the other
functions that have similar prints? Or why would they even be called?
Are they ever called if we don't have a connector?
I think for now, it's simplest to just drop the use of connector_ptr
from the prints, instead of these odd if()s. Or assume that we do have
connector_ptr, if the driver is designed that way.
>
> ret = _cdns_mhdp_hdcp_disable(mhdp);
> if (ret) {
> @@ -487,13 +491,19 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct
> *work)
> struct cdns_mhdp_device *mhdp = container_of(hdcp,
> struct cdns_mhdp_device,
> hdcp);
> - struct drm_device *dev = mhdp->connector.dev;
> + struct drm_device *dev = NULL;
> struct drm_connector_state *state;
>
> + if (mhdp->connector_ptr)
> + dev = mhdp->connector_ptr->dev;
> +
> + if (!dev)
> + return;
> +
> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> mutex_lock(&mhdp->hdcp.mutex);
> - if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> - state = mhdp->connector.state;
> + if (mhdp->connector_ptr && mhdp->hdcp.value !=
> DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
Earlier in the function you check if there's a connector, and if not,
dev is NULL, and there's return. So the code can never reach here if
connector_ptr is NULL.
> + state = mhdp->connector_ptr->state;
> state->content_protection = mhdp->hdcp.value;
The only real thing this function does are the two lines above.
So I think you can just :
if (!mhdp->connector_ptr || !mhdp->connector_ptr->dev)
return;
(or something similar) at the beginning of the function. Or should it be
inside the connection_mutex, this one is a scheduled work...
Tomi
> }
> mutex_unlock(&mhdp->hdcp.mutex);
