Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/bridge: NULL pointer dereference on disabled connectors during SRO readout - [High] drm/sro: out-of-bounds array access on concurrent connector hotplug - [Medium] drm/bridge: SRO readout aborts on unrouted/inactive bridges - [Low] drm/bridge: spurious WARN_ON for connectors missing SRO state --
commit e3458795912787bd3a45246c30b0044f0dcbc3e3 Author: Maxime Ripard <[email protected]> drm/bridge: Handle bridges with hardware state readout This commit adds a drm_bridge_atomic_readout_priv_state wrapper that looks up the connector and CRTC states for a bridge's encoder during hardware state readout. It also provides a separate drm_private_state_funcs vtable for bridges supporting SRO readout. > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 9d5529765f5fb..8aa94f842634c 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c [ ... ] > @@ -555,6 +556,64 @@ drm_bridge_atomic_create_priv_state(struct > drm_private_obj *obj) > return &state->base; > } > > +static struct drm_connector_state * > +find_connector_state_for_encoder(struct drm_atomic_sro_state *state, > + struct drm_encoder *encoder) > +{ > + struct drm_connector_list_iter conn_iter; > + struct drm_connector *connector; > + > + drm_connector_list_iter_begin(drm_atomic_sro_state_get_device(state), > + &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + struct drm_connector_state *conn_state = > + drm_atomic_sro_get_connector_state(state, connector); [Severity: High] Can connector->index exceed the pre-allocated SRO array bounds here? The SRO readout state is allocated based on dev->mode_config.num_connector. However, if a connector is hotplugged concurrently (e.g., DP MST) or removed during early probe, connector->index (which is assigned via IDA) can exceed the current count. Since drm_atomic_sro_get_connector_state() unconditionally accesses state->connectors[index] without bounds checking, could this cause an out-of-bounds array access if the live connector list contains newer connectors? > + > + if (WARN_ON(!conn_state)) [Severity: Low] Will this produce a spurious WARN_ON for connectors missing SRO state? If DRM_ATOMIC_READOUT_SKIP_MISSING_READOUT is used, connectors lacking atomic_sro_readout_state skip readout, leaving their state NULL. This iteration will then trigger the warning on every bridge readout for every skipped connector during boot. > + continue; > + > + if (encoder == conn_state->best_encoder) { > + drm_connector_list_iter_end(&conn_iter); > + return conn_state; > + } > + } > + drm_connector_list_iter_end(&conn_iter); > + > + return NULL; > +} > + > +static int > +drm_bridge_atomic_readout_priv_state(struct drm_private_obj *obj, > + struct drm_atomic_sro_state *state, > + struct drm_private_state *priv_state) > +{ > + struct drm_bridge *bridge = drm_priv_to_bridge(obj); > + struct drm_encoder *encoder = bridge->encoder; > + const struct drm_bridge_funcs *bridge_funcs = bridge->funcs; > + struct drm_crtc_state *crtc_state; > + struct drm_bridge_state *bridge_state; > + struct drm_connector_state *conn_state; > + int ret; > + > + conn_state = find_connector_state_for_encoder(state, encoder); > + if (!conn_state) > + return -EINVAL; [Severity: Medium] Will returning -EINVAL here abort the entire SRO readout for unrouted bridges? If the bootloader leaves a bridge or encoder inactive, best_encoder may be NULL for the connector, causing find_connector_state_for_encoder() to return NULL. By propagating -EINVAL, the entire device's SRO readout fails and falls back to a hardware reset, which breaks seamless display handover (fastboot) and causes display flickering. > + > + crtc_state = drm_atomic_sro_get_crtc_state(state, conn_state->crtc); [Severity: High] Can conn_state->crtc be NULL here if the display pipeline is disabled? In a disabled or unassigned pipeline, a connector might have best_encoder populated but lack an active CRTC. Calling drm_atomic_sro_get_crtc_state() passes this potentially NULL crtc to drm_crtc_index(crtc): drivers/gpu/drm/drm_atomic_sro.c:drm_atomic_sro_get_crtc_state() { unsigned int index = drm_crtc_index(crtc); ... } Will this cause a NULL pointer dereference during hardware readout? > + if (!crtc_state) > + return -EINVAL; > + > + bridge_state = drm_priv_to_bridge_state(priv_state); > + if (bridge_funcs->atomic_sro_readout_state) { > + ret = bridge_funcs->atomic_sro_readout_state( > + bridge, state, bridge_state, crtc_state, conn_state); > + if (WARN_ON(ret)) > + return ret; > + } > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
