Re: [PATCH v2 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 1 potential issue(s) to consider:
- [High] Imprecise abort (kernel panic) in .atomic_post_disable() due to unclocked MMIO access after a failure in .atomic_pre_enable().
--

commit 4f4c185244ef7ea6a900e7c373d8e29c18ab92a6
Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

    drm/msm/hdmi: Handle msm_hdmi_power_on() errors during .atomic_pre_enable()
    
    Properly handle power up failures during .atomic_pre_enable() by
    aborting the .atomic_pre_enable() to avoid accessing registers why
    blocks did not bring up.  Otherwise this could lead to imprecise aborts
    when accessing MMIO while 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);
>  
> -	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;
> +			}
> +			hdmi->power_on = true;
> +		}
>  	}

[Severity: High]
Does this early return cause an imprecise abort during atomic_post_disable()?

Since atomic_pre_enable() returns void, the DRM core will consider the state
committed even if it returns early here. This leaves hdmi->power_on = false
and the PHY clocks and PM domain powered off.

When the pipeline is later disabled, msm_hdmi_bridge_atomic_post_disable()
will execute:

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) {
    ...
}

Because msm_hdmi_set_mode() and msm_hdmi_phy_powerdown() are called
unconditionally outside the if (hdmi->power_on) block, won't they write to
MMIO registers while the power domain is off, causing an access fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-drm-msm-hdmi-fixes-before-eliza-v2-0-b097cf1eac77@oss.qualcomm.com?part=2