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. > [ ... ] > @@ -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. 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? > 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. > seq_printf(m, "hw : %lu\n", clkrate); > seq_printf(m, "mode: %lu\n", mode_clock); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3...@oss.qualcomm.com?part=1
