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 <[email protected]> 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/[email protected]?part=17
