Re: [PATCH 2/9] drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless access to global atomic state pointers in commit path causes TOCTOU races and potential NULL pointer dereferences.
- [High] CRTC vblank events are prematurely consumed in the plane's atomic_update callback.
--

commit 7172c80142608f5bf7d738f64624b3135c44fdba
Author: Ze Huang <ze.huang@oss.qualcomm.com>
Subject: drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers

This patch replaces the simple display pipe in the aspeed DRM driver with
explicit plane, CRTC, and encoder objects. It removes the intermediate layer
and wires up the standard atomic helpers for modesetting and page flips.

> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> index 7877a57b8e265..3294795c31c4c 100644
> --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c

[ ... ]

>  static int aspeed_gfx_set_pixel_fmt(struct aspeed_gfx *priv, u32 *bpp)
>  {
> -	struct drm_crtc *crtc = &priv->pipe.crtc;
> +	struct drm_crtc *crtc = &priv->crtc;
>  	struct drm_device *drm = crtc->dev;
>  	const u32 format = crtc->primary->state->fb->format->format;
>  	u32 ctrl1;

[Severity: High]
This is a pre-existing issue, but does this lockless access to the global
state pointers in aspeed_gfx_set_pixel_fmt risk a NULL pointer dereference or
TOCTOU race?

If a concurrent non-blocking atomic commit disables the primary plane while
this runs in a worker thread, the global state could be swapped, potentially
making the framebuffer pointer NULL.

Would it be safer to extract the commit-specific state using the atomic
helpers instead of reading directly from crtc->primary->state->fb?

[ ... ]

>  static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
>  {
> -	struct drm_display_mode *m = &priv->pipe.crtc.state->adjusted_mode;
> +	struct drm_display_mode *m = &priv->crtc.state->adjusted_mode;
>  	u32 ctrl1, d_offset, t_count, bpp;
>  	int err;

[Severity: High]
This is also a pre-existing issue carried over during the conversion, but can
reading directly from the global crtc.state cause a similar TOCTOU race?

Reading the adjusted_mode directly from the global state pointer bypasses the
atomic commit's local state. Could this function be updated to use the local
state passed to the atomic hooks?

[ ... ]

> -static void aspeed_gfx_pipe_update(struct drm_simple_display_pipe *pipe,
> -				   struct drm_plane_state *plane_state)
> +static void aspeed_gfx_plane_helper_atomic_update(struct drm_plane *plane,
> +						  struct drm_atomic_commit *state)
>  {
> -	struct aspeed_gfx *priv = drm_pipe_to_aspeed_gfx(pipe);
> -	struct drm_crtc *crtc = &pipe->crtc;
> -	struct drm_framebuffer *fb = pipe->plane.state->fb;
> +	struct aspeed_gfx *priv = container_of(plane, struct aspeed_gfx, plane);
> +	struct drm_crtc *crtc = &priv->crtc;
> +	struct drm_framebuffer *fb = plane->state->fb;
>  	struct drm_pending_vblank_event *event;
>  	struct drm_gem_dma_object *gem;

[Severity: High]
This is a pre-existing issue, but does consuming the vblank event directly in
the plane update phase risk premature signaling?

Looking at aspeed_gfx_plane_helper_atomic_update, the driver steals the event
from the global state (crtc->state->event) before the CRTC is fully enabled
and programmed. 

Could userspace prematurely free or overwrite the framebuffer while the
hardware is still scanning it out due to this early signaling? 

Would it be more appropriate to handle the event in an atomic_flush hook
where standard atomic flush mechanisms can ensure hardware changes are fully
complete?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com?part=2