Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Polling monitor presence using `drm_dp_dpcd_read_link_status()` reads from link training status registers (DP_LANE0_1_STATUS), which can disrupt the DP sink's state machine. - [High] Calling `cdns_mhdp_update_link_status()` from the `.detect()` callback continuously re-programs live video registers during DRM polling. - [High] Missing `drm_dp_aux_unregister` on the firmware load timeout error path in `cdns_mhdp_attach`. - [Medium] In no-hpd mode, link status evaluation is bypassed, preventing recovery from main link failures. - [Medium] A hardcoded 100ms timeout in `cdns_mhdp_attach` to wait for asynchronous firmware loading is too short and will cause initialization failures. --
commit 2c6e286375fe2e77fec97f0ea17066c50894dd26 Author: Rahul T R <[email protected]> drm: bridge: cdns-mhdp8546: Add no-hpd property This patch adds a 'no-hpd' property to support boards where the HPD line cannot be used for hotplug detection. It implements a polling mechanism using AUX reads to detect monitor presence when hardware HPD signals are unavailable. > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > index 36c07b71fe04b..5e1bad8fc73ea 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c [ ... ] > @@ -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] Will this 100ms timeout cause initialization failures? Async firmware loading can routinely take longer than 100ms on a busy system or when reading from slow storage. > + if (ret == 0) { > + dev_err(mhdp->dev, "%s: Timeout waiting for fw > loading\n", > + __func__); > + return -ETIMEDOUT; [Severity: High] Does this error path leak the registered AUX channel? Earlier in cdns_mhdp_attach(), the DRM DP AUX channel is registered using drm_dp_aux_register(). Returning directly here skips the aux_unregister error path label at the end of the function, leaving the AUX channel registered but in an invalid state. > + } > + > + 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] Is it safe to read link training status registers to poll for monitor presence? drm_dp_dpcd_read_link_status() reads the DP_LANE0_1_STATUS (0x202) register. According to the DRM subsystem guidelines, reading link training status registers (0x202-0x207) outside of the expected training sequence can trigger unintended state machine transitions in DP/eDP sinks. Could this cause hardware state corruption or screen flickering? > + hpd_pulse = false; [Severity: Medium] Does hardcoding hpd_pulse to false bypass link retraining? Later in cdns_mhdp_update_link_status(), link validation logic checks if (hpd_pulse && old_plugged == mhdp->plugged). By hardcoding this to false, that logic is skipped. If the main high-speed link loses synchronization but the monitor remains connected, wouldn't the driver fail to retrain the link? [Severity: High] Does this also cause live video stream registers to be reprogrammed on every polling event? Since hpd_pulse is false, the function skips the early return path for a healthy link (goto out;). It then falls through to the end of the function where it unconditionally executes cdns_mhdp_sst_enable(mhdp, current_mode) if mhdp->bridge_enabled is true. When called via periodic DRM polling via the .detect() callback, won't this cause severe screen flickering or sync loss by continuously overwriting active hardware display timing registers? > + 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
