On Tuesday, 21 July 2026 11:10:43 Central European Summer Time [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 5 potential issue(s) > to consider: > - [High] Incorrect checksum validation range breaks 4-lane FRL error parsing. > - [High] Redefinition of `SCDC_CH_LOCK_MASK` breaks TMDS lock checking for > HDMI 2.0 sinks. > - [Medium] Stale state leakage for `rs_corrections` across debugfs reads. > - [Medium] Reserved FRL rates cause debugfs read failures and unreachable > dead code. > - [Medium] Concurrent debugfs reads cause a data race on `priv->state`. > -- > > --- Patch [3]: [PATCH v7 3/5] drm/scdc-helper: Implement parsing and printing > HDMI 2.1 fields --- > commit c31217f1c2c58d659c6532a48bf298ea495b08b1 > Author: Nicolas Frattaroli <[email protected]> > > drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields > > HDMI 2.1 redefines previously reserved fields in SCDC for various new uses. > Implement reading and outputting these fields over debugfs. > > > diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c > > b/drivers/gpu/drm/display/drm_scdc_helper.c > > index 5871fc101815d..7e17272d52f23 100644 > > --- a/drivers/gpu/drm/display/drm_scdc_helper.c > > +++ b/drivers/gpu/drm/display/drm_scdc_helper.c > > [ ... ] > > > -static int drm_scdc_parse_error_counters(const u8 scdc[256], u16 > > counter[3]) > > +static int drm_scdc_parse_error_counters(const u8 scdc[256], u16 > > counter[4], > > + unsigned int num_lanes) > > { > > + u8 end_reg; > > u8 sum = 0; > > int i; > > > > - for (i = SCDC_ERR_DET_0_L; i <= SCDC_ERR_DET_CHECKSUM ; i++) > > + switch (num_lanes) { > > + case 3: > > + end_reg = SCDC_ERR_DET_CHECKSUM; > > + break; > > + case 4: > > + end_reg = SCDC_ERR_DET_3_H; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + for (i = SCDC_ERR_DET_0_L; i <= end_reg; i++) > > sum = wrapping_add(u8, sum, scdc[i]); > > [Severity: High] > Does this break the checksum validation for 4-lane FRL links? > > The standard HDMI checksum is designed to sum registers 0x50 through 0x56 > (SCDC_ERR_DET_CHECKSUM) to zero. > > By extending the loop's end_reg to SCDC_ERR_DET_3_H (0x58) for 4-lane mode, > won't this incorporate non-checksum data into the sum and incorrectly return > -EPROTO whenever Lane 3 has errors or its valid bit is set? > > [ ... ] > > > @@ -356,11 +423,26 @@ int drm_scdc_read_state(struct drm_connector > > *connector, struct drm_scdc_state * > > > > state->scrambling_detected = buf[SCDC_SCRAMBLER_STATUS] & > > SCDC_SCRAMBLING_STATUS; > > > > + state->rate = FIELD_GET(SCDC_FRL_RATE, buf[SCDC_CONFIG_1]); > > + num_lanes = drm_scdc_num_frl_lanes(state->rate); > > + if (num_lanes < 0) > > + return num_lanes; > > [Severity: Medium] > If a sink reports a reserved FRL rate (between 7 and 15), does this cause > debugfs reads to fail entirely? > > drm_scdc_num_frl_lanes() returns -EINVAL for these reserved rates, causing > drm_scdc_read_state() to immediately abort. This makes the reserved rate > strings in drm_scdc_frl_rate_str() unreachable dead code. > > > + if (!num_lanes) > > + num_lanes = 3; > > + > > + state->ffe_levels = FIELD_GET(SCDC_FFE_LEVELS, buf[SCDC_CONFIG_1]); > > + > > drm_scdc_parse_status0_flags(buf[SCDC_STATUS_FLAGS_0], &state->stf); > > - ret = drm_scdc_parse_error_counters(buf, state->error_count); > > + drm_scdc_parse_status1_2_flags(buf[SCDC_STATUS_FLAGS_1], > > + buf[SCDC_STATUS_FLAGS_2], &state->stf); > > + ret = drm_scdc_parse_error_counters(buf, state->error_count, num_lanes); > > if (ret) > > return ret; > > > > + if (num_lanes == 4 && (buf[SCDC_ERR_DET_RS_H] & SCDC_CHANNEL_VALID)) > > + state->rs_corrections = (buf[SCDC_ERR_DET_RS_H] & > > ~SCDC_CHANNEL_VALID) << 8 | > > + buf[SCDC_ERR_DET_RS_L]; > > + > > return 0; > > } > > [Severity: Medium] > Does this leak stale state across debugfs reads? > > If a 4-lane link transitions to a state where the RS channel valid bit is > cleared, or if it drops to 3 lanes, the missing else branch here leaves the > old rs_corrections value intact. > > Since the state struct is reused across reads, could this expose stale > correction counts to userspace? > > [ ... ] > > > @@ -406,6 +488,8 @@ static int scdc_status_show(struct seq_file *m, void > > *data) > > { > > struct scdc_debugfs_priv *priv = m->private; > > struct drm_scdc_state *st = &priv->state; > > struct drm_connector *connector = priv->connector; > > struct drm_scdc *scdc = &connector->display_info.hdmi.scdc; > > int i, ret; > > > > drm_connector_get(connector); > > > > if (connector->status != connector_status_connected) { > > ret = -ENODEV; > > goto err_conn_put; > > } > > > > if (scdc->supported) { > > ret = drm_scdc_read_state(connector, st); > > [Severity: Medium] > Can concurrent debugfs reads cause a data race on the shared state structure? > > Multiple threads reading the scdc_status debugfs file share the same private > context. Calling drm_scdc_read_state() mutates the shared I2C buffer > (state->scdc) and its bitfields directly without serialization, which could > corrupt I2C reads and mangle the debugfs output. > > [ ... ] > > > diff --git a/include/drm/display/drm_scdc.h b/include/drm/display/drm_scdc.h > > index 3d58f37e8ed8e..7f0b05b2f2800 100644 > > --- a/include/drm/display/drm_scdc.h > > +++ b/include/drm/display/drm_scdc.h > > @@ -46,14 +48,25 @@ > > #define SCDC_CONFIG_0 0x30 > > #define SCDC_READ_REQUEST_ENABLE (1 << 0) > > > > +#define SCDC_CONFIG_1 0x31 > > +#define SCDC_FRL_RATE 0x0f > > +#define SCDC_FFE_LEVELS 0xf0 > > + > > #define SCDC_STATUS_FLAGS_0 0x40 > > +#define SCDC_DSC_FAIL (1 << 7) > > +#define SCDC_FLT_READY (1 << 6) > > +#define SCDC_LN3_LOCK (1 << 4) > > #define SCDC_CH2_LOCK (1 << 3) > > #define SCDC_CH1_LOCK (1 << 2) > > #define SCDC_CH0_LOCK (1 << 1) > > -#define SCDC_CH_LOCK_MASK (SCDC_CH2_LOCK | SCDC_CH1_LOCK | SCDC_CH0_LOCK) > > +#define SCDC_CH_LOCK_MASK (SCDC_LN3_LOCK | SCDC_CH2_LOCK | SCDC_CH1_LOCK > > | \ > > + SCDC_CH0_LOCK) > > [Severity: High] > Can this redefinition break TMDS lock checking for HDMI 2.0 sinks? > > Since HDMI 2.0 sinks only have 3 lanes, the SCDC_LN3_LOCK bit will always > be 0. Existing drivers performing an equality check against this mask > will systematically fail to detect the lock. > >
One of those (state race) is already in the previous patch's review, the rest are all delirious nonsense.
