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 <[email protected]> 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/[email protected]?part=2
