Re: [PATCH v3 2/7] drm/msm/hdmi: Handle msm_hdmi_power_on() errors during .atomic_pre_enable()

[sashiko-bot@kernel.org]

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 <krzysztof.kozlowski@oss.qualcomm.com>
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: <stable@vger.kernel.org>

> 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-dcf8956878ca@oss.qualcomm.com?part=2