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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

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