[PATCH] e1000e: Use deferrable timer for watchdog

[PATCH] e1000e: Use deferrable timer for watchdog

[Parag Warudkar]

Reduce wakeups from idle per second.

Signed-off-by: Parag Warudkar <parag.warudkar@gmail.com>

--- linux-2.6/drivers/net/e1000e/netdev.c	2007-12-07 10:04:39.000000000 -0500
+++ linux-2.6-work/drivers/net/e1000e/netdev.c	2007-12-18 20:45:59.000000000 -0500
@@ -3899,7 +3899,7 @@
  		goto err_eeprom;
  	}

-	init_timer(&adapter->watchdog_timer);
+	init_timer_deferrable(&adapter->watchdog_timer);
  	adapter->watchdog_timer.function = &e1000_watchdog;
  	adapter->watchdog_timer.data = (unsigned long) adapter;

Re: [PATCH] e1000e: Use deferrable timer for watchdog

[Kok, Auke]

Parag Warudkar wrote:
> 
> Reduce wakeups from idle per second.
> 
> Signed-off-by: Parag Warudkar <parag.warudkar@gmail.com>
> 
> --- linux-2.6/drivers/net/e1000e/netdev.c    2007-12-07
> 10:04:39.000000000 -0500
> +++ linux-2.6-work/drivers/net/e1000e/netdev.c    2007-12-18
> 20:45:59.000000000 -0500
> @@ -3899,7 +3899,7 @@
>          goto err_eeprom;
>      }
> 
> -    init_timer(&adapter->watchdog_timer);
> +    init_timer_deferrable(&adapter->watchdog_timer);
>      adapter->watchdog_timer.function = &e1000_watchdog;
>      adapter->watchdog_timer.data = (unsigned long) adapter;
> 


see my reply to "Re: [PATCH] e1000: Use deferrable timer for watchdog" - IOW no,
we don't want this

Auke

Re: [PATCH] e1000e: Use deferrable timer for watchdog

[Kok, Auke]

Parag Warudkar wrote:
> 
> Reduce wakeups from idle per second.
> 
> Signed-off-by: Parag Warudkar <parag.warudkar@gmail.com>
> 
> --- linux-2.6/drivers/net/e1000e/netdev.c    2007-12-07
> 10:04:39.000000000 -0500
> +++ linux-2.6-work/drivers/net/e1000e/netdev.c    2007-12-18
> 20:45:59.000000000 -0500
> @@ -3899,7 +3899,7 @@
>          goto err_eeprom;
>      }
> 
> -    init_timer(&adapter->watchdog_timer);
> +    init_timer_deferrable(&adapter->watchdog_timer);
>      adapter->watchdog_timer.function = &e1000_watchdog;
>      adapter->watchdog_timer.data = (unsigned long) adapter;


I can't even apply this patch and the e1000 one... not only is it whitespace
damaged it is also not properly formatted as patch at all. If you want me to take
these patches seriously, then please fix the formatting issues.

Auke

Re: [PATCH] e1000e: Use deferrable timer for watchdog

[Parag Warudkar]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

On Dec 20, 2007 12:05 PM, Kok, Auke <auke-jan.h.kok@intel.com> wrote:
>
> I can't even apply this patch and the e1000 one... not only is it whitespace
> damaged it is also not properly formatted as patch at all. If you want me to take
> these patches seriously, then please fix the formatting issues.

Sigh - I use Pine, follow Documents/email-clients.txt for the
recommended settings and obviously the pathces are not generated with
whitespace damage at my end as I test those before sending out.

So although I hate to see this happen there is nothing at this moment
that I can do - except for attaching the patch instead of inlining it.
Since they have already been reviewed inline, please see if the
attached patches work for you.

[parag@mini linux-2.6]$ scripts/checkpatch.pl --no-tree
../../Patches/e1000_main.c.patch
total: 0 errors, 0 warnings, 8 lines checked

Your patch has no obvious style problems and is ready for submission.
[parag@mini linux-2.6]$
[parag@mini linux-2.6]$ vim drivers/net/e1000/e1000_main.c
[parag@mini linux-2.6]$ patch -p1 < ../../Patches/e1000_main.c.patch
patching file drivers/net/e1000/e1000_main.c

[parag@mini linux-2.6]$ scripts/checkpatch.pl --no-tree
../../Patches/e1000e-netdev.c.patch
total: 0 errors, 0 warnings, 8 lines checked

Your patch has no obvious style problems and is ready for submission.
[parag@mini linux-2.6]$ patch -p1 < ../../Patches/e1000e-netdev.c.patch
patching file drivers/net/e1000e/netdev.c

Thanks

Parag

[-- Attachment #2: e1000_main.c.patch --]
[-- Type: application/octet-stream, Size: 587 bytes --]

Signed-off-by: Parag Warudkar <parag.warudkar@gmail.com>

--- linux-2.6/drivers/net/e1000/e1000_main.c	2007-12-07 10:04:39.000000000 -0500
+++ linux-2.6-work/drivers/net/e1000/e1000_main.c	2007-12-18 20:38:38.000000000 -0500
@@ -1030,7 +1030,7 @@
 	adapter->tx_fifo_stall_timer.function = &e1000_82547_tx_fifo_stall;
 	adapter->tx_fifo_stall_timer.data = (unsigned long) adapter;
 
-	init_timer(&adapter->watchdog_timer);
+	init_timer_deferrable(&adapter->watchdog_timer);
 	adapter->watchdog_timer.function = &e1000_watchdog;
 	adapter->watchdog_timer.data = (unsigned long) adapter;
 

[-- Attachment #3: e1000e-netdev.c.patch --]
[-- Type: application/octet-stream, Size: 472 bytes --]

Signed-off-by: Parag Warudkar <parag.warudkar@gmail.com>

--- linux-2.6/drivers/net/e1000e/netdev.c	2007-12-07 10:04:39.000000000 -0500
+++ linux-2.6-work/drivers/net/e1000e/netdev.c	2007-12-18 20:45:59.000000000 -0500
@@ -3899,7 +3899,7 @@
 		goto err_eeprom;
 	}
 
-	init_timer(&adapter->watchdog_timer);
+	init_timer_deferrable(&adapter->watchdog_timer);
 	adapter->watchdog_timer.function = &e1000_watchdog;
 	adapter->watchdog_timer.data = (unsigned long) adapter;
 

Re: [PATCH] e1000e: Use deferrable timer for watchdog

[Kok, Auke]

Parag Warudkar wrote:
> On Dec 20, 2007 12:05 PM, Kok, Auke <auke-jan.h.kok@intel.com> wrote:
>> I can't even apply this patch and the e1000 one... not only is it whitespace
>> damaged it is also not properly formatted as patch at all. If you want me to take
>> these patches seriously, then please fix the formatting issues.
> 
> Sigh - I use Pine, follow Documents/email-clients.txt for the
> recommended settings and obviously the pathces are not generated with
> whitespace damage at my end as I test those before sending out.
> 
> So although I hate to see this happen there is nothing at this moment
> that I can do - except for attaching the patch instead of inlining it.
> Since they have already been reviewed inline, please see if the
> attached patches work for you.

here's what the files in my Maildir spool look like in vim (my vim displays a '»'
char for tabs and a "¶" for EOL):

 76 --- linux-2.6/drivers/net/e1000e/netdev.c»      2007-12-07 10:04:39.00000000
 77 +++ linux-2.6-work/drivers/net/e1000e/netdev.c» 2007-12-18 20:45:59.00000000
 78 @@ -3899,7 +3899,7 @@¶
 79   »     »       goto err_eeprom;¶
 80   »     }¶
 81 ¶
 82 -»      init_timer(&adapter->watchdog_timer);¶
 83 +»      init_timer_deferrable(&adapter->watchdog_timer);¶
 84   »     adapter->watchdog_timer.function = &e1000_watchdog;¶
 85   »     adapter->watchdog_timer.data = (unsigned long) adapter;¶
 86 ¶
 87 --¶

notice that there are two spaces instead of 1. Also there's no line heading the
diff with 'diff a/foo b/foo' which is what throws of stg. And the -p option is
missing.


as for content, the patch looks OK with me. I ran the numbers and allthough there
was a slight average delay in the link up detection time it is negligeable (less
than 0.2sec difference over a bunch of measurements), and I confirmed your
powertop numbers are correct. As for the timer interval, the watchdog may already
be delayed up to 3 seconds safely, this doesn't change that.

I'll forward the patch, Care to make one for e100? plenty of laptops with those
still around! The embedded guys would love it I think.

Thanks,

Auke