Re: [PATCH v6 11/28] drm/bridge: Handle bridges with hardware state readout

[sashiko-bot@kernel.org]

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

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