RE: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

2016-05-16 Thread Brown, Aaron F 
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@lists.osuosl.org] On
> Behalf Of Denys Vlasenko
> Sent: Friday, May 6, 2016 12:42 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>
> Cc: intel-wired-...@lists.osuosl.org; Denys Vlasenko
> <dvlas...@redhat.com>; LKML <linux-kernel@vger.kernel.org>;
> net...@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if
> TIMINCA is zero
> 
> Users report that under VMWare, er32(TIMINCA) returns zero.
> This causes division by zero at init time as follows:
> 
>  ==>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;
>  >  rem = do_div(temp, incvalue);
> 
> This change makes kernel survive this, and users report that
> NIC does work after this change.
> 
> Since on real hardware incvalue is never zero, this should not affect
> real hardware use case.
> 
> Signed-off-by: Denys Vlasenko <dvlas...@redhat.com>
> CC: Jeff Kirsher <jeffrey.t.kirs...@intel.com>
> CC: "Ruinskiy, Dima" <dima.ruins...@intel.com>
> CC: intel-wired-...@lists.osuosl.org
> CC: net...@vger.kernel.org
> CC: LKML <linux-kernel@vger.kernel.org>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

As Mark Rustad pointed out I recall this was earlier rejected as something that 
is a VMWare error and it should be fixed there so that existing VMs will start 
working without installing a new driver.  Having said that, it does not seem to 
be causing any harm in my testing, so...

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

RE: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

2016-05-16 Thread Brown, Aaron F 
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@lists.osuosl.org] On
> Behalf Of Denys Vlasenko
> Sent: Friday, May 6, 2016 12:42 PM
> To: Kirsher, Jeffrey T 
> Cc: intel-wired-...@lists.osuosl.org; Denys Vlasenko
> ; LKML ;
> net...@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if
> TIMINCA is zero
> 
> Users report that under VMWare, er32(TIMINCA) returns zero.
> This causes division by zero at init time as follows:
> 
>  ==>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;
>  >  rem = do_div(temp, incvalue);
> 
> This change makes kernel survive this, and users report that
> NIC does work after this change.
> 
> Since on real hardware incvalue is never zero, this should not affect
> real hardware use case.
> 
> Signed-off-by: Denys Vlasenko 
> CC: Jeff Kirsher 
> CC: "Ruinskiy, Dima" 
> CC: intel-wired-...@lists.osuosl.org
> CC: net...@vger.kernel.org
> CC: LKML 
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

As Mark Rustad pointed out I recall this was earlier rejected as something that 
is a VMWare error and it should be fixed there so that existing VMs will start 
working without installing a new driver.  Having said that, it does not seem to 
be causing any harm in my testing, so...

Tested-by: Aaron Brown

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

2016-05-10 Thread Mark D Rustad 

Jarod Wilson  wrote:


On Fri, May 06, 2016 at 11:43:17PM +, Rustad, Mark D wrote:

Denys Vlasenko  wrote:


Users report that under VMWare, er32(TIMINCA) returns zero.
This causes division by zero at init time as follows:

==>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;
>  rem = do_div(temp, incvalue);

This change makes kernel survive this, and users report that
NIC does work after this change.

Since on real hardware incvalue is never zero, this should not affect
real hardware use case.

...
I seem to recall that this was rejected before because it really is  
VMWare's

bug and, if they fix it, any existing VMs that use this will just work.
Changing the driver will only fix it for vms that install a new driver. I
don't object to doing it, it just seems like not the most effective  
place to

address the issue.


You could also have people who never update VMWare, for whom a kernel
work-around would be better. I think it'd be best to address it both at
the driver level and the emulated hardware level, to improve things for
the most possible users. Those who update neither hypervisor or
kernel/driver, well, they reap what they sow.


That is a sound argument for doing both. I would expect that there are more  
frozen VM images than host environments, but I can certainly imagine that  
some choose to freeze their host. Of course if everything is frozen there  
is no point at all. :-)


I am on an extended vacation, and don't work on e1000e anyway, so I will  
quit my kibitzing here.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

2016-05-10 Thread Mark D Rustad 

Jarod Wilson  wrote:


On Fri, May 06, 2016 at 11:43:17PM +, Rustad, Mark D wrote:

Denys Vlasenko  wrote:


Users report that under VMWare, er32(TIMINCA) returns zero.
This causes division by zero at init time as follows:

==>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;
>  rem = do_div(temp, incvalue);

This change makes kernel survive this, and users report that
NIC does work after this change.

Since on real hardware incvalue is never zero, this should not affect
real hardware use case.

...
I seem to recall that this was rejected before because it really is  
VMWare's

bug and, if they fix it, any existing VMs that use this will just work.
Changing the driver will only fix it for vms that install a new driver. I
don't object to doing it, it just seems like not the most effective  
place to

address the issue.


You could also have people who never update VMWare, for whom a kernel
work-around would be better. I think it'd be best to address it both at
the driver level and the emulated hardware level, to improve things for
the most possible users. Those who update neither hypervisor or
kernel/driver, well, they reap what they sow.


That is a sound argument for doing both. I would expect that there are more  
frozen VM images than host environments, but I can certainly imagine that  
some choose to freeze their host. Of course if everything is frozen there  
is no point at all. :-)


I am on an extended vacation, and don't work on e1000e anyway, so I will  
quit my kibitzing here.


--
Mark Rustad, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

2016-05-10 Thread Jarod Wilson 
On Fri, May 06, 2016 at 11:43:17PM +, Rustad, Mark D wrote:
> Denys Vlasenko  wrote:
> 
> >Users report that under VMWare, er32(TIMINCA) returns zero.
> >This causes division by zero at init time as follows:
> >
> > ==>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;
> > >  rem = do_div(temp, incvalue);
> >
> >This change makes kernel survive this, and users report that
> >NIC does work after this change.
> >
> >Since on real hardware incvalue is never zero, this should not affect
> >real hardware use case.
...
> I seem to recall that this was rejected before because it really is VMWare's
> bug and, if they fix it, any existing VMs that use this will just work.
> Changing the driver will only fix it for vms that install a new driver. I
> don't object to doing it, it just seems like not the most effective place to
> address the issue.

You could also have people who never update VMWare, for whom a kernel
work-around would be better. I think it'd be best to address it both at
the driver level and the emulated hardware level, to improve things for
the most possible users. Those who update neither hypervisor or
kernel/driver, well, they reap what they sow.

-- 
Jarod Wilson
ja...@redhat.com

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

2016-05-10 Thread Jarod Wilson 
On Fri, May 06, 2016 at 11:43:17PM +, Rustad, Mark D wrote:
> Denys Vlasenko  wrote:
> 
> >Users report that under VMWare, er32(TIMINCA) returns zero.
> >This causes division by zero at init time as follows:
> >
> > ==>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;
> > >  rem = do_div(temp, incvalue);
> >
> >This change makes kernel survive this, and users report that
> >NIC does work after this change.
> >
> >Since on real hardware incvalue is never zero, this should not affect
> >real hardware use case.
...
> I seem to recall that this was rejected before because it really is VMWare's
> bug and, if they fix it, any existing VMs that use this will just work.
> Changing the driver will only fix it for vms that install a new driver. I
> don't object to doing it, it just seems like not the most effective place to
> address the issue.

You could also have people who never update VMWare, for whom a kernel
work-around would be better. I think it'd be best to address it both at
the driver level and the emulated hardware level, to improve things for
the most possible users. Those who update neither hypervisor or
kernel/driver, well, they reap what they sow.

-- 
Jarod Wilson
ja...@redhat.com

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

2016-05-06 Thread Rustad, Mark D 

Denys Vlasenko  wrote:


Users report that under VMWare, er32(TIMINCA) returns zero.
This causes division by zero at init time as follows:

 ==>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;
 >  rem = do_div(temp, incvalue);

This change makes kernel survive this, and users report that
NIC does work after this change.

Since on real hardware incvalue is never zero, this should not affect
real hardware use case.

Signed-off-by: Denys Vlasenko 
CC: Jeff Kirsher 
CC: "Ruinskiy, Dima" 
CC: intel-wired-...@lists.osuosl.org
CC: net...@vger.kernel.org
CC: LKML 
---
 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 269087c..0626935 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4315,7 +4315,8 @@ static cycle_t e1000e_cyclecounter_read(const  
struct cyclecounter *cc)


time_delta = systim_next - systim;
temp = time_delta;
-   rem = do_div(temp, incvalue);
+   /* VMWare users have seen incvalue of zero, don't div / 
0 */
+   rem = incvalue ? do_div(temp, incvalue) : (time_delta 
!= 0);

systim = systim_next;



I seem to recall that this was rejected before because it really is  
VMWare's bug and, if they fix it, any existing VMs that use this will just  
work. Changing the driver will only fix it for vms that install a new  
driver. I don't object to doing it, it just seems like not the most  
effective place to address the issue.


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

2016-05-06 Thread Rustad, Mark D 

Denys Vlasenko  wrote:


Users report that under VMWare, er32(TIMINCA) returns zero.
This causes division by zero at init time as follows:

 ==>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;
 >  rem = do_div(temp, incvalue);

This change makes kernel survive this, and users report that
NIC does work after this change.

Since on real hardware incvalue is never zero, this should not affect
real hardware use case.

Signed-off-by: Denys Vlasenko 
CC: Jeff Kirsher 
CC: "Ruinskiy, Dima" 
CC: intel-wired-...@lists.osuosl.org
CC: net...@vger.kernel.org
CC: LKML 
---
 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 269087c..0626935 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4315,7 +4315,8 @@ static cycle_t e1000e_cyclecounter_read(const  
struct cyclecounter *cc)


time_delta = systim_next - systim;
temp = time_delta;
-   rem = do_div(temp, incvalue);
+   /* VMWare users have seen incvalue of zero, don't div / 
0 */
+   rem = incvalue ? do_div(temp, incvalue) : (time_delta 
!= 0);

systim = systim_next;



I seem to recall that this was rejected before because it really is  
VMWare's bug and, if they fix it, any existing VMs that use this will just  
work. Changing the driver will only fix it for vms that install a new  
driver. I don't object to doing it, it just seems like not the most  
effective place to address the issue.


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail