Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Unconditional call to the optional `destroy_plane_state` callback causes a NULL pointer dereference if the driver does not implement it. - [Medium] The fallback priority between `create_plane_state` and `reset_plane` contradicts the intended design, prioritizing the legacy hook. --
--- Patch [1]: [PATCH 01/60] drm/simple-kms: Add create_plane_state hook --- commit 2eb693cbd25d991d0fc7bf28e45683c97579b97a Author: Maxime Ripard <[email protected]> drm/simple-kms: Add create_plane_state hook This commit introduces a create_plane_state hook to the simple-kms helper to allow drivers to allocate and initialize pristine state without side effects. It updates drm_simple_kms_plane_reset() to use this new hook while falling back to the legacy reset_plane hook if it is not provided. > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c > b/drivers/gpu/drm/drm_simple_kms_helper.c > index 8e1d07b9f1e30..efb643ab64487 100644 > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c [ ... ] > @@ -286,10 +286,29 @@ static void drm_simple_kms_plane_reset(struct drm_plane > *plane) > struct drm_simple_display_pipe *pipe; > > pipe = container_of(plane, struct drm_simple_display_pipe, plane); > - if (!pipe->funcs || !pipe->funcs->reset_plane) > + if (!pipe->funcs) > return drm_atomic_helper_plane_reset(plane); > > - return pipe->funcs->reset_plane(pipe); > + if (pipe->funcs->reset_plane) > + return pipe->funcs->reset_plane(pipe); > + > + if (pipe->funcs->create_plane_state) { [Severity: Medium] Does this evaluation order match the intended design? The commit message states the code will use create_plane_state to allocate a fresh state, falling back to reset_plane. But here, if a driver provides both during a migration, it returns early and create_plane_state will not be executed. Should create_plane_state be prioritized before checking reset_plane? > + struct drm_plane_state *state; > + > + if (plane->state) { > + pipe->funcs->destroy_plane_state(pipe, plane->state); [Severity: High] Can this cause a NULL pointer dereference? If a driver implements create_plane_state but relies on the default destruction logic (meaning pipe->funcs->destroy_plane_state is NULL), this unconditional call will panic when plane->state is already set, such as during suspend/resume or consecutive resets. Other functions in this file check for NULL and safely fall back to drm_atomic_helper_plane_destroy_state(). > + plane->state = NULL; > + } > + > + state = pipe->funcs->create_plane_state(pipe); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
