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