drivers/gpu/drm/gud/gud_drv.c | 9 ++++- drivers/gpu/drm/gud/gud_internal.h | 4 ++ drivers/gpu/drm/gud/gud_pipe.c | 59 +++++++++++++++++++++--------- 3 files changed, 53 insertions(+), 19 deletions(-)
gud_plane_atomic_update() currently handles both crtc state and
framebuffer updates - the complexity has led to a few accidental
NULL pointer dereferences.
Commit dc2d5ddb193e ("drm/gud: fix NULL fb and crtc dereferences
on USB disconnect") [1] fixed an earlier dereference but planes
can also be disabled in non-hotplug paths (e.g. display disables
via the desktop environment). The drm_dev_enter() call would not
cause an early return in those and subsequently oops on
dereferencing crtc:
BUG: kernel NULL pointer dereference, address: 00000000000005c8
CPU: 6 UID: 1000 PID: 3473 Comm: kwin_wayland Not tainted 6.18.2-200.vanilla.gud.fc42.x86_64 #1 PREEMPT(lazy)
RIP: 0010:gud_plane_atomic_update+0x148/0x470 [gud]
<TASK>
drm_atomic_helper_commit_planes+0x28e/0x310
drm_atomic_helper_commit_tail+0x2a/0x70
commit_tail+0xf1/0x150
drm_atomic_helper_commit+0x13c/0x180
drm_atomic_commit+0xb1/0xe0
info ? __pfx___drm_printfn_info+0x10/0x10
drm_mode_atomic_ioctl+0x70f/0x7c0
? __pfx_drm_mode_atomic_ioctl+0x10/0x10
drm_ioctl_kernel+0xae/0x100
drm_ioctl+0x2a8/0x550
? __pfx_drm_mode_atomic_ioctl+0x10/0x10
__x64_sys_ioctl+0x97/0xe0
do_syscall_64+0x7e/0x7f0
? __ct_user_enter+0x56/0xd0
? do_syscall_64+0x158/0x7f0
? __ct_user_enter+0x56/0xd0
? do_syscall_64+0x158/0x7f0
entry_SYSCALL_64_after_hwframe+0x76/0x7e
Split out crtc handling from gud_plane_atomic_update() into
atomic_enable() and atomic_disable() functions to delegate
crtc state transitioning work to the DRM helpers.
To preserve the gud state commit sequence [2], switch to
the runtime PM version of drm_atomic_helper_commit_tail() which
ensures that crtcs are enabled (hence sending the
GUD_REQ_SET_CONTROLLER_ENABLE and GUD_REQ_SET_DISPLAY_ENABLE
requests) before a framebuffer update is sent.
[1] https://lore.kernel.org/all/20251231055039.44266-1-me@shenghaoyang.info/
[2] https://github.com/notro/gud/wiki/GUD-Protocol#display-state
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202601142159.0v8ilfVs-lkp@intel.com/
Fixes: 73cfd166e045 ("drm/gud: Replace simple display pipe with DRM atomic helpers")
Signed-off-by: Shenghao Yang <me@shenghaoyang.info>
---
v3: Dropped stable AUTOSEL opt out
v2: Moved controller and display control commands to crtc
enable / disable functions.
[v2]: https://lore.kernel.org/lkml/20260201095956.21042-1-me@shenghaoyang.info/
[v1]: https://lore.kernel.org/lkml/20260118125044.54467-1-me@shenghaoyang.info/
drivers/gpu/drm/gud/gud_drv.c | 9 ++++-
drivers/gpu/drm/gud/gud_internal.h | 4 ++
drivers/gpu/drm/gud/gud_pipe.c | 59 +++++++++++++++++++++---------
3 files changed, 53 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index 42135a48d92e..a2000991ecbe 100644
--- a/drivers/gpu/drm/gud/gud_drv.c
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -339,7 +339,9 @@ static int gud_stats_debugfs(struct seq_file *m, void *data)
}
static const struct drm_crtc_helper_funcs gud_crtc_helper_funcs = {
- .atomic_check = drm_crtc_helper_atomic_check
+ .atomic_check = drm_crtc_helper_atomic_check,
+ .atomic_enable = gud_crtc_atomic_enable,
+ .atomic_disable = gud_crtc_atomic_disable,
};
static const struct drm_crtc_funcs gud_crtc_funcs = {
@@ -364,6 +366,10 @@ static const struct drm_plane_funcs gud_plane_funcs = {
DRM_GEM_SHADOW_PLANE_FUNCS,
};
+static const struct drm_mode_config_helper_funcs gud_mode_config_helpers = {
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
static const struct drm_mode_config_funcs gud_mode_config_funcs = {
.fb_create = drm_gem_fb_create_with_dirty,
.atomic_check = drm_atomic_helper_check,
@@ -499,6 +505,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
drm->mode_config.min_height = le32_to_cpu(desc.min_height);
drm->mode_config.max_height = le32_to_cpu(desc.max_height);
drm->mode_config.funcs = &gud_mode_config_funcs;
+ drm->mode_config.helper_private = &gud_mode_config_helpers;
/* Format init */
formats_dev = devm_kmalloc(dev, GUD_FORMATS_MAX_NUM, GFP_KERNEL);
diff --git a/drivers/gpu/drm/gud/gud_internal.h b/drivers/gpu/drm/gud/gud_internal.h
index d27c31648341..a5b7e53cf79c 100644
--- a/drivers/gpu/drm/gud/gud_internal.h
+++ b/drivers/gpu/drm/gud/gud_internal.h
@@ -62,6 +62,10 @@ int gud_usb_set_u8(struct gud_device *gdrm, u8 request, u8 val);
void gud_clear_damage(struct gud_device *gdrm);
void gud_flush_work(struct work_struct *work);
+void gud_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state);
+void gud_crtc_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state);
int gud_plane_atomic_check(struct drm_plane *plane,
struct drm_atomic_state *state);
void gud_plane_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 4b77be94348d..46b47452b315 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -580,6 +580,44 @@ int gud_plane_atomic_check(struct drm_plane *plane,
return ret;
}
+void gud_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+ struct drm_device *drm = crtc->dev;
+ struct gud_device *gdrm = to_gud_device(drm);
+ int idx;
+
+ if (!drm_dev_enter(drm, &idx))
+ return;
+
+ if (crtc->state->mode_changed || crtc->state->connectors_changed) {
+ gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
+ gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
+ }
+
+ gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
+
+ drm_dev_exit(idx);
+}
+
+void gud_crtc_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+ struct drm_device *drm = crtc->dev;
+ struct gud_device *gdrm = to_gud_device(drm);
+ int idx;
+
+ if (!drm_dev_enter(drm, &idx))
+ return;
+
+ gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, 0);
+
+ if (crtc->state->mode_changed || crtc->state->connectors_changed)
+ gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
+
+ drm_dev_exit(idx);
+}
+
void gud_plane_atomic_update(struct drm_plane *plane,
struct drm_atomic_state *atomic_state)
{
@@ -607,24 +645,12 @@ void gud_plane_atomic_update(struct drm_plane *plane,
mutex_unlock(&gdrm->damage_lock);
}
- if (!drm_dev_enter(drm, &idx))
+ if (!crtc || !drm_dev_enter(drm, &idx))
return;
- if (!old_state->fb)
- gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
-
- if (fb && (crtc->state->mode_changed || crtc->state->connectors_changed))
- gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
-
- if (crtc->state->active_changed)
- gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
-
- if (!fb)
- goto ctrl_disable;
-
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
if (ret)
- goto ctrl_disable;
+ goto out;
drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
drm_atomic_for_each_plane_damage(&iter, &damage)
@@ -632,9 +658,6 @@ void gud_plane_atomic_update(struct drm_plane *plane,
drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-ctrl_disable:
- if (!crtc->state->enable)
- gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
-
+out:
drm_dev_exit(idx);
}
--
2.52.0Hi,
thanks for the update.
Am 03.02.26 um 18:26 schrieb Shenghao Yang:
> gud_plane_atomic_update() currently handles both crtc state and
> framebuffer updates - the complexity has led to a few accidental
> NULL pointer dereferences.
>
> Commit dc2d5ddb193e ("drm/gud: fix NULL fb and crtc dereferences
> on USB disconnect") [1] fixed an earlier dereference but planes
> can also be disabled in non-hotplug paths (e.g. display disables
> via the desktop environment). The drm_dev_enter() call would not
> cause an early return in those and subsequently oops on
> dereferencing crtc:
>
> BUG: kernel NULL pointer dereference, address: 00000000000005c8
> CPU: 6 UID: 1000 PID: 3473 Comm: kwin_wayland Not tainted 6.18.2-200.vanilla.gud.fc42.x86_64 #1 PREEMPT(lazy)
> RIP: 0010:gud_plane_atomic_update+0x148/0x470 [gud]
> <TASK>
> drm_atomic_helper_commit_planes+0x28e/0x310
> drm_atomic_helper_commit_tail+0x2a/0x70
> commit_tail+0xf1/0x150
> drm_atomic_helper_commit+0x13c/0x180
> drm_atomic_commit+0xb1/0xe0
> info ? __pfx___drm_printfn_info+0x10/0x10
> drm_mode_atomic_ioctl+0x70f/0x7c0
> ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> drm_ioctl_kernel+0xae/0x100
> drm_ioctl+0x2a8/0x550
> ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> __x64_sys_ioctl+0x97/0xe0
> do_syscall_64+0x7e/0x7f0
> ? __ct_user_enter+0x56/0xd0
> ? do_syscall_64+0x158/0x7f0
> ? __ct_user_enter+0x56/0xd0
> ? do_syscall_64+0x158/0x7f0
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Split out crtc handling from gud_plane_atomic_update() into
> atomic_enable() and atomic_disable() functions to delegate
> crtc state transitioning work to the DRM helpers.
>
> To preserve the gud state commit sequence [2], switch to
> the runtime PM version of drm_atomic_helper_commit_tail() which
> ensures that crtcs are enabled (hence sending the
> GUD_REQ_SET_CONTROLLER_ENABLE and GUD_REQ_SET_DISPLAY_ENABLE
> requests) before a framebuffer update is sent.
>
> [1] https://lore.kernel.org/all/20251231055039.44266-1-me@shenghaoyang.info/
> [2] https://github.com/notro/gud/wiki/GUD-Protocol#display-state
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202601142159.0v8ilfVs-lkp@intel.com/
> Fixes: 73cfd166e045 ("drm/gud: Replace simple display pipe with DRM atomic helpers")
> Signed-off-by: Shenghao Yang <me@shenghaoyang.info>
> ---
> v3: Dropped stable AUTOSEL opt out
> v2: Moved controller and display control commands to crtc
> enable / disable functions.
>
> [v2]: https://lore.kernel.org/lkml/20260201095956.21042-1-me@shenghaoyang.info/
> [v1]: https://lore.kernel.org/lkml/20260118125044.54467-1-me@shenghaoyang.info/
>
> drivers/gpu/drm/gud/gud_drv.c | 9 ++++-
> drivers/gpu/drm/gud/gud_internal.h | 4 ++
> drivers/gpu/drm/gud/gud_pipe.c | 59 +++++++++++++++++++++---------
> 3 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> index 42135a48d92e..a2000991ecbe 100644
> --- a/drivers/gpu/drm/gud/gud_drv.c
> +++ b/drivers/gpu/drm/gud/gud_drv.c
> @@ -339,7 +339,9 @@ static int gud_stats_debugfs(struct seq_file *m, void *data)
> }
>
> static const struct drm_crtc_helper_funcs gud_crtc_helper_funcs = {
> - .atomic_check = drm_crtc_helper_atomic_check
> + .atomic_check = drm_crtc_helper_atomic_check,
> + .atomic_enable = gud_crtc_atomic_enable,
> + .atomic_disable = gud_crtc_atomic_disable,
> };
>
> static const struct drm_crtc_funcs gud_crtc_funcs = {
> @@ -364,6 +366,10 @@ static const struct drm_plane_funcs gud_plane_funcs = {
> DRM_GEM_SHADOW_PLANE_FUNCS,
> };
>
> +static const struct drm_mode_config_helper_funcs gud_mode_config_helpers = {
> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +};
> +
> static const struct drm_mode_config_funcs gud_mode_config_funcs = {
> .fb_create = drm_gem_fb_create_with_dirty,
> .atomic_check = drm_atomic_helper_check,
> @@ -499,6 +505,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> drm->mode_config.min_height = le32_to_cpu(desc.min_height);
> drm->mode_config.max_height = le32_to_cpu(desc.max_height);
> drm->mode_config.funcs = &gud_mode_config_funcs;
> + drm->mode_config.helper_private = &gud_mode_config_helpers;
>
> /* Format init */
> formats_dev = devm_kmalloc(dev, GUD_FORMATS_MAX_NUM, GFP_KERNEL);
> diff --git a/drivers/gpu/drm/gud/gud_internal.h b/drivers/gpu/drm/gud/gud_internal.h
> index d27c31648341..a5b7e53cf79c 100644
> --- a/drivers/gpu/drm/gud/gud_internal.h
> +++ b/drivers/gpu/drm/gud/gud_internal.h
> @@ -62,6 +62,10 @@ int gud_usb_set_u8(struct gud_device *gdrm, u8 request, u8 val);
>
> void gud_clear_damage(struct gud_device *gdrm);
> void gud_flush_work(struct work_struct *work);
> +void gud_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_state *state);
> +void gud_crtc_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_state *state);
> int gud_plane_atomic_check(struct drm_plane *plane,
> struct drm_atomic_state *state);
> void gud_plane_atomic_update(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 4b77be94348d..46b47452b315 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -580,6 +580,44 @@ int gud_plane_atomic_check(struct drm_plane *plane,
> return ret;
> }
>
> +void gud_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_state *state)
> +{
> + struct drm_device *drm = crtc->dev;
> + struct gud_device *gdrm = to_gud_device(drm);
> + int idx;
> +
> + if (!drm_dev_enter(drm, &idx))
> + return;
> +
> + if (crtc->state->mode_changed || crtc->state->connectors_changed) {
I think you can do this unconditionally. Atomic_enable is supposed to be
a full modeset and take some time. For simple pageflips, this function
doesn't run at all.
> + gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
> + gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
> + }
> +
> + gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
Why not write '1' unconditionally? IIRC the active bit is always true in
atomic_enable.
> +
> + drm_dev_exit(idx);
> +}
> +
> +void gud_crtc_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_state *state)
> +{
> + struct drm_device *drm = crtc->dev;
> + struct gud_device *gdrm = to_gud_device(drm);
> + int idx;
> +
> + if (!drm_dev_enter(drm, &idx))
> + return;
> +
> + gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, 0);
> +
> + if (crtc->state->mode_changed || crtc->state->connectors_changed)
Again, just assume this is true.
Best regards
Thomas
> + gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
> +
> + drm_dev_exit(idx);
> +}
> +
> void gud_plane_atomic_update(struct drm_plane *plane,
> struct drm_atomic_state *atomic_state)
> {
> @@ -607,24 +645,12 @@ void gud_plane_atomic_update(struct drm_plane *plane,
> mutex_unlock(&gdrm->damage_lock);
> }
>
> - if (!drm_dev_enter(drm, &idx))
> + if (!crtc || !drm_dev_enter(drm, &idx))
> return;
>
> - if (!old_state->fb)
> - gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
> -
> - if (fb && (crtc->state->mode_changed || crtc->state->connectors_changed))
> - gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
> -
> - if (crtc->state->active_changed)
> - gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
> -
> - if (!fb)
> - goto ctrl_disable;
> -
> ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> if (ret)
> - goto ctrl_disable;
> + goto out;
>
> drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
> drm_atomic_for_each_plane_damage(&iter, &damage)
> @@ -632,9 +658,6 @@ void gud_plane_atomic_update(struct drm_plane *plane,
>
> drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>
> -ctrl_disable:
> - if (!crtc->state->enable)
> - gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
> -
> +out:
> drm_dev_exit(idx);
> }
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
© 2016 - 2026 Red Hat, Inc.