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

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