Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm/hdmi: system crash in post_disable on unclocked MMIO due to pre_enable error path --
commit 15a43ff6d8f2324644d0916454344e0a91654014 Author: Jiale Yao <[email protected]> drm/msm/hdmi: handle runtime resume failure during power on This commit attempts to handle runtime PM failures in msm_hdmi_power_on() by returning errors to msm_hdmi_bridge_atomic_pre_enable(). It unwinds the PHY resources and aborts the sequence early if power-on or PHY resource enablement fails. > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > index 157f19bd90b4e..7d88657be0473 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c [ ... ] > @@ -289,8 +294,16 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct > drm_bridge *bridge, > > mutex_lock(&hdmi->state_mutex); > if (!hdmi->power_on) { > - msm_hdmi_phy_resource_enable(phy); > - msm_hdmi_power_on(bridge); > + ret = msm_hdmi_phy_resource_enable(phy); > + if (ret) > + goto unlock; > + > + ret = msm_hdmi_power_on(bridge); > + if (ret) { > + msm_hdmi_phy_resource_disable(phy); > + goto unlock; > + } > + [Severity: High] Does this error path lead to a system crash later during teardown? If msm_hdmi_power_on() fails, this new path turns off the PHY resources and jumps to the unlock label, leaving hdmi->power_on = false. However, msm_hdmi_bridge_atomic_pre_enable() returns void, so the DRM core is unaware of the failure and assumes the display pipeline was successfully enabled. When the DRM core later tears down the pipeline, it will call msm_hdmi_bridge_atomic_post_disable(). Looking at that function: drivers/gpu/drm/msm/hdmi/hdmi_bridge.c:msm_hdmi_bridge_atomic_post_disable() { ... /* Keep the HDMI enabled if the HPD is enabled */ mutex_lock(&hdmi->state_mutex); msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled); msm_hdmi_phy_powerdown(phy); if (hdmi->power_on) { ... } These MMIO writes in msm_hdmi_set_mode() and msm_hdmi_phy_powerdown() are executed unconditionally, before the hdmi->power_on check. Because the clocks were never enabled (or were disabled during this error unwind), will these unconditional MMIO writes cause a system crash (Synchronous External Abort) by accessing unclocked hardware? > hdmi->power_on = true; > } > mutex_unlock(&hdmi->state_mutex); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
