via-rhine: WOL band-aid

PATCH In review Roger Luethi 3 participants 5 messages Nov 30, 2004 – Dec 2, 2004 Network drivers

Share & tools

Roger Luethi

author

2004-11-30 22:45 UTC #0 lore raw

PATCH 1/1 via-rhine: WOL band-aid

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 <[email protected]>

drivers/net/via-rhine.c renamed from drivers/net/via-rhine.c.orig +13 -5

@@ -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);

Jeff Garzik

2004-11-30 23:26 UTC #1 lore raw

Quoted 8 lines · 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.

Quoted 4 lines

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

Roger Luethi

author

2004-11-30 23:47 UTC #2 lore raw

Quoted 2 lines · Jeff Garzik

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

Sven Ladegast

2004-12-01 00:56 UTC #4 lore raw

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

In reply to Roger Luethi

Quoted 2 lines · Roger Luethi

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

Signature · 2 lines

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

via-rhine-patched.c renamed from via-rhine.c +13 -5

@@ -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);

Jeff Garzik

2004-12-02 22:41 UTC #3 lore raw

In reply to Roger Luethi

Quoted 7 lines · Roger Luethi

Quoted 1 line · Jeff Garzik
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