Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] drm/atomic: global SRO readout in commit_tail causes NULL derefs and data races - [High] drm: SRO omits drm_colorop objects breaking initialization - [High] drm: list corruption on bridge detach due to ignored init failure - [Medium] drm: drm_mode_config_create_initial_state omits encoder reset --
commit 5a139aeb014c8ea3fdfb3bca244ee293e5da8224 Author: Maxime Ripard <[email protected]> drm/atomic-helper: Document State Read-Out as a state initialization approach This commit updates the atomic state reset and initialization documentation to describe the State Read-Out (SRO) infrastructure as a solution for reconstructing software state from hardware registers. It cross-references the new SRO section and mentions drm_mode_config_create_initial_state() as a combined approach. > diff --git a/drivers/gpu/drm/drm_atomic_sro.c > b/drivers/gpu/drm/drm_atomic_sro.c > --- a/drivers/gpu/drm/drm_atomic_sro.c > +++ b/drivers/gpu/drm/drm_atomic_sro.c [ ... ] > struct drm_atomic_sro_state { > struct drm_device *dev; > struct __drm_atomic_sro_plane *planes; > struct __drm_atomic_sro_crtc *crtcs; > struct __drm_atomic_sro_connector *connectors; > struct __drm_atomic_sro_private_obj *private_objs; > unsigned int num_private_objs; > }; [Severity: High] Does this struct lack a colorops array? Since drm_colorop objects do not embed drm_private_obj, omitting them here cascades into all the readout and install functions missing colorop processing entirely. When drivers use drm_mode_config_create_initial_state() and successfully read out state via SRO, they skip drm_mode_config_reset(). This means drm_colorop_reset() is never called and colorop->state remains uninitialized (NULL). Subsequent atomic commits will call drm_atomic_get_colorop_state(), which calls drm_atomic_helper_colorop_duplicate_state(). This helper encounters WARN_ON(!colorop->state) and returns NULL, causing the atomic commit to fail consistently with -ENOMEM. > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c [ ... ] > @@ -2087,6 +2088,45 @@ static void commit_tail(struct drm_atomic_commit > *state, bool nonblock) > > drm_atomic_helper_commit_cleanup_done(state); > [ ... ] > + if (!nonblock) > + drm_atomic_sro_readout_and_compare(state); > > drm_atomic_commit_put(state); > } [Severity: Critical] Is this regression causing a NULL pointer dereference on unsupported drivers? This path directly calls funcs->atomic_sro_build_state(dev) without verifying if funcs or the function pointer is non-NULL. For supported drivers, atomic_sro_build_state reads the hardware state for all CRTCs, planes, and connectors device-wide. Because blocking commits only lock the specific objects being modified, reading unrelated objects can cause data races with concurrent non-blocking commits that are mutating them. Does this also risk MMIO hangs by accessing powered-down objects without taking appropriate power management references? > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c [ ... ] > +int drm_mode_config_create_initial_state(struct drm_device *dev) > +{ > + int ret; > + > + if (dev->mode_config.funcs->atomic_sro_readout_state) { > + ret = dev->mode_config.funcs->atomic_sro_readout_state(dev); > + if (!ret) > + return 0; > + } > + > + /* Reset the hardware first ... */ > + if (dev->mode_config.funcs->hw_reset) > + dev->mode_config.funcs->hw_reset(dev); > + > + /* ... then allocate a new pristine state */ > + return drm_mode_config_create_state(dev); > +} [Severity: Medium] Does this fallback path omit iterating over encoders to call their .reset hooks? When drm_mode_config_create_initial_state() falls back to hardware reset, hw_reset() defaults to CRTC-only resets via drm_mode_config_helper_hw_reset(), and drm_mode_config_create_state() skips encoders. Without a generic reset path, drivers relying on encoder->funcs->reset (like loongson) will fail to reset encoder registers, which is a regression that potentially causes display failures. > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c [ ... ] > @@ -649,8 +751,14 @@ int drm_bridge_attach(struct drm_encoder *encoder, > struct drm_bridge *bridge, > goto err_reset_bridge; > } > > - drm_atomic_private_obj_init(bridge->dev, &bridge->base, > - &drm_bridge_priv_state_funcs); > + if (bridge->funcs && > + bridge->funcs->atomic_sro_readout_state && > + bridge->funcs->atomic_sro_compare_state) > + drm_atomic_private_obj_init(bridge->dev, &bridge->base, > bridge->name, > + > &drm_bridge_priv_state_funcs_with_sro); > + else > + drm_atomic_private_obj_init(bridge->dev, &bridge->base, > bridge->name, > + &drm_bridge_priv_state_funcs); > > return 0; [Severity: High] Can ignoring the return value of drm_atomic_private_obj_init() here lead to a double list_del() and kernel crash upon teardown? If drm_atomic_private_obj_init() fails, it deletes and poisons the list node: drivers/gpu/drm/drm_atomic.c:drm_atomic_private_obj_init() { ... if (IS_ERR(state)) { ret = PTR_ERR(state); goto err_list_del; } ... err_list_del: list_del(&obj->head); ... } Because drm_bridge_attach() unconditionally returns 0, the bridge remains registered with a poisoned private object list head. When drm_bridge_detach() is later called, it calls drm_atomic_private_obj_fini(&bridge->base), which performs a blind second list_del() on the poisoned node. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=19
