Re: [PATCH RFC/DO NOT MERGE 07/12] drm/msm/hdmi: Split PHY init from power up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The HDMI pixel clock is disabled before the 20ms vblank delay, breaking the intended shutdown sequence and risking hardware hangs.
- [Medium] Unconditional clock disable and PHY deinitialization can cause unbalanced clock teardown or CCF warnings.

Pre-existing issues:
- [High] Hardware registers are accessed after dropping the runtime PM reference.
- [High] Missing return value checks for hardware initialization functions, such as `pm_runtime_resume_and_get()` and `msm_hdmi_phy_resource_enable()`.
--

commit 821f230494b55344c7aaf6bb73ba3e416ff8cb25
Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Subject: drm/msm/hdmi: Split PHY init from power up

This commit reworks and splits the msm_hdmi_power_on() sequence into
smaller steps. It separates out runtime PM enablement, introduces a new
msm_hdmi_phy_init() step, and splits clock preparation and reparenting into
msm_hdmi_clk_prepare() which runs after PHY initialization.

> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 9d027f96ae243..ead6ba3b8192a 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -15,12 +15,18 @@
>  
>  static void msm_hdmi_power_on(struct drm_bridge *bridge)
>  {
> -	struct drm_device *dev = bridge->dev;
>  	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>  	struct hdmi *hdmi = hdmi_bridge->hdmi;
> -	int ret;
>  
>  	pm_runtime_resume_and_get(&hdmi->pdev->dev);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
pm_runtime_resume_and_get() here?

If it fails, the power domain is not enabled and the PM usage count is not
incremented. Proceeding to access hardware registers will cause an SError
or panic, and the subsequent pm_runtime_put() in the disable path will
underflow the usage count.

> +}

[ ... ]

> @@ -341,6 +359,8 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>  	msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled);
>  
>  	msm_hdmi_phy_powerdown(phy);
> +	msm_hdmi_clk_unprepare(bridge);
> +	msm_hdmi_phy_deinit(phy);
>  
>  	if (hdmi->power_on) {

[Severity: Medium]
Can this unconditional clock disable and PHY deinitialization cause
unbalanced clock teardown or Common Clock Framework warnings?

If atomic_post_disable() is called to sanitize the hardware during initial
modeset (where hdmi->power_on is false), msm_hdmi_clk_unprepare() will be
called without a matching prior enable. This bypasses the state tracking
guard and triggers a WARN in the CCF.

[Severity: High]
Does this code disable the HDMI pixel clock before the 20ms vblank delay
inside power_off()?

Because msm_hdmi_clk_unprepare() is called before power_off(), the
pipeline is frozen mid-frame before the delay executes. Powering down a
frozen display controller mid-frame frequently causes system-wide bus hangs
(AXI bus stalls) on ARM SoCs.

>  		power_off(bridge);

[Severity: High]
This is a pre-existing issue, but can the code following power_off() access
hardware registers after the runtime PM reference has been dropped?

The power_off() function drops the runtime PM reference via
pm_runtime_put(). Immediately after, msm_hdmi_bridge_atomic_post_disable()
calls msm_hdmi_audio_update() if the display info is HDMI.

This accesses MMIO registers via hdmi_read() and hdmi_write(). It risks an
asynchronous SError (kernel panic) if the power domain is turned off in the
background before the accesses complete.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-drm-msm-hdmi-eliza-v1-0-67843277de17@oss.qualcomm.com?part=7