Re: [PATCH v2 37/37] drm/bridge: Remove legacy bridge callback support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unconditional NULL pointer dereference for bridges missing atomic state callbacks.

Pre-existing issues:
- [High] Lockless modifications of `encoder->bridge_chain` and `dev->mode_config.privobj_list` cause list corruption.
--

--- Patch [37]: [PATCH v2 37/37] drm/bridge: Remove legacy bridge callback support ---
commit 7f35866581b3c5c247d4a1cdd1535127f3999993
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: Remove legacy bridge callback support

This commit removes legacy bridge callback support, as all bridge drivers
are expected to have been converted to atomic variants. The private object
state is now initialized unconditionally at attach time.

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 6ead9b75ae3de..6fb443984dff3 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -629,9 +624,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  			goto err_reset_bridge;
>  	}
>  
> -	if (drm_bridge_is_atomic(bridge))
> -		drm_atomic_private_obj_init(bridge->dev, &bridge->base,
> -					    &drm_bridge_priv_state_funcs);
> +	drm_atomic_private_obj_init(bridge->dev, &bridge->base,
> +				    &drm_bridge_priv_state_funcs);

[Severity: High]
Does this unconditionally crash for bridges that don't implement atomic state
callbacks?

If a bridge (like aspire_ec_bridge_funcs in
drivers/platform/arm64/acer-aspire1-ec.c) is missing .atomic_create_state,
drm_atomic_private_obj_init() will call drm_bridge_atomic_create_priv_state()
which blindly executes bridge->funcs->atomic_create_state(bridge).

Could this result in a NULL pointer dereference during initialization?

>  	return 0;

[ ... ]

> @@ -664,8 +658,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (WARN_ON(!bridge->dev))
>  		return;
>  
> -	if (drm_bridge_is_atomic(bridge))
> -		drm_atomic_private_obj_fini(&bridge->base);
> +	drm_atomic_private_obj_fini(&bridge->base);

[Severity: High]
This is a pre-existing issue, but does drm_bridge_detach() remove the bridge
from the encoder's bridge chain list locklessly?

Looking at drm_bridge_detach() just below this block:

	list_del(&bridge->chain_node);

It performs this deletion without acquiring encoder->bridge_chain_mutex.
Could this race with list iterations like drm_atomic_bridge_chain_disable()
that do hold the mutex?

Additionally, drm_atomic_private_obj_fini() modifies
dev->mode_config.privobj_list without synchronization. Can this cause list
corruption if a bridge driver is unbound via sysfs during active modesets?

>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-drm-all-atomic-bridges-v2-0-21d03cbca446@kernel.org?part=37