Re: [PATCH v4 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] The patch handles power-up failures by returning early from `atomic_pre_enable`, but leaves unconditional MMIO accesses unguarded in `atomic_post_disable`, leading to a fatal imprecise abort.

Pre-existing issues:
- [High] `msm_hdmi_set_timings` accesses MMIO registers before the HDMI power domain is enabled.
--

commit 8b49f93cb61baf0b2ba06628a0200e4134a1eb45
Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

drm/msm/hdmi: Handle msm_hdmi_power_on() errors during .atomic_pre_enable()

This patch properly handles power up failures during .atomic_pre_enable()
by aborting early to avoid accessing registers when blocks did not bring up,
preventing imprecise aborts when the entire power domain is off.

> 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 this function access MMIO registers
before the HDMI power domain is enabled?

msm_hdmi_set_timings() writes to registers like REG_HDMI_TOTAL:

    hdmi_write(hdmi, REG_HDMI_TOTAL,
        HDMI_TOTAL_H_TOTAL(mode->htotal - 1) | ...

Since this happens before msm_hdmi_power_on() enables the power domain and
clocks, could this trigger an imprecise abort if HPD is not currently keeping
the power domain active (for example, when userspace forces a display
connection)?

>  
> -	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 this new early return path cause a crash later in the modeset sequence?

If msm_hdmi_power_on() fails, we return early leaving hdmi->power_on = false.
When the DRM core later calls msm_hdmi_bridge_atomic_post_disable(), it will
execute:

    msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled);
    msm_hdmi_phy_powerdown(phy);

Both of these functions access MMIO registers unconditionally. Since the
power domain was never brought up, could these unconditional MMIO accesses
trigger an imprecise abort?

> +			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/20260911-drm-msm-hdmi-fixes-before-eliza-v4-0-87bdb9f1987f@oss.qualcomm.com?part=2