Re: [PATCH v2 1/8] drm/arcpgu: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

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 <ze.huang@oss.qualcomm.com>

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-1133a8fc3785@oss.qualcomm.com?part=1