Re: [PATCH v7 net-next 3/7] selftests/ptp: Add testptp support for attributes ioctls

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] The new `-x N -a` (PTP_SYS_OFFSET_EXTENDED_ATTRS) print loop
  in `main()` (tools/testing/selftests/ptp/testptp.c lines 671-722)…
- [Low] In `main()` of tools/testing/selftests/ptp/testptp.c, the new
  `-a` modifier is accepted and sets `use_attrs = 1` (line 327-329) but…

--

selftests/ptp: Add testptp support for attributes ioctls

This adds a "-a" modifier to testptp so that the new
PTP_SYS_OFFSET_EXTENDED_ATTRS and PTP_SYS_OFFSET_PRECISE_ATTRS ioctls can
be exercised alongside the existing -x and -X requests. It also prints the
new extended_attrs/precise_attrs capabilities and reports unrecognized
clock ids in print_system_timestamp().

A couple of questions below.

> diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
> index d603d9f82237..62b1d0340d71 100644
> --- a/tools/testing/selftests/ptp/testptp.c
> +++ b/tools/testing/selftests/ptp/testptp.c
> @@ -148,6 +148,8 @@ static void usage(char *progname)
>  		" -T val     set the ptp clock time to 'val' seconds\n"
>  		" -x val     get an extended ptp clock time with the desired number of samples (up to %d)\n"
>  		" -X         get a ptp clock cross timestamp\n"
> +		" -a         also report clock quality attributes (error_bound,\n"
> +		"            status, timescale); use together with -x or -X\n"
>  		" -y val     pre/post tstamp timebase to use {realtime|monotonic|monotonic-raw}\n"
>  		" -z         test combinations of rising/falling external time stamp flags\n",
>  		progname, PTP_MAX_SAMPLES);

[ ... ]

> @@ -317,6 +324,9 @@ int main(int argc, char *argv[])
>  				return -1;
>  			}
>  			break;
> +		case 'a':
> +			use_attrs = 1;
> +			break;

[Severity: Low]
The usage text states that -a must be used "together with -x or -X", but is
that requirement enforced anywhere in main()?

Both new request blocks are gated on a companion option:

	if (getextended && use_attrs) {
	...
	if (getcross && use_attrs) {

and the two legacy blocks are now additionally gated on !use_attrs, so
"testptp -d /dev/ptp0 -a" (or -a combined with something unrelated like -k
or -g) issues no attrs ioctl, prints nothing about attributes, and exits 0.

main() already diagnoses the analogous dependency for -w and -H:

	if (pulsewidth >= 0 && perout < 0) {
		puts("-w can only be specified together with -p");
		return -1;
	}

	if (perout_phase >= 0 && perout < 0) {
		puts("-H can only be specified together with -p");
		return -1;
	}

Would it make sense to add the equivalent check for -a without -x or -X?

>  		case 'X':
>  			getcross = 1;
>  			break;

[ ... ]

> @@ -654,7 +668,60 @@ int main(int argc, char *argv[])
>  		free(soe);
>  	}
>  
> -	if (getcross) {
> +	if (getextended && use_attrs) {
> +		attrs_data = calloc(1, sizeof(*attrs_data) +
> +				    getextended * sizeof(struct ptp_timestamp));
> +		if (!attrs_data) {
> +			perror("calloc");
> +			return -1;
> +		}
> +
> +		attrs_data->request.num_samples = getextended;
> +		attrs_data->request.clock_id = ext_clockid;
> +
> +		if (ioctl(fd, PTP_SYS_OFFSET_EXTENDED_ATTRS, attrs_data)) {
> +			perror("PTP_SYS_OFFSET_EXTENDED_ATTRS");
> +		} else {
> +			printf("extended attrs timestamp request returned %d samples\n",
> +			       getextended);
> +
> +			for (i = 0; i < getextended; i++) {
> +				struct ptp_timestamp *ts = &attrs_data->timestamps[i];
> +
> +				printf("  sample #%u:\n", i);
> +				printf("    sys before: %lld ns\n",
> +				       (long long)ts->pre_systime.sys_time);
> +				printf("    sys_counter_id: %u\n",
> +				       ts->pre_systime.sys_counter_id);
> +				printf("    sys_counter:    %llu\n",
> +				       (unsigned long long)ts->pre_systime.sys_counter);
> +				printf("    phc time:   %lld.%09u\n",
> +				       ts->devtime.device_time.sec,
> +				       ts->devtime.device_time.nsec);
> +				if (ts->devtime.attrs.valid & PTP_ATTRS_VALID_ERROR_BOUND)
> +					printf("    error_bound: %u ns\n",
> +					       ts->devtime.attrs.error_bound);
> +				else
> +					printf("    error_bound: not reported\n");
> +				if (ts->devtime.attrs.valid & PTP_ATTRS_VALID_STATUS)
> +					printf("    status: %u\n",
> +					       ts->devtime.attrs.status);
> +				else
> +					printf("    status: not reported\n");
> +				if (ts->devtime.attrs.valid & PTP_ATTRS_VALID_TIMESCALE)
> +					printf("    timescale: %u\n",
> +					       ts->devtime.attrs.timescale);
> +				else
> +					printf("    timescale: not reported\n");
> +				printf("    sys after:  %lld ns\n",
> +				       (long long)ts->post_systime.sys_time);

[Severity: Medium]
Should the post snapshot print the counter fields too?

This loop prints pre_systime.sys_time, pre_systime.sys_counter_id and
pre_systime.sys_counter, but for the post snapshot only sys_time. It also
never prints sys_rawtime for either snapshot, although the kernel side fills
all four fields of both snapshots in ptp_sys_offset_extended_attrs():

drivers/ptp/ptp_chardev.c:ptp_sys_offset_extended_attrs() {
	...
	tstamp->post_systime.sys_time = ktime_to_ns(sts.post_sts.systime);
	tstamp->post_systime.sys_rawtime = ktime_to_ns(sts.post_sts.monoraw);
	ptp_fill_sys_counter(&tstamp->post_systime, sts.post_sts.cycles,
			     sts.post_sts.cs_id);
	...
}

The -X -a printer added in the same patch does surface those fields:

	printf("raw time:    %lld ns\n",
	       (long long)ts->systime.sys_rawtime);
	printf("sys_counter_id: %u\n",
	       ts->systime.sys_counter_id);
	printf("sys_counter:    %llu\n",
	       (unsigned long long)ts->systime.sys_counter);

so the two new consumers of struct ptp_sys_time disagree about which
returned fields are shown. As it stands, "-x N -a" output is identical
whether the post-read counter source is correctly identified or left at
zero.

The commit message says:

    Also print the system counter value and its identifier (sys_counter,
    sys_counter_id) returned in struct ptp_sys_time, so the counter source
    is visible

Does that hold for the extended path, given that post_systime.sys_counter
and post_systime.sys_counter_id are not printed?

> +			}
> +		}
> +
> +		free(attrs_data);
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909073136.12863-1-akiyano%40amazon.com