Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1446713 > unrolled thread

[PATCH net] e1000e: fix PTP on e1000_pch_lpt variants

Back to article view | Back to linux.kernel

Contents

  [PATCH net] e1000e: fix PTP on e1000_pch_lpt variants Jarod Wilson <jarod@redhat.com> - 2016-07-19 22:30 +0200
    Re: [Intel-wired-lan] [PATCH net] e1000e: fix PTP on e1000_pch_lpt  variants "Rustad, Mark D" <mark.d.rustad@intel.com> - 2016-07-19 22:50 +0200
      Re: [Intel-wired-lan] [PATCH net] e1000e: fix PTP on e1000_pch_lpt  variants Jarod Wilson <jarod@redhat.com> - 2016-07-20 19:10 +0200
    Re: [PATCH net] e1000e: fix PTP on e1000_pch_lpt variants Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-07-20 13:10 +0200
    [PATCH net-next 1/2] e1000e: factor out systim sanitization Jarod Wilson <jarod@redhat.com> - 2016-07-23 18:50 +0200
    [PATCH v2 net-next 0/2] e1000e: fix PTP on e1000_pch_variants Jarod Wilson <jarod@redhat.com> - 2016-07-23 18:50 +0200
      [PATCH net-next 2/2] e1000e: fix PTP on e1000_pch_lpt variants Jarod Wilson <jarod@redhat.com> - 2016-07-23 18:50 +0200
      Re: [Intel-wired-lan] [PATCH v2 net-next 0/2] e1000e: fix PTP on  e1000_pch_variants Jesse Brandeburg <jesse.brandeburg@intel.com> - 2016-07-25 20:00 +0200

#1446713 — [PATCH net] 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. 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

[toc] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1447159

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

[toc] | [prev] | [next] | [standalone]

#1448964 — [PATCH net-next 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 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1448966 — [PATCH net-next 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.

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web