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

[Théo Lebrun]

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@xxxxxxxxxxx> # Raspberry Pi CM5, min bracket 37 ns -> 981 ns
> Reviewed-by: Nicolai Buchwitz <nb@xxxxxxxxxxx>
> Signed-off-by: James Clark <jjc@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx/
>
> 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@xxxxxxxxxxx>

Thanks James,

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