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

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

[James Clark]

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?

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. */
+		rmb();
 		ptp_read_system_postts(sts);
 		secl = gem_readl(bp, TSL);
 		sech = gem_readl(bp, TSH);
-- 
2.47.3

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

[Nicolai Buchwitz]

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

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

[Théo Lebrun]

Hello Nicolai,

On Tue Sep 8, 2026 at 11:12 AM CEST, Nicolai Buchwitz wrote:
> 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.

Agreed we want to use our existing helper and an explicit memory
barrier. Our mistake here is an helper called gem_readl() that doesn't
call readl()! Proper naming would maybe have helped catch this earlier.

>>  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 for testing Nicolai! I don't have any Pi 5 setup (yet hopefully).

Thanks,

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

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@jclark.com>
> ---
> 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