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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Reading `DP_LANE0_1_STATUS` (0x202) for connection polling violates DRM guidelines and disrupts link training state machines.
- [High] The `.detect()` callback unconditionally triggers hardware stream re-enablement during periodic polling in `no-hpd` mode.
- [High] Resource leak of `drm_dp_aux` if firmware loading times out during `cdns_mhdp_attach()`, leading to a Use-After-Free.
- [Medium] Hardcoded 100ms timeout for asynchronous firmware loading in `cdns_mhdp_attach()` can spuriously fail bridge attachment.
- [Medium] Changing DPCD read failures to `dev_err()` will flood the kernel log with false-positive errors during normal operations.
--

--- Patch [2]: [PATCH v5 2/2] drm: bridge: cdns-mhdp8546: Add no-hpd property ---
commit d6385d712222c0d749e282f8179c659c2e87c45d
Author: Rahul T R <r-ravikumar@ti.com>
Subject: drm: bridge: cdns-mhdp8546: Add no-hpd property

This commit adds a 'no-hpd' property to support boards where the HPD line
cannot be used for hotplug detection due to hardware limitations. When set,
the driver uses auxiliary channel (AUX) DPCD reads to detect monitor presence
by polling the connection status via the .detect() callback.

> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
[ ... ]
> @@ -788,9 +792,14 @@ static ssize_t cdns_mhdp_transfer(struct drm_dp_aux *aux,
>  		ret = cdns_mhdp_dpcd_read(mhdp, msg->address,
>  					  msg->buffer, msg->size);
>  		if (ret) {
> -			dev_dbg(mhdp->dev,
> -				"Failed to read DPCD addr %u\n",
> -				msg->address);
> +			if (mhdp->no_hpd)
> +				dev_dbg(mhdp->dev,
> +					"Failed to read DPCD addr %u\n",
> +					msg->address);
> +			else
> +				dev_err(mhdp->dev,
> +					"Failed to read DPCD addr %u\n",
> +					msg->address);

[Severity: Medium]
Will this change spam the kernel log with false-positive errors during normal
operations?

DP AUX channel transactions routinely fail during normal operations, such as
when a disconnected monitor is probed, during DP sleep states, or when the
DRM DP helper retries transient link training failures. Emitting a dev_err()
on every AUX read failure might flood the kernel log on standard hardware.

[ ... ]
> @@ -1523,6 +1532,26 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
>  
>  	spin_unlock(&mhdp->start_lock);
>  
> +	if (mhdp->no_hpd) {
> +		/*
> +		 * In no-hpd mode there is no interrupt to signal firmware
> +		 * readiness. The firmware loads asynchronously after probe(),
> +		 * so we must wait here until the uCPU is running before
> +		 * attempting the first AUX channel poll for monitor presence.
> +		 */
> +		ret = wait_event_timeout(mhdp->fw_load_wq,
> +					 mhdp->hw_state == MHDP_HW_READY,
> +					 msecs_to_jiffies(100));

[Severity: Medium]
Could this hardcoded 100ms timeout spuriously fail bridge attachment?

Firmware loading from a filesystem (via request_firmware_nowait) can easily
take hundreds of milliseconds or longer depending on storage performance and
system load. If the timeout expires prematurely, the driver aborts with
-ETIMEDOUT, which could permanently break the display pipeline for that boot.

> +		if (ret == 0) {
> +			dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n",
> +				__func__);
> +			return -ETIMEDOUT;

[Severity: High]
Does this direct return leak the drm_dp_aux resource?

If the firmware loading timeout expires, this returns -ETIMEDOUT directly
instead of jumping to the aux_unregister error handling label.

Since drm_dp_aux_register() sets up an I2C adapter and the DRM core does not
call .detach() if attach fails, bypassing drm_dp_aux_unregister() leaks the
I2C adapter. Because the memory backing the drm_dp_aux structure is
devm-managed, unloading the module frees the memory while the adapter is still
registered, which can cause a use-after-free when the I2C subsystem accesses
it.

> +		}
> +
> +		cdns_mhdp_update_link_status(mhdp);
> +		return 0;
> +	}
[ ... ]
> @@ -2100,7 +2132,16 @@ static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp)
>  
>  	mutex_lock(&mhdp->link_mutex);
>  
> -	mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
> +	if (mhdp->no_hpd) {
> +		ret = drm_dp_dpcd_read_link_status(&mhdp->aux, status);

[Severity: High]
Does reading the link status here violate DRM subsystem guidelines and disrupt
link training state machines?

The DRM core polls .detect() every 10 seconds when no-hpd is set, which
calls cdns_mhdp_update_link_status(). This unconditionally calls
drm_dp_dpcd_read_link_status() which reads DP_LANE0_1_STATUS (0x202).

According to DRM guidelines, registers in the link training status range
(0x202-0x207) participate in the CR/EQ training state machine and are not
safe for arbitrary read access outside of a link training sequence. Reading
this register continuously for presence detection can induce hardware
instability, screen flickering, and link drops. Should a safe register like
DP_DPCD_REV (0x000) be used instead?

> +		hpd_pulse = false;

[Severity: High]
Does hardcoding hpd_pulse to false cause the active display stream to be
re-initialized on every 10-second poll?

In no-hpd mode, .detect() is polled every 10 seconds. Because hpd_pulse is
hardcoded to false here, the later logic bypasses the check that validates
link status without state mutation:

    if (hpd_pulse && old_plugged == mhdp->plugged) {
        ret = drm_dp_dpcd_read_link_status(&mhdp->aux, status);
        if (!ret && ...)
            goto out;
        ...
    }

Instead, execution falls through to the end of the function. If
mhdp->bridge_enabled is true, this causes cdns_mhdp_sst_enable() to be
called:

    if (mhdp->bridge_enabled) {
        ...
        cdns_mhdp_sst_enable(mhdp, current_mode);
    }

This repeatedly re-programs active stream and framer configuration registers
on every poll, which can cause the active video stream to reset and glitch
every 10 seconds.

> +		if (ret < 0)
> +			mhdp->plugged = false;
> +		else
> +			mhdp->plugged = true;
> +	} else {
> +		mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728125336.776116-1-y-d@ti.com?part=2