Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] drm_bridge_attach() ignores the return value of drm_atomic_private_obj_init(), hiding allocation failures. - [High] cdns_mhdp_bridge_atomic_duplicate_state() triggers a fatal NULL pointer dereference if bridge state is NULL. - [High] cdns_mhdp_bridge_atomic_destroy_state() crashes when passed a NULL state during teardown. -- commit e02bc473e33cd57fec3ea4025b707696ba1bef80 Author: Maxime Ripard <[email protected]> Date: Mon Jun 8 14:35:45 2026 +0000 drm/atomic-state-helper: Rename __drm_atomic_helper_bridge_reset() __drm_atomic_helper_bridge_reset() is used to initialize a newly allocated drm_bridge_state, and is being typically called by the drm_bridge_funcs.atomic_reset implementation. Since we want to consolidate DRM objects state allocation around the atomic_create_state callback that will only allocate and initialize a new drm_bridge_state instance, we will need to call __drm_atomic_helper_bridge_reset() from both the atomic_reset and atomic_create_state hooks. To avoid any confusion, we can thus rename __drm_atomic_helper_bridge_reset() to __drm_atomic_helper_bridge_state_init(). > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > index 46779b49545bd..2e74dc33e0859 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > @@ -1929,7 +1929,7 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge) > if (!cdns_mhdp_state) > return ERR_PTR(-ENOMEM); > > - __drm_atomic_helper_bridge_reset(bridge, &cdns_mhdp_state->base); > + __drm_atomic_helper_bridge_state_init(bridge, &cdns_mhdp_state->base); [Severity: High] This isn't a bug introduced by this patch, but since we are consolidating state allocations, what happens if the state is NULL? In drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:cdns_mhdp_bridge_atomic_duplicate_state(), the function calls into the core duplicate helper without validating that the source state is non-NULL: state = kzalloc_obj(*state); if (!state) return NULL; __drm_atomic_helper_bridge_duplicate_state(bridge, &state->base); If bridge->base.state is NULL, does this cause a crash during memcpy in __drm_atomic_helper_private_obj_duplicate_state()? [Severity: High] This is also a pre-existing issue, but a similar problem seems to exist in the teardown path. In drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:cdns_mhdp_bridge_atomic_destroy_state(), the function dereferences the state parameter without verifying it is non-NULL: struct cdns_mhdp_bridge_state *cdns_mhdp_state; cdns_mhdp_state = to_cdns_mhdp_bridge_state(state); if (cdns_mhdp_state->current_mode) { Could this cause a NULL pointer dereference if atomic_destroy_state() is called after a failed initialization? > return &cdns_mhdp_state->base; > } [ ... ] > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > b/drivers/gpu/drm/drm_atomic_state_helper.c > index cc70508d4fdba..f79d259fe5506 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c [ ... ] > @@ -848,7 +848,7 @@ drm_atomic_helper_bridge_reset(struct drm_bridge *bridge) > if (!bridge_state) > return ERR_PTR(-ENOMEM); > > - __drm_atomic_helper_bridge_reset(bridge, bridge_state); > + __drm_atomic_helper_bridge_state_init(bridge, bridge_state); > return bridge_state; > } [Severity: High] This isn't a bug introduced by this patch, but I noticed an error propagation issue when reviewing the atomic state allocation lifecycle. In drivers/gpu/drm/drm_bridge.c:drm_bridge_attach(), the return value of drm_atomic_private_obj_init() is ignored: if (drm_bridge_is_atomic(bridge)) drm_atomic_private_obj_init(bridge->dev, &bridge->base, &drm_bridge_priv_state_funcs); return 0; If the initial atomic state allocation fails, drm_atomic_private_obj_init() returns -ENOMEM, but drm_bridge_attach() continues and returns 0. Does this leave the bridge successfully attached to the pipeline but with a NULL atomic state (bridge->base.state == NULL)? Could this set up the NULL pointer dereferences mentioned earlier, and should drm_bridge_attach() be updated to handle this error? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
