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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The SCDC error counter checksum validation incorrectly includes Lane 3 error registers, unconditionally breaking SCDC reads when errors occur on 4-lane FRL connections.
- [Medium] The `state->rs_corrections` field is not cleared when the Reed-Solomon channel becomes invalid, leaking stale state.
- [Medium] Encountering a reserved FRL rate causes `drm_scdc_read_state` to abort, breaking forward compatibility.
--

--- Patch [3]: [PATCH v9 3/5] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields ---
commit da086d3bc120a56452dfbe550079b981dd21770b
Author: Nicolas Frattaroli <[email protected]>

drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

This commit implements reading and parsing HDMI 2.1 FRL-specific
fields over SCDC. It extracts FRL rate, FFE levels, Lane 3 error
counters, and Reed-Solomon corrections to output via debugfs.

> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
[ ... ]
> @@ -286,14 +319,41 @@ drm_scdc_parse_status0_flags(u8 val, struct drm_scdc_status_flags *flags)
[ ... ]
> -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;

[Severity: High]
Does this inclusion of Lane 3 error registers break the checksum calculation?

The SCDC checksum byte (SCDC_ERR_DET_CHECKSUM) only covers bytes up to 0x56.
By setting end_reg to SCDC_ERR_DET_3_H (0x58), the zero-sum calculation will
include the Lane 3 error registers (0x57 and 0x58). 

If a 4-lane connection experiences any Lane 3 errors, the sum will be non-zero
and cause drm_scdc_parse_error_counters() to abort with -EPROTO, breaking
SCDC reads exactly when errors are present.

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	for (i = SCDC_ERR_DET_0_L; i <= end_reg; i++)
>  		sum = wrapping_add(u8, sum, scdc[i]);
[ ... ]
> @@ -350,11 +417,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]
Will this direct error return abort all SCDC parsing for future FRL rates?

If a newer display uses a currently-reserved FRL rate (e.g., rates 7-15),
drm_scdc_num_frl_lanes() will return -EINVAL.

Propagating this error immediately prevents users from reading any SCDC state
(including TMDS and scrambling info) for displays with newer reserved rates.

> +	if (!num_lanes)
> +		num_lanes = 3;
[ ... ]
> +	ret = drm_scdc_parse_error_counters(buf, state->error_count, num_lanes);
>  	if (ret)
>  		return ret;
>  
> +	if (state->rate && (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];

[Severity: Medium]
Should state->rs_corrections be explicitly cleared when the channel is invalid?

Unlike the channel error counters which are explicitly zeroed out when
SCDC_CHANNEL_VALID is not set, rs_corrections lacks an else branch.

If the drm_scdc_state structure is reused by a caller instead of being freshly
allocated, this could leak stale Reed-Solomon correction values from a
previous read.

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3