[Intel-wired-lan] [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64

[Intel-wired-lan] [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64

[Denys Vlasenko]

"incvalue" variable holds a result of "er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK"
and used in "do_div(temp, incvalue)" as a divisor.

Thus, "u64 incvalue" declaration is probably a mistake.
Even though it seems to be a harmless one, let's fix it.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: intel-wired-lan at lists.osuosl.org
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 9b4ec13..967311b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4300,7 +4300,8 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	}
 
 	if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
-		u64 incvalue, time_delta, rem, temp;
+		u64 time_delta, rem, temp;
+		u32 incvalue;
 		int i;
 
 		/* errata for 82574/82583 possible bad bits read from SYSTIMH/L
-- 
1.8.1.4

[Intel-wired-lan] [PATCH 2/3] e1000e: e1000e_cyclecounter_read(): fix er32(SYSTIML) overflow check

[Denys Vlasenko]

If two consecutive reads of the counter are the same, it is also not an overflow.
"systimel_1 < systimel_2" should be "systimel_1 <= systimel_2".

Before the patch, we could perform an *erroneous* correction:

Let's say that systimel_1 == systimel_2 == 0xffffffff.
"systimel_1 < systimel_2" is false, we think it's an overflow,
we read "systimeh = er32(SYSTIMH)" which meanwhile had incremented,
and use "(systimeh << 32) + systimel_2" value which is 2^32 too large.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: intel-wired-lan at lists.osuosl.org
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 967311b..99d0e6e 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4287,7 +4287,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systimeh = er32(SYSTIMH);
 	systimel_2 = er32(SYSTIML);
 	/* Check for overflow. If there was no overflow, use the values */
-	if (systimel_1 < systimel_2) {
+	if (systimel_1 <= systimel_2) {
 		systim = (cycle_t)systimel_1;
 		systim |= (cycle_t)systimeh << 32;
 	} else {
-- 
1.8.1.4

[Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed

[Denys Vlasenko]

SYSTIMH:SYSTIML registers are incremented by 24-bit value TIMINCA[23..0]

er32(SYSTIML) are probably moderately expensive (they are pci bus reads).
Can we avoid one of them? Yes, we can.

If the SYSTIML value we see is smaller than 0xff000000, the overflow
into SYSTIMH would require at least two increments.

We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order.

Even if one increment happens between them, the overflow into SYSTIMH
is impossible, and we can avoid doing another er32(SYSTIML) read
and overflow check.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: intel-wired-lan at lists.osuosl.org
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 99d0e6e..6f17f89 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4275,7 +4275,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	struct e1000_adapter *adapter = container_of(cc, struct e1000_adapter,
 						     cc);
 	struct e1000_hw *hw = &adapter->hw;
-	u32 systimel_1, systimel_2, systimeh;
+	u32 systimel, systimeh;
 	cycle_t systim, systim_next;
 	/* SYSTIMH latching upon SYSTIML read does not work well.
 	 * This means that if SYSTIML overflows after we read it but before
@@ -4283,21 +4283,21 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	 * will experience a huge non linear increment in the systime value
 	 * to fix that we test for overflow and if true, we re-read systime.
 	 */
-	systimel_1 = er32(SYSTIML);
+	systimel = er32(SYSTIML);
 	systimeh = er32(SYSTIMH);
-	systimel_2 = er32(SYSTIML);
-	/* Check for overflow. If there was no overflow, use the values */
-	if (systimel_1 <= systimel_2) {
-		systim = (cycle_t)systimel_1;
-		systim |= (cycle_t)systimeh << 32;
-	} else {
-		/* There was an overflow, read again SYSTIMH, and use
-		 * systimel_2
-		 */
-		systimeh = er32(SYSTIMH);
-		systim = (cycle_t)systimel_2;
-		systim |= (cycle_t)systimeh << 32;
+	/* Is systimel is so large that overflow is possible? */
+	if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) {
+		u32 systimel_2 = er32(SYSTIML);
+		if (systimel > systimel_2) {
+			/* There was an overflow, read again SYSTIMH, and use
+			 * systimel_2
+			 */
+			systimeh = er32(SYSTIMH);
+			systimel = systimel_2;
+		}
 	}
+	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;
-- 
1.8.1.4

[Intel-wired-lan] [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64

[Ruinskiy, Dima]

Probably a good idea. do_div() truncates the divisor to 32 bits anyways.

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Denys Vlasenko
Sent: Wednesday, 20 April, 2016 18:46
To: intel-wired-lan@lists.osuosl.org
Cc: Denys Vlasenko
Subject: [Intel-wired-lan] [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64

"incvalue" variable holds a result of "er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK"
and used in "do_div(temp, incvalue)" as a divisor.

Thus, "u64 incvalue" declaration is probably a mistake.
Even though it seems to be a harmless one, let's fix it.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: intel-wired-lan at lists.osuosl.org
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 9b4ec13..967311b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4300,7 +4300,8 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	}
 
 	if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
-		u64 incvalue, time_delta, rem, temp;
+		u64 time_delta, rem, temp;
+		u32 incvalue;
 		int i;
 
 		/* errata for 82574/82583 possible bad bits read from SYSTIMH/L
-- 
1.8.1.4

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
---------------------------------------------------------------------
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.

[Intel-wired-lan] [PATCH 2/3] e1000e: e1000e_cyclecounter_read(): fix er32(SYSTIML) overflow check

[Ruinskiy, Dima]

Agree.

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Denys Vlasenko
Sent: Wednesday, 20 April, 2016 18:46
To: intel-wired-lan@lists.osuosl.org
Cc: Denys Vlasenko
Subject: [Intel-wired-lan] [PATCH 2/3] e1000e: e1000e_cyclecounter_read(): fix er32(SYSTIML) overflow check

If two consecutive reads of the counter are the same, it is also not an overflow.
"systimel_1 < systimel_2" should be "systimel_1 <= systimel_2".

Before the patch, we could perform an *erroneous* correction:

Let's say that systimel_1 == systimel_2 == 0xffffffff.
"systimel_1 < systimel_2" is false, we think it's an overflow,
we read "systimeh = er32(SYSTIMH)" which meanwhile had incremented,
and use "(systimeh << 32) + systimel_2" value which is 2^32 too large.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: intel-wired-lan at lists.osuosl.org
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 967311b..99d0e6e 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4287,7 +4287,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systimeh = er32(SYSTIMH);
 	systimel_2 = er32(SYSTIML);
 	/* Check for overflow. If there was no overflow, use the values */
-	if (systimel_1 < systimel_2) {
+	if (systimel_1 <= systimel_2) {
 		systim = (cycle_t)systimel_1;
 		systim |= (cycle_t)systimeh << 32;
 	} else {
-- 
1.8.1.4

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
---------------------------------------------------------------------
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.

[Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed

[Ruinskiy, Dima]

Not sure I follow (perhaps I am missing some background). Why do you assume there can be only 1 or 2 increments?

In general, unless you are combating an active performance issue, I am not a fan of making the code more complex, in order to save one PCI read (how expensive are they anyway?)

--Dima

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Denys Vlasenko
Sent: Wednesday, 20 April, 2016 18:46
To: intel-wired-lan@lists.osuosl.org
Cc: Denys Vlasenko
Subject: [Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed

SYSTIMH:SYSTIML registers are incremented by 24-bit value TIMINCA[23..0]

er32(SYSTIML) are probably moderately expensive (they are pci bus reads).
Can we avoid one of them? Yes, we can.

If the SYSTIML value we see is smaller than 0xff000000, the overflow into SYSTIMH would require at least two increments.

We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order.

Even if one increment happens between them, the overflow into SYSTIMH is impossible, and we can avoid doing another er32(SYSTIML) read and overflow check.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: intel-wired-lan at lists.osuosl.org
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 99d0e6e..6f17f89 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4275,7 +4275,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	struct e1000_adapter *adapter = container_of(cc, struct e1000_adapter,
 						     cc);
 	struct e1000_hw *hw = &adapter->hw;
-	u32 systimel_1, systimel_2, systimeh;
+	u32 systimel, systimeh;
 	cycle_t systim, systim_next;
 	/* SYSTIMH latching upon SYSTIML read does not work well.
 	 * This means that if SYSTIML overflows after we read it but before @@ -4283,21 +4283,21 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	 * will experience a huge non linear increment in the systime value
 	 * to fix that we test for overflow and if true, we re-read systime.
 	 */
-	systimel_1 = er32(SYSTIML);
+	systimel = er32(SYSTIML);
 	systimeh = er32(SYSTIMH);
-	systimel_2 = er32(SYSTIML);
-	/* Check for overflow. If there was no overflow, use the values */
-	if (systimel_1 <= systimel_2) {
-		systim = (cycle_t)systimel_1;
-		systim |= (cycle_t)systimeh << 32;
-	} else {
-		/* There was an overflow, read again SYSTIMH, and use
-		 * systimel_2
-		 */
-		systimeh = er32(SYSTIMH);
-		systim = (cycle_t)systimel_2;
-		systim |= (cycle_t)systimeh << 32;
+	/* Is systimel is so large that overflow is possible? */
+	if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) {
+		u32 systimel_2 = er32(SYSTIML);
+		if (systimel > systimel_2) {
+			/* There was an overflow, read again SYSTIMH, and use
+			 * systimel_2
+			 */
+			systimeh = er32(SYSTIMH);
+			systimel = systimel_2;
+		}
 	}
+	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;
--
1.8.1.4

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
---------------------------------------------------------------------
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.

[Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed

[Denys Vlasenko]

On 04/21/2016 04:49 PM, Ruinskiy, Dima wrote:
> Not sure I follow (perhaps I am missing some background).
> Why do you assume there can be only 1 or 2 increments?

Different e1000e models have different timer cycle periods:

#define INCVALUE_96MHz          125
#define INCVALUE_SHIFT_96MHz    17
#define INCPERIOD_SHIFT_96MHz   2
#define INCPERIOD_96MHz         (12 >> INCPERIOD_SHIFT_96MHz)

#define INCVALUE_25MHz          40
#define INCVALUE_SHIFT_25MHz    18
#define INCPERIOD_25MHz         1

#define INCVALUE_24MHz          125
#define INCVALUE_SHIFT_24MHz    14
#define INCPERIOD_24MHz         3

The fastest is 96MHz, ~10ns, and we update the counter every 3 periods,
so two updates are 30ns apart.

That is.... fast. PCIe reads are in ~100ns territory.

So yes, you seem to be right. There can be more than two updates
between two PCIe reads.

[Intel-wired-lan] [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Denys Vlasenko
> Sent: Wednesday, April 20, 2016 8:46 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Subject: [Intel-wired-lan] [PATCH 1/3] e1000e: e1000e_cyclecounter_read():
> incvalue is 32 bits, not 64
> 
> "incvalue" variable holds a result of "er32(TIMINCA) &
> E1000_TIMINCA_INCVALUE_MASK"
> and used in "do_div(temp, incvalue)" as a divisor.
> 
> Thus, "u64 incvalue" declaration is probably a mistake.
> Even though it seems to be a harmless one, let's fix it.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: intel-wired-lan at lists.osuosl.org
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

This patch threw a checkpatch warning for me:
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)

But all my regression systems are happy so...

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

[Intel-wired-lan] [PATCH 2/3] e1000e: e1000e_cyclecounter_read(): fix er32(SYSTIML) overflow check

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Denys Vlasenko
> Sent: Wednesday, April 20, 2016 8:46 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Subject: [Intel-wired-lan] [PATCH 2/3] e1000e: e1000e_cyclecounter_read():
> fix er32(SYSTIML) overflow check
> 
> If two consecutive reads of the counter are the same, it is also not an
> overflow.
> "systimel_1 < systimel_2" should be "systimel_1 <= systimel_2".
> 
> Before the patch, we could perform an *erroneous* correction:
> 
> Let's say that systimel_1 == systimel_2 == 0xffffffff.
> "systimel_1 < systimel_2" is false, we think it's an overflow,
> we read "systimeh = er32(SYSTIMH)" which meanwhile had incremented,
> and use "(systimeh << 32) + systimel_2" value which is 2^32 too large.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: intel-wired-lan at lists.osuosl.org
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This patch also gives me the possible unwrapped commit description (prefer a maximum 75 chars per line) warning but otherwise seems fine.

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

[Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Denys Vlasenko
> Sent: Wednesday, April 20, 2016 8:46 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Subject: [Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read():
> do overflow check only if needed
> 
> SYSTIMH:SYSTIML registers are incremented by 24-bit value TIMINCA[23..0]
> 
> er32(SYSTIML) are probably moderately expensive (they are pci bus reads).
> Can we avoid one of them? Yes, we can.
> 
> If the SYSTIML value we see is smaller than 0xff000000, the overflow
> into SYSTIMH would require at least two increments.
> 
> We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order.
> 
> Even if one increment happens between them, the overflow into SYSTIMH
> is impossible, and we can avoid doing another er32(SYSTIML) read
> and overflow check.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: intel-wired-lan at lists.osuosl.org
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)

This one throws a different checkpatch warning:
WARNING: Missing a blank line after declarations
#61: FILE: drivers/net/ethernet/intel/e1000e/netdev.c:4327:
+               u32 systimel_2 = er32(SYSTIML);
+               if (systimel > systimel_2) {

Still just a warning and I'm not seeing functional problems with it.

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

[Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed

[Denys Vlasenko]

On 04/26/2016 01:12 AM, Brown, Aaron F wrote:
>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
>> Behalf Of Denys Vlasenko
>> Sent: Wednesday, April 20, 2016 8:46 AM
>> To: intel-wired-lan at lists.osuosl.org
>> Cc: Denys Vlasenko <dvlasenk@redhat.com>
>> Subject: [Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read():
>> do overflow check only if needed
>>
>> SYSTIMH:SYSTIML registers are incremented by 24-bit value TIMINCA[23..0]
>>
>> er32(SYSTIML) are probably moderately expensive (they are pci bus reads).
>> Can we avoid one of them? Yes, we can.
>>
>> If the SYSTIML value we see is smaller than 0xff000000, the overflow
>> into SYSTIMH would require at least two increments.
>>
>> We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order.
>>
>> Even if one increment happens between them, the overflow into SYSTIMH
>> is impossible, and we can avoid doing another er32(SYSTIML) read
>> and overflow check.
>>
...
> Still just a warning and I'm not seeing functional problems with it.
> 
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Aaron, there were some comments from Dima Ruinskiy <dima.ruinskiy@intel.com>
that I might be underestimating the rate how quickly this counter increments:
that it may be in fact incrementing something like every 40ns.

In this case, more than one increment may happen between these two PCIe reads:

	systimel = er32(SYSTIML);
	systimeh = er32(SYSTIMH);

and my check

	/* Is systimel is so large that overflow is possible? */
	if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) {

can fail to catch an overflow.

If you (Intel guys) indeed see that as possible, then either drop the 3/3
patch, or tweak the constant so that more than one increment is needed
for overflow. Say:

	/* Is systimel is so large that overflow is possible? */
	if (systimel >= (u32)0xf0000000) {

This particular constant in comparison allows to skip overflow check
if we are more than 16 updates away from overflow - should be ample.

[Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed

[Brown, Aaron F]

> From: Denys Vlasenko [mailto:dvlasenk at redhat.com]
> Sent: Tuesday, April 26, 2016 9:03 AM
> To: Brown, Aaron F <aaron.f.brown@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH 3/3] e1000e:
> e1000e_cyclecounter_read(): do overflow check only if needed
> 
> On 04/26/2016 01:12 AM, Brown, Aaron F wrote:
> >> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org]
> On
> >> Behalf Of Denys Vlasenko
> >> Sent: Wednesday, April 20, 2016 8:46 AM
> >> To: intel-wired-lan at lists.osuosl.org
> >> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> >> Subject: [Intel-wired-lan] [PATCH 3/3] e1000e:
> e1000e_cyclecounter_read():
> >> do overflow check only if needed
> >>
> >> SYSTIMH:SYSTIML registers are incremented by 24-bit value
> TIMINCA[23..0]
> >>
> >> er32(SYSTIML) are probably moderately expensive (they are pci bus
> reads).
> >> Can we avoid one of them? Yes, we can.
> >>
> >> If the SYSTIML value we see is smaller than 0xff000000, the overflow
> >> into SYSTIMH would require at least two increments.
> >>
> >> We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order.
> >>
> >> Even if one increment happens between them, the overflow into SYSTIMH
> >> is impossible, and we can avoid doing another er32(SYSTIML) read
> >> and overflow check.
> >>
> ...
> > Still just a warning and I'm not seeing functional problems with it.
> >
> > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> 
> Aaron, there were some comments from Dima Ruinskiy
> <dima.ruinskiy@intel.com>
> that I might be underestimating the rate how quickly this counter increments:
> that it may be in fact incrementing something like every 40ns.

I did see that come through, for some reason the comment did not get captured in https://patchwork.ozlabs.org/patch/612833/ which I was reading from when I ran it through my regression rig.  Thanks for catching it.

Yes, I guess we should hold off on this until Raanan can weigh in.  Jeff, please ignore thisTested by for now.

> 
> In this case, more than one increment may happen between these two PCIe
> reads:
> 
> 	systimel = er32(SYSTIML);
> 	systimeh = er32(SYSTIMH);
> 
> and my check
> 
> 	/* Is systimel is so large that overflow is possible? */
> 	if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) {
> 
> can fail to catch an overflow.
> 
> If you (Intel guys) indeed see that as possible, then either drop the 3/3
> patch, or tweak the constant so that more than one increment is needed
> for overflow. Say:
> 
> 	/* Is systimel is so large that overflow is possible? */
> 	if (systimel >= (u32)0xf0000000) {
> 
> This particular constant in comparison allows to skip overflow check
> if we are more than 16 updates away from overflow - should be ample.