Re: [PATCH v6 28/28] drm/bridge: sii902x: Implement hw state readout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/bridge: sii902x: unconditional deref of optional atomic_sro_get_current_crtc hook
- [Medium] drm/bridge: silent masking of I2C errors via regmap_test_bits
--

commit 688ff92bfdba29f9af2e8eb0898f519167caccaa
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: sii902x: Implement hw state readout

This commit implements hardware state readout for the sii902x bridge.
It introduces connector and state readout callbacks utilizing the atomic
SRO (State ReadOut) infrastructure.

> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 9aac44ed5b677..332945f3c9f7f 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
[ ... ]
> @@ -537,6 +539,80 @@ sii902x_bridge_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> +static int
> +sii902x_bridge_connector_readout(struct drm_bridge *bridge,
> +				 struct drm_atomic_sro_state *state,
> +				 struct drm_connector_state *conn_state)
> +{
> +	struct sii902x *sii902x = bridge_to_sii902x(bridge);
> +	struct drm_connector *connector = conn_state->connector;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_encoder *encoder;
> +	struct drm_crtc *crtc;
> +
> +	if (regmap_test_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, SII902X_SYS_CTRL_PWR_DWN))
> +		return 0;

[Severity: Medium]
Will a negative error code from a failed I2C read evaluate as true here?

If regmap_test_bits() encounters an underlying hardware error (such as
-EIO or -ETIMEDOUT), it returns a negative value. In C, this evaluates to
true, which seems like it would cause the function to incorrectly assume
the device is powered down, skip the readout, and mask the hardware error
by returning 0.

> +
> +	encoder = bridge->encoder;
> +	crtc = encoder->funcs->atomic_sro_get_current_crtc(encoder);

[Severity: High]
Could this dereference a NULL pointer?

As an external I2C HDMI bridge, sii902x can be attached to arbitrary display
controllers. The DRM core permits encoder->funcs to be NULL for simple
managed encoders, and atomic_sro_get_current_crtc is an optional hook.

If the attached encoder lacks the funcs pointer or does not implement
the atomic_sro_get_current_crtc hook, this unconditional dereference
would result in a kernel panic.

> +	if (!crtc)
> +		return -ENODEV;
[ ... ]
> +static int sii902x_bridge_readout_state(struct drm_bridge *bridge,
> +					struct drm_atomic_sro_state *state,
> +					struct drm_bridge_state *bridge_state,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct sii902x *sii902x = bridge_to_sii902x(bridge);
> +	u32 input_format;
> +
> +	if (regmap_test_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, SII902X_SYS_CTRL_PWR_DWN))
> +		return 0;

[Severity: Medium]
Is this subject to the same error masking as in
sii902x_bridge_connector_readout()?

A negative return value from an I2C failure here will also evaluate as true,
skipping the bridge state readout and leaving the state object uninitialized
without propagating the failure.

> +
> +	switch (sii902x->bus_width) {
> +	case 16:
> +		input_format = MEDIA_BUS_FMT_RGB565_1X16;
> +		break;
[ ... ]

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