1. Patchew
  2. linux
  3. View series

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

James Clark posted 1 patch 2 weeks, 1 day ago
There is a newer version of this series
drivers/net/ethernet/cadence/macb_ptp.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
[PATCH net v2] net: macb: fix ordering around PTP timestamp read
Posted by James Clark 2 weeks, 1 day ago 
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. The internal
smp_rmb() in the pre-timestamp path also does not guarantee ordering
against the subsequent MMIO read.

Add rmb() before and after the bracketed nanoseconds read in both the
normal and seconds rollover paths so the system timestamps bracket the
PHC read. Adding the post-read barrier increases the minimum interval on
the same Raspberry Pi 5 to approximately 1 us.

Fixes: e51bb5c2784c ("net: macb: ptp: Switch to gettimex64() interface")
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>
Signed-off-by: James Clark <jjc@jclark.com>
---
Changes in v2:
- Add rmb() before the PHC read in both paths, following Théo Lebrun's
  feedback. Explain why the pre-timestamp path's smp_rmb() is insufficient
  for MMIO ordering.
- Wrap the post-read barrier comments.
- Add Nicolai Buchwitz's Tested-by and Reviewed-by tags.
- Drop RFC.

v1: https://lore.kernel.org/netdev/20260908053150.28694-1-jjc@jclark.com/

Nicolai's review and testing were on v1. The additional pre-read barriers
in v2 address Théo's feedback.

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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index e5195d7da..4fb287608 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -50,7 +50,15 @@ 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);
+	/* ptp_read_system_prets() uses smp_rmb() internally,
+	 * which does not guarantee ordering against MMIO reads.
+	 */
+	rmb();
 	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);
@@ -62,7 +70,15 @@ static int gem_tsu_get_time(struct ptp_clock_info *ptp, struct timespec64 *ts,
 		 * (assume all done within 1s)
 		 */
 		ptp_read_system_prets(sts);
+		/* ptp_read_system_prets() uses smp_rmb() internally,
+		 * which does not guarantee ordering against MMIO reads.
+		 */
+		rmb();
 		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: [PATCH net v2] net: macb: fix ordering around PTP timestamp read
Posted by Jakub Kicinski 1 week, 3 days ago 
On Thu, 10 Sep 2026 11:06:52 +0700 James Clark wrote:
> +	/* ptp_read_system_prets() uses smp_rmb() internally,
> +	 * which does not guarantee ordering against MMIO reads.
> +	 */

IMHO the fact that the gem_readl() is a relaxed read is more relevant /
unusual in this case?

Also clashiko points out that this function may be called per packet
with sts as NULL so it's worth wrapping these barriers in if, I think
that's a good suggestion?

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910040658.18359-1-jjc@jclark.com

> +	rmb();
>  	first = gem_readl(bp, TN);
> +	/* Ensure the PHC read completes before taking
> +	 * the post timestamp.
> +	 */
> +	rmb();
-- 
pw-bot: cr
Re: [PATCH net v2] net: macb: fix ordering around PTP timestamp read
Posted by Théo Lebrun 2 weeks, 1 day ago 
Hello James,

On Thu Sep 10, 2026 at 6:06 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. The internal
> smp_rmb() in the pre-timestamp path also does not guarantee ordering
> against the subsequent MMIO read.
>
> Add rmb() before and after the bracketed nanoseconds read in both the
> normal and seconds rollover paths so the system timestamps bracket the
> PHC read. Adding the post-read barrier increases the minimum interval on
> the same Raspberry Pi 5 to approximately 1 us.
>
> Fixes: e51bb5c2784c ("net: macb: ptp: Switch to gettimex64() interface")
> 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>
> Signed-off-by: James Clark <jjc@jclark.com>
> ---
> Changes in v2:
> - Add rmb() before the PHC read in both paths, following Théo Lebrun's
>   feedback. Explain why the pre-timestamp path's smp_rmb() is insufficient
>   for MMIO ordering.
> - Wrap the post-read barrier comments.
> - Add Nicolai Buchwitz's Tested-by and Reviewed-by tags.
> - Drop RFC.
>
> v1: https://lore.kernel.org/netdev/20260908053150.28694-1-jjc@jclark.com/
>
> Nicolai's review and testing were on v1. The additional pre-read barriers
> in v2 address Théo's feedback.
[...]
>  drivers/net/ethernet/cadence/macb_ptp.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index e5195d7da..4fb287608 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -50,7 +50,15 @@ 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);
> +	/* ptp_read_system_prets() uses smp_rmb() internally,
> +	 * which does not guarantee ordering against MMIO reads.
> +	 */
> +	rmb();
>  	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);
> @@ -62,7 +70,15 @@ static int gem_tsu_get_time(struct ptp_clock_info *ptp, struct timespec64 *ts,
>  		 * (assume all done within 1s)
>  		 */
>  		ptp_read_system_prets(sts);
> +		/* ptp_read_system_prets() uses smp_rmb() internally,
> +		 * which does not guarantee ordering against MMIO reads.
> +		 */
> +		rmb();
>  		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);

Honestly I wouldn't be surprised to see this code without comments.
Especially as the rmb() were added in a separate commit so git
blame/log will point to your commit message which is plentiful. It
would have been different if the rmb were part of the commit
introducing gettimex64 support, with a commit message which would
probably not talk about why rmb are required.

Don't bother sending a new revision just for that though!
With or without the code comments:

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks James,

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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.