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

[Théo Lebrun]

Hello James,

(added Nicolai as Cc)

On Tue Sep 8, 2026 at 7:31 AM CEST, 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.

Thanks for the nice commit message!

> Fixes: e51bb5c2784c ("net: macb: ptp: Switch to gettimex64() interface")
> Signed-off-by: James Clark <jjc@xxxxxxxxxx>
> ---
> This uses rmb() to preserve the existing accessor and endianness handling.
> Would an ordered MMIO accessor be preferable for these two reads?
>
> 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);

I think we should also add a memory barrier inbetween the first
ptp_read_system_prets() and the gem_readl(). That requires ordering
which isn't guaranteed.

Asking an LLM to double check, it tells me that we might have one by
accident: ptp_read_system_prets()
       -> ktime_get_snapshot_id()
       -> read_seqcount_retry() in exit path, after clock->read() call
       -> smp_rmb()

Do you agree?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com