[Intel-wired-lan] [iwl-net v1 0/2] igc/igb: Fix missing time sync events

[Intel-wired-lan] [iwl-net v1 0/2] igc/igb: Fix missing time sync events

[Vinicius Costa Gomes]

Hi,

It was reported that i225/i226 could sometimes miss some time sync
events when two or more types of events (PPS and Timestamps was used
by the reporter) are being used at the same time under heavy traffic.

The core issue is that the driver was double clearing interrupts, as
the register is both "read to clear" and "write 1 to clear"
(documented in section 8.16.1 of the datasheet), and the handler was
doing both. Which could cause events to be missed if they happen
between the "read" and the "write".

Removing the write fixes the issue.

It was tracked down to commit 2c344ae24501 ("igc: Add support for TX
timestamping"), in which I added support for basic timestamp
operations, the issue is that as the hardware operates very similarly
to i210, I used igb code as inspiration. And indeed, the same double
clearing is present there.

But in the igb case, I haven't seen myself or heard about any issues
that seem related to this. So I think it's more like a possible issue.
But it seems like a good idea to fix it there was well.


Vinicius Costa Gomes (2):
  igc: Fix missing time sync events
  igb: Fix missing time sync events

 drivers/net/ethernet/intel/igb/igb_main.c | 23 +++++------------------
 drivers/net/ethernet/intel/igc/igc_main.c | 12 +-----------
 2 files changed, 6 insertions(+), 29 deletions(-)

-- 
2.43.2

[Intel-wired-lan] [iwl-net v1 1/2] igc: Fix missing time sync events

[Vinicius Costa Gomes]

Fix "double" clearing of interrupts, which can cause external events
or timestamps to be missed.

The IGC_TSIRC Time Sync Interrupt Cause register can be cleared in two
ways, by either reading it or by writing '1' into the specific cause
bit. This is documented in section 8.16.1.

The following flow was used:
 1. read IGC_TSIRC into 'tsicr';
 2. handle the interrupts present in 'tsirc' and mark them in 'ack';
 3. write 'ack' into IGC_TSICR;

As both (1) and (3) will clear the interrupt cause, if an interrupt
happens between (1) and (3) it will be ignored, causing events to be
missed.

Remove the extra clear in (3).

Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index ba8d3fe186ae..39b6a8d64de3 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -5302,25 +5302,22 @@ igc_features_check(struct sk_buff *skb, struct net_device *dev,
 
 static void igc_tsync_interrupt(struct igc_adapter *adapter)
 {
-	u32 ack, tsauxc, sec, nsec, tsicr;
 	struct igc_hw *hw = &adapter->hw;
+	u32 tsauxc, sec, nsec, tsicr;
 	struct ptp_clock_event event;
 	struct timespec64 ts;
 
 	tsicr = rd32(IGC_TSICR);
-	ack = 0;
 
 	if (tsicr & IGC_TSICR_SYS_WRAP) {
 		event.type = PTP_CLOCK_PPS;
 		if (adapter->ptp_caps.pps)
 			ptp_clock_event(adapter->ptp_clock, &event);
-		ack |= IGC_TSICR_SYS_WRAP;
 	}
 
 	if (tsicr & IGC_TSICR_TXTS) {
 		/* retrieve hardware timestamp */
 		igc_ptp_tx_tstamp_event(adapter);
-		ack |= IGC_TSICR_TXTS;
 	}
 
 	if (tsicr & IGC_TSICR_TT0) {
@@ -5334,7 +5331,6 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
 		wr32(IGC_TSAUXC, tsauxc);
 		adapter->perout[0].start = ts;
 		spin_unlock(&adapter->tmreg_lock);
-		ack |= IGC_TSICR_TT0;
 	}
 
 	if (tsicr & IGC_TSICR_TT1) {
@@ -5348,7 +5344,6 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
 		wr32(IGC_TSAUXC, tsauxc);
 		adapter->perout[1].start = ts;
 		spin_unlock(&adapter->tmreg_lock);
-		ack |= IGC_TSICR_TT1;
 	}
 
 	if (tsicr & IGC_TSICR_AUTT0) {
@@ -5358,7 +5353,6 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
 		event.index = 0;
 		event.timestamp = sec * NSEC_PER_SEC + nsec;
 		ptp_clock_event(adapter->ptp_clock, &event);
-		ack |= IGC_TSICR_AUTT0;
 	}
 
 	if (tsicr & IGC_TSICR_AUTT1) {
@@ -5368,11 +5362,7 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
 		event.index = 1;
 		event.timestamp = sec * NSEC_PER_SEC + nsec;
 		ptp_clock_event(adapter->ptp_clock, &event);
-		ack |= IGC_TSICR_AUTT1;
 	}
-
-	/* acknowledge the interrupts */
-	wr32(IGC_TSICR, ack);
 }
 
 /**
-- 
2.43.2

Re: [Intel-wired-lan] [iwl-net v1 1/2] igc: Fix missing time sync events

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Fri Feb 16 2024, Vinicius Costa Gomes wrote:
> Fix "double" clearing of interrupts, which can cause external events
> or timestamps to be missed.
>
> The IGC_TSIRC Time Sync Interrupt Cause register can be cleared in two
> ways, by either reading it or by writing '1' into the specific cause
> bit. This is documented in section 8.16.1.
>
> The following flow was used:
>  1. read IGC_TSIRC into 'tsicr';
>  2. handle the interrupts present in 'tsirc' and mark them in 'ack';
>  3. write 'ack' into IGC_TSICR;
>
> As both (1) and (3) will clear the interrupt cause, if an interrupt
> happens between (1) and (3) it will be ignored, causing events to be
> missed.
>
> Remove the extra clear in (3).
>
> Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

No obvious issues found while testing.

Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
Tested-by: Kurt Kanzenbach <kurt@linutronix.de> # Intel i225

Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

[Intel-wired-lan] [iwl-net v1 2/2] igb: Fix missing time sync events

[Vinicius Costa Gomes]

Fix "double" clearing of interrupts, which can cause external events
or timestamps to be missed.

The E1000_TSIRC Time Sync Interrupt Cause register can be cleared in two
ways, by either reading it or by writing '1' into the specific cause
bit. This is documented in section 8.16.1.

The following flow was used:
    1. read E1000_TSIRC into 'tsicr';
    2. handle the interrupts present into 'tsirc' and mark them in 'ack';
    3. write 'ack' into E1000_TSICR;

As both (1) and (3) will clear the interrupt cause, if an interrupt
happens between (1) and (3) it will be ignored, causing events to be
missed.

Remove the extra clear in (3).

Fixes: 00c65578b47b ("igb: enable internal PPS for the i210")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index cebb44f51d5f..7662c42e35c1 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6985,44 +6985,31 @@ static void igb_extts(struct igb_adapter *adapter, int tsintr_tt)
 static void igb_tsync_interrupt(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
-	u32 ack = 0, tsicr = rd32(E1000_TSICR);
+	u32 tsicr = rd32(E1000_TSICR);
 	struct ptp_clock_event event;
 
 	if (tsicr & TSINTR_SYS_WRAP) {
 		event.type = PTP_CLOCK_PPS;
 		if (adapter->ptp_caps.pps)
 			ptp_clock_event(adapter->ptp_clock, &event);
-		ack |= TSINTR_SYS_WRAP;
 	}
 
 	if (tsicr & E1000_TSICR_TXTS) {
 		/* retrieve hardware timestamp */
 		schedule_work(&adapter->ptp_tx_work);
-		ack |= E1000_TSICR_TXTS;
 	}
 
-	if (tsicr & TSINTR_TT0) {
+	if (tsicr & TSINTR_TT0)
 		igb_perout(adapter, 0);
-		ack |= TSINTR_TT0;
-	}
 
-	if (tsicr & TSINTR_TT1) {
+	if (tsicr & TSINTR_TT1)
 		igb_perout(adapter, 1);
-		ack |= TSINTR_TT1;
-	}
 
-	if (tsicr & TSINTR_AUTT0) {
+	if (tsicr & TSINTR_AUTT0)
 		igb_extts(adapter, 0);
-		ack |= TSINTR_AUTT0;
-	}
 
-	if (tsicr & TSINTR_AUTT1) {
+	if (tsicr & TSINTR_AUTT1)
 		igb_extts(adapter, 1);
-		ack |= TSINTR_AUTT1;
-	}
-
-	/* acknowledge the interrupts */
-	wr32(E1000_TSICR, ack);
 }
 
 static irqreturn_t igb_msix_other(int irq, void *data)
-- 
2.43.2

Re: [Intel-wired-lan] [iwl-net v1 2/2] igb: Fix missing time sync events

[Richard Cochran]

On Fri, Feb 16, 2024 at 05:04:54PM -0800, Vinicius Costa Gomes wrote:
> Fix "double" clearing of interrupts, which can cause external events
> or timestamps to be missed.
> 
> The E1000_TSIRC Time Sync Interrupt Cause register can be cleared in two
> ways, by either reading it or by writing '1' into the specific cause
> bit. This is documented in section 8.16.1.

That is not what the doc says.

It says this:

    Note: Once ICR.Time_Sync is set, the internal value of this
          register should be cleared by writing 1b to *all* bits
          or cleared by a read to enable receiving an additional
          ICR.Time_Sync interrupt.

    - Intel® Ethernet Controller I210 Datasheet,
      Revision Number: 3.1, June 2017, page 469

It says *all* bits.  This implies that the interrupt sources may be
cleared individually.  Because the non-acked bits are still pending,
the level interrupt should still be active, and the "missing" event
will be handled at the next invocation of the handler.

 
> The following flow was used:
>     1. read E1000_TSIRC into 'tsicr';
>     2. handle the interrupts present into 'tsirc' and mark them in 'ack';
>     3. write 'ack' into E1000_TSICR;
> 
> As both (1) and (3) will clear the interrupt cause, if an interrupt
> happens between (1) and (3) it will be ignored, causing events to be
> missed.

Are you sure?

Does setting ICR.Time_Sync[TXTS] also clear ICR.Time_Sync[RXTS] ?

That is what you seem to be saying.

Thanks,
Richard

Re: [Intel-wired-lan] [iwl-net v1 2/2] igb: Fix missing time sync events

[Richard Cochran]

On Sat, Feb 17, 2024 at 07:05:07AM -0800, Richard Cochran wrote:

> Does setting ICR.Time_Sync[TXTS] also clear ICR.Time_Sync[RXTS] ?
> 
> That is what you seem to be saying.

Okay, so you really mean that if the _same_ bit becomes set between
the read and the acknowledgment, then that event will be missed,
right?

In that case, thank you for fixing this more than nine year old bug!

Acked-by: Richard Cochran <richardcochran@gmail.com>

 

Re: [Intel-wired-lan] [iwl-net v1 2/2] igb: Fix missing time sync events

[Vinicius Costa Gomes]

Richard Cochran <richardcochran@gmail.com> writes:

> On Sat, Feb 17, 2024 at 07:05:07AM -0800, Richard Cochran wrote:
>
>> Does setting ICR.Time_Sync[TXTS] also clear ICR.Time_Sync[RXTS] ?
>> 
>> That is what you seem to be saying.
>
> Okay, so you really mean that if the _same_ bit becomes set between
> the read and the acknowledgment, then that event will be missed,
> right?
>

Yeah, I will reword the commit messages so it's clearer that it's the
same bit.

> In that case, thank you for fixing this more than nine year old bug!
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
>
>  

Thank you,
-- 
Vinicius

Re: [Intel-wired-lan] [iwl-net v1 0/2] igc/igb: Fix missing time sync events

[Richard Cochran]

FYI, this cover letter is missing netdev, lkml on CC.

Thanks,
Richard