Re: [PATCH v5] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing primary plane visibility check allows CRTC to be enabled with disabled plane, causing DMA scanout from physical address 0.
--

commit 0780d3c6b6d7797f539151d0b77405608d2d3ea1
Author: Davide Bonatto <bonatto.davide@gmail.com>
Subject: drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers

The drm_simple_display_pipe helper is considered legacy/deprecated in
favor of explicitly managing individual plane, CRTC, and encoder objects
using standard atomic helpers. Migrate arcpgu to explicitly route mode
validation and atomic hooks through standard helper functions.

> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index c93d61ac0bb75..c6308fee568db 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
[ ... ]
> @@ -189,54 +203,127 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
[ ... ]
> +static int arc_pgu_plane_atomic_check(struct drm_plane *plane,
> +				      struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc_state *crtc_state;
> +
> +	if (!new_plane_state->crtc)
> +		return 0;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
> +	if (!crtc_state)
> +		return -EINVAL;
> +
> +	return drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
> +						   DRM_PLANE_NO_SCALING,
> +						   DRM_PLANE_NO_SCALING,
> +						   false, true);

[Severity: High]
Does this code allow the CRTC to remain enabled without a visible primary
plane, potentially causing the DMA controller to scan from physical address 0?

When drm_atomic_helper_check_plane_state() runs, it returns 0 (success)
if the plane is not visible (e.g., when the framebuffer is NULL).

If a userspace application issues an atomic commit that sets the primary
plane's FB_ID to 0 without also disabling the CRTC, the atomic core sets
new_plane_state->fb = NULL and new_plane_state->visible = false.

Could we add a check here verifying that the primary plane is visible when
the CRTC is enabled (e.g., if (crtc_state->enable && !new_plane_state->visible)
return -EINVAL;) to prevent scanning from address 0?

> +}
> +
> +static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> +				     struct drm_atomic_commit *state)
> +{
> +	struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +	if (!crct_state->enable)
> +		return 0;
> +
> +	if (!crct_state->plane_mask)
> +		return -EINVAL;

[Severity: High]
Is this check sufficient to guarantee that the primary plane has a valid
framebuffer?

Setting the primary plane's FB_ID to 0 does not change the plane's crtc
mapping, meaning crct_state->plane_mask remains non-zero. This check will
incorrectly approve the commit, allowing it to proceed.

The commit would then invoke arc_pgu_plane_atomic_disable() which writes 0 to
ARCPGU_REG_BUF0_ADDR. Since the CRTC remains enabled, the hardware DMA
controller could begin scanning out pixels from physical address 0, causing bus
errors, IOMMU faults, or memory corruption.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909125806.174851-1-bonatto.davide@gmail.com?part=1