Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm/dp: skipped HPD high processing in msm_dp_hpd_plug_handle() - [Medium] drm/msm/dp: runtime PM reference leak in msm_dp_bridge_detect() --
commit 1182d7f2f2e876c63dd6756de1f85227702b93b4 Author: Dmitry Baryshkov <[email protected]> drm/msm/dp: don't take extra references on multiple plug events This commit skips subsequent HPD plug events in msm_dp_hpd_plug_handle() when dp->plugged is already set so that extra runtime PM references are not taken before an unplug event arrives. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index fa8942922b533..ce919a26fba28 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -412,6 +412,9 @@ static int msm_dp_hpd_plug_handle(struct > msm_dp_display_private *dp) > > guard(mutex)(&dp->plugged_lock); > > + if (dp->plugged) > + return 0; [Severity: High] Can returning early here when dp->plugged is true cause msm_dp_display_process_hpd_high() to be skipped completely when a display is connected? In drivers/gpu/drm/display/drm_bridge_connector.c, drm_bridge_connector_detect_ctx() invokes the bridge detect callback before notifying HPD: drm_bridge_connector_detect_ctx() detect->funcs->detect() -> msm_dp_bridge_detect() drm_bridge_connector_hpd_notify() -> msm_dp_bridge_hpd_notify() -> msm_dp_hpd_plug_handle() When msm_dp_bridge_detect() in drivers/gpu/drm/msm/dp/dp_display.c runs first (for instance at boot or resume when a monitor is already connected, or during connector probing before the threaded HPD IRQ runs), it reads the DPCD capabilities into a local buffer and sets priv->plugged = true without calling msm_dp_display_process_hpd_high(): drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_detect() { ... status = connector_status_connected; priv->plugged = true; ... } When msm_dp_hpd_plug_handle() is then called, it sees dp->plugged == true and returns 0 immediately. Similarly, if msm_dp_hpd_plug_handle() runs first and msm_dp_display_process_hpd_high() returns an error (such as when a branch device is connected with sink_count == 0 or on a transient AUX error), msm_dp_hpd_plug_handle() still sets dp->plugged = true at the end of the function: drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_hpd_plug_handle() { ... ret = msm_dp_display_process_hpd_high(dp); drm_dbg_dp(dp->drm_dev, "After, type=%d sink_count=%d\n", dp->msm_dp_display.connector_type, dp->link->sink_count); dp->plugged = true; return ret; } Because msm_dp_display_process_hpd_high() is never completed in either case, will dp->panel->link_info, LTTPR state, the connector EDID, and dp->link->sink_count remain uninitialized? [Severity: Medium] How does this single-reference model in msm_dp_hpd_plug_handle() interact with the runtime PM reference handling in msm_dp_bridge_detect()? In drivers/gpu/drm/msm/dp/dp_display.c, msm_dp_bridge_detect() unconditionally acquires a runtime PM reference on &dp->pdev->dev at entry and only releases it at the end if !priv->plugged: drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_detect() { guard(mutex)(&priv->plugged_lock); ret = pm_runtime_resume_and_get(&dp->pdev->dev); if (ret) { DRM_ERROR("failed to pm_runtime_resume\n"); return status; } ... status = connector_status_connected; priv->plugged = true; ... end: if (!priv->plugged) { if (phy_deinit) { msm_dp_aux_enable_xfers(priv->aux, false); msm_dp_display_host_phy_exit(priv); } pm_runtime_put_sync(&dp->pdev->dev); } return status; } When a sink is connected and priv->plugged is already true, won't every call to msm_dp_bridge_detect() leak an extra runtime PM reference on &dp->pdev->dev because msm_dp_hpd_unplug_handle() only calls pm_runtime_put_sync(&pdev->dev) once when transitioning dp->plugged from true to false? Conversely, if priv->plugged is true (holding a reference from msm_dp_hpd_plug_handle()) and drm_dp_read_dpcd_caps() or drm_dp_read_desc() fails inside msm_dp_bridge_detect(), msm_dp_bridge_detect() sets priv->plugged = false and only drops the reference it took at entry, causing msm_dp_hpd_unplug_handle() to return early on !dp->plugged without releasing the reference acquired by msm_dp_hpd_plug_handle(). > + > ret = pm_runtime_resume_and_get(&pdev->dev); > if (ret) { > DRM_ERROR("failed to pm_runtime_resume\n"); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
