Re: [PATCH v6 16/28] drm/atomic_helper: Compare actual and readout states once the commit is done

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm: null pointer dereference in drm_atomic_sro_readout_and_compare() due to missing SRO support checks
- [High] drm: concurrent hardware state readout races with disjoint atomic commits
--

commit 54159ebf5886b308c147c6831cca5d526d0cafd5
Author: Maxime Ripard <mripard@kernel.org>
Subject: drm/atomic_helper: Compare actual and readout states once the commit is done

This commit introduces a new hook for every state, atomic_sro_compare_state,
to read out a new state every time a blocking commit is made, and compare
the readout one with the committed one.

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9d006f98413a5..fa8a337650138 100644
> --- 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);
>  
> +	/*
> +	 * drm_atomic_sro_readout_and_compare() will compare the @state new
> +	 * states with whatever it will read from the hardware.
[ ... ]
> +	 * Either way, we're past that point here, so we wouldn't delay the
> +	 * fence signalling.
> +	 */
> +	if (!nonblock)
> +		drm_atomic_sro_readout_and_compare(state);

[Severity: Critical]
Will this unconditionally execute for all blocking commits, bypassing any
check for whether the underlying DRM driver actually supports SRO?

Since this is a newly introduced, optional SRO hook, the vast majority of
existing DRM atomic drivers do not implement atomic_sro_build_state, and
some may not even define helper_private.

> +
>  	drm_atomic_commit_put(state);
>  }
> diff --git a/drivers/gpu/drm/drm_atomic_sro.c b/drivers/gpu/drm/drm_atomic_sro.c
> index 15ecf88714faa..cd7015f8c333c 100644
> --- a/drivers/gpu/drm/drm_atomic_sro.c
> +++ b/drivers/gpu/drm/drm_atomic_sro.c
[ ... ]
> @@ -725,3 +789,144 @@ void drm_atomic_sro_install_state(struct drm_atomic_sro_state *state)
[ ... ]
> +bool drm_atomic_sro_readout_and_compare(struct drm_atomic_commit *commit)
> +{
> +	struct drm_device *dev = commit->dev;
> +	const struct drm_mode_config_helper_funcs *funcs =
> +		dev->mode_config.helper_private;
> +	struct drm_printer p = drm_err_printer(dev, NULL);
> +	struct drm_private_state *new_obj_state;
> +	struct drm_private_obj *obj;
> +	struct drm_plane_state *new_plane_state;
> +	struct drm_plane *plane;
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_crtc *crtc;
> +	struct drm_connector_state *new_conn_state;
> +	struct drm_connector *conn;
> +	struct drm_atomic_sro_state *readout_state;
> +	unsigned int i;
> +	bool identical = true;
> +
> +	readout_state = funcs->atomic_sro_build_state(dev);

[Severity: Critical]
Could this result in a NULL pointer dereference for non-SRO drivers?

The function extracts the helper functions via dev->mode_config.helper_private
and immediately dereferences funcs->atomic_sro_build_state(dev) without
verifying if the pointer exists. Should the code explicitly verify
if (funcs && funcs->atomic_sro_build_state) or use
drm_atomic_sro_device_can_readout(dev) before proceeding?

[Severity: High]
Does reading the hardware state of unrelated objects here introduce hardware
data races with concurrent commits? 

This initiates a device-wide hardware state readout from within
commit_tail() during a partial atomic commit. An atomic commit only holds
drm_modeset_locks for the specific objects being modified in that
transaction. 

By unconditionally reading the hardware state of all other unrelated,
unlocked objects across the device, this introduces a data race with
concurrent atomic commits that may be actively powering down or reconfiguring
those disjoint objects. Reading unclocked or powered-down peripheral
registers during a concurrent reprogram sequence can trigger bus hangs,
system lockups, or yield corrupted readout data. Should the readout scope be
restricted solely to the objects locked within the drm_atomic_commit context?

> +	if (WARN_ON(IS_ERR(readout_state)))
> +		return false;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=16