[PATCH 2.6] via-rhine: WOL band-aid

[PATCH 2.6] via-rhine: WOL band-aid

[Roger Luethi]

After I disabled legacy WOL (i.e. controlled by EEPROM rather than
driver) in 2.6.9, several people reported regressions. Legacy WOL had
worked for them, but now it didn't anymore. The Right Way (TM) to fix
this will get the driver to set up working WOL for all hardware, but a
simpler solution will have to do for the time being: If a user requests
magic packet WOL, the driver re-enables legacy WOL. Yeah, I know it's
cheating.

This version applies against -mm. I suggest to put it there for testing
and into 2.6.11 if feedback is good.

Thanks to Pavel Ruzicka for testing.

Roger

Signed-off-by: Roger Luethi <rl@hellgate.ch>

--- 2.6-mm/drivers/net/via-rhine.c.orig	2004-11-30 23:28:46.663676720 +0100
+++ 2.6-mm/drivers/net/via-rhine.c	2004-11-30 23:32:00.928144032 +0100
@@ -654,7 +654,7 @@ static void __devinit rhine_reload_eepro
 
 	/* Turn off EEPROM-controlled wake-up (magic packet) */
 	if (rp->quirks & rqWOL)
-		iowrite8(ioread8(ioaddr + ConfigA) & 0xFE, ioaddr + ConfigA);
+		iowrite8(ioread8(ioaddr + ConfigA) & 0xFC, ioaddr + ConfigA);
 
 }
 
@@ -1905,8 +1905,14 @@ static void rhine_shutdown (struct devic
 	if (rp->quirks & rq6patterns)
 		iowrite8(0x04, ioaddr + 0xA7);
 
-	if (rp->wolopts & WAKE_MAGIC)
+	if (rp->wolopts & WAKE_MAGIC) {
 		iowrite8(WOLmagic, ioaddr + WOLcrSet);
+		/*
+		 * Turn EEPROM-controlled wake-up back on -- some hardware may
+		 * not cooperate otherwise.
+		 */
+		iowrite8(ioread8(ioaddr + ConfigA) | 0x03, ioaddr + ConfigA);
+	}
 
 	if (rp->wolopts & (WAKE_BCAST|WAKE_MCAST))
 		iowrite8(WOLbmcast, ioaddr + WOLcgSet);
@@ -1917,9 +1923,11 @@ static void rhine_shutdown (struct devic
 	if (rp->wolopts & WAKE_UCAST)
 		iowrite8(WOLucast, ioaddr + WOLcrSet);
 
-	/* Enable legacy WOL (for old motherboards) */
-	iowrite8(0x01, ioaddr + PwcfgSet);
-	iowrite8(ioread8(ioaddr + StickyHW) | 0x04, ioaddr + StickyHW);
+	if (rp->wolopts) {
+		/* Enable legacy WOL (for old motherboards) */
+		iowrite8(0x01, ioaddr + PwcfgSet);
+		iowrite8(ioread8(ioaddr + StickyHW) | 0x04, ioaddr + StickyHW);
+	}
 
 	/* Hit power state D3 (sleep) */
 	iowrite8(ioread8(ioaddr + StickyHW) | 0x03, ioaddr + StickyHW);

Re: [PATCH 2.6] via-rhine: WOL band-aid

[Jeff Garzik]

Roger Luethi wrote:
> After I disabled legacy WOL (i.e. controlled by EEPROM rather than
> driver) in 2.6.9, several people reported regressions. Legacy WOL had
> worked for them, but now it didn't anymore. The Right Way (TM) to fix
> this will get the driver to set up working WOL for all hardware, but a
> simpler solution will have to do for the time being: If a user requests
> magic packet WOL, the driver re-enables legacy WOL. Yeah, I know it's
> cheating.
> 
> This version applies against -mm. I suggest to put it there for testing
> and into 2.6.11 if feedback is good.
> 
> Thanks to Pavel Ruzicka for testing.

I don't object to the patch, but I wonder if anything can be done to 
reduce the usage of "magic numbers" (numeric rather than named constants)?

	Jeff

Re: [PATCH 2.6] via-rhine: WOL band-aid

[Roger Luethi]

On Tue, 30 Nov 2004 18:17:52 -0500, Jeff Garzik wrote:
> I don't object to the patch, but I wonder if anything can be done to 
> reduce the usage of "magic numbers" (numeric rather than named constants)?

That's a non-trivial task if you want to do it properly. There be dragons.
Magic numbers may be evil but at least they are honest and convenient.

Roger

Re: [PATCH 2.6] via-rhine: WOL band-aid

[Jeff Garzik]

Roger Luethi wrote:
> On Tue, 30 Nov 2004 18:17:52 -0500, Jeff Garzik wrote:
> 
>>I don't object to the patch, but I wonder if anything can be done to 
>>reduce the usage of "magic numbers" (numeric rather than named constants)?
> 
> 
> That's a non-trivial task if you want to do it properly. There be dragons.
> Magic numbers may be evil but at least they are honest and convenient.

The reason why they are evil is that I have no clue what a change like

-         iowrite8(ioread8(ioaddr + ConfigA) & 0xFE, ioaddr + ConfigA);
+         iowrite8(ioread8(ioaddr + ConfigA) & 0xFC, ioaddr + ConfigA);

does.

So two main points:

1) Linux is "evolution not revolution".  I'm not asking for you to 
change every magic number in the driver immediately to named constants. 
  Do it over time as you patch the driver.  Add a couple WOL constants 
now, a few more constants later, ...

2) Avoiding magic numbers is important to "reviewability" and long term 
maintenance.  Five years from now, changing "0xFE" to "0xFC" is just as 
indecipherable as it is today, but without the memory of the current WOL 
  discussions and experiences to guide us.  Source code needs to be 
_readable_.

As I said in the other email though, I applied your patch to the 
internal via-rhine queue, inside the netdev-2.6 queue.  I simply ask 
that you consider some remove-magic-numbers patches in the future.

	Jeff

Re: [PATCH 2.6] via-rhine: WOL band-aid (patch against 2.6.9)

[Sven Ladegast]

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

On Tuesday 30 November 2004 23:40, Roger Luethi wrote:

> This version applies against -mm. I suggest to put it there for testing
> and into 2.6.11 if feedback is good.

And here is the "backport" to 2.6.9 for current users waiting for this patch. 
It is working for me on an ECS Elitegroup KT-600A mainboard with VT8237 and a 
Rhine-II network controller.

Sven
-- 
Sven Ladegast, Friedrich-Fröbel-Straße 11, 93310 Arnstadt / Germany
Phone: +49-175-5334308, PGP-key: 0x5856A5ED


[-- Attachment #2: via-rhine-corrected-2.6.9.diff --]
[-- Type: text/x-diff, Size: 1444 bytes --]

--- via-rhine.c	2004-10-18 23:55:28.000000000 +0200
+++ via-rhine-patched.c	2004-12-01 01:02:18.000000000 +0100
@@ -664,7 +664,7 @@ static void __devinit rhine_reload_eepro
 
 	/* Turn off EEPROM-controlled wake-up (magic packet) */
 	if (rp->quirks & rqWOL)
-		writeb(readb(ioaddr + ConfigA) & 0xFE, ioaddr + ConfigA);
+		writeb(readb(ioaddr + ConfigA) & 0xFC, ioaddr + ConfigA);
 
 }
 
@@ -1917,8 +1917,14 @@ static void rhine_shutdown (struct devic
 	if (rp->quirks & rq6patterns)
 		writeb(0x04, ioaddr + 0xA7);
 
-	if (rp->wolopts & WAKE_MAGIC)
+	if (rp->wolopts & WAKE_MAGIC) {
 		writeb(WOLmagic, ioaddr + WOLcrSet);
+		/*
+		 * Turn EEPROM-controlled wake-up back on -- some hardware may
+		 * not cooperate otherwise.
+		 */
+		writeb(readb(ioaddr + ConfigA) | 0x03, ioaddr + ConfigA);
+	}
 
 	if (rp->wolopts & (WAKE_BCAST|WAKE_MCAST))
 		writeb(WOLbmcast, ioaddr + WOLcgSet);
@@ -1929,9 +1935,11 @@ static void rhine_shutdown (struct devic
 	if (rp->wolopts & WAKE_UCAST)
 		writeb(WOLucast, ioaddr + WOLcrSet);
 
-	/* Enable legacy WOL (for old motherboards) */
-	writeb(0x01, ioaddr + PwcfgSet);
-	writeb(readb(ioaddr + StickyHW) | 0x04, ioaddr + StickyHW);
+	if (rp->wolopts) {
+		/* Enable legacy WOL (for old motherboards) */
+		writeb(0x01, ioaddr + PwcfgSet);
+		writeb(readb(ioaddr + StickyHW) | 0x04, ioaddr + StickyHW);
+	}
 
 	/* Hit power state D3 (sleep) */
 	writeb(readb(ioaddr + StickyHW) | 0x03, ioaddr + StickyHW);