Re: [PATCH v2 03/78] drm/atomic-state-helper: Rename __drm_atomic_helper_bridge_reset()

[sashiko-bot@kernel.org]

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 <mripard@kernel.org>
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/20260608-drm-no-more-bridge-reset-v2-0-0a91018bf886@kernel.org?part=3