| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c5b98cba-80cf-4659-9e3d-f11bafbfe0b9@ti.com>
Date: Mon, 24 Nov 2025 17:14:51 +0530
From: Harikrishna shenoy <h-shenoy@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
CC: <Laurent.pinchart@...asonboard.com>, <airlied@...il.com>,
<andrzej.hajda@...el.com>, <andy.yan@...k-chips.com>,
<aradhya.bhatia@...ux.dev>, <devarsht@...com>, <dianders@...omium.org>,
<dri-devel@...ts.freedesktop.org>, <javierm@...hat.com>,
<jernej.skrabec@...il.com>, <jonas@...boo.se>,
<linux-kernel@...r.kernel.org>, <linux@...blig.org>,
<luca.ceresoli@...tlin.com>, <lumag@...nel.org>, <lyude@...hat.com>,
<maarten.lankhorst@...ux.intel.com>, <mordan@...ras.ru>,
<mripard@...nel.org>, <neil.armstrong@...aro.org>, <rfoss@...nel.org>,
<s-jain1@...com>, <simona@...ll.ch>, <tzimmermann@...e.de>, <u-kumar1@...com>
Subject: Re: [PATCH RESEND v9 1/6] drm/bridge: cadence: cdns-mhdp8546-core:
Set the mhdp connector earlier in atomic_enable()
On 21/11/25 18:42, Tomi Valkeinen wrote:
>
>
> On 20/11/2025 14:14, Harikrishna Shenoy wrote:
>> From: Jayesh Choudhary <j-choudhary@...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 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 <j-choudhary@...com>
>> Signed-off-by: Harikrishna Shenoy <h-shenoy@...com>
>> ---
>> .../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));
>
Hi Tomi,
> This code looks odd, and there are a few other similar if-elses too.
> What does this do?
>
> Correct me if I'm wrong but:
>
> With DRM_BRIDGE_ATTACH_NO_CONNECTOR, connector_ptr is set at
> cdns_mhdp_atomic_enable(). If connector_ptr is set, I assume we also
> always have dev there.
>
> Without DRM_BRIDGE_ATTACH_NO_CONNECTOR, connector_ptr is set at
> cdns_mhdp_connector_init(). If it is set, we have called
> drm_connector_init(), and dev is valid.
>
> So... If we have connector_ptr, connector_ptr->dev is always set? So no
> need to check for the dev.
Yes this is correct, will remove the redundant checks here.
>
> But overall I'm confused about the 'else' block. Why do we have these
> code blocks that (in the current upstream code) check for
> 'mhdp->connector.dev' (I think that's checking if the
> cdns_mhdp_connector_init() has been called), and then, e.g. here, call
> either drm_kms_helper_hotplug_event() or drm_bridge_hpd_notify(). I
> don't understand what is the idea here.
>
For the else block, in code I can find some explanation in
cdns_mhdp_irq_handler function, but not sure of reasoning behind the
'else' as even in 'if' condition check is for connector but inside the
block drm_kms_helper_hotplug_event is called with bridge.dev.
> Also, in the above case, the code checks for 'bridge_attached'. It is
> set in cdns_mhdp_attach(), after calling cdns_mhdp_connector_init().
> So... If bridge_attached is true, we always have a connector if
> DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. And if
> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set, we never have a connector,
> because it's set only in atomic enable.
>
> So the 'if' is effectively checking for DRM_BRIDGE_ATTACH_NO_CONNECTOR
> flag? It still doesn't explain why we need to check it here.
>
> Tomi
>
>> @@ -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);
>> + }
>>
>> 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) {
>> + state = mhdp->connector_ptr->state;
>> state->content_protection = mhdp->hdcp.value;
>> }
>> mutex_unlock(&mhdp->hdcp.mutex);
>
Powered by blists - more mailing lists