On Thu Jul 16, 2026 at 5:15 PM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
> - [High] Use-After-Free in debugfs when reading CRTC state without a lock
> - [High] Unchecked `encoder_node` passed to `of_drm_find_and_get_bridge()`
> - [Medium] Dynamic pixel format changes will not be programmed into the
> hardware
> --
>
> --- Patch [1]: [PATCH v2 1/8] drm/arcpgu: replace struct
> drm_simple_display_pipe with regular atomic helpers ---
> commit 053328e64a955beacd393bc82052adf8a231ec8a
> Author: Ze Huang <[email protected]>
>
> drm/arcpgu: replace struct drm_simple_display_pipe with regular atomic helpers
>
> This replaces the deprecated simple-KMS display pipe interface with regular
> atomic helpers. It instantiates the plane, CRTC, and encoder directly.
>
>> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
>> index c93d61ac0bb75..e0cce4798f3ed 100644
>> --- a/drivers/gpu/drm/tiny/arcpgu.c
>> +++ b/drivers/gpu/drm/tiny/arcpgu.c
>> [ ... ]
>> @@ -218,35 +223,95 @@ static void arc_pgu_disable(struct
>> drm_simple_display_pipe *pipe)
>> [ ... ]
>> +static int arcpgu_plane_helper_atomic_check(struct drm_plane *plane,
>> + struct drm_atomic_commit *commit)
>> +{
>> + struct drm_plane_state *plane_state =
>> drm_atomic_get_new_plane_state(commit, plane);
>> + struct drm_crtc *crtc = plane_state->crtc;
>> + struct drm_crtc_state *crtc_state = NULL;
>> +
>> + if (crtc)
>> + crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
>> +
>> + return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>> + DRM_PLANE_NO_SCALING,
>> + DRM_PLANE_NO_SCALING,
>> + false, false);
>> +}
>
> [Severity: Medium]
> Will dynamic pixel format changes be programmed into the hardware with this
> check?
>
> When a DRM client issues an atomic commit that only changes the framebuffer's
> pixel format without changing the display mode, this function delegates to
> drm_atomic_helper_check_plane_state() but doesn't check if fb->format
> changed.
>
> Because crtc_state->mode_changed isn't set to true on format changes, and
> the hardware's pixel format register is only updated in arc_pgu_mode_set()
> during atomic_enable(), does this mean the hardware might continue using the
> old format and cause display corruption? The previous simple-kms helper
> handled this by forcing a modeset when the format changed.
This does not appear to be introduced by this conversion. I'd like to
keep the old behavior.
>
>> [ ... ]
>> @@ -285,6 +353,30 @@ static int arcpgu_load(struct arcpgu_drm_private
>> *arcpgu)
>> [ ... ]
>> /*
>> * There is only one output port inside each device. It is linked with
>> * encoder endpoint.
>> @@ -293,29 +385,24 @@ static int arcpgu_load(struct arcpgu_drm_private
>> *arcpgu)
>> if (endpoint_node) {
>> encoder_node = of_graph_get_remote_port_parent(endpoint_node);
>> of_node_put(endpoint_node);
>> - } else {
>> - connector = &arcpgu->sim_conn;
>> - dev_info(drm->dev, "no encoder found. Assumed virtual LCD on
>> simulation platform\n");
>> - ret = arcpgu_drm_sim_init(drm, connector);
>> - if (ret < 0)
>> - return ret;
>> - }
>>
>> - ret = drm_simple_display_pipe_init(drm, &arcpgu->pipe,
>> &arc_pgu_pipe_funcs,
>> - arc_pgu_supported_formats,
>> -
>> ARRAY_SIZE(arc_pgu_supported_formats),
>> - NULL, connector);
>> - if (ret)
>> - return ret;
>> -
>> - if (encoder_node) {
>> /* Locate drm bridge from the hdmi encoder DT node */
>> struct drm_bridge *bridge __free(drm_bridge_put) =
>> of_drm_find_and_get_bridge(encoder_node);
>
> [Severity: High]
> Could this lead to attaching an unrelated bridge or indefinitely deferring
> the probe?
>
> In arcpgu_load(), if endpoint_node has no remote parent, encoder_node will
> be NULL. In the previous code, the bridge lookup was guarded by an explicit
> if (encoder_node) check. Now, of_drm_find_and_get_bridge(NULL) is called
> unconditionally.
Yes, it is possible
>
> If a NULL node is passed, the bridge lookup iterates through registered
> bridges and might return the first bridge with a NULL of_node, incorrectly
> attaching it. If none are found, it fails with -EPROBE_DEFER, which might
> leave the system without a working display. Should this retain the
> encoder_node check?
>
I'll check the encoder_node after of parent get
encoder_node = of_graph_get_remote_port_parent(endpoint_node);
+ if (!encoder_node)
+ return -ENODEV;
of_node_put(endpoint_node);
>> if (!bridge)
>> return -EPROBE_DEFER;
>
>> [ ... ]
>> @@ -342,7 +429,7 @@ static int arcpgu_show_pxlclock(struct seq_file *m, void
>> *arg)
>> struct drm_device *drm = node->minor->dev;
>> struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(drm);
>> unsigned long clkrate = clk_get_rate(arcpgu->clk);
>> - unsigned long mode_clock = arcpgu->pipe.crtc.mode.crtc_clock * 1000;
>> + unsigned long mode_clock = arcpgu->crtc.state->mode.crtc_clock * 1000;
>
> [Severity: High]
> Does this introduce a use-after-free risk when reading the CRTC state?
>
> In arcpgu_show_pxlclock(), the new code accesses arcpgu->crtc.state without
> holding the crtc->mutex lock.
>
> If a user reads the pxlclock debugfs file concurrently with a display mode
> setting operation completing, drm_atomic_helper_swap_state() might swap out
> the old CRTC state and free it asynchronously. Dereferencing this dynamically
> allocated state pointer without synchronization could lead to a kernel oops.
>
> The previous simple-KMS code avoided this by reading pipe.crtc.mode, which
> is embedded in the CRTC struct itself and updated safely for legacy access.
>
I'll lock it first
ret = drm_modeset_lock_single_interruptible(&arcpgu->crtc.mutex);
if (ret)
return ret;
mode_clock = arcpgu->crtc.state->mode.crtc_clock * 1000;
drm_modeset_unlock(&arcpgu->crtc.mutex);
>> seq_printf(m, "hw : %lu\n", clkrate);
>> seq_printf(m, "mode: %lu\n", mode_clock);
