Re: [PATCH i-g-t] lib/igt_amd: parse link_settings records by label

[Alex Hung <alex.hung@amd.com>]

On 8/31/26 14:56, Roman.Li@amd.com wrote:
> From: Roman Li <Roman.Li@amd.com>
> 
> [Why]
> igt_amd_read_link_settings() depends on an artifact of a kernel bug.
> 
> dp_link_settings_read() passes strlen() of each format string as the
> size argument to snprintf() and advances its output pointer by that
> same fixed amount, so each of the Current, Verified, Reported and
> Preferred records is truncated at a NUL written inside the buffer.
> The parser rewrites those NULs to ';' and uses them as strtok
> delimiters.
> 
> A kernel fix for that truncation is pending [1]. It makes the node
> emit a single, properly terminated string, at which point the whole
> buffer collapses into one strtok token: only Current is parsed and
> the remaining records stay zero. amd_ilr@ilr-link-training-configs
> then writes a lane count of 0 taken from the Reported record, the
> driver rejects it as invalid, the link is left at its previous
> configuration, and the trained lane count no longer matches the
> requested one:
> 
>    Write training setting - lane count:0, supported link rate idx:0
>    Actual link result - lane count:4, link rate:0x1E
>    Failed assertion: reported_lc == data->lane_count[CURRENT]
> 
> Parsing by label is correct either way, so this can land ahead of
> the kernel change.
> 
> [How]
> Locate each record by its label with memmem() over the number of
> bytes actually read, then parse it with sscanf(). Searching the raw
> buffer rather than treating it as a string keeps the parser working
> whether or not the records are separated by NUL bytes, so both old
> and fixed kernels are handled.
> 
> Tested on eDP with both kernel variants: amd_ilr passes in both
> cases.
> 
> [1] "drm/amd/display: fix malformed link_settings debugfs output"
How about using Link as below?

Link: 
https://lore.kernel.org/amd-gfx/20260624180829.4775-12-george.zhang@amd.com/> 

> Cc: Harry Wentland <harry.wentland@amd.com>
> Assisted-by: Copilot:Claude-Opus-5
> Signed-off-by: Roman Li <Roman.Li@amd.com>
> ---
>   lib/igt_amd.c | 46 +++++++++++++++++++++++++---------------------
>   1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/igt_amd.c b/lib/igt_amd.c
> index 47e587e54..79052979c 100644
> --- a/lib/igt_amd.c
> +++ b/lib/igt_amd.c
> @@ -833,10 +833,11 @@ int igt_amd_trigger_hotplug(int drm_fd, char *connector_name)
>   void igt_amd_read_link_settings(
>   	int drm_fd, const char *connector_name, int *lane_count, int *link_rate, int *link_spread)
>   {
> -	int fd, ret;
> +	static const char * const labels[] = {
> +		"Current:", "Verified:", "Reported:", "Preferred:"
> +	};
> +	int fd, ret, i;
>   	char buf[101];
> -	int i = 0;
> -	char *token_end, *val_token;
>   
>   	fd = igt_debugfs_connector_dir(drm_fd, connector_name, O_RDONLY);
>   	if (fd < 0) {
> @@ -844,29 +845,32 @@ void igt_amd_read_link_settings(
>   			 connector_name);
>   		return;
>   	}
> -	ret = igt_debugfs_simple_read(fd, DEBUGFS_DP_LINK_SETTINGS, buf, sizeof(buf));
> +
> +	ret = igt_debugfs_simple_read(fd, DEBUGFS_DP_LINK_SETTINGS, buf,
> +				      sizeof(buf));
This is an unnecessary modification.

>   	igt_assert_f(ret >= 0, "Reading %s for connector %s failed.\n",
>   		     DEBUGFS_DP_LINK_SETTINGS, connector_name);
>   
>   	close(fd);
>   
> -	/* Between current, verified, reported, and preferred are null terminators,
> -	 * replace them with ';' to use as the delimiter for strtok. */
> -	while (strlen(buf) < sizeof(buf) - 1 && buf[strlen(buf)] == '\0')
> -		buf[strlen(buf)] = ';';
> -
> -	/* Parse values read from file. */
> -	for (char *token = strtok_r(buf, ";", &token_end);
> -	     token != NULL;
> -	     token = strtok_r(NULL, ";", &token_end))
> -	{
> -		strtok_r(token, ": ", &val_token);
> -		lane_count[i] = strtol(val_token, &val_token, 10);
> -		link_rate[i] = strtol(val_token, &val_token, 16);
> -		link_spread[i] = strtol(val_token, &val_token, 10);
> -		i++;
> -
> -		if (i > 3) return;
> +	/*
> +	 * Some kernels separate the records with NUL bytes, so locate each
> +	 * one in the raw buffer rather than treating it as a single string.
> +	 */
> +	for (i = 0; i < 4; i++) {
What's hardcoded 4 here? Should it be ARRAY_SIZE(labels)?

> +		const char *label = labels[i];
> +		char *rec = memmem(buf, ret, label, strlen(label));
> +		unsigned int rate;
> +		int lanes, spread;
> +
> +		igt_assert_f(rec, "Missing %s record in %s for connector %s\n",
> +			     label, DEBUGFS_DP_LINK_SETTINGS, connector_name);
> +		igt_assert_eq(sscanf(rec + strlen(label), "%d %x %d",
> +				     &lanes, &rate, &spread), 3);
> +
> +		lane_count[i] = lanes;
> +		link_rate[i] = rate;
> +		link_spread[i] = spread;
>   	}
>   }
>