Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] Silent link training failure in atomic_enable leaves userspace with a broken display state. - [High] Moving DP link training to `atomic_enable()` leaves the unplug handler performing unbalanced clock disable and PHY power off, which corrupts the PHY power state. - [Medium] Commit message and implementation mismatch regarding hpd_notify(). Pre-existing issues: - [High] Missing PM runtime put on error paths in `msm_dp_bridge_atomic_enable` causes a usage counter leak. - [High] Failure during `msm_dp_ctrl_on_stream()` permanently leaks link clocks, stream clocks, and PHY power. -- --- Patch [4]: [PATCH v5 04/10] drm/msm/dp: Move link training to atomic_enable() --- commit c64d2d707d517a12463d9c1b31e96a4e25a831e3 Author: Jessica Zhang <[email protected]> drm/msm/dp: Move link training to atomic_enable() This commit moves DP link training from the HPD handler into atomic_enable(). It also states that plug and unplug handlers are called directly in hpd_notify() to prevent preemption by other events. > Finally, call the plug/unplug handlers directly in hpd_notify() instead > of queueing them in the event thread so that they aren't preempted by > other events. [Severity: Medium] Is this description accurate regarding hpd_notify()? Looking at msm_dp_bridge_hpd_notify(), the code continues to queue events via msm_dp_add_event() rather than calling the handlers directly: drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_hpd_notify() { if (!msm_dp_display->link_ready && status == connector_status_connected) msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0); else if (msm_dp_display->link_ready && status == connector_status_disconnected) msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0); } Could the commit message or implementation be updated to match? > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index a05144de3b93f..e7b1ed5491c49 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -436,11 +436,6 @@ static int msm_dp_display_process_hpd_high(struct > msm_dp_display_private *dp) > msm_dp_link_psm_config(dp->link, &dp->panel->link_info, false); > > msm_dp_link_reset_phy_params_vx_px(dp->link); > - rc = msm_dp_ctrl_on_link(dp->ctrl); > - if (rc) { > - DRM_ERROR("failed to complete DP link training\n"); > - goto end; > - } [Severity: High] Does removing the link training here leave the unplug handler performing an unbalanced clock disable and PHY power off? If a user unplugs the display while in ST_MAINLINK_READY (before userspace issues an atomic commit), msm_dp_hpd_unplug_handle() will call msm_dp_ctrl_off_link(). This blindly calls clk_bulk_disable_unprepare() and phy_power_off() on clocks and PHYs that were never enabled, leading to an unbalanced clock disable warning and decrementing the PHY power_count to a negative value. Once the power_count drops to -1, the next phy_power_on() increments it to 0 and bypasses the actual hardware callback, which seems like it would break the display until reboot. > > msm_dp_add_event(dp, EV_USER_NOTIFICATION, true, 0); > [ ... ] > @@ -1695,6 +1690,11 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge > *drm_bridge, > force_link_train = true; > } > > + rc = msm_dp_ctrl_on_link(msm_dp_display->ctrl); > + if (rc) > + DRM_ERROR("Failed link training (rc=%d)\n", rc); > + // TODO: schedule drm_connector_set_link_status_property() > + [Severity: High] Will this leave userspace with a silently broken display state if msm_dp_ctrl_on_link() fails? The error is logged, but execution proceeds to enable the display anyway. Without the mentioned drm_connector_set_link_status_property() update to mark the link as BAD, does userspace have any way to know it must trigger a recovery modeset? [Severity: High] This is a pre-existing issue, but does this function leak a PM runtime usage counter on its error paths? Earlier in msm_dp_bridge_atomic_enable(), a reference is acquired via pm_runtime_resume_and_get(), but the early returns do not release it: drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_atomic_enable() { ... hpd_state = msm_dp_display->hpd_state; if (hpd_state != ST_DISPLAY_OFF && hpd_state != ST_MAINLINK_READY) { mutex_unlock(&msm_dp_display->event_mutex); return; } rc = msm_dp_display_set_mode(dp, &msm_dp_display->msm_dp_mode); if (rc) { DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc); mutex_unlock(&msm_dp_display->event_mutex); return; } ... } If either condition fails, the function returns early without calling pm_runtime_put_sync(). Will this permanently prevent the display controller from entering suspend states? [Severity: High] This is also a pre-existing issue, but can a failure during stream setup permanently leak link clocks, stream clocks, and PHY power? In msm_dp_ctrl_on_stream(): drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_on_stream() { ... msm_dp_ctrl_enable_mainlink_clks(ctrl); ret = msm_dp_ctrl_set_tu(ctrl); if (ret) return ret; ... } If msm_dp_ctrl_set_tu() (or a subsequent step) fails, the function returns without disabling the clocks and PHY power enabled earlier. The caller (msm_dp_display_enable) then skips setting msm_dp_display->power_on = true. Later, msm_dp_display_disable() bypasses teardown: drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_disable() { if (!msm_dp_display->power_on) return 0; ... } Does this bypass cement the resource leak and permanently block PM? > msm_dp_display_enable(msm_dp_display, force_link_train); > > rc = msm_dp_display_post_enable(dp); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
