Re: [PATCH v2 2/8] 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:

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?

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=2