Re: [PATCH v2 14/78] drm/bridge: imx8mp-hdmi-pvi: Switch to atomic_create_state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbalanced pm_runtime_put and potential unclocked register access if pm_runtime_resume_and_get fails in atomic_enable.
- [High] Use-After-Free and unmapped memory access due to devm_ resource lifetime mismatch when the device is unbound.
--

commit 935043b57c7562e813344d06040c4edaf524faa0
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: imx8mp-hdmi-pvi: Switch to atomic_create_state

The drm_bridge_funcs.atomic_reset callback and its
drm_atomic_helper_bridge_reset() helper are deprecated.
Switch to the atomic_create_state callback and its
drm_atomic_helper_bridge_create_state() counterpart.

> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
> index 7d5fda7173e5f..a31d4fc76f35b 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
> @@ -131,7 +131,7 @@ static const struct drm_bridge_funcs imx_hdmi_pvi_bridge_funcs = {
>  	.atomic_get_input_bus_fmts = imx8mp_hdmi_pvi_bridge_get_input_bus_fmts,
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> -	.atomic_reset = drm_atomic_helper_bridge_reset,
> +	.atomic_create_state = drm_atomic_helper_bridge_create_state,
>  };
>  
>  static int imx8mp_hdmi_pvi_probe(struct platform_device *pdev)

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling in
imx8mp_hdmi_pvi_bridge_enable() leave the bridge in a state that could
trigger a regression later?

If pm_runtime_resume_and_get() fails in the enable path, it returns without
setting a flag:

drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:imx8mp_hdmi_pvi_bridge_enable() {
	...
	if (WARN_ON(pm_runtime_resume_and_get(pvi->dev)))
		return;
	...
}

Later, imx8mp_hdmi_pvi_bridge_disable() unconditionally writes to the registers
and decrements the usage counter:

drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:imx8mp_hdmi_pvi_bridge_disable() {
	...
	writel(0x0, pvi->regs + HTX_PVI_CTRL);

	pm_runtime_put(pvi->dev);
}

Could this unconditional write to an unclocked IP block cause a synchronous
data abort, and lead to a PM usage counter underflow?

[Severity: High]
This is a pre-existing issue, but could this resource allocation pattern
cause a regression during device unbind?

The bridge structure and I/O registers are allocated using devres in the
probe function:

drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:imx8mp_hdmi_pvi_probe() {
	...
	pvi = devm_drm_bridge_alloc(&pdev->dev, struct imx8mp_hdmi_pvi,
				    bridge, &imx_hdmi_pvi_bridge_funcs);
	...
	pvi->regs = devm_platform_ioremap_resource(pdev, 0);
	...
}

If the driver is unbound, these devm-managed resources will be freed and
unmapped. However, since drm_bridge_remove() does not synchronize with active
display pipelines, the DRM core might still hold references to the bridge.

If the pipeline is torn down after unbind, imx8mp_hdmi_pvi_bridge_disable()
would be called:

drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:imx8mp_hdmi_pvi_bridge_disable() {
	...
	writel(0x0, pvi->regs + HTX_PVI_CTRL);
	...
}

Would this result in accessing unmapped registers and the freed bridge
structure during pipeline teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-drm-no-more-bridge-reset-v2-0-0a91018bf886@kernel.org?part=14