Re: [PATCH v6 2/2] drm: bridge: cdns-mhdp8546: Add no-hpd property

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Polling `.detect()` breaks DPMS and wastes power by continuously retraining the link after the display is disabled.
- [High] Skipping `cdns_mhdp_bridge_hpd_enable()` in no-hpd mode permanently masks SW event interrupts, breaking HDCP.

Pre-existing issues:
- [Critical] Accessing `bridge.base.state` outside of atomic commit context without modeset locks leads to a Use-After-Free.
--

--- Patch [2]: [PATCH v6 2/2] drm: bridge: cdns-mhdp8546: Add no-hpd property ---
commit 33969e025e143110f913b15c51739edb9cc917f9
Author: Rahul T R <[email protected]>
Subject: drm: bridge: cdns-mhdp8546: Add no-hpd property

Add a 'no-hpd' boolean property to support boards where the HPD line
cannot be used for hotplug detection due to hardware limitations.

> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 36c07b71fe04b..c81c742e56ac8 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c

[ ... ]

> @@ -698,7 +700,9 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
>  	 * MHDP_HW_STOPPED happens only due to driver removal when
>  	 * bridge should already be detached.
>  	 */
> -	cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);
> +
> +	if (!mhdp->no_hpd)
> +		cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);

[Severity: High]
Does skipping cdns_mhdp_bridge_hpd_enable() here and in cdns_mhdp_attach()
permanently mask software event interrupts?

The CDNS_APB_INT_MASK_SW_EVENT_INT interrupt is multiplexed for multiple
events, including HDCP status (CDNS_HDCP_TX_STATUS).

If it is never unmasked in no-hpd mode, it looks like HDCP initialization
and communication will timeout and fail completely.

[ ... ]

> @@ -2012,6 +2040,16 @@ static enum drm_connector_status
>  cdns_mhdp_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
>  {
>  	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> +	bool hw_ready;
> +
> +	if (mhdp->no_hpd) {
> +		spin_lock(&mhdp->start_lock);
> +		hw_ready = mhdp->hw_state == MHDP_HW_READY;
> +		spin_unlock(&mhdp->start_lock);
> +
> +		if (hw_ready)
> +			cdns_mhdp_update_link_status(mhdp);
> +	}

[Severity: High]
Will this polling cause the link to be continuously retrained even when the
display is intentionally disabled?

When the display is disabled for power management (DPMS blanking),
cdns_mhdp_update_link_status() will see the link is down and execute
cdns_mhdp_link_up():

cdns_mhdp_update_link_status() {
    ...
    if (!mhdp->link_up) {
        ret = cdns_mhdp_link_up(mhdp);
        if (ret < 0)
            goto out;
    }
}

Since the .detect() callback is periodically invoked by the DRM core (every
10 seconds), does this unconditionally wake the monitor up from sleep and waste
power by ignoring the mhdp->bridge_enabled state?

[Severity: Critical]
This is a pre-existing issue, but does exposing cdns_mhdp_update_link_status()
to periodic polling significantly expand a use-after-free race window?

Later in cdns_mhdp_update_link_status(), the code accesses the atomic state
holding only the link_mutex:

cdns_mhdp_update_link_status() {
    ...
    state = drm_priv_to_bridge_state(mhdp->bridge.base.state);
    ...
    cdns_bridge_state = to_cdns_mhdp_bridge_state(state);
    ...
    current_mode = cdns_bridge_state->current_mode;
}

Since drm_private_obj.state is managed by the DRM atomic framework, if a
concurrent non-blocking atomic commit completes during this poll, the old
state can be swapped and freed asynchronously by
drm_atomic_state_default_clear(), leaving this thread dereferencing freed
memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2