Re: [PATCH v8 18/39] drm/bridge: Handle bridges with hardware state readout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/bridge: missing bridge detach on init failure in
  drm_bridge_attach()
--

commit 0fb1d1b23ec990c1a040f81e5f14e9c95302ae67
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: Handle bridges with hardware state readout

This commit adds hardware state readout support for DRM bridges by
providing a private state readout callback and a dedicated
drm_private_state_funcs table for bridges implementing
atomic_sro_readout_state.

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b6cdbc172f689..c0224dcc8aa50 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c

[ ... ]

> @@ -677,8 +760,17 @@ 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, bridge->name,
> -				    &drm_bridge_priv_state_funcs);
> +	if (bridge->funcs &&
> +	    bridge->funcs->atomic_sro_readout_state)
> +		ret = drm_atomic_private_obj_init(bridge->dev, &bridge->base,
> +						  bridge->name,
> +						  &drm_bridge_priv_state_funcs_with_sro);
> +	else
> +		ret = drm_atomic_private_obj_init(bridge->dev, &bridge->base,
> +						  bridge->name,
> +						  &drm_bridge_priv_state_funcs);
> +	if (ret)
> +		goto err_reset_bridge;
>  
>  	return 0;

[Severity: Medium]
Does jumping to err_reset_bridge when drm_atomic_private_obj_init() fails
leave bridge->funcs->attach() un-undone?

bridge->funcs->attach() has already succeeded at this point, and
err_reset_bridge unlinks bridge->chain_node and clears bridge->dev and
bridge->encoder without calling bridge->funcs->detach(bridge):

drivers/gpu/drm/drm_bridge.c:drm_bridge_attach() {
    ...
	if (bridge->funcs->attach) {
		ret = bridge->funcs->attach(bridge, encoder, flags);
		if (ret < 0)
			goto err_reset_bridge;
	}
    ...
err_reset_bridge:
	bridge->dev = NULL;
	bridge->encoder = NULL;
	mutex_lock(&encoder->bridge_chain_mutex);
	list_del(&bridge->chain_node);
	mutex_unlock(&encoder->bridge_chain_mutex);
    ...
}

Because the bridge is removed from encoder->bridge_chain, subsequent
drm_encoder_cleanup() will not detach it either, leaking or leaving
dangling whatever bridge->funcs->attach() registered (such as connectors,
DP AUX channels, or downstream bridges).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-state-readout-v8-0-cbe78624e1bc@kernel.org?part=18