Re: [PATCH 32/60] drm/sun4i: sun8i: Convert to atomic_create_state

2026-07-09 Thread sashiko-bot 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) 
to consider:

Pre-existing issues:
- [High] sun4i_crtc_init() returns NULL on plane creation failure, but the 
caller checks for errors using IS_ERR(), leading to a NULL pointer dereference.
--

--- Patch [32]: [PATCH 32/60] drm/sun4i: sun8i: Convert to atomic_create_state 
---
commit f2f31601b0b32de3a56fe2a7c233cf40904cfee8
Author: Maxime Ripard 

drm/sun4i: sun8i: Convert to atomic_create_state

This commit converts the sun8i plane reset hooks to use
atomic_create_state instead of drm_atomic_helper_plane_reset. It was
generated using a Coccinelle semantic patch.

> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c 
> b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> index 00756ab78b4eb..bad102134726b 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer 
dereference in the sun4i display driver initialization path. 

In sun4i_crtc_init(), if plane creation fails, it returns NULL:

drivers/gpu/drm/sun4i/sun4i_crtc.c:sun4i_crtc_init() {
planes = sunxi_engine_layers_init(drm, engine);
if (IS_ERR(planes)) {
dev_err(drm->dev, "Couldn't create the planes\n");
return NULL;
}
...
}

However, the caller in sun4i_tcon_bind() checks the return value using 
IS_ERR():

drivers/gpu/drm/sun4i/sun4i_tcon.c:sun4i_tcon_bind() {
...
tcon->crtc = sun4i_crtc_init(drm, engine, tcon);
if (IS_ERR(tcon->crtc)) {
dev_err(dev, "Couldn't create our CRTC\n");
...
}

Since IS_ERR(NULL) evaluates to false, the error is missed and execution
continues. This NULL pointer is later dereferenced in sun4i_rgb_init():

drivers/gpu/drm/sun4i/sun4i_rgb.c:sun4i_rgb_init() {
...
/* The RGB encoder can only work with the TCON channel 0 */
rgb->encoder.possible_crtcs = drm_crtc_mask(&tcon->crtc->crtc);
...
}

Should sun4i_crtc_init() return an ERR_PTR (like ERR_PTR(-ENOMEM)) instead
of NULL when plane creation fails, so that the caller's IS_ERR() check 
catches it?

-- 
Sashiko AI review ยท 
https://sashiko.dev/#/patchset/[email protected]?part=32

[PATCH 32/60] drm/sun4i: sun8i: Convert to atomic_create_state

2026-07-09 Thread Maxime Ripard 
The plane only initializes a pristine state in its reset hook
using drm_atomic_helper_plane_reset(), which is equivalent to what
atomic_create_state expects. Convert to it.

The conversion was done using the following Coccinelle semantic patch:

@@
identifier funcs;
symbol drm_atomic_helper_plane_reset;
symbol drm_atomic_helper_plane_create_state;
@@

struct drm_plane_funcs funcs = {
  ...,
- .reset = drm_atomic_helper_plane_reset,
+ .atomic_create_state = drm_atomic_helper_plane_create_state,
  ...,
};

@match_struct_reset@
identifier funcs, reset_func;
@@
struct drm_plane_funcs funcs = {
...,
.reset = reset_func,
...,
};

@reset_uses_helpers depends on match_struct_reset@
identifier match_struct_reset.reset_func;
@@

 void reset_func(...)
 {
<+...
(
__drm_atomic_helper_plane_reset(...);
|
__drm_gem_reset_shadow_plane(...);
)
...+>
 }

@match_struct_destroy@
identifier funcs, destroy_func;
@@
struct drm_plane_funcs funcs = {
...,
.atomic_destroy_state = destroy_func,
...,
};

@script:python renamed_func@
old_name << match_struct_reset.reset_func;
new_name;
@@
if old_name.endswith("_reset"):
coccinelle.new_name = old_name.replace("_reset", "_create_state")
else:
coccinelle.new_name = old_name

@update_struct depends on match_struct_reset && reset_uses_helpers@
identifier match_struct_reset.funcs, match_struct_reset.reset_func;
identifier renamed_func.new_name;
@@
struct drm_plane_funcs funcs = {
...,
-   .reset = reset_func,
+   .atomic_create_state = new_name,
...,
};

@drop_destroy depends on update_struct && match_struct_destroy@
identifier match_struct_reset.reset_func;
identifier match_struct_destroy.destroy_func;
identifier container_func;
identifier P;
symbol drm_atomic_helper_plane_destroy_state;
symbol __drm_atomic_helper_plane_destroy_state;
@@

 void reset_func(struct drm_plane *P)
 {
...
(
-   if (P->state) {
-   <+...
(
-   drm_atomic_helper_plane_destroy_state(P, P->state);
|
-   __drm_atomic_helper_plane_destroy_state(P->state);
|
-   P->funcs->atomic_destroy_state(P, P->state);
|
-   destroy_func(P, P->state);
)
-   ...+>
-   }
|
-   drm_WARN_ON_ONCE(P->dev, P->state);
|
-   WARN_ON(P->state);
)
...
(
-   kfree(P->state);
|
-   kfree(container_func(P->state));
|
// kfree is optional
)
(
-   P->state = NULL;
|
// plane->state clearing is optional
)
...
 }

@drop_destroy_mtk depends on update_struct@
identifier P;
symbol __drm_atomic_helper_plane_destroy_state;
symbol to_mtk_plane_state;
@@

 void mtk_plane_reset(struct drm_plane *P)
 {
...
-   if (P->state) {
-   __drm_atomic_helper_plane_destroy_state(P->state);
-   ...
-   } else {
...
-   }
...
 }

@transform_nv50_wndw depends on update_struct@
identifier S;
@@

 void nv50_wndw_reset(...)
 {
...
-   if (WARN_ON(!(S = kzalloc_obj(*S
+   S = kzalloc_obj(*S);
+   if (WARN_ON(!S))
return;
...
 }

@transform_kzalloc depends on update_struct@
identifier match_struct_reset.reset_func;
identifier P, S;
statement ST;
statement list STL;
@@

 void reset_func(struct drm_plane *P)
 {
<...
S = kzalloc_obj(*S);
(
-   if (S)
-   {
-   STL
-   }
+   if (!S) return;
+
+   STL
|
-   if (S) ST
+   if (!S) return;
+
+   ST
)
...>
 }

@transform_body depends on update_struct@
identifier match_struct_reset.reset_func;
identifier renamed_func.new_name;
identifier S, P;
expression PS;
@@
- void reset_func(struct drm_plane *P)
+ struct drm_plane_state *new_name(struct drm_plane *P)
{
...
S = kzalloc_obj(*S);
...
(
if (!S) {
...
-   return;
+   return ERR_PTR(-ENOMEM);
}
|
if (WARN_ON(!S)) {
...
-   return;
+   return ERR_PTR(-ENOMEM);
}
|
if (S == NULL) {
...
-   return;
+   return ERR_PTR(-ENOMEM);
}
)
...
(
-   __drm_atomic_helper_plane_reset(P, PS);
+   __drm_atomic_helper_plane_state_init(PS, P);
|
-   __drm_gem_reset_shadow_plane(P, PS);
+   __drm_gem_shadow_plane_state_init(P, PS);
)
...
}

@update_early_return depends on update_struct@
identifier match_struct_reset.reset_func;
identifier renamed_func.new_name;
identifier P;
expression PS;
@@
 struct drm_plane_state *new_name(struct drm_plane *P)
{
<+...
-   return;
+   return ERR_PTR(-EINVAL);
...+>
}

@update_return_plane depends on update_struct@
identifier match_struct_reset.reset_func;
identifier renamed_func.new_name;
identifier P;
expression PS;
@@
 struct drm_plane_state *new_name(struct drm_plane *P)
{
...
__drm_atomic_helper_plane_state_init(PS, P);