Re: [PATCH v7] drm/bridge: dw-hdmi-qp: serialize HDMI audio callbacks with PHY enable/disable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/bridge: dw-hdmi-qp: unbalanced PHY disable on init
  failure
--

commit 964bd3d6c9dd93511a7cc0697725f1d96d4a0a68
Author: Frank Zhang <rmxpzlb@gmail.com>

drm/bridge: dw-hdmi-qp: serialize HDMI audio callbacks with PHY enable/disable

This commit adds a mutex and an enabled flag to struct dw_hdmi_qp's phy
sub-structure to serialize HDMI audio and infoframe callbacks against PHY
initialization and disablement.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> index 62835485d66a1..ef5ccf0f609ce 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c

[ ... ]

> @@ -572,10 +597,25 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	scoped_guard(mutex, &hdmi->mutex)
>  		hdmi->curr_conn = connector;
>  
> -	hdmi->phy.ops->init(hdmi, hdmi->phy.data);
> +	ret = hdmi->phy.ops->init(hdmi, hdmi->phy.data);
> +	if (unlikely(ret)) {
> +		dev_err(hdmi->dev, "failed to initialize PHY: %d\n", ret);
> +		scoped_guard(mutex, &hdmi->mutex) {
> +			drm_connector_put(hdmi->curr_conn);
> +			hdmi->curr_conn = NULL;
> +		}
> +
> +		hdmi->phy.enabled = false;
> +		hdmi->tmds_char_rate = 0;
> +
> +		mutex_unlock(&hdmi->phy.lock);
> +		return;
> +	}
> +	hdmi->phy.enabled = true;
>  
>  	dw_hdmi_qp_mod(hdmi, HDCP2_BYPASS, HDCP2_BYPASS, HDCP2LOGIC_CONFIG0);
>  	dw_hdmi_qp_mod(hdmi, op_mode, OPMODE_DVI, LINK_CONFIG0);
> +	mutex_unlock(&hdmi->phy.lock);
>  
>  	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
>  }
> @@ -593,12 +633,14 @@ static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
>  		}
>  	}
>  
> +	guard(mutex)(&hdmi->phy.lock);
>  	hdmi->tmds_char_rate = 0;
>  
>  	connector = drm_atomic_get_old_connector_for_encoder(state, bridge->encoder);
>  	drm_connector_hdmi_disable_scrambling(connector);
>  
>  	hdmi->phy.ops->disable(hdmi, hdmi->phy.data);
> +	hdmi->phy.enabled = false;
>  }

[Severity: Medium]
If hdmi->phy.ops->init() fails in dw_hdmi_qp_bridge_atomic_enable(), can
the subsequent dw_hdmi_qp_bridge_atomic_disable() call
hdmi->phy.ops->disable() on a PHY that was never enabled?

Because atomic_enable returns void, the DRM atomic state still considers
the bridge enabled and will call dw_hdmi_qp_bridge_atomic_disable() on the
next modeset disable.  In the Rockchip glue driver, init() and disable()
wrap phy_power_on() and phy_power_off().  An unbalanced phy_power_off()
underflows phy->power_count from 0 to -1, which causes the next
phy_power_on() to increment phy->power_count back to 0 and return 0
without powering on the hardware; dw_hdmi_qp_bridge_atomic_enable() then
sets hdmi->phy.enabled = true and accesses controller registers with the
PHY off, triggering an SError panic.

In addition, the error path in dw_hdmi_qp_bridge_atomic_enable() returns
without calling drm_connector_hdmi_disable_scrambling(connector) to undo
the earlier drm_connector_hdmi_enable_scrambling() call and cancel
connector->hdmi.scdc_work.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112101155.47777-1-rmxpzlb@gmail.com?part=1