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 <[email protected]> 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/[email protected]?part=16
