| Message ID | 1468959902-25071-1-git-send-email-jarod@redhat.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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.
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(-)
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
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
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(-)
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; }
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(-)