* [PATCH 2/3] e1000e: e1000e_cyclecounter_read(): fix er32(SYSTIML) overflow check
2016-04-19 12:34 [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64 Denys Vlasenko
@ 2016-04-19 12:34 ` Denys Vlasenko
2016-04-19 12:34 ` [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed Denys Vlasenko
2016-04-19 20:57 ` [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64 Jeff Kirsher
2 siblings, 0 replies; 5+ messages in thread
From: Denys Vlasenko @ 2016-04-19 12:34 UTC (permalink / raw)
To: Jeff Kirsher
Cc: Denys Vlasenko, Jesse Brandeburg, Shannon Nelson, Carolyn Wyborny,
Don Skidmore, Bruce Allan, John Ronciak, Mitch Williams,
David S. Miller, LKML, netdev
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: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Shannon Nelson <shannon.nelson@intel.com>
CC: Carolyn Wyborny <carolyn.wyborny@intel.com>
CC: Don Skidmore <donald.c.skidmore@intel.com>
CC: Bruce Allan <bruce.w.allan@intel.com>
CC: John Ronciak <john.ronciak@intel.com>
CC: Mitch Williams <mitch.a.williams@intel.com>
CC: David S. Miller <davem@davemloft.net>
CC: LKML <linux-kernel@vger.kernel.org>
CC: netdev@vger.kernel.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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed
2016-04-19 12:34 [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64 Denys Vlasenko
2016-04-19 12:34 ` [PATCH 2/3] e1000e: e1000e_cyclecounter_read(): fix er32(SYSTIML) overflow check Denys Vlasenko
@ 2016-04-19 12:34 ` Denys Vlasenko
2016-04-19 20:57 ` [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64 Jeff Kirsher
2 siblings, 0 replies; 5+ messages in thread
From: Denys Vlasenko @ 2016-04-19 12:34 UTC (permalink / raw)
To: Jeff Kirsher
Cc: Denys Vlasenko, Jesse Brandeburg, Shannon Nelson, Carolyn Wyborny,
Don Skidmore, Bruce Allan, John Ronciak, Mitch Williams,
David S. Miller, LKML, netdev
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: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Shannon Nelson <shannon.nelson@intel.com>
CC: Carolyn Wyborny <carolyn.wyborny@intel.com>
CC: Don Skidmore <donald.c.skidmore@intel.com>
CC: Bruce Allan <bruce.w.allan@intel.com>
CC: John Ronciak <john.ronciak@intel.com>
CC: Mitch Williams <mitch.a.williams@intel.com>
CC: David S. Miller <davem@davemloft.net>
CC: LKML <linux-kernel@vger.kernel.org>
CC: netdev@vger.kernel.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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64
2016-04-19 12:34 [PATCH 1/3] e1000e: e1000e_cyclecounter_read(): incvalue is 32 bits, not 64 Denys Vlasenko
2016-04-19 12:34 ` [PATCH 2/3] e1000e: e1000e_cyclecounter_read(): fix er32(SYSTIML) overflow check Denys Vlasenko
2016-04-19 12:34 ` [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): do overflow check only if needed Denys Vlasenko
@ 2016-04-19 20:57 ` Jeff Kirsher
2016-04-20 15:43 ` Denys Vlasenko
2 siblings, 1 reply; 5+ messages in thread
From: Jeff Kirsher @ 2016-04-19 20:57 UTC (permalink / raw)
To: Denys Vlasenko; +Cc: David S. Miller, netdev
[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]
On Tue, 2016-04-19 at 14:34 +0200, Denys Vlasenko wrote:
> "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: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: Jesse Brandeburg <jesse.brandeburg@intel.com>
> CC: Shannon Nelson <shannon.nelson@intel.com>
> CC: Carolyn Wyborny <carolyn.wyborny@intel.com>
> CC: Don Skidmore <donald.c.skidmore@intel.com>
> CC: Bruce Allan <bruce.w.allan@intel.com>
> CC: John Ronciak <john.ronciak@intel.com>
> CC: Mitch Williams <mitch.a.williams@intel.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: LKML <linux-kernel@vger.kernel.org>
> CC: netdev@vger.kernel.org
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
First of all, trimmed down the recipient list since almost all of the
reviewers you added have nothing to do with e1000e.
Any chance you can send this to the "correct" list intel-wired-
lan@lists.osuosl.org? Kind of amazing that your shotgun blast approach
in emailing out the patch series managed to miss sending it to the one
email list that handles Intel Wired LAN kernel patches.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread