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

["Ze Huang" <ze.huang@oss.qualcomm.com>]

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 <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.

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);