Re: [PATCH v7 3/5] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Tuesday, 21 July 2026 11:10:43 Central European Summer Time sashiko-bot@kernel.org 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 <nicolas.frattaroli@collabora.com>
> 
> 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.