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

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

On Thu Jul 16, 2026 at 5:27 PM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Low] Missing `inline` keyword on `to_aspeed_gfx` in a header file.
>
> Pre-existing issues:
> - [Critical] Direct dereference of `obj->state` inside atomic helpers leads to potential use-after-free and NULL pointer dereferences.
> --
>
> --- Patch [2]: [PATCH v2 2/8] drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers ---
> (Note: The format of this report is altered to a free-form summary due to
> automated recitation restrictions. Code snippets are not quoted directly.)
>
> commit d1e92d6b727d3655d12a72430c723784264108f5
> Author: Ze Huang <ze.huang@oss.qualcomm.com>
> Subject: drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers
>
> This commit replaces the simple display pipe with explicit plane, CRTC, and
> encoder objects in the aspeed DRM driver. It moves callbacks to their
> respective atomic helpers and shifts page-flip event handling to the CRTC
> atomic flush path.
>
> Issue 1: Missing inline keyword
> File: drivers/gpu/drm/aspeed/aspeed_gfx.h
> Function: to_aspeed_gfx
>
> [Severity: Low]
> Does this function need the inline keyword? The to_aspeed_gfx function is
> defined as static in a header file without being marked inline. Could this
> cause duplicate unreferenced copies of the function to be emitted in every
> translation unit that includes the header, potentially leading to binary
> bloat or compiler warnings when compiled with -Werror?

Will do, here and other places in series

>
> Issue 2: Direct state dereference in atomic context
> File: drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> Functions: aspeed_gfx_set_pixel_fmt and aspeed_gfx_crtc_mode_set_nofb
>
> [Severity: Critical]
> This is a pre-existing issue, but is it safe to directly dereference the
> object state pointers here? In aspeed_gfx_set_pixel_fmt, the code directly
> accesses priv->plane.state->fb->format->format. Similarly, in
> aspeed_gfx_crtc_mode_set_nofb, it directly accesses
> priv->crtc.state->adjusted_mode.
>
> Since this is accessed during atomic commits, if a second non-blocking
> commit disables the plane before the first commit reaches its atomic enable
> execution, the plane state could be updated to a new state with a NULL
> framebuffer. When the first commit's worker eventually reads the state,
> could this lead to a NULL pointer dereference and a kernel crash? Would it
> be safer to pass the specific plane and CRTC states down from the atomic
> helpers instead of reading them directly from the objects?

As noted in my reply to patch 5, I'll access the state with atomic helpers
instead.