Re: [RFC PATCH] drm/bridge: cdns-dsi: Propagate resume reset and clock failures

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Error returned by `cdns_dsi_resume` exposes unprotected register access and PHY teardown in `atomic_post_disable`, leading to a bus hang.
- [High] Hardware registers are accessed in `probe` before deasserting the device reset.
--

commit 4c1e6021cc122efbeb34743b0b51f25a1dc5f221
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

drm/bridge: cdns-dsi: Propagate resume reset and clock failures

cdns_dsi_resume() ignores reset_control_deassert() and
clk_prepare_enable() failures for resources that probe treats as
required.  The host can then appear resumed even though the DSI reset or
clock transitions failed.

Propagate resume failures and unwind transitions that already succeeded.

> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 0dd85e26248cc..0cbb8528350c4 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -1233,12 +1233,27 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops = {
>  static int __maybe_unused cdns_dsi_resume(struct device *dev)
>  {
>  	struct cdns_dsi *dsi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = reset_control_deassert(dsi->dsi_p_rst);

[Severity: High]
This is a pre-existing issue, but I noticed hardware registers might be
accessed in probe before deasserting the device reset:

In cdns_dsi_drm_probe():
    ...
	val = readl(dsi->regs + ID_REG);
    ...

If the bootloader or previous boot stage leaves the reset line asserted,
could attempting to read the APB register stall the bus and cause an external
abort, or return invalid data causing probe to fail?

> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(dsi->dsi_p_clk);
> +	if (ret)
> +		goto err_assert_reset;
>  
> -	reset_control_deassert(dsi->dsi_p_rst);
> -	clk_prepare_enable(dsi->dsi_p_clk);
> -	clk_prepare_enable(dsi->dsi_sys_clk);
> +	ret = clk_prepare_enable(dsi->dsi_sys_clk);
> +	if (ret)
> +		goto err_disable_p_clk;
>  
>  	return 0;
> +
> +err_disable_p_clk:
> +	clk_disable_unprepare(dsi->dsi_p_clk);
> +err_assert_reset:
> +	reset_control_assert(dsi->dsi_p_rst);
> +	return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does propagating this error
expose unprotected register access during teardown?

If cdns_dsi_resume() returns an error, pm_runtime_get_sync() in
cdns_dsi_bridge_atomic_pre_enable() will fail and return early:

In cdns_dsi_bridge_atomic_pre_enable():
    ...
	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
		return;
    ...

However, because atomic_pre_enable() returns void, the DRM core still
considers the bridge enabled. Later, during display pipeline teardown,
cdns_dsi_bridge_atomic_post_disable() is called. Will it unconditionally read
MCTL_MAIN_DATA_CTL with clocks disabled, causing a bus hang?

In cdns_dsi_bridge_atomic_post_disable():
    ...
	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
    ...

Additionally, does it blindly call phy_power_off() and phy_exit() even though
the PHY was never initialized, causing kernel warnings?

In cdns_dsi_bridge_atomic_post_disable():
    ...
	phy_power_off(dsi->dphy);
	phy_exit(dsi->dphy);
    ...

>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624055906.53195-1-pengpeng@iscas.ac.cn?part=1