forcedeth: option to disable 100Hz timer (try 2)

forcedeth: option to disable 100Hz timer (try 2)

[Mikhail Kshevetskiy]

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

On some hardware no TX done interrupts are generated, thus special
100Hz timer interrupt is required to handle this situation properly.
Other device do not require that timer interrupt feature. 

Forcedeth has a DEV_NEED_TIMERIRQ flag to mark the broken devices.
Unfortunately, nobody know the actual list of broken devices, so all
device has this flag on. Other problem, this flag is not user visible,
so the kernel recompilation is required to disable timer interrupts and
test a device.

This patch add a "disable_timerirq" option to disable interrupt 
timer mentioned above. This may be extremely useful for laptop users.

Mikhail Kshevetskiy

[-- Attachment #2: forcedeth.diff --]
[-- Type: text/x-patch, Size: 1547 bytes --]

--- linux-2.6.27-rc5/drivers/net/forcedeth.c.orig	2008-09-02 00:58:07.000000000 +0400
+++ linux-2.6.27-rc5/drivers/net/forcedeth.c	2008-09-02 02:02:34.000000000 +0400
@@ -842,6 +842,16 @@
 static int poll_interval = -1;
 
 /*
+ * On some hardware no TX done interrupts are generated, thus special
+ * timer interrupt is required to handle this situation properly.
+ *
+ * Setting this variable to 1 leads to removing DEV_NEED_TIMERIRQ from
+ * the driver_data flags and thus disable 100Hz timer mentioned above.
+ * This may be extremely usefull for laptop users.
+ */
+static int disable_timerirq = 0;
+
+/*
  * MSI interrupts
  */
 enum {
@@ -5704,7 +5714,7 @@
 			np->msi_flags |= 0x0001;
 	}
 
-	if (id->driver_data & DEV_NEED_TIMERIRQ)
+	if (id->driver_data & DEV_NEED_TIMERIRQ && !disable_timerirq)
 		np->irqmask |= NVREG_IRQ_TIMER;
 	if (id->driver_data & DEV_NEED_LINKTIMER) {
 		dprintk(KERN_INFO "%s: link timer on.\n", pci_name(pci_dev));
@@ -6180,6 +6190,8 @@
 MODULE_PARM_DESC(dma_64bit, "High DMA is enabled by setting to 1 and disabled by setting to 0.");
 module_param(phy_cross, int, 0);
 MODULE_PARM_DESC(phy_cross, "Phy crossover detection for Realtek 8201 phy is enabled by setting to 1 and disabled by setting to 0.");
+module_param(disable_timerirq, int, 0);
+MODULE_PARM_DESC(disable_timerirq, "100Hz timer required for some hardware is disabled by setting to 1 and enabled by setting to 0.");
 
 MODULE_AUTHOR("Manfred Spraul <manfred@colorfullife.com>");
 MODULE_DESCRIPTION("Reverse Engineered nForce ethernet driver");

Re: forcedeth: option to disable 100Hz timer (try 2)

[Bodo Eggert]

Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com> wrote:

> On some hardware no TX done interrupts are generated, thus special
> 100Hz timer interrupt is required to handle this situation properly.
> Other device do not require that timer interrupt feature.
> 
> Forcedeth has a DEV_NEED_TIMERIRQ flag to mark the broken devices.
> Unfortunately, nobody know the actual list of broken devices, so all
> device has this flag on. Other problem, this flag is not user visible,
> so the kernel recompilation is required to disable timer interrupts and
> test a device.

Why don't you disable the interrupt after the first TX_done interrupt
(no RX pending, no RX done)? Maybe you can printk "forcedeth device [x,y] does
not need timer interrupts, please send this line to you@example.org", too.
Maybe you can even disable it if no TX is to be done.

OTOH, this fix is available, it's possibly the best to use it for now.

Re: forcedeth: option to disable 100Hz timer (try 2)

[Andrew Morton]

On Tue, 9 Sep 2008 23:34:35 +0400
Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com> wrote:

> 
> On some hardware no TX done interrupts are generated, thus special
> 100Hz timer interrupt is required to handle this situation properly.
> Other device do not require that timer interrupt feature. 
> 
> Forcedeth has a DEV_NEED_TIMERIRQ flag to mark the broken devices.
> Unfortunately, nobody know the actual list of broken devices, so all
> device has this flag on. Other problem, this flag is not user visible,
> so the kernel recompilation is required to disable timer interrupts and
> test a device.
> 
> This patch add a "disable_timerirq" option to disable interrupt 
> timer mentioned above. This may be extremely useful for laptop users.

Why do you feel that the timer-based completions need to be disabled? 
Is it causing some problem?

Re: forcedeth: option to disable 100Hz timer (try 2)

[Robert Hancock]

Andrew Morton wrote:
> On Tue, 9 Sep 2008 23:34:35 +0400
> Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com> wrote:
> 
>> On some hardware no TX done interrupts are generated, thus special
>> 100Hz timer interrupt is required to handle this situation properly.
>> Other device do not require that timer interrupt feature. 
>>
>> Forcedeth has a DEV_NEED_TIMERIRQ flag to mark the broken devices.
>> Unfortunately, nobody know the actual list of broken devices, so all
>> device has this flag on. Other problem, this flag is not user visible,
>> so the kernel recompilation is required to disable timer interrupts and
>> test a device.
>>
>> This patch add a "disable_timerirq" option to disable interrupt 
>> timer mentioned above. This may be extremely useful for laptop users.
> 
> Why do you feel that the timer-based completions need to be disabled? 
> Is it causing some problem?

100 unnecessary CPU wakeups per second imposes some power usage cost, 
especially on laptops with CPU C-states..

Re: forcedeth: option to disable 100Hz timer (try 2)

[Andrew Morton]

On Wed, 10 Sep 2008 18:18:20 -0600
Robert Hancock <hancockr@shaw.ca> wrote:

> Andrew Morton wrote:
> > On Tue, 9 Sep 2008 23:34:35 +0400
> > Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com> wrote:
> > 
> >> On some hardware no TX done interrupts are generated, thus special
> >> 100Hz timer interrupt is required to handle this situation properly.
> >> Other device do not require that timer interrupt feature. 
> >>
> >> Forcedeth has a DEV_NEED_TIMERIRQ flag to mark the broken devices.
> >> Unfortunately, nobody know the actual list of broken devices, so all
> >> device has this flag on. Other problem, this flag is not user visible,
> >> so the kernel recompilation is required to disable timer interrupts and
> >> test a device.
> >>
> >> This patch add a "disable_timerirq" option to disable interrupt 
> >> timer mentioned above. This may be extremely useful for laptop users.
> > 
> > Why do you feel that the timer-based completions need to be disabled? 
> > Is it causing some problem?
> 
> 100 unnecessary CPU wakeups per second imposes some power usage cost, 
> especially on laptops with CPU C-states..

Is that the only reason for the change?  We still don't know...



Anyway, it's certainly _sufficient_ reason, however the implementation
is pretty sad - most people won't even know that the option exists so
they'll continue to chew more power than they need to.

How do we fix this?  Perhaps disable the timer by default, then wait
for the first tx timeout and then enable the timer at that stage, while
printing a message saying "add module option <foo> to prevent this
once-off timeout from happening"?

Re: forcedeth: option to disable 100Hz timer (try 2)

[Mikhail Kshevetskiy]

On Wed, 10 Sep 2008 17:36:30 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 10 Sep 2008 18:18:20 -0600
> Robert Hancock <hancockr@shaw.ca> wrote:
> 
> > Andrew Morton wrote:
> > > On Tue, 9 Sep 2008 23:34:35 +0400
> > > Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com> wrote:
> > > 
> > >> On some hardware no TX done interrupts are generated, thus special
> > >> 100Hz timer interrupt is required to handle this situation properly.
> > >> Other device do not require that timer interrupt feature. 
> > >>
> > >> Forcedeth has a DEV_NEED_TIMERIRQ flag to mark the broken devices.
> > >> Unfortunately, nobody know the actual list of broken devices, so all
> > >> device has this flag on. Other problem, this flag is not user visible,
> > >> so the kernel recompilation is required to disable timer interrupts and
> > >> test a device.
> > >>
> > >> This patch add a "disable_timerirq" option to disable interrupt 
> > >> timer mentioned above. This may be extremely useful for laptop users.
> > > 
> > > Why do you feel that the timer-based completions need to be disabled? 
> > > Is it causing some problem?
> > 
> > 100 unnecessary CPU wakeups per second imposes some power usage cost, 
> > especially on laptops with CPU C-states..
> 
> Is that the only reason for the change?  We still don't know...
> 
> 
> 
> Anyway, it's certainly _sufficient_ reason, however the implementation
> is pretty sad - most people won't even know that the option exists so
> they'll continue to chew more power than they need to.
> 
> How do we fix this?  Perhaps disable the timer by default, then wait
> for the first tx timeout and then enable the timer at that stage, while
> printing a message saying "add module option <foo> to prevent this
> once-off timeout from happening"?

I'll try this

Re: forcedeth: option to disable 100Hz timer (try 2)

[Robert Hancock]

Andrew Morton wrote:
> On Wed, 10 Sep 2008 18:18:20 -0600
> Robert Hancock <hancockr@shaw.ca> wrote:
> 
>> Andrew Morton wrote:
>>> On Tue, 9 Sep 2008 23:34:35 +0400
>>> Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com> wrote:
>>>
>>>> On some hardware no TX done interrupts are generated, thus special
>>>> 100Hz timer interrupt is required to handle this situation properly.
>>>> Other device do not require that timer interrupt feature. 
>>>>
>>>> Forcedeth has a DEV_NEED_TIMERIRQ flag to mark the broken devices.
>>>> Unfortunately, nobody know the actual list of broken devices, so all
>>>> device has this flag on. Other problem, this flag is not user visible,
>>>> so the kernel recompilation is required to disable timer interrupts and
>>>> test a device.
>>>>
>>>> This patch add a "disable_timerirq" option to disable interrupt 
>>>> timer mentioned above. This may be extremely useful for laptop users.
>>> Why do you feel that the timer-based completions need to be disabled? 
>>> Is it causing some problem?
>> 100 unnecessary CPU wakeups per second imposes some power usage cost, 
>> especially on laptops with CPU C-states..
> 
> Is that the only reason for the change?  We still don't know...
> 
> 
> 
> Anyway, it's certainly _sufficient_ reason, however the implementation
> is pretty sad - most people won't even know that the option exists so
> they'll continue to chew more power than they need to.
> 
> How do we fix this?  Perhaps disable the timer by default, then wait
> for the first tx timeout and then enable the timer at that stage, while
> printing a message saying "add module option <foo> to prevent this
> once-off timeout from happening"?

It at least provides some way forward, where those that care can add the 
option and find out if their chipset can have the timer disabled in the 
driver by default in the future..

I'm not sure how long the TX timeout is, but I suspect it would be too 
disruptive to only enable the interrupt after a timeout. Enabling by 
default and disabling after a TX done interrupt was received would 
likely be a better approach (or one of the more creative approaches that 
others have mentioned). But it's quite likely that at least some of the 
chipsets that this driver supports don't need this timer nonsense at all.

Re: forcedeth: option to disable 100Hz timer (try 2)

[Andrew Morton]

On Wed, 10 Sep 2008 22:25:03 -0600 Robert Hancock <hancockr@shaw.ca> wrote:

> Enabling by 
> default and disabling after a TX done interrupt was received would 
> likely be a better approach

yup, that's far better.

> But it's quite likely that at least some of the 
> chipsets that this driver supports don't need this timer nonsense at all

Well yes.  If all the chipsets need the timer then there's nothing we
can do to improve it.

Re: forcedeth: option to disable 100Hz timer (try 2)

[Mikhail Kshevetskiy]

On Wed, 10 Sep 2008 22:19:47 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 10 Sep 2008 22:25:03 -0600 Robert Hancock <hancockr@shaw.ca> wrote:
> 
> > Enabling by 
> > default and disabling after a TX done interrupt was received would 
> > likely be a better approach
> 
> yup, that's far better.

How it will work, if network cable unpluged? This is quite often situation
for laptop with wifi?

> 
> > But it's quite likely that at least some of the 
> > chipsets that this driver supports don't need this timer nonsense at all
> 
> Well yes.  If all the chipsets need the timer then there's nothing we
> can do to improve it.
> 

Re: forcedeth: option to disable 100Hz timer (try 2)

[Alan Cox]

> > But it's quite likely that at least some of the 
> > chipsets that this driver supports don't need this timer nonsense at all
> 
> Well yes.  If all the chipsets need the timer then there's nothing we
> can do to improve it.

Surely you only need the timer running when transmits are active and
sufficient transmits to make it worth recovering the queued memory ?