Re: [PATCH v6 19/28] drm/atomic-helper: Document State Read-Out as a state initialization approach

[sashiko-bot@kernel.org]

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 <mripard@kernel.org>

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/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=19