e1000e: allow non-monotonic SYSTIM readings
Subsystems (2)
RFC PATCH 1/1 e1000e: allow non-monotonic SYSTIM readings
It seems with some NICs supported by the e1000e driver a SYSTIM reading may occasionally be few microseconds before the previous reading and if enabled also pass e1000e_sanitize_systim() without reaching the maximum number of rereads, even if the function is modified to check three consecutive readings (i.e. it doesn't look like a double read error). This causes an underflow in the timecounter and the PHC time jumps hours ahead.
This was observed on 82574, I217 and I219. The fastest way to reproduce it is to run a program that continuously calls the PTP_SYS_OFFSET ioctl on the PHC.
Modify e1000e_phc_gettime() to use timecounter_cyc2time() instead of timecounter_read() in order to allow non-monotonic SYSTIM readings and prevent the PHC from jumping.
--- drivers/net/ethernet/intel/e1000e/ptp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/e1000e/ptp.c +9 -2
@@ -173,10 +173,14 @@ static int e1000e_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts) struct e1000_adapter *adapter = container_of(ptp, struct e1000_adapter, ptp_clock_info);unsigned long flags; -u64 ns; +u64 cycles, ns;spin_lock_irqsave (&adapter->systim_lock, flags); - ns =timecounter_read (&adapter->tc); + +/* Use timecounter_cyc2time() to allow non-monotonic SYSTIM readings */ + cycles = adapter->cc.read (&adapter->cc); + ns =timecounter_cyc2time (&adapter->tc, cycles); +spin_unlock_irqrestore (&adapter->systim_lock, flags); *ts =ns_to_timespec64 (ns);
@@ -233,6 +237,9 @@ static void e1000e_systim_overflow_work(struct work_struct *work)struct e1000_hw *hw = &adapter->hw;struct timespec64 ts; +/* Update the timecounter */ +timecounter_read (&adapter->tc); + adapter->ptp_clock_info.gettime64 (&adapter->ptp_clock_info, &ts); e_dbg("SYSTIM overflow check at %lld.%09lu\n",
Signature · 1 line
2.17.1
Keller
RFC PATCH 1/1 e1000e: allow non-monotonic SYSTIM readings
Hi Miroslav
Quoted 16 lines
-----Original Message-----
From: Miroslav Lichvar [mailto:mlichvar at redhat.com]
Sent: Wednesday, October 17, 2018 9:16 AM
To: intel-wired-lan at lists.osuosl.org
Cc: Miroslav Lichvar <[email protected]>; Keller, Jacob E <[email protected]>; Richard Cochran <[email protected]>
Subject: [RFC PATCH] e1000e: allow non-monotonic SYSTIM readingsIt seems with some NICs supported by the e1000e driver a SYSTIM reading may occasionally be few microseconds before the previous reading and if enabled also pass
e1000e_sanitize_systim()without reaching the maximum number of rereads, even if the function is modified to check three consecutive readings (i.e. it doesn't look like a double read error). This causes an underflow in the timecounter and the PHC time jumps hours ahead.
Quoted 4 lines
This was observed on 82574, I217 and I219. The fastest way to reproduce it is to run a program that continuously calls the
PTP_SYS_OFFSETioctl on the PHC.
Ouch. Good catch.
Quoted 4 lines
Modify
e1000e_phc_gettime()to usetimecounter_cyc2time()instead oftimecounter_read()in order to allow non-monotonic SYSTIM readings and prevent the PHC from jumping.
Right. I think this makes sense. It does mean we will report the clock jumping back, but that *is* what the hardware reports here so I think it makes sense.
Quoted 31 lines
Cc Jacob Keller <[email protected]>Cc Richard Cochran <[email protected]>Signed-off-by Miroslav Lichvar <[email protected]>--- drivers/net/ethernet/intel/e1000e/ptp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/net/ethernet/intel/e1000e/ptp.c +6
-2@@ -173,10 +173,14 @@ static int e1000e_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts) struct e1000_adapter *adapter = container_of(ptp, struct e1000_adapter, ptp_clock_info);unsigned long flags; -u64 ns; +u64 cycles, ns;spin_lock_irqsave (&adapter->systim_lock, flags); - ns =timecounter_read (&adapter->tc); + +/* Use timecounter_cyc2time() to allow non-monotonic SYSTIM readings */ + cycles = adapter->cc.read (&adapter->cc); + ns =timecounter_cyc2time (&adapter->tc, cycles); +spin_unlock_irqrestore (&adapter->systim_lock, flags);drivers/net/ethernet/intel/e1000e/ptp.c +0
-0*ts =ns_to_timespec64 (ns);
drivers/net/ethernet/intel/e1000e/ptp.c static void e1000e_systim_overflow_work(struct · line 243
Quoted 7 lines
drivers/net/ethernet/intel/e1000e/ptp.c +3
-0@@ -233,6 +237,9 @@ static void e1000e_systim_overflow_work(struct work_struct *work)struct e1000_hw *hw = &adapter->hw;struct timespec64 ts; +/* Update the timecounter */ +timecounter_read (&adapter->tc); +
yea, we previously depended on the gettime64 to do timecounter_read implicitly.
drivers/net/ethernet/intel/e1000e/ptp.c static void e1000e_systim_overflow_work(struct work_struct *work) · line 243
Quoted 1 line
drivers/net/ethernet/intel/e1000e/ptp.c +0
-0adapter->ptp_clock_info.gettime64 (&adapter->ptp_clock_info, &ts);
Can't we drop this line now?
Quoted 4 lines
drivers/net/ethernet/intel/e1000e/ptp.c +0
-1e_dbg("SYSTIM overflow check at %lld.%09lu\n", --2.17.1
RFC PATCH 1/1 e1000e: allow non-monotonic SYSTIM readings
Quoted 11 lines · Jacob E
drivers/net/ethernet/intel/e1000e/ptp.c static void e1000e_systim_overflow_work(struct · line 243
Quoted 8 lines
drivers/net/ethernet/intel/e1000e/ptp.c +3
-0@@ -233,6 +237,9 @@ static void e1000e_systim_overflow_work(struct work_struct *work)struct e1000_hw *hw = &adapter->hw;struct timespec64 ts; +/* Update the timecounter */ +timecounter_read (&adapter->tc); +yea, we previously depended on the gettime64 to do
timecounter_readimplicitly.
Quoted 3 lines
drivers/net/ethernet/intel/e1000e/ptp.c static void e1000e_systim_overflow_work(struct work_struct *work) · line 243
Quoted 1 line
drivers/net/ethernet/intel/e1000e/ptp.c +0
-0adapter->ptp_clock_info.gettime64 (&adapter->ptp_clock_info, &ts);Can't we drop this line now?
Yes. We can save the value returned by the timecounter_read call and convert it to timespec for the debug message below. I'll send a new patch.
Quoted 2 lines
drivers/net/ethernet/intel/e1000e/ptp.c static void e1000e_systim_overflow_work(struct work_struct *work) · line 245
Quoted 2 lines
drivers/net/ethernet/intel/e1000e/ptp.c +0
-0e_dbg("SYSTIM overflow check at %lld.%09lu\n",
Thanks,
Signature · 1 line
Miroslav Lichvar
Brown
RFC PATCH 1/1 e1000e: allow non-monotonic SYSTIM readings
In reply to Miroslav LichvarQuoted 24 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Miroslav Lichvar
Sent: Wednesday, October 17, 2018 9:16 AM
To: intel-wired-lan at lists.osuosl.org
Cc: Richard Cochran <[email protected]>
Subject: [Intel-wired-lan] [RFC PATCH] e1000e: allow non-monotonic SYSTIM readingsIt seems with some NICs supported by the e1000e driver a SYSTIM reading may occasionally be few microseconds before the previous reading and if enabled also pass
e1000e_sanitize_systim()without reaching the maximum number of rereads, even if the function is modified to check three consecutive readings (i.e. it doesn't look like a double read error). This causes an underflow in the timecounter and the PHC time jumps hours ahead.This was observed on 82574, I217 and I219. The fastest way to reproduce it is to run a program that continuously calls the
PTP_SYS_OFFSETioctl on the PHC.Modify
e1000e_phc_gettime()to usetimecounter_cyc2time()instead oftimecounter_read()in order to allow non-monotonic SYSTIM readings and prevent the PHC from jumping.
Quoted 7 lines
Cc Jacob Keller <[email protected]>Cc Richard Cochran <[email protected]>Signed-off-by Miroslav Lichvar <[email protected]>--- drivers/net/ethernet/intel/e1000e/ptp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)