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

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

[Daiwei Li]

Hi,

It appears this change breaks PTP on the 82580 controller, as ptp4l reports:

> timed out while polling for tx timestamp increasing tx_timestamp_timeout or
> increasing kworker priority may correct this issue, but a driver bug likely
> causes it

The 82580 controller has a hardware bug in which reading TSICR doesn't clear
it. See this thread
https://lore.kernel.org/all/CDCB8BE0.1EC2C%25matthew.vick@intel.com/ where the
bug was confirmed by an Intel employee. Any chance we could add back the ack
for 82580 specifically? Thanks!

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

[Vinicius Costa Gomes]

Daiwei Li <daiweili@gmail.com> writes:

> Hi,
>
> It appears this change breaks PTP on the 82580 controller, as ptp4l reports:
>
>> timed out while polling for tx timestamp increasing tx_timestamp_timeout or
>> increasing kworker priority may correct this issue, but a driver bug likely
>> causes it
>
> The 82580 controller has a hardware bug in which reading TSICR doesn't clear
> it. See this thread
> https://lore.kernel.org/all/CDCB8BE0.1EC2C%25matthew.vick@intel.com/ where the
> bug was confirmed by an Intel employee. Any chance we could add back the ack
> for 82580 specifically? Thanks!

Of course, I'll prepare a patch for that. 

Thanks for digging that one up.


Cheers,
-- 
Vinicius

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

[Daiwei Li]

> Of course, I'll prepare a patch for that.

Excellent, thank you!


On Fri, Aug 9, 2024 at 9:39 AM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Daiwei Li <daiweili@gmail.com> writes:
>
> > Hi,
> >
> > It appears this change breaks PTP on the 82580 controller, as ptp4l reports:
> >
> >> timed out while polling for tx timestamp increasing tx_timestamp_timeout or
> >> increasing kworker priority may correct this issue, but a driver bug likely
> >> causes it
> >
> > The 82580 controller has a hardware bug in which reading TSICR doesn't clear
> > it. See this thread
> > https://lore.kernel.org/all/CDCB8BE0.1EC2C%25matthew.vick@intel.com/ where the
> > bug was confirmed by an Intel employee. Any chance we could add back the ack
> > for 82580 specifically? Thanks!
>
> Of course, I'll prepare a patch for that.
>
> Thanks for digging that one up.
>
>
> Cheers,
> --
> Vinicius

[iwl-net v2 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 the same
interrupt happens again 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")
Acked-by: Richard Cochran <richardcochran@gmail.com>
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 v2 2/2] igb: Fix missing time sync events

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Vinicius Costa Gomes
> Sent: Wednesday, February 21, 2024 5:27 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Neftin, Sasha <sasha.neftin@intel.com>; Gomes, Vinicius <vinicius.gomes@intel.com>; netdev@vger.kernel.org; richardcochran@gmail.com; kurt@linutronix.de; Brandeburg, Jesse <jesse.brandeburg@intel.com>; linux-kernel@vger.kernel.org; Eric Dumazet <edumazet@google.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Jeff Kirsher <jeffrey.t.kirsher@intel.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [iwl-net v2 2/2] igb: Fix missing time sync events
>
> 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 the same
> interrupt happens again 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")
> Acked-by: Richard Cochran <richardcochran@gmail.com>
> 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(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)