On Fri, Sep 11, 2026 at 12:29:35PM +0200, Krzysztof Kozlowski wrote:
> 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.
>
> Cc: <[email protected]>
> Fixes: ba3d7bf3a7ab ("drm/msm/hdmi: Convert PHY files according to new
> design")
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 +++++++++++++++++++++---------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 157f19bd90b4..db85400e574e 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -13,14 +13,16 @@
> #include "msm_kms.h"
> #include "hdmi.h"
>
> -static void msm_hdmi_power_on(struct drm_bridge *bridge)
> +static int 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);
> + ret = pm_runtime_resume_and_get(&hdmi->pdev->dev);
> + if (ret)
> + return ret;
>
> if (hdmi->extp_clk) {
> DBG("pixclock: %lu", hdmi->pixclock);
> @@ -29,9 +31,14 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
> DRM_DEV_ERROR(dev->dev, "failed to set extp clk rate:
> %d\n", ret);
>
> ret = clk_prepare_enable(hdmi->extp_clk);
> - if (ret)
> + if (ret) {
> DRM_DEV_ERROR(dev->dev, "failed to enable extp clk:
> %d\n", ret);
> + pm_runtime_put(&hdmi->pdev->dev);
> + return ret;
> + }
> }
> +
> + return 0;
> }
>
> static void power_off(struct drm_bridge *bridge)
> @@ -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) {
Thinking about it... Why would it be power_on here? I think the if() is
redundant.
> + 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;
> + }
> }
> - mutex_unlock(&hdmi->state_mutex);
>
> if (connector->display_info.is_hdmi)
> msm_hdmi_audio_update(hdmi);
>
> --
> 2.53.0
>
--
With best wishes
Dmitry
