| Message ID | 20240217010455.58258-3-vinicius.gomes@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net,v1,1/2] igc: Fix missing time sync events | expand |
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
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>
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,
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)
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(-)