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/[email protected]/
> > > > > [2] https://github.com/notro/gud/wiki/GUD-Protocol#display-state
> > > > >
> > > > > Reported-by: kernel test robot <[email protected]>
> > > > > Reported-by: Dan Carpenter <[email protected]>
> > > > > Closes: https://lore.kernel.org/r/[email protected]/
> > > > > Fixes: 73cfd166e045 ("drm/gud: Replace simple display pipe with DRM
> > > > > atomic helpers")
> > > > > Signed-off-by: Shenghao Yang <[email protected]>
> > > >
> > > > AFAICT this looks good.
> > > >
> > > > Reviewed-by: Thomas Zimmermann <[email protected]>
> > >
> > > Acked-by: Ruben Wauters <[email protected]>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/[email protected]/ > > > > > [v3]: > > > > > https://lore.kernel.org/lkml/[email protected]/ > > > > > [v2]: > > > > > https://lore.kernel.org/lkml/[email protected]/ > > > > > [v1]: > > > > > https://lore.kernel.org/lkml/[email protected]/ > > > > > > > > > > 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); > > > > > } >
signature.asc
Description: This is a digitally signed message part
