Re: [PATCH v7 34/34] 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: missing mutex lock in readout callbacks
- [Medium] drm/bridge: sii902x: ignored regmap_test_bits() error in readout_state()
--

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

drm/bridge: sii902x: Implement hw state readout

This commit implements hardware state readout callbacks for the sii902x
bridge driver and enables DRM_BRIDGE_OP_CONNECTOR_HW_READOUT.

> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index e067c0636e7f5..ab5ffcf6882f2 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
[ ... ]
> @@ -537,6 +539,89 @@ 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;
> +	int ret;
> +
> +	ret = regmap_test_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, SII902X_SYS_CTRL_PWR_DWN);

[Severity: High]
Should sii902x_bridge_connector_readout() and sii902x_bridge_readout_state()
hold sii902x->mutex around their regmap_test_bits() calls?

In drivers/gpu/drm/bridge/sii902x.c, sii902x_regmap_config disables regmap's
internal locking:

static const struct regmap_config sii902x_regmap_config = {
	.reg_bits = 8,
	.val_bits = 8,
	.disable_locking = true, /* struct sii902x mutex should be enough */
	...
};

All other post-init regmap callers in this driver (such as
sii902x_interrupt(), sii902x_detect(), sii902x_edid_read(),
sii902x_bridge_mode_set(), and the audio callbacks) serialize access via
sii902x->mutex.

Without holding sii902x->mutex here, an unlocked regmap_test_bits() during
initial state readout or post-commit drm_atomic_sro_readout_and_compare()
can race with concurrent HPD interrupt handling, DDC operations, or ALSA
audio callbacks and clobber regmap's shared map->work_buf or interleave I2C
sequences.

> +	/* I2C error */
> +	if (ret < 0)
> +		return ret;
> +	/* Device is powered down, nothing to read out */
> +	if (ret)
> +		return 0;
[ ... ]
> +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]
What happens in sii902x_bridge_readout_state() if regmap_test_bits() fails
with a negative error code?

Because a negative errno is non-zero, the if condition evaluates to true and
sii902x_bridge_readout_state() returns 0 as though the device were powered
down, leaving bridge_state->input_bus_cfg and bridge_state->output_bus_cfg
unpopulated.

Should this check for ret < 0 and propagate the error like
sii902x_bridge_connector_readout() does above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-drm-state-readout-v7-0-25e5163d75c3@kernel.org?part=34