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. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
