[PATCH net] e1000e: fix PTP on e1000_pch_lpt variants

[PATCH net] e1000e: fix PTP on e1000_pch_lpt variants

[Jarod Wilson]

I've got reports that the Intel I-218V NIC in Intel NUC5i5RYH systems used
as a PTP slave experiences random ~10 hour clock jumps, which are resolved
if the same workaround for the 82574 and 82583 is employed. Switching from
an if to a select, because the list of NIC types could well grow further
and we'd already have to wrap the conditionals.

CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 2b2e2f8..866fea0 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4335,7 +4335,10 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systim = (cycle_t)systimel;
 	systim |= (cycle_t)systimeh << 32;
 
-	if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
+	switch (hw->mac.type) {
+	case e1000_82574:
+	case e1000_82583:
+	case e1000_pch_lpt:
 		u64 time_delta, rem, temp;
 		u32 incvalue;
 		int i;
@@ -4360,6 +4363,9 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 			    (rem == 0))
 				break;
 		}
+		break;
+	default:
+		break;
 	}
 	return systim;
 }
-- 
1.8.3.1

Re: [Intel-wired-lan] [PATCH net] e1000e: fix PTP on e1000_pch_lpt variants

[Rustad, Mark D]

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

Jarod Wilson <jarod@redhat.com> wrote:

> I've got reports that the Intel I-218V NIC in Intel NUC5i5RYH systems used
> as a PTP slave experiences random ~10 hour clock jumps, which are resolved
> if the same workaround for the 82574 and 82583 is employed. Switching from
> an if to a select, because the list of NIC types could well grow further
> and we'd already have to wrap the conditionals.
>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: intel-wired-lan@lists.osuosl.org
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c  
> b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 2b2e2f8..866fea0 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -4335,7 +4335,10 @@ static cycle_t e1000e_cyclecounter_read(const  
> struct cyclecounter *cc)
>  	systim = (cycle_t)systimel;
>  	systim |= (cycle_t)systimeh << 32;
>
> -	if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
> +	switch (hw->mac.type) {
> +	case e1000_82574:
> +	case e1000_82583:
> +	case e1000_pch_lpt:
>  		u64 time_delta, rem, temp;
>  		u32 incvalue;
>  		int i;

I don't think that it is acceptable to declare local variables inside a  
switch statement quite like this. At a minimum, a new block needs to be  
opened to allow the declarations.

> @@ -4360,6 +4363,9 @@ static cycle_t e1000e_cyclecounter_read(const  
> struct cyclecounter *cc)
>  			    (rem == 0))
>  				break;
>  		}
> +		break;
> +	default:
> +		break;
>  	}
>  	return systim;
>  }

--
Mark Rustad, Networking Division, Intel Corporation

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [Intel-wired-lan] [PATCH net] e1000e: fix PTP on e1000_pch_lpt variants

[Jarod Wilson]

On Tue, Jul 19, 2016 at 08:49:03PM +0000, Rustad, Mark D wrote:
> Jarod Wilson <jarod@redhat.com> wrote:
> 
> >I've got reports that the Intel I-218V NIC in Intel NUC5i5RYH systems used
> >as a PTP slave experiences random ~10 hour clock jumps, which are resolved
> >if the same workaround for the 82574 and 82583 is employed. Switching from
> >an if to a select, because the list of NIC types could well grow further
> >and we'd already have to wrap the conditionals.
> >
> >CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >CC: intel-wired-lan@lists.osuosl.org
> >CC: netdev@vger.kernel.org
> >Signed-off-by: Jarod Wilson <jarod@redhat.com>
> >---
> > drivers/net/ethernet/intel/e1000e/netdev.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> >b/drivers/net/ethernet/intel/e1000e/netdev.c
> >index 2b2e2f8..866fea0 100644
> >--- a/drivers/net/ethernet/intel/e1000e/netdev.c
> >+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> >@@ -4335,7 +4335,10 @@ static cycle_t
> >e1000e_cyclecounter_read(const struct cyclecounter *cc)
> > 	systim = (cycle_t)systimel;
> > 	systim |= (cycle_t)systimeh << 32;
> >
> >-	if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
> >+	switch (hw->mac.type) {
> >+	case e1000_82574:
> >+	case e1000_82583:
> >+	case e1000_pch_lpt:
> > 		u64 time_delta, rem, temp;
> > 		u32 incvalue;
> > 		int i;
> 
> I don't think that it is acceptable to declare local variables
> inside a switch statement quite like this. At a minimum, a new block
> needs to be opened to allow the declarations.

Gah, sorry, I think testing was done with an if, made a late change to a
switch without doing sufficient re-testing. I'll fix that up and re-test.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH net] e1000e: fix PTP on e1000_pch_lpt variants

[Sergei Shtylyov]

Hello.

On 7/19/2016 11:25 PM, Jarod Wilson wrote:

> I've got reports that the Intel I-218V NIC in Intel NUC5i5RYH systems used
> as a PTP slave experiences random ~10 hour clock jumps, which are resolved
> if the same workaround for the 82574 and 82583 is employed. Switching from
> an if to a select, because the list of NIC types could well grow further

    s/select/switch/?

> and we'd already have to wrap the conditionals.
>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: intel-wired-lan@lists.osuosl.org
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
[...]

MBR, Sergei

[PATCH v2 net-next 0/2] e1000e: fix PTP on e1000_pch_variants

[Jarod Wilson]

This little series factors out the systim sanitization code first, then
adds e1000_pch_lpt as a new case in the switch that calls the sanitize
function, fixing PTP clock issues I've had reported against an Intel
I-218V NIC in an Intel NUC5ik5RYH system.

Jarod Wilson (2):
  e1000e: factor out systim sanitization
  e1000e: fix PTP on e1000_pch_lpt variants

Note: this series replaces the previously submitted singleton patch that
was, er, broken, titled the same.

Reported-by: Rupesh Patel <rupatel@redhat.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>

 drivers/net/ethernet/intel/e1000e/netdev.c | 72 +++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 27 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/2] e1000e: factor out systim sanitization

[Jarod Wilson]

This is prepatory work for an expanding list of adapter families that have
occasional ~10 hour clock jumps when being used for PTP. Factor out the
sanitization function and convert to a switch statement, rather than using
an if.

CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 71 ++++++++++++++++++------------
 1 file changed, 44 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 41f32c0..955b294 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4303,6 +4303,42 @@ void e1000e_reinit_locked(struct e1000_adapter *adapter)
 }
 
 /**
+ * e1000e_sanitize_systim - sanitize raw cycle counter reads
+ * @hw: pointer to the HW structure
+ * @systim: cycle_t value read, sanitized and returned
+ *
+ * Errata for 82574/82583 possible bad bits read from SYSTIMH/L:
+ * check to see that the time is incrementing at a reasonable
+ * rate and is a multiple of incvalue.
+ **/
+static cycle_t e1000e_sanitize_systim(struct e1000_hw *hw, cycle_t systim)
+{
+	u64 time_delta, rem, temp;
+	cycle_t systim_next;
+	u32 incvalue;
+	int i;
+
+	incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
+	for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
+		/* latch SYSTIMH on read of SYSTIML */
+		systim_next = (cycle_t)er32(SYSTIML);
+		systim_next |= (cycle_t)er32(SYSTIMH) << 32;
+
+		time_delta = systim_next - systim;
+		temp = time_delta;
+		/* VMWare users have seen incvalue of zero, don't div / 0 */
+		rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);
+
+		systim = systim_next;
+
+		if ((time_delta < E1000_82574_SYSTIM_EPSILON) && (rem == 0))
+			break;
+	}
+
+	return systim;
+}
+
+/**
  * e1000e_cyclecounter_read - read raw cycle counter (used by time counter)
  * @cc: cyclecounter structure
  **/
@@ -4312,7 +4348,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 						     cc);
 	struct e1000_hw *hw = &adapter->hw;
 	u32 systimel, systimeh;
-	cycle_t systim, systim_next;
+	cycle_t systim;
 	/* SYSTIMH latching upon SYSTIML read does not work well.
 	 * This means that if SYSTIML overflows after we read it but before
 	 * we read SYSTIMH, the value of SYSTIMH has been incremented and we
@@ -4335,32 +4371,13 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systim = (cycle_t)systimel;
 	systim |= (cycle_t)systimeh << 32;
 
-	if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
-		u64 time_delta, rem, temp;
-		u32 incvalue;
-		int i;
-
-		/* errata for 82574/82583 possible bad bits read from SYSTIMH/L
-		 * check to see that the time is incrementing at a reasonable
-		 * rate and is a multiple of incvalue
-		 */
-		incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
-		for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
-			/* latch SYSTIMH on read of SYSTIML */
-			systim_next = (cycle_t)er32(SYSTIML);
-			systim_next |= (cycle_t)er32(SYSTIMH) << 32;
-
-			time_delta = systim_next - systim;
-			temp = time_delta;
-			/* VMWare users have seen incvalue of zero, don't div / 0 */
-			rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);
-
-			systim = systim_next;
-
-			if ((time_delta < E1000_82574_SYSTIM_EPSILON) &&
-			    (rem == 0))
-				break;
-		}
+	switch (hw->mac.type) {
+	case e1000_82574:
+	case e1000_82583:
+		systim = e1000e_sanitize_systim(hw, systim);
+		break;
+	default:
+		break;
 	}
 	return systim;
 }
-- 
1.8.3.1

[PATCH net-next 2/2] e1000e: fix PTP on e1000_pch_lpt variants

[Jarod Wilson]

I've got reports that the Intel I-218V NIC in Intel NUC5i5RYH systems used
as a PTP slave experiences random ~10 hour clock jumps, which are resolved
if the same workaround for the 82574 and 82583 is employed.

Reported-by: Rupesh Patel <rupatel@redhat.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 955b294..206bd6a 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4374,6 +4374,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	switch (hw->mac.type) {
 	case e1000_82574:
 	case e1000_82583:
+	case e1000_pch_lpt:
 		systim = e1000e_sanitize_systim(hw, systim);
 		break;
 	default:
-- 
1.8.3.1

Re: [Intel-wired-lan] [PATCH v2 net-next 0/2] e1000e: fix PTP on e1000_pch_variants

[Jesse Brandeburg]

On Sat, 23 Jul 2016 12:44:32 -0400
Jarod Wilson <jarod@redhat.com> wrote:

> This little series factors out the systim sanitization code first, then
> adds e1000_pch_lpt as a new case in the switch that calls the sanitize
> function, fixing PTP clock issues I've had reported against an Intel
> I-218V NIC in an Intel NUC5ik5RYH system.
> 
> Jarod Wilson (2):
>   e1000e: factor out systim sanitization
>   e1000e: fix PTP on e1000_pch_lpt variants
> 

Thanks for your patch Jarod, the refactor itself is fine and a good
idea, and thanks for working on the fix!

This code should have been using a feature flag, and the alert that
you're having to add more device IDs to the switch statement makes it
even more obvious.

Please see line 406 of e1000e/e1000.h, where the flags are declared,
add a flag for this workaround (to flags2), and and add some code in the
e1000_info_tbl entry to set the flag for the appropriate mac(s)

Then the runtime code should only check the flag, and if any further
devices require the workaround we just add the flag to that device, or
if this is init code, just always call the workaround funtion and have
the function itself make sure the right flags2 is set or return.

The code has somehow gotten away from this model in some places and any
new code we add should be doing it the right way.

Thanks,
 Jesse

[PATCH net-next 3/2] e1000e: convert systim overflow check to use flags2

[Jarod Wilson]

Per Jesse Brandeburg, e1000e should be using flags for this sort of thing.

Littering functional code with device-specific checks is much messier than
simply checking a flag, and having device-specific init set flags as needed.
There are probably a number of other cases in the e1000e code that
could/should be converted similarly.

At present, this patch is a follow-up to the previously submitted 2
patches, but I can redo the series to omit the conversion to a switch
entirely, if so desired.

Suggested-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/82571.c   |  6 ++++--
 drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
 drivers/net/ethernet/intel/e1000e/ich8lan.c |  3 ++-
 drivers/net/ethernet/intel/e1000e/netdev.c  | 10 ++--------
 4 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/82571.c b/drivers/net/ethernet/intel/e1000e/82571.c
index 7fd4d54..6b03c85 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c
@@ -2032,7 +2032,8 @@ const struct e1000_info e1000_82574_info = {
 				  | FLAG2_DISABLE_ASPM_L0S
 				  | FLAG2_DISABLE_ASPM_L1
 				  | FLAG2_NO_DISABLE_RX
-				  | FLAG2_DMA_BURST,
+				  | FLAG2_DMA_BURST
+				  | FLAG2_CHECK_SYSTIM_OVERFLOW,
 	.pba			= 32,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
 	.get_variants		= e1000_get_variants_82571,
@@ -2053,7 +2054,8 @@ const struct e1000_info e1000_82583_info = {
 				  | FLAG_HAS_CTRLEXT_ON_LOAD,
 	.flags2			= FLAG2_DISABLE_ASPM_L0S
 				  | FLAG2_DISABLE_ASPM_L1
-				  | FLAG2_NO_DISABLE_RX,
+				  | FLAG2_NO_DISABLE_RX
+				  | FLAG2_CHECK_SYSTIM_OVERFLOW,
 	.pba			= 32,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
 	.get_variants		= e1000_get_variants_82571,
diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index ef96cd1..879cca4 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -452,6 +452,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca);
 #define FLAG2_PCIM2PCI_ARBITER_WA         BIT(11)
 #define FLAG2_DFLT_CRC_STRIPPING          BIT(12)
 #define FLAG2_CHECK_RX_HWTSTAMP           BIT(13)
+#define FLAG2_CHECK_SYSTIM_OVERFLOW       BIT(14)
 
 #define E1000_RX_DESC_PS(R, i)	    \
 	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 3e11322..395ca26 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -5885,7 +5885,8 @@ const struct e1000_info e1000_pch_lpt_info = {
 				  | FLAG_HAS_JUMBO_FRAMES
 				  | FLAG_APME_IN_WUC,
 	.flags2			= FLAG2_HAS_PHY_STATS
-				  | FLAG2_HAS_EEE,
+				  | FLAG2_HAS_EEE
+				  | FLAGS2_CHECK_SYSTIM_OVERFLOW,
 	.pba			= 26,
 	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 206bd6a..1b2df11 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4371,15 +4371,9 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systim = (cycle_t)systimel;
 	systim |= (cycle_t)systimeh << 32;
 
-	switch (hw->mac.type) {
-	case e1000_82574:
-	case e1000_82583:
-	case e1000_pch_lpt:
+	if (adapter->flags2 & FLAG2_CHECK_SYSTIM_OVERFLOW)
 		systim = e1000e_sanitize_systim(hw, systim);
-		break;
-	default:
-		break;
-	}
+
 	return systim;
 }
 
-- 
1.8.3.1

Re: [PATCH net-next 3/2] e1000e: convert systim overflow check to use flags2

[Jeff Kirsher]

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

On Tue, 2016-07-26 at 13:39 -0400, Jarod Wilson wrote:
> Per Jesse Brandeburg, e1000e should be using flags for this sort of
> thing.
> 
> Littering functional code with device-specific checks is much messier
> than
> simply checking a flag, and having device-specific init set flags as
> needed.
> There are probably a number of other cases in the e1000e code that
> could/should be converted similarly.
> 
> At present, this patch is a follow-up to the previously submitted 2
> patches, but I can redo the series to omit the conversion to a switch
> entirely, if so desired.
> 
> Suggested-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: intel-wired-lan@lists.osuosl.org
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/net/ethernet/intel/e1000e/82571.c   |  6 ++++--
>  drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
>  drivers/net/ethernet/intel/e1000e/ich8lan.c |  3 ++-
>  drivers/net/ethernet/intel/e1000e/netdev.c  | 10 ++--------
>  4 files changed, 9 insertions(+), 11 deletions(-)

This generates a compile issue, so I will be dropping this patch and will
await an update.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH net-next 3/2] e1000e: convert systim overflow check to use flags2

[Jarod Wilson]

On Tue, Jul 26, 2016 at 10:53:55AM -0700, Jeff Kirsher wrote:
> On Tue, 2016-07-26 at 13:39 -0400, Jarod Wilson wrote:
> > Per Jesse Brandeburg, e1000e should be using flags for this sort of
> > thing.
> > 
> > Littering functional code with device-specific checks is much messier
> > than
> > simply checking a flag, and having device-specific init set flags as
> > needed.
> > There are probably a number of other cases in the e1000e code that
> > could/should be converted similarly.
> > 
> > At present, this patch is a follow-up to the previously submitted 2
> > patches, but I can redo the series to omit the conversion to a switch
> > entirely, if so desired.
> > 
> > Suggested-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > CC: intel-wired-lan@lists.osuosl.org
> > CC: netdev@vger.kernel.org
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > ---
> >  drivers/net/ethernet/intel/e1000e/82571.c   |  6 ++++--
> >  drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
> >  drivers/net/ethernet/intel/e1000e/ich8lan.c |  3 ++-
> >  drivers/net/ethernet/intel/e1000e/netdev.c  | 10 ++--------
> >  4 files changed, 9 insertions(+), 11 deletions(-)
> 
> This generates a compile issue, so I will be dropping this patch and will
> await an update.

Yeah, just sent a note off-list, I really shouldn't send patches while on
conference calls. It's a typo in ich8lan.c, where I put FLAGS2_ instead of
FLAG2_. I'll just redo the entire set omitting the switch conversion.

-- 
Jarod Wilson
jarod@redhat.com

[PATCH v3 net-next 0/2] e1000e: fix PTP on e1000_pch_variants

[Jarod Wilson]

This little series factors out the systim sanitization code first, then
adds e1000_pch_lpt as a new case in the switch that calls the sanitize
function, fixing PTP clock issues I've had reported against an Intel
I-218V NIC in an Intel NUC5ik5RYH system.

Jarod Wilson (2):
  e1000e: factor out systim sanitization
  e1000e: fix PTP on e1000_pch_lpt variants

Note: this series replaces the previously submitted singleton patch that
was, er, broken, titled the same.

v3: set and use feature flag to decide to call sanitize function

Reported-by: Rupesh Patel <rupatel@redhat.com>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>

 drivers/net/ethernet/intel/e1000e/netdev.c | 72 +++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 27 deletions(-)

-- 
1.8.3.1

[PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Jarod Wilson]

This is prepatory work for an expanding list of adapter families that have
occasional ~10 hour clock jumps when being used for PTP. Factor out the
sanitization function and convert to using a feature (bug) flag, per
suggestion from Jesse Brandeburg.

Littering functional code with device-specific checks is much messier than
simply checking a flag, and having device-specific init set flags as needed.
There are probably a number of other cases in the e1000e code that
could/should be converted similarly.

Suggested-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/82571.c  |  6 ++-
 drivers/net/ethernet/intel/e1000e/e1000.h  |  1 +
 drivers/net/ethernet/intel/e1000e/netdev.c | 66 ++++++++++++++++++------------
 3 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/82571.c b/drivers/net/ethernet/intel/e1000e/82571.c
index 7fd4d54..6b03c85 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c
@@ -2032,7 +2032,8 @@ const struct e1000_info e1000_82574_info = {
 				  | FLAG2_DISABLE_ASPM_L0S
 				  | FLAG2_DISABLE_ASPM_L1
 				  | FLAG2_NO_DISABLE_RX
-				  | FLAG2_DMA_BURST,
+				  | FLAG2_DMA_BURST
+				  | FLAG2_CHECK_SYSTIM_OVERFLOW,
 	.pba			= 32,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
 	.get_variants		= e1000_get_variants_82571,
@@ -2053,7 +2054,8 @@ const struct e1000_info e1000_82583_info = {
 				  | FLAG_HAS_CTRLEXT_ON_LOAD,
 	.flags2			= FLAG2_DISABLE_ASPM_L0S
 				  | FLAG2_DISABLE_ASPM_L1
-				  | FLAG2_NO_DISABLE_RX,
+				  | FLAG2_NO_DISABLE_RX
+				  | FLAG2_CHECK_SYSTIM_OVERFLOW,
 	.pba			= 32,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
 	.get_variants		= e1000_get_variants_82571,
diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index ef96cd1..879cca4 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -452,6 +452,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca);
 #define FLAG2_PCIM2PCI_ARBITER_WA         BIT(11)
 #define FLAG2_DFLT_CRC_STRIPPING          BIT(12)
 #define FLAG2_CHECK_RX_HWTSTAMP           BIT(13)
+#define FLAG2_CHECK_SYSTIM_OVERFLOW       BIT(14)
 
 #define E1000_RX_DESC_PS(R, i)	    \
 	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 41f32c0..1b2df11 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4303,6 +4303,42 @@ void e1000e_reinit_locked(struct e1000_adapter *adapter)
 }
 
 /**
+ * e1000e_sanitize_systim - sanitize raw cycle counter reads
+ * @hw: pointer to the HW structure
+ * @systim: cycle_t value read, sanitized and returned
+ *
+ * Errata for 82574/82583 possible bad bits read from SYSTIMH/L:
+ * check to see that the time is incrementing at a reasonable
+ * rate and is a multiple of incvalue.
+ **/
+static cycle_t e1000e_sanitize_systim(struct e1000_hw *hw, cycle_t systim)
+{
+	u64 time_delta, rem, temp;
+	cycle_t systim_next;
+	u32 incvalue;
+	int i;
+
+	incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
+	for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
+		/* latch SYSTIMH on read of SYSTIML */
+		systim_next = (cycle_t)er32(SYSTIML);
+		systim_next |= (cycle_t)er32(SYSTIMH) << 32;
+
+		time_delta = systim_next - systim;
+		temp = time_delta;
+		/* VMWare users have seen incvalue of zero, don't div / 0 */
+		rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);
+
+		systim = systim_next;
+
+		if ((time_delta < E1000_82574_SYSTIM_EPSILON) && (rem == 0))
+			break;
+	}
+
+	return systim;
+}
+
+/**
  * e1000e_cyclecounter_read - read raw cycle counter (used by time counter)
  * @cc: cyclecounter structure
  **/
@@ -4312,7 +4348,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 						     cc);
 	struct e1000_hw *hw = &adapter->hw;
 	u32 systimel, systimeh;
-	cycle_t systim, systim_next;
+	cycle_t systim;
 	/* SYSTIMH latching upon SYSTIML read does not work well.
 	 * This means that if SYSTIML overflows after we read it but before
 	 * we read SYSTIMH, the value of SYSTIMH has been incremented and we
@@ -4335,33 +4371,9 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systim = (cycle_t)systimel;
 	systim |= (cycle_t)systimeh << 32;
 
-	if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
-		u64 time_delta, rem, temp;
-		u32 incvalue;
-		int i;
-
-		/* errata for 82574/82583 possible bad bits read from SYSTIMH/L
-		 * check to see that the time is incrementing at a reasonable
-		 * rate and is a multiple of incvalue
-		 */
-		incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
-		for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
-			/* latch SYSTIMH on read of SYSTIML */
-			systim_next = (cycle_t)er32(SYSTIML);
-			systim_next |= (cycle_t)er32(SYSTIMH) << 32;
-
-			time_delta = systim_next - systim;
-			temp = time_delta;
-			/* VMWare users have seen incvalue of zero, don't div / 0 */
-			rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);
-
-			systim = systim_next;
+	if (adapter->flags2 & FLAG2_CHECK_SYSTIM_OVERFLOW)
+		systim = e1000e_sanitize_systim(hw, systim);
 
-			if ((time_delta < E1000_82574_SYSTIM_EPSILON) &&
-			    (rem == 0))
-				break;
-		}
-	}
 	return systim;
 }
 
-- 
1.8.3.1

RE: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Avargil, Raanan]

This is prepatory work for an expanding list of adapter families that have occasional ~10 hour clock jumps when being used for PTP. Factor out the sanitization function and convert to using a feature (bug) flag, per suggestion from Jesse Brandeburg.

Littering functional code with device-specific checks is much messier than simply checking a flag, and having device-specific init set flags as needed.
There are probably a number of other cases in the e1000e code that could/should be converted similarly.

Suggested-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/82571.c  |  6 ++-  drivers/net/ethernet/intel/e1000e/e1000.h  |  1 +  drivers/net/ethernet/intel/e1000e/netdev.c | 66 ++++++++++++++++++------------
 3 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/82571.c b/drivers/net/ethernet/intel/e1000e/82571.c
index 7fd4d54..6b03c85 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c
@@ -2032,7 +2032,8 @@ const struct e1000_info e1000_82574_info = {
 				  | FLAG2_DISABLE_ASPM_L0S
 				  | FLAG2_DISABLE_ASPM_L1
 				  | FLAG2_NO_DISABLE_RX
-				  | FLAG2_DMA_BURST,
+				  | FLAG2_DMA_BURST
+				  | FLAG2_CHECK_SYSTIM_OVERFLOW,
 	.pba			= 32,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
 	.get_variants		= e1000_get_variants_82571,
@@ -2053,7 +2054,8 @@ const struct e1000_info e1000_82583_info = {
 				  | FLAG_HAS_CTRLEXT_ON_LOAD,
 	.flags2			= FLAG2_DISABLE_ASPM_L0S
 				  | FLAG2_DISABLE_ASPM_L1
-				  | FLAG2_NO_DISABLE_RX,
+				  | FLAG2_NO_DISABLE_RX
+				  | FLAG2_CHECK_SYSTIM_OVERFLOW,
 	.pba			= 32,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
 	.get_variants		= e1000_get_variants_82571,
diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index ef96cd1..879cca4 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -452,6 +452,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca);
 #define FLAG2_PCIM2PCI_ARBITER_WA         BIT(11)
 #define FLAG2_DFLT_CRC_STRIPPING          BIT(12)
 #define FLAG2_CHECK_RX_HWTSTAMP           BIT(13)
+#define FLAG2_CHECK_SYSTIM_OVERFLOW       BIT(14)
 
 #define E1000_RX_DESC_PS(R, i)	    \
 	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i])) diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 41f32c0..1b2df11 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4303,6 +4303,42 @@ void e1000e_reinit_locked(struct e1000_adapter *adapter)  }
 
 /**
+ * e1000e_sanitize_systim - sanitize raw cycle counter reads
+ * @hw: pointer to the HW structure
+ * @systim: cycle_t value read, sanitized and returned
+ *
+ * Errata for 82574/82583 possible bad bits read from SYSTIMH/L:
+ * check to see that the time is incrementing at a reasonable
+ * rate and is a multiple of incvalue.
+ **/
+static cycle_t e1000e_sanitize_systim(struct e1000_hw *hw, cycle_t 
+systim) {
+	u64 time_delta, rem, temp;
+	cycle_t systim_next;
+	u32 incvalue;
+	int i;
+
+	incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
+	for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
+		/* latch SYSTIMH on read of SYSTIML */
+		systim_next = (cycle_t)er32(SYSTIML);
+		systim_next |= (cycle_t)er32(SYSTIMH) << 32;
+
+		time_delta = systim_next - systim;
+		temp = time_delta;
+		/* VMWare users have seen incvalue of zero, don't div / 0 */
+		rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);
+
+		systim = systim_next;
+
+		if ((time_delta < E1000_82574_SYSTIM_EPSILON) && (rem == 0))
+			break;
+	}
+
+	return systim;
+}
+
+/**
  * e1000e_cyclecounter_read - read raw cycle counter (used by time counter)
  * @cc: cyclecounter structure
  **/
@@ -4312,7 +4348,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 						     cc);
 	struct e1000_hw *hw = &adapter->hw;
 	u32 systimel, systimeh;
-	cycle_t systim, systim_next;
+	cycle_t systim;
 	/* SYSTIMH latching upon SYSTIML read does not work well.
 	 * This means that if SYSTIML overflows after we read it but before
 	 * we read SYSTIMH, the value of SYSTIMH has been incremented and we @@ -4335,33 +4371,9 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systim = (cycle_t)systimel;
 	systim |= (cycle_t)systimeh << 32;
 
-	if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
-		u64 time_delta, rem, temp;
-		u32 incvalue;
-		int i;
-
-		/* errata for 82574/82583 possible bad bits read from SYSTIMH/L
-		 * check to see that the time is incrementing at a reasonable
-		 * rate and is a multiple of incvalue
-		 */
-		incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
-		for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
-			/* latch SYSTIMH on read of SYSTIML */
-			systim_next = (cycle_t)er32(SYSTIML);
-			systim_next |= (cycle_t)er32(SYSTIMH) << 32;
-
-			time_delta = systim_next - systim;
-			temp = time_delta;
-			/* VMWare users have seen incvalue of zero, don't div / 0 */
-			rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);
-
-			systim = systim_next;
+	if (adapter->flags2 & FLAG2_CHECK_SYSTIM_OVERFLOW)
+		systim = e1000e_sanitize_systim(hw, systim);
 
-			if ((time_delta < E1000_82574_SYSTIM_EPSILON) &&
-			    (rem == 0))
-				break;
-		}
-	}
 	return systim;
 }
 
--
1.8.3.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Looks ok to me.
Adding Chris who asked what happens if we reach the max retry counter (E1000_MAX_82574_SYSTIM_REREAD)?
This counter is set to 50. 
Can you, for testing purposes, decreased this value (or even set it to 0) and see what happens?

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Jarod Wilson]

On Wed, Jul 27, 2016 at 02:09:13PM +0000, Avargil, Raanan wrote:
>> This is prepatory work for an expanding list of adapter families that have occasional ~10 hour clock jumps when being used for PTP. Factor out the sanitization function and convert to using a feature (bug) flag, per suggestion from Jesse Brandeburg.
>> 
>> Littering functional code with device-specific checks is much messier than simply checking a flag, and having device-specific init set flags as needed.
>> There are probably a number of other cases in the e1000e code that could/should be converted similarly.
> 
> Looks ok to me.
> Adding Chris who asked what happens if we reach the max retry counter (E1000_MAX_82574_SYSTIM_REREAD)?
> This counter is set to 50. 
> Can you, for testing purposes, decreased this value (or even set it to 0) and see what happens?

Unfortunately, I don't have direct access to the affected hardware myself,
so I'd have to prep a test build, hand it off to someone and play relay. I
could do that, but it'd have some lag and possible multiple round-trips...
Anyone inside Intel have hardware handy to test on? :p

-- 
Jarod Wilson
jarod@redhat.com

Re: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Jarod Wilson]

On Wed, Jul 27, 2016 at 11:01:55AM -0400, Jarod Wilson wrote:
> On Wed, Jul 27, 2016 at 02:09:13PM +0000, Avargil, Raanan wrote:
> >> This is prepatory work for an expanding list of adapter families that have occasional ~10 hour clock jumps when being used for PTP. Factor out the sanitization function and convert to using a feature (bug) flag, per suggestion from Jesse Brandeburg.
> >> 
> >> Littering functional code with device-specific checks is much messier than simply checking a flag, and having device-specific init set flags as needed.
> >> There are probably a number of other cases in the e1000e code that could/should be converted similarly.
> > 
> > Looks ok to me.
> > Adding Chris who asked what happens if we reach the max retry counter (E1000_MAX_82574_SYSTIM_REREAD)?
> > This counter is set to 50. 
> > Can you, for testing purposes, decreased this value (or even set it to 0) and see what happens?
> 
> Unfortunately, I don't have direct access to the affected hardware myself,
> so I'd have to prep a test build, hand it off to someone and play relay. I
> could do that, but it'd have some lag and possible multiple round-trips...
> Anyone inside Intel have hardware handy to test on? :p

Was tied up with other work the middle of last week, then on vacation for
a bit. There was some testing feedback provided from someone at neither
Red Hat or Intel, but I'm not sure where it leaves us right now. What
needs to happen next?

-- 
Jarod Wilson
jarod@redhat.com

RE: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Jarod Wilson
> Sent: Monday, August 1, 2016 6:32 PM
> To: Avargil, Raanan <raanan.avargil@intel.com>
> Cc: Hall, Christopher S <christopher.s.hall@intel.com>;
> netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out
> systim sanitization
> 
> On Wed, Jul 27, 2016 at 11:01:55AM -0400, Jarod Wilson wrote:
> > On Wed, Jul 27, 2016 at 02:09:13PM +0000, Avargil, Raanan wrote:
> > >> This is prepatory work for an expanding list of adapter families that have
> occasional ~10 hour clock jumps when being used for PTP. Factor out the
> sanitization function and convert to using a feature (bug) flag, per suggestion
> from Jesse Brandeburg.
> > >>
> > >> Littering functional code with device-specific checks is much messier
> than simply checking a flag, and having device-specific init set flags as
> needed.
> > >> There are probably a number of other cases in the e1000e code that
> could/should be converted similarly.
> > >
> > > Looks ok to me.
> > > Adding Chris who asked what happens if we reach the max retry counter
> (E1000_MAX_82574_SYSTIM_REREAD)?
> > > This counter is set to 50.
> > > Can you, for testing purposes, decreased this value (or even set it to 0)
> and see what happens?
> >
> > Unfortunately, I don't have direct access to the affected hardware myself,
> > so I'd have to prep a test build, hand it off to someone and play relay. I
> > could do that, but it'd have some lag and possible multiple round-trips...
> > Anyone inside Intel have hardware handy to test on? :p
> 
> Was tied up with other work the middle of last week, then on vacation for
> a bit. There was some testing feedback provided from someone at neither
> Red Hat or Intel, but I'm not sure where it leaves us right now. What
> needs to happen next?

Probably nothing else needs to be done on your end.  I was out for the last week and a half and am now running the patches through a series of regression test covering a fair number of the different e1000e parts.  I will also try to duplicate Tim Woodford' success on a NUC with an i218 in my lab.  Assuming nothing jumps out at me I'll probably give it a tested-by later this week so that Jeff can push it on up.

> 
> --
> Jarod Wilson
> jarod@redhat.com
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Jarod Wilson]

On Tue, Aug 02, 2016 at 07:33:01AM +0000, Brown, Aaron F wrote:
> > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> > Behalf Of Jarod Wilson
> > Sent: Monday, August 1, 2016 6:32 PM
> > To: Avargil, Raanan <raanan.avargil@intel.com>
> > Cc: Hall, Christopher S <christopher.s.hall@intel.com>;
> > netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out
> > systim sanitization
> > 
> > On Wed, Jul 27, 2016 at 11:01:55AM -0400, Jarod Wilson wrote:
> > > On Wed, Jul 27, 2016 at 02:09:13PM +0000, Avargil, Raanan wrote:
> > > >> This is prepatory work for an expanding list of adapter families that have
> > occasional ~10 hour clock jumps when being used for PTP. Factor out the
> > sanitization function and convert to using a feature (bug) flag, per suggestion
> > from Jesse Brandeburg.
> > > >>
> > > >> Littering functional code with device-specific checks is much messier
> > than simply checking a flag, and having device-specific init set flags as
> > needed.
> > > >> There are probably a number of other cases in the e1000e code that
> > could/should be converted similarly.
> > > >
> > > > Looks ok to me.
> > > > Adding Chris who asked what happens if we reach the max retry counter
> > (E1000_MAX_82574_SYSTIM_REREAD)?
> > > > This counter is set to 50.
> > > > Can you, for testing purposes, decreased this value (or even set it to 0)
> > and see what happens?
> > >
> > > Unfortunately, I don't have direct access to the affected hardware myself,
> > > so I'd have to prep a test build, hand it off to someone and play relay. I
> > > could do that, but it'd have some lag and possible multiple round-trips...
> > > Anyone inside Intel have hardware handy to test on? :p
> > 
> > Was tied up with other work the middle of last week, then on vacation for
> > a bit. There was some testing feedback provided from someone at neither
> > Red Hat or Intel, but I'm not sure where it leaves us right now. What
> > needs to happen next?
> 
> Probably nothing else needs to be done on your end.  I was out for the last week and a half and am now running the patches through a series of regression test covering a fair number of the different e1000e parts.  I will also try to duplicate Tim Woodford' success on a NUC with an i218 in my lab.  Assuming nothing jumps out at me I'll probably give it a tested-by later this week so that Jeff can push it on up.

Looking for a status update on this one, not seeing it pushed to DaveM
just yet.

-- 
Jarod Wilson
jarod@redhat.com

RE: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Woodford, Timothy W.]

>> This is prepatory work for an expanding list of adapter families that have occasional ~10 hour clock jumps when being used for PTP. Factor out the sanitization function and convert to using a feature (bug) flag, per suggestion from Jesse Brandeburg.
>> 
>> Littering functional code with device-specific checks is much messier than simply checking a flag, and having device-specific init set flags as needed.
>> There are probably a number of other cases in the e1000e code that could/should be converted similarly.
> 
> Looks ok to me.
> Adding Chris who asked what happens if we reach the max retry counter (E1000_MAX_82574_SYSTIM_REREAD)?
> This counter is set to 50. 
> Can you, for testing purposes, decreased this value (or even set it to 0) and see what happens?

I have one of the affected NICs.  I inserted some test code into the module to check the value of i after the for loop inside the sanitize function exits to check how many iterations of the loop were being run.  I ran a PTP test setup for a few hours with E1000_MAX_82574_SYSTIM_REREAD=50, and the maximum number was 5 iterations.
It looks like setting E1000_MAX_82574_SYSTIM_REREAD to 0 will just disable the return value checking and will have the same results as disabling the sanitization function.  I'll set the max retry counter to 1 and run an overnight test to see what happens.

Tim Woodford

RE: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Woodford, Timothy W.]

>>> This is prepatory work for an expanding list of adapter families that have occasional ~10 hour clock jumps when being used for PTP. Factor out the sanitization function and convert to using a feature (bug) flag, per suggestion from Jesse Brandeburg.
>>> 
>>> Littering functional code with device-specific checks is much messier than simply checking a flag, and having device-specific init set flags as needed.
>>> There are probably a number of other cases in the e1000e code that could/should be converted similarly.
>> 
>> Looks ok to me.
>> Adding Chris who asked what happens if we reach the max retry counter (E1000_MAX_82574_SYSTIM_REREAD)?
>> This counter is set to 50. 
>> Can you, for testing purposes, decreased this value (or even set it to 0) and see what happens?
>  I'll set the max retry counter to 1 and run an overnight test to see what happens.

After running with this configuration for about 36 hours, I haven't seen any timing jumps.  Either this configuration eliminates the error, or it makes it significantly less likely to occur.

Tim Woodford

RE: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Woodford, Timothy W.
> Sent: Friday, July 29, 2016 7:41 AM
> To: Woodford, Timothy W. <Timothy.Woodford@jhuapl.edu>; Avargil,
> Raanan <raanan.avargil@intel.com>; Jarod Wilson <jarod@redhat.com>;
> linux-kernel@vger.kernel.org; Hall, Christopher S
> <christopher.s.hall@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out
> systim sanitization
> 
> >>> This is prepatory work for an expanding list of adapter families that have
> occasional ~10 hour clock jumps when being used for PTP. Factor out the
> sanitization function and convert to using a feature (bug) flag, per suggestion
> from Jesse Brandeburg.
> >>>
> >>> Littering functional code with device-specific checks is much messier than
> simply checking a flag, and having device-specific init set flags as needed.
> >>> There are probably a number of other cases in the e1000e code that
> could/should be converted similarly.
> >>
> >> Looks ok to me.
> >> Adding Chris who asked what happens if we reach the max retry counter
> (E1000_MAX_82574_SYSTIM_REREAD)?
> >> This counter is set to 50.
> >> Can you, for testing purposes, decreased this value (or even set it to 0)
> and see what happens?
> >  I'll set the max retry counter to 1 and run an overnight test to see what
> happens.
> 
> After running with this configuration for about 36 hours, I haven't seen any
> timing jumps.  Either this configuration eliminates the error, or it makes it
> significantly less likely to occur.
> 
> Tim Woodford

Feel free to throw a Tested-by: on it if you like.  Not a big deal either way, I managed to get enough cycles in on it I'm pretty happy with it as well.

> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

RE: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim sanitization

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Jarod Wilson
> Sent: Tuesday, July 26, 2016 11:26 AM
> To: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH net-next v3 1/2] e1000e: factor out systim
> sanitization
> 
> This is prepatory work for an expanding list of adapter families that have
> occasional ~10 hour clock jumps when being used for PTP. Factor out the
> sanitization function and convert to using a feature (bug) flag, per
> suggestion from Jesse Brandeburg.
> 
> Littering functional code with device-specific checks is much messier than
> simply checking a flag, and having device-specific init set flags as needed.
> There are probably a number of other cases in the e1000e code that
> could/should be converted similarly.
> 
> Suggested-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: intel-wired-lan@lists.osuosl.org
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/net/ethernet/intel/e1000e/82571.c  |  6 ++-
>  drivers/net/ethernet/intel/e1000e/e1000.h  |  1 +
>  drivers/net/ethernet/intel/e1000e/netdev.c | 66 ++++++++++++++++++-------
> -----
>  3 files changed, 44 insertions(+), 29 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[PATCH net-next v3 2/2] e1000e: fix PTP on e1000_pch_lpt variants

[Jarod Wilson]

I've got reports that the Intel I-218V NIC in Intel NUC5i5RYH systems used
as a PTP slave experiences random ~10 hour clock jumps, which are resolved
if the same workaround for the 82574 and 82583 is employed, so set the
appropriate flag2 in e1000_pch_lpt_info too.

Reported-by: Rupesh Patel <rupatel@redhat.com>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: intel-wired-lan@lists.osuosl.org
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 3e11322..f3aaca7 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -5885,7 +5885,8 @@ const struct e1000_info e1000_pch_lpt_info = {
 				  | FLAG_HAS_JUMBO_FRAMES
 				  | FLAG_APME_IN_WUC,
 	.flags2			= FLAG2_HAS_PHY_STATS
-				  | FLAG2_HAS_EEE,
+				  | FLAG2_HAS_EEE
+				  | FLAG2_CHECK_SYSTIM_OVERFLOW,
 	.pba			= 26,
 	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
-- 
1.8.3.1

RE: [Intel-wired-lan] [PATCH net-next v3 2/2] e1000e: fix PTP on e1000_pch_lpt variants

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Jarod Wilson
> Sent: Tuesday, July 26, 2016 11:26 AM
> To: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH net-next v3 2/2] e1000e: fix PTP on
> e1000_pch_lpt variants
> 
> I've got reports that the Intel I-218V NIC in Intel NUC5i5RYH systems used
> as a PTP slave experiences random ~10 hour clock jumps, which are resolved
> if the same workaround for the 82574 and 82583 is employed, so set the
> appropriate flag2 in e1000_pch_lpt_info too.
> 
> Reported-by: Rupesh Patel <rupatel@redhat.com>
> CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: intel-wired-lan@lists.osuosl.org
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/net/ethernet/intel/e1000e/ich8lan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Re: [Intel-wired-lan] [PATCH net] e1000e: fix PTP on e1000_pch_lpt variants

[kbuild test robot]

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

Hi,

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Jarod-Wilson/e1000e-fix-PTP-on-e1000_pch_lpt-variants/20160725-040850
config: i386-randconfig-x001-201630 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   drivers/net/ethernet/intel/e1000e/netdev.c: In function 'e1000e_cyclecounter_read':
>> drivers/net/ethernet/intel/e1000e/netdev.c:4342:3: error: a label can only be part of a statement and a declaration is not a statement
      u64 time_delta, rem, temp;
      ^~~
>> drivers/net/ethernet/intel/e1000e/netdev.c:4343:3: error: expected expression before 'u32'
      u32 incvalue;
      ^~~
>> drivers/net/ethernet/intel/e1000e/netdev.c:4344:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
      int i;
      ^~~
>> drivers/net/ethernet/intel/e1000e/netdev.c:4350:3: error: 'incvalue' undeclared (first use in this function)
      incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
      ^~~~~~~~
   drivers/net/ethernet/intel/e1000e/netdev.c:4350:3: note: each undeclared identifier is reported only once for each function it appears in

vim +4342 drivers/net/ethernet/intel/e1000e/netdev.c

ab507c9a Denys Vlasenko 2016-04-20  4336  	systim |= (cycle_t)systimeh << 32;
b67e1913 Bruce Allan    2012-12-27  4337  
cf608919 Jarod Wilson   2016-07-19  4338  	switch (hw->mac.type) {
cf608919 Jarod Wilson   2016-07-19  4339  	case e1000_82574:
cf608919 Jarod Wilson   2016-07-19  4340  	case e1000_82583:
cf608919 Jarod Wilson   2016-07-19  4341  	case e1000_pch_lpt:
fb5277f2 Denys Vlasenko 2016-04-20 @4342  		u64 time_delta, rem, temp;
fb5277f2 Denys Vlasenko 2016-04-20 @4343  		u32 incvalue;
5e7ff970 Todd Fujinaka  2014-05-03 @4344  		int i;
5e7ff970 Todd Fujinaka  2014-05-03  4345  
5e7ff970 Todd Fujinaka  2014-05-03  4346  		/* errata for 82574/82583 possible bad bits read from SYSTIMH/L
5e7ff970 Todd Fujinaka  2014-05-03  4347  		 * check to see that the time is incrementing at a reasonable
5e7ff970 Todd Fujinaka  2014-05-03  4348  		 * rate and is a multiple of incvalue
5e7ff970 Todd Fujinaka  2014-05-03  4349  		 */
5e7ff970 Todd Fujinaka  2014-05-03 @4350  		incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
5e7ff970 Todd Fujinaka  2014-05-03  4351  		for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
5e7ff970 Todd Fujinaka  2014-05-03  4352  			/* latch SYSTIMH on read of SYSTIML */
5e7ff970 Todd Fujinaka  2014-05-03  4353  			systim_next = (cycle_t)er32(SYSTIML);

:::::: The code at line 4342 was first introduced by commit
:::::: fb5277f2c2e4db4a29740ff071072a688892d2df e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64

:::::: TO: Denys Vlasenko <dvlasenk@redhat.com>
:::::: CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 27508 bytes --]