Re: [PATCH v2 17/78] drm/bridge: imx8qxp-pixel-combiner: Switch to atomic_create_state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race on shared register `PC_CTRL_REG` during concurrent `mode_set` calls can cause hardware state corruption.
- [High] Incomplete cleanup in the probe error path leads to Use-After-Free of devm-allocated DRM bridge.
- [High] Unconditional hardware register access and unbalanced clock disable in `imx8qxp_pc_runtime_suspend()` when the clock fails to enable.
- [High] Unconditional hardware register accesses and unbalanced clock disable in `imx8qxp_pc_bridge_mode_set()` if power management or clock enablement fails.
--

commit 10038747477552d55e11391e7ea8feb1269ccfa4
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: imx8qxp-pixel-combiner: 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 counterpart.

> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> index e0ee51a9ca7f8..941ce2f91e9e5 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> @@ -251,7 +251,7 @@ imx8qxp_pc_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>  static const struct drm_bridge_funcs imx8qxp_pc_bridge_funcs = {
>  	.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,
>  	.mode_valid		= imx8qxp_pc_bridge_mode_valid,
>  	.attach			= imx8qxp_pc_bridge_attach,
>  	.mode_set		= imx8qxp_pc_bridge_mode_set,

[Severity: High]
This isn't a bug introduced by this patch, but looking at
imx8qxp_pc_bridge_mode_set(), the format configuration uses a software
read-modify-write (RMW) sequence on PC_CTRL_REG:

	val = imx8qxp_pc_read(pc, PC_CTRL_REG);
	if (ch->stream_id == 0) {
		val &= ~PC_DISP0_PIX_DATA_FORMAT_MASK;
		val |= PC_DISP0_PIX_DATA_FORMAT(RGB);
	} else {
		val &= ~PC_DISP1_PIX_DATA_FORMAT_MASK;
		val |= PC_DISP1_PIX_DATA_FORMAT(RGB);
	}
	imx8qxp_pc_write(pc, PC_CTRL_REG, val);

Because the driver supports parallel, non-blocking atomic commits, this
function can run concurrently on multiple CPUs for different channels. Does
this concurrent unprotected RMW cause one of the channel's format
configurations to be overwritten, leading to incorrect display colors or
format?

[Severity: High]
This is a pre-existing issue, but also in imx8qxp_pc_bridge_mode_set(), there
appears to be an unconditional hardware register access if power management or
clock enablement fails:

	ret = clk_prepare_enable(pc->clk_apb);
	if (ret)
		DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n",
			      __func__,  ret);

	/* HSYNC to pixel link is active low. */
	imx8qxp_pc_write_clr(pc, PC_CTRL_REG,
			     PC_DISP_HSYNC_POLARITY(ch->stream_id));

If pm_runtime_get_sync() or clk_prepare_enable() fails, the function prints
an error but continues to execute hardware register writes. On this platform,
could accessing unclocked or unpowered registers cause an external abort and
kernel panic?

Furthermore, does it lead to a clock imbalance by calling
clk_disable_unprepare() at the end when the clock failed to enable?

[Severity: High]
This isn't a bug introduced by this patch, but looking at
imx8qxp_pc_runtime_suspend(), there is a similar pattern:

	ret = clk_prepare_enable(pc->clk_apb);
	if (ret)
		DRM_DEV_ERROR(pc->dev, "%s: failed to enable apb clock: %d\n",
			      __func__,  ret);

	/* Disable pixel combiner by full reset. */
	imx8qxp_pc_write_clr(pc, PC_SW_RESET_REG, PC_FULL_RESET_N);

If clk_prepare_enable() fails, the function proceeds to access PC_SW_RESET_REG.
Does this cause a bus lockup or asynchronous external abort due to accessing
registers when their clock domain is gated?

Also, does it unconditionally call clk_disable_unprepare() later, resulting
in a clock imbalance?

[Severity: High]
This is a pre-existing issue, but the imx8qxp_pc_bridge_probe() function
seems to have a fragile error path:

	free_child:
		of_node_put(child);

		if (i == 1 && pc->ch[0] && pc->ch[0]->bridge.next_bridge)
			drm_bridge_remove(&pc->ch[0]->bridge);

It hardcodes the check to unregister only the first channel. If the second
channel lacks a valid reg property, of_property_read_u32() fails without
updating i, leaving i == 0. Will the error path then skip unregistering ch[0]?

Furthermore, if the first channel processed is reg = <1>, could an error on
the second channel (reg = <0>) fail to unregister ch[1]?

Because the bridge structures are allocated with devm_drm_bridge_alloc(),
they will be freed by devres on probe failure. Does this leave dangling
pointers in the DRM subsystem's global bridge list, causing a use-after-free
when the list is subsequently traversed?

>  	.atomic_disable		= imx8qxp_pc_bridge_atomic_disable,
>  	.atomic_get_input_bus_fmts =
>  				imx8qxp_pc_bridge_atomic_get_input_bus_fmts,
>  	.atomic_get_output_bus_fmts =
>  				imx8qxp_pc_bridge_atomic_get_output_bus_fmts,
>  };

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