Re: [RFC PATCH net] net: macb: fix ordering around PTP timestamp read

[Nicolai Buchwitz <nb@tipi-net.de>]

Hi James

On 8.9.2026 07:31, James Clark wrote:
> PTP_SYS_OFFSET_EXTENDED returns system timestamps that do not correctly
> bracket the PHC register read on MACB/GEM. On a Raspberry Pi 5, the
> returned interval can be as short as 37 ns, while an ordered register
> read takes approximately 1 us. This biases the midpoint used by 
> phc2sys,
> causing CLOCK_REALTIME to run approximately 0.5 us ahead when 
> synchronized
> to the PHC.
> 
> gem_tsu_get_time() reads the nanoseconds register using the driver's
> relaxed MMIO accessor. On weakly ordered systems, the subsequent system
> timestamp can be taken before the register read completes.
> 
> Add rmb() after the bracketed nanoseconds read in both the normal and
> seconds rollover paths, ensuring that the read completes before the 
> post
> timestamp is taken. With the fix, the minimum interval on the same
> Raspberry Pi 5 increases to approximately 1 us.
> 
> Fixes: e51bb5c2784c ("net: macb: ptp: Switch to gettimex64() 
> interface")
> Signed-off-by: James Clark <jjc@jclark.com>
> ---
> This uses rmb() to preserve the existing accessor and endianness 
> handling.
> Would an ordered MMIO accessor be preferable for these two reads?

AFAIU rmb() fits better here. Switching to readl() would also order the 
read
on arm64, but only for hw_readl(). hw_readl_native() uses __raw_readl(), 
which
has no ordered version, so that path would still need a barrier.

> 
> Reproducer:
> 
> #include <fcntl.h>
> #include <linux/ptp_clock.h>
> #include <stdio.h>
> #include <sys/ioctl.h>
> 
> #define DEVICE "/dev/ptp0"
> 
> int main(void)
> {
> 	struct ptp_sys_offset_extended ex = { .n_samples = 25 };
> 	long long min = -1;
> 	int fd = open(DEVICE, O_RDONLY);
> 
> 	if (fd < 0) {
> 		perror(DEVICE);
> 		return 1;
> 	}
> 	for (int batch = 0; batch < 40; batch++) {
> 		if (ioctl(fd, PTP_SYS_OFFSET_EXTENDED, &ex) < 0) {
> 			perror("PTP_SYS_OFFSET_EXTENDED");
> 			return 1;
> 		}
> 		for (unsigned int i = 0; i < ex.n_samples; i++) {
> 			long long bracket = (ex.ts[i][2].sec - ex.ts[i][0].sec) * 
> 1000000000LL
> 				+ (long long)ex.ts[i][2].nsec - ex.ts[i][0].nsec;
> 			if (min < 0 || bracket < min)
> 				min = bracket;
> 		}
> 	}
> 	printf("min bracket: %lld ns\n", min);
> 	return 0;
> }
> 
>  drivers/net/ethernet/cadence/macb_ptp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c 
> b/drivers/net/ethernet/cadence/macb_ptp.c
> index e5195d7da..8209ec190 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -51,6 +51,8 @@ static int gem_tsu_get_time(struct ptp_clock_info 
> *ptp, struct timespec64 *ts,
>  	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
>  	ptp_read_system_prets(sts);
>  	first = gem_readl(bp, TN);
> +	/* Ensure the PHC read completes before taking the post timestamp. */
> +	rmb();
>  	ptp_read_system_postts(sts);
>  	secl = gem_readl(bp, TSL);
>  	sech = gem_readl(bp, TSH);
> @@ -63,6 +65,8 @@ static int gem_tsu_get_time(struct ptp_clock_info 
> *ptp, struct timespec64 *ts,
>  		 */
>  		ptp_read_system_prets(sts);
>  		ts->tv_nsec = gem_readl(bp, TN);
> +		/* Ensure the PHC read completes before taking the post timestamp. 
> */

nit: comment should be wrapped, to fit in the usual line length

> +		rmb();
>  		ptp_read_system_postts(sts);
>  		secl = gem_readl(bp, TSL);
>  		sech = gem_readl(bp, TSH);


Tested-by: Nicolai Buchwitz <nb@tipi-net.de> # Raspberry Pi CM5, min 
bracket 37 ns -> 981 ns
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai