Re: [PATCH v2 1/1] net/e1000: use device timestamp for igc read_clock() operation

[Bruce Richardson <bruce.richardson@intel.com>]

On Sat, Nov 08, 2025 at 04:06:13PM +0800, Song Yoong Siang wrote:
> Change eth_igc_read_clock() to read from hardware timestamp registers
> (E1000_SYSTIML/E1000_SYSTIMH) instead of using system clock_gettime().
> 
> This ensures that the clock reading is consistent with the hardware's
> internal time base used for Qbv cycle and launch time scheduling,
> providing better accuracy for Time-Sensitive Networking applications.
> 
> Fixes: 9630f7c71ecd ("net/igc: enable launch time offloading")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Zage <david.zage@intel.com>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
> ---
> v1: https://patches.dpdk.org/project/dpdk/patch/20251107031507.3890366-1-yoong.siang.song@intel.com/
> 
> changelog:
> v1 -> v2
> - reuse the existing eth_igc_timesync_read_time() (Soumyadeep).
> ---
>  drivers/net/intel/e1000/igc_ethdev.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/intel/e1000/igc_ethdev.c b/drivers/net/intel/e1000/igc_ethdev.c
> index b9c91d2446..d4edc82668 100644
> --- a/drivers/net/intel/e1000/igc_ethdev.c
> +++ b/drivers/net/intel/e1000/igc_ethdev.c
> @@ -2813,6 +2813,12 @@ eth_igc_timesync_read_time(struct rte_eth_dev *dev, struct timespec *ts)
>  {
>  	struct e1000_hw *hw = IGC_DEV_PRIVATE_HW(dev);
>  
> +	/*
> +	 * Reading the SYSTIML register latches the upper 32 bits to the SYSTIMH
> +	 * shadow register for coherent access. As long as we read SYSTIML first
> +	 * followed by SYSTIMH, we avoid race conditions where the time rolls
> +	 * over between the two register reads.
> +	 */

Not sure this is true. If the nsec value == 999,999,999 on read, then the
rollover occurs, the resulting timestamp will be almost 1 second out,
right?

Instead, you should probably do one of:
* read nsec, then sec, then nsec again and verify that nsec2 > nsec1
* read sec, nsec, then sec, and verify that sec2 == sec1

in both casees retrying the reads if the condition fails.

>  	ts->tv_nsec = E1000_READ_REG(hw, E1000_SYSTIML);
>  	ts->tv_sec = E1000_READ_REG(hw, E1000_SYSTIMH);
>  
> @@ -2972,10 +2978,10 @@ eth_igc_timesync_disable(struct rte_eth_dev *dev)
>  static int
>  eth_igc_read_clock(__rte_unused struct rte_eth_dev *dev, uint64_t *clock)
>  {
> -	struct timespec system_time;
> +	struct timespec ts;
>  
> -	clock_gettime(CLOCK_REALTIME, &system_time);
> -	*clock = system_time.tv_sec * NSEC_PER_SEC + system_time.tv_nsec;
> +	eth_igc_timesync_read_time(dev, &ts);
> +	*clock = rte_timespec_to_ns(&ts);
>  
>  	return 0;
>  }
> -- 
> 2.48.1
>