drivers/gpu/drm/gud/gud_drv.c | 9 ++++- drivers/gpu/drm/gud/gud_internal.h | 4 +++ drivers/gpu/drm/gud/gud_pipe.c | 54 ++++++++++++++++++++---------- 3 files changed, 48 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>
---
v5: Send SET_CONTROLLER_ENABLE and SET_STATE_COMMIT unconditionally on
crtc enable
v4: Send SET_DISPLAY_ENABLE=1 unconditionally on crtc enable
v3: Dropped stable AUTOSEL opt out
v2: Moved controller and display control commands to crtc
enable / disable functions.
[v4]: https://lore.kernel.org/lkml/20260218054711.63982-1-me@shenghaoyang.info/
[v3]: https://lore.kernel.org/lkml/20260203172630.10077-1-me@shenghaoyang.info/
[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 | 54 ++++++++++++++++++++----------
3 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index d0122d477610..17c2dead2c13 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..587d6dd2b32e 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -580,6 +580,39 @@ 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;
+
+ 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, 1);
+
+ 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);
+ 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 +640,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 +653,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.53.0Hi
Am 22.02.26 um 06:45 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>
AFAICT this looks good.
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Best regards
Thomas
> ---
> v5: Send SET_CONTROLLER_ENABLE and SET_STATE_COMMIT unconditionally on
> crtc enable
> v4: Send SET_DISPLAY_ENABLE=1 unconditionally on crtc enable
> v3: Dropped stable AUTOSEL opt out
> v2: Moved controller and display control commands to crtc
> enable / disable functions.
>
> [v4]: https://lore.kernel.org/lkml/20260218054711.63982-1-me@shenghaoyang.info/
> [v3]: https://lore.kernel.org/lkml/20260203172630.10077-1-me@shenghaoyang.info/
> [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 | 54 ++++++++++++++++++++----------
> 3 files changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> index d0122d477610..17c2dead2c13 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..587d6dd2b32e 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -580,6 +580,39 @@ 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;
> +
> + 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, 1);
> +
> + 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);
> + 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 +640,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 +653,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)
On Wed, 2026-02-25 at 09:52 +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 22.02.26 um 06:45 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>
>
> AFAICT this looks good.
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Ruben Wauters <rubenru09@aol.com>
This will likely require a CC stable for 6.19 at least, possibly also
6.18, though I'm not 100% sure how long that one will last with 7.0-rc1
being released.
I can add this when I merge it.
>
> Best regards
> Thomas
>
> > ---
> > v5: Send SET_CONTROLLER_ENABLE and SET_STATE_COMMIT unconditionally on
> > crtc enable
> > v4: Send SET_DISPLAY_ENABLE=1 unconditionally on crtc enable
> > v3: Dropped stable AUTOSEL opt out
> > v2: Moved controller and display control commands to crtc
> > enable / disable functions.
> >
> > [v4]: https://lore.kernel.org/lkml/20260218054711.63982-1-me@shenghaoyang.info/
> > [v3]: https://lore.kernel.org/lkml/20260203172630.10077-1-me@shenghaoyang.info/
> > [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 | 54 ++++++++++++++++++++----------
> > 3 files changed, 48 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> > index d0122d477610..17c2dead2c13 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..587d6dd2b32e 100644
> > --- a/drivers/gpu/drm/gud/gud_pipe.c
> > +++ b/drivers/gpu/drm/gud/gud_pipe.c
> > @@ -580,6 +580,39 @@ 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;
> > +
> > + 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, 1);
> > +
> > + 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);
> > + 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 +640,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 +653,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);
> > }
On Wed, 2026-02-25 at 11:52 +0000, Ruben Wauters wrote:
> On Wed, 2026-02-25 at 09:52 +0100, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 22.02.26 um 06:45 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>
> >
> > AFAICT this looks good.
> >
> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>
> Acked-by: Ruben Wauters <rubenru09@aol.com>
>
> This will likely require a CC stable for 6.19 at least, possibly also
> 6.18, though I'm not 100% sure how long that one will last with 7.0-rc1
> being released.
Just as a note, it seems that 6.18 has been designated a longterm
support release, and as such it would be a good idea/appropriate to
also backport this to 6.18, since the bug exists in that version too.
Shenghao, Would you prefer for me to backport this on merge when it
fails to apply it, or would you like me to add the cc stable tags, and
on failure, for you to backport it yourself?
Ruben
>
> I can add this when I merge it.
> >
> > Best regards
> > Thomas
> >
> > > ---
> > > v5: Send SET_CONTROLLER_ENABLE and SET_STATE_COMMIT unconditionally on
> > > crtc enable
> > > v4: Send SET_DISPLAY_ENABLE=1 unconditionally on crtc enable
> > > v3: Dropped stable AUTOSEL opt out
> > > v2: Moved controller and display control commands to crtc
> > > enable / disable functions.
> > >
> > > [v4]: https://lore.kernel.org/lkml/20260218054711.63982-1-me@shenghaoyang.info/
> > > [v3]: https://lore.kernel.org/lkml/20260203172630.10077-1-me@shenghaoyang.info/
> > > [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 | 54 ++++++++++++++++++++----------
> > > 3 files changed, 48 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> > > index d0122d477610..17c2dead2c13 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..587d6dd2b32e 100644
> > > --- a/drivers/gpu/drm/gud/gud_pipe.c
> > > +++ b/drivers/gpu/drm/gud/gud_pipe.c
> > > @@ -580,6 +580,39 @@ 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;
> > > +
> > > + 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, 1);
> > > +
> > > + 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);
> > > + 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 +640,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 +653,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);
> > > }
Hi Ruben,
> Just as a note, it seems that 6.18 has been designated a longterm
> support release, and as such it would be a good idea/appropriate to
> also backport this to 6.18, since the bug exists in that version too.
>
> Shenghao, Would you prefer for me to backport this on merge when it
> fails to apply it, or would you like me to add the cc stable tags, and
> on failure, for you to backport it yourself?
I'm good with either! Maybe we go with what's more convenient for you?
The backport resolution (I think) should just be a single line move:
+ drm->mode_config.helper_private = &gud_mode_config_helpers;
Thanks,
Shenghao
On 4/3/26 21:53, Ruben Wauters wrote:
> On Wed, 2026-02-25 at 11:52 +0000, Ruben Wauters wrote:
>> On Wed, 2026-02-25 at 09:52 +0100, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 22.02.26 um 06:45 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>
>>>
>>> AFAICT this looks good.
>>>
>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> Acked-by: Ruben Wauters <rubenru09@aol.com>
>>
>> This will likely require a CC stable for 6.19 at least, possibly also
>> 6.18, though I'm not 100% sure how long that one will last with 7.0-rc1
>> being released.
>
> Just as a note, it seems that 6.18 has been designated a longterm
> support release, and as such it would be a good idea/appropriate to
> also backport this to 6.18, since the bug exists in that version too.
>
> Shenghao, Would you prefer for me to backport this on merge when it
> fails to apply it, or would you like me to add the cc stable tags, and
> on failure, for you to backport it yourself?
>
> Ruben
>>
>> I can add this when I merge it.
>>>
>>> Best regards
>>> Thomas
>>>
>>>> ---
>>>> v5: Send SET_CONTROLLER_ENABLE and SET_STATE_COMMIT unconditionally on
>>>> crtc enable
>>>> v4: Send SET_DISPLAY_ENABLE=1 unconditionally on crtc enable
>>>> v3: Dropped stable AUTOSEL opt out
>>>> v2: Moved controller and display control commands to crtc
>>>> enable / disable functions.
>>>>
>>>> [v4]: https://lore.kernel.org/lkml/20260218054711.63982-1-me@shenghaoyang.info/
>>>> [v3]: https://lore.kernel.org/lkml/20260203172630.10077-1-me@shenghaoyang.info/
>>>> [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 | 54 ++++++++++++++++++++----------
>>>> 3 files changed, 48 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
>>>> index d0122d477610..17c2dead2c13 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..587d6dd2b32e 100644
>>>> --- a/drivers/gpu/drm/gud/gud_pipe.c
>>>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
>>>> @@ -580,6 +580,39 @@ 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;
>>>> +
>>>> + 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, 1);
>>>> +
>>>> + 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);
>>>> + 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 +640,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 +653,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);
>>>> }
On Sun, 2026-03-08 at 01:37 +0800, Shenghao Yang wrote:
> Hi Ruben,
>
> > Just as a note, it seems that 6.18 has been designated a longterm
> > support release, and as such it would be a good idea/appropriate to
> > also backport this to 6.18, since the bug exists in that version too.
> >
> > Shenghao, Would you prefer for me to backport this on merge when it
> > fails to apply it, or would you like me to add the cc stable tags, and
> > on failure, for you to backport it yourself?
>
> I'm good with either! Maybe we go with what's more convenient for you?
> The backport resolution (I think) should just be a single line move:
>
> + drm->mode_config.helper_private = &gud_mode_config_helpers;
>
> Thanks,
>
> Shenghao
>
> On 4/3/26 21:53, Ruben Wauters wrote:
> > On Wed, 2026-02-25 at 11:52 +0000, Ruben Wauters wrote:
> > > On Wed, 2026-02-25 at 09:52 +0100, Thomas Zimmermann wrote:
> > > > Hi
> > > >
> > > > Am 22.02.26 um 06:45 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>
> > > >
> > > > AFAICT this looks good.
> > > >
> > > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> > >
> > > Acked-by: Ruben Wauters <rubenru09@aol.com>
Applied to drm-misc-fixes, thanks!
Ruben
> > >
> > > This will likely require a CC stable for 6.19 at least, possibly also
> > > 6.18, though I'm not 100% sure how long that one will last with 7.0-rc1
> > > being released.
> >
> > Just as a note, it seems that 6.18 has been designated a longterm
> > support release, and as such it would be a good idea/appropriate to
> > also backport this to 6.18, since the bug exists in that version too.
> >
> > Shenghao, Would you prefer for me to backport this on merge when it
> > fails to apply it, or would you like me to add the cc stable tags, and
> > on failure, for you to backport it yourself?
> >
> > Ruben
> > >
> > > I can add this when I merge it.
> > > >
> > > > Best regards
> > > > Thomas
> > > >
> > > > > ---
> > > > > v5: Send SET_CONTROLLER_ENABLE and SET_STATE_COMMIT unconditionally on
> > > > > crtc enable
> > > > > v4: Send SET_DISPLAY_ENABLE=1 unconditionally on crtc enable
> > > > > v3: Dropped stable AUTOSEL opt out
> > > > > v2: Moved controller and display control commands to crtc
> > > > > enable / disable functions.
> > > > >
> > > > > [v4]: https://lore.kernel.org/lkml/20260218054711.63982-1-me@shenghaoyang.info/
> > > > > [v3]: https://lore.kernel.org/lkml/20260203172630.10077-1-me@shenghaoyang.info/
> > > > > [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 | 54 ++++++++++++++++++++----------
> > > > > 3 files changed, 48 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> > > > > index d0122d477610..17c2dead2c13 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..587d6dd2b32e 100644
> > > > > --- a/drivers/gpu/drm/gud/gud_pipe.c
> > > > > +++ b/drivers/gpu/drm/gud/gud_pipe.c
> > > > > @@ -580,6 +580,39 @@ 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;
> > > > > +
> > > > > + 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, 1);
> > > > > +
> > > > > + 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);
> > > > > + 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 +640,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 +653,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);
> > > > > }
>
© 2016 - 2026 Red Hat, Inc.