Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Aborting `atomic_pre_enable()` on power-on failure guarantees an imprecise abort (system crash) in `atomic_post_disable()` due to unconditional accesses to unpowered PHY and HDMI controller registers. Pre-existing issues: - [High] Unclocked MMIO access in `msm_hdmi_set_timings()` before the HDMI PM runtime is resumed. -- commit 9912da31672d32069f2e4d118d3714a533a0157a Author: Krzysztof Kozlowski <[email protected]> drm/msm/hdmi: Handle msm_hdmi_power_on() errors during .atomic_pre_enable() This commit adds error handling for msm_hdmi_power_on() during the .atomic_pre_enable() phase to prevent register accesses if the power domain fails to come up. Cc: <[email protected]> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > index 157f19bd90b4e..db85400e574e4 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c [ ... ] > @@ -287,13 +294,18 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct > drm_bridge *bridge, > > msm_hdmi_set_timings(hdmi, &crtc_state->adjusted_mode); [Severity: High] This is a pre-existing issue, but does calling msm_hdmi_set_timings() here risk unclocked MMIO access? This register access occurs before the power domain is reliably enabled by msm_hdmi_power_on() later in this function. If the connector is forced on while disconnected (so HPD is disabled and no background PM reference is held), the AHB/APB clocks for the HDMI block are off. Writing to these registers without an active clock can immediately stall the bus and cause an imprecise external abort (kernel panic). > - mutex_lock(&hdmi->state_mutex); > - if (!hdmi->power_on) { > - msm_hdmi_phy_resource_enable(phy); > - msm_hdmi_power_on(bridge); > - hdmi->power_on = true; > + scoped_guard(mutex, &hdmi->state_mutex) { > + if (!hdmi->power_on) { > + if (msm_hdmi_phy_resource_enable(phy)) > + return; > + > + if (msm_hdmi_power_on(bridge)) { > + msm_hdmi_phy_resource_disable(phy); > + return; [Severity: High] Does aborting here guarantee a crash later during teardown? When this error path disables PHY resources and returns early, it leaves hdmi->power_on = false. However, because atomic_pre_enable() returns void, the DRM core is unaware of the failure and continues the modeset sequence. Eventually, the DRM core will call msm_hdmi_bridge_atomic_post_disable(), which unconditionally writes to unpowered registers: msm_hdmi_bridge_atomic_post_disable() { ... msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled); msm_hdmi_phy_powerdown(phy); } Since the PHY clocks were explicitly disabled by this error path, accessing these registers without an active clock will cause an imprecise external abort on MSM hardware. > + } > + hdmi->power_on = true; > + } > } > - mutex_unlock(&hdmi->state_mutex); > > if (connector->display_info.is_hdmi) > msm_hdmi_audio_update(hdmi); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260908-drm-msm-hdmi-fixes-before-eliza-v3-0-dcf895687...@oss.qualcomm.com?part=2
