[2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Paul Rolland]

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

Hello,

This patch is based on Linux 2.4.32, and I've verified the same problem
exists on 2.6.15.4.
Working on a machine with a 2.4.32 kernel, I was surprised to see the driver
complaining when setting the speed to 100FD using mii-tool, but accepting
the setting with ethtool.
Digging into the code, I found that there is some confusion with :
 - DUPLEX_FULL and FULL_DUPLEX,
 - DUPLEX_HALF and HALF_DUPLEX
in the code :
...
                           spddplx += (mii_reg & 0x100)
                                       ? FULL_DUPLEX :
                                       HALF_DUPLEX;
                           retval = e1000_set_spd_dplx(adapter,
                                                       spddplx);
...
and
int
e1000_set_spd_dplx(struct e1000_adapter *adapter, uint16_t spddplx)
{
        adapter->hw.autoneg = 0;

        switch(spddplx) {
        case SPEED_10 + DUPLEX_HALF:
                adapter->hw.forced_speed_duplex = e1000_10_half;
                break;
....
when the constants don't have the same value.

This patch is simply changing the code in the e1000_set_spd_dplx to use the
same constants as does the caller of the function : FULL_DUPLEX and
HALF_DUPLEX
whose values are not 0, to make sure we have had a successfull init
(DUPLEX_HALF value is 0, and the DUPLEX_xxx are defined in ethtool.h, thus
are probably not meant to be used in the mii interface).

Signed-off-by: Paul Rolland <rol@as2917.net>

diff -urN linux-2.4.32-orig/drivers/net/e1000/e1000_main.c
linux-2.4.32/drivers/net/e1000/e1000_main.c
--- linux-2.4.32-orig/drivers/net/e1000/e1000_main.c    Mon Apr  4 01:42:19
2005
+++ linux-2.4.32/drivers/net/e1000/e1000_main.c Sat Feb 25 09:36:23 2006
@@ -2944,23 +2944,23 @@
        adapter->hw.autoneg = 0;
 
        switch(spddplx) {
-       case SPEED_10 + DUPLEX_HALF:
+       case SPEED_10 + HALF_DUPLEX:
                adapter->hw.forced_speed_duplex = e1000_10_half;
                break;
-       case SPEED_10 + DUPLEX_FULL:
+       case SPEED_10 + FULL_DUPLEX:
                adapter->hw.forced_speed_duplex = e1000_10_full;
                break;
-       case SPEED_100 + DUPLEX_HALF:
+       case SPEED_100 + HALF_DUPLEX:
                adapter->hw.forced_speed_duplex = e1000_100_half;
                break;
-       case SPEED_100 + DUPLEX_FULL:
+       case SPEED_100 + FULL_DUPLEX:
                adapter->hw.forced_speed_duplex = e1000_100_full;
                break;
-       case SPEED_1000 + DUPLEX_FULL:
+       case SPEED_1000 + FULL_DUPLEX:
                adapter->hw.autoneg = 1;
                adapter->hw.autoneg_advertised = ADVERTISE_1000_FULL;
                break;
-       case SPEED_1000 + DUPLEX_HALF: /* not supported */
+       case SPEED_1000 + HALF_DUPLEX: /* not supported */
        default:
                DPRINTK(PROBE, ERR, 
                        "Unsupported Speed/Duplexity configuration\n");


Paul Rolland, rol(at)as2917.net
ex-AS2917 Network administrator and Peering Coordinator

--

Please no HTML, I'm not a browser - Pas d'HTML, je ne suis pas un navigateur 
"Some people dream of success... while others wake up and work hard at it" 

"I worry about my child and the Internet all the time, even though she's too 
young to have logged on yet. Here's what I worry about. I worry that 10 or 15 
years from now, she will come to me and say 'Daddy, where were you when they 
took freedom of the press away from the Internet?'"
--Mike Godwin, Electronic Frontier Foundation 
 

[-- Attachment #2: e1000.patch --]
[-- Type: application/octet-stream, Size: 1487 bytes --]

diff -urN linux-2.4.32-orig/drivers/net/e1000/e1000_main.c linux-2.4.32/drivers/net/e1000/e1000_main.c
--- linux-2.4.32-orig/drivers/net/e1000/e1000_main.c    Mon Apr  4 01:42:19 2005
+++ linux-2.4.32/drivers/net/e1000/e1000_main.c Sat Feb 25 09:36:23 2006
@@ -2944,23 +2944,23 @@
        adapter->hw.autoneg = 0;
 
        switch(spddplx) {
-       case SPEED_10 + DUPLEX_HALF:
+       case SPEED_10 + HALF_DUPLEX:
                adapter->hw.forced_speed_duplex = e1000_10_half;
                break;
-       case SPEED_10 + DUPLEX_FULL:
+       case SPEED_10 + FULL_DUPLEX:
                adapter->hw.forced_speed_duplex = e1000_10_full;
                break;
-       case SPEED_100 + DUPLEX_HALF:
+       case SPEED_100 + HALF_DUPLEX:
                adapter->hw.forced_speed_duplex = e1000_100_half;
                break;
-       case SPEED_100 + DUPLEX_FULL:
+       case SPEED_100 + FULL_DUPLEX:
                adapter->hw.forced_speed_duplex = e1000_100_full;
                break;
-       case SPEED_1000 + DUPLEX_FULL:
+       case SPEED_1000 + FULL_DUPLEX:
                adapter->hw.autoneg = 1;
                adapter->hw.autoneg_advertised = ADVERTISE_1000_FULL;
                break;
-       case SPEED_1000 + DUPLEX_HALF: /* not supported */
+       case SPEED_1000 + HALF_DUPLEX: /* not supported */
        default:
                DPRINTK(PROBE, ERR, 
                        "Unsupported Speed/Duplexity configuration\n");

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Willy TARREAU]

Hello Paul,

On Sat, Feb 25, 2006 at 11:08:49AM +0100, Paul Rolland wrote:
> Hello,
> 
> This patch is based on Linux 2.4.32, and I've verified the same problem
> exists on 2.6.15.4.

it's mangled, tabs have been turned into whitespaces. I fixed it so please
use the appended one.

> Working on a machine with a 2.4.32 kernel, I was surprised to see the driver
> complaining when setting the speed to 100FD using mii-tool, but accepting
> the setting with ethtool.
> Digging into the code, I found that there is some confusion with :
>  - DUPLEX_FULL and FULL_DUPLEX,
>  - DUPLEX_HALF and HALF_DUPLEX
> in the code :
> ...
>                            spddplx += (mii_reg & 0x100)
>                                        ? FULL_DUPLEX :
>                                        HALF_DUPLEX;
>                            retval = e1000_set_spd_dplx(adapter,
>                                                        spddplx);
> ...
> and
> int
> e1000_set_spd_dplx(struct e1000_adapter *adapter, uint16_t spddplx)
> {
>         adapter->hw.autoneg = 0;
> 
>         switch(spddplx) {
>         case SPEED_10 + DUPLEX_HALF:
>                 adapter->hw.forced_speed_duplex = e1000_10_half;
>                 break;
> ....
> when the constants don't have the same value.
> 
> This patch is simply changing the code in the e1000_set_spd_dplx to use the
> same constants as does the caller of the function : FULL_DUPLEX and
> HALF_DUPLEX
> whose values are not 0, to make sure we have had a successfull init
> (DUPLEX_HALF value is 0, and the DUPLEX_xxx are defined in ethtool.h, thus
> are probably not meant to be used in the mii interface).
> 
> Signed-off-by: Paul Rolland <rol@as2917.net>
> 
> diff -urN linux-2.4.32-orig/drivers/net/e1000/e1000_main.c
> linux-2.4.32/drivers/net/e1000/e1000_main.c
> --- linux-2.4.32-orig/drivers/net/e1000/e1000_main.c    Mon Apr  4 01:42:19
> 2005
> +++ linux-2.4.32/drivers/net/e1000/e1000_main.c Sat Feb 25 09:36:23 2006
> @@ -2944,23 +2944,23 @@
>         adapter->hw.autoneg = 0;
>  
>         switch(spddplx) {
> -       case SPEED_10 + DUPLEX_HALF:
> +       case SPEED_10 + HALF_DUPLEX:
>                 adapter->hw.forced_speed_duplex = e1000_10_half;
>                 break;
> -       case SPEED_10 + DUPLEX_FULL:
> +       case SPEED_10 + FULL_DUPLEX:
>                 adapter->hw.forced_speed_duplex = e1000_10_full;
>                 break;
> -       case SPEED_100 + DUPLEX_HALF:
> +       case SPEED_100 + HALF_DUPLEX:
>                 adapter->hw.forced_speed_duplex = e1000_100_half;
>                 break;
> -       case SPEED_100 + DUPLEX_FULL:
> +       case SPEED_100 + FULL_DUPLEX:
>                 adapter->hw.forced_speed_duplex = e1000_100_full;
>                 break;
> -       case SPEED_1000 + DUPLEX_FULL:
> +       case SPEED_1000 + FULL_DUPLEX:
>                 adapter->hw.autoneg = 1;
>                 adapter->hw.autoneg_advertised = ADVERTISE_1000_FULL;
>                 break;
> -       case SPEED_1000 + DUPLEX_HALF: /* not supported */
> +       case SPEED_1000 + HALF_DUPLEX: /* not supported */
>         default:
>                 DPRINTK(PROBE, ERR, 
>                         "Unsupported Speed/Duplexity configuration\n");
> 
> 
> Paul Rolland, rol(at)as2917.net
> ex-AS2917 Network administrator and Peering Coordinator

Regards,
Willy


diff -urN linux-2.4.32-orig/drivers/net/e1000/e1000_main.c linux-2.4.32/drivers/net/e1000/e1000_main.c
--- linux-2.4.32-orig/drivers/net/e1000/e1000_main.c    Mon Apr  4 01:42:19 2005
+++ linux-2.4.32/drivers/net/e1000/e1000_main.c Sat Feb 25 09:36:23 2006
@@ -2944,23 +2944,23 @@
 	adapter->hw.autoneg = 0;
 
 	switch(spddplx) {
-	case SPEED_10 + DUPLEX_HALF:
+	case SPEED_10 + HALF_DUPLEX:
 		adapter->hw.forced_speed_duplex = e1000_10_half;
 		break;
-	case SPEED_10 + DUPLEX_FULL:
+	case SPEED_10 + FULL_DUPLEX:
 		adapter->hw.forced_speed_duplex = e1000_10_full;
 		break;
-	case SPEED_100 + DUPLEX_HALF:
+	case SPEED_100 + HALF_DUPLEX:
 		adapter->hw.forced_speed_duplex = e1000_100_half;
 		break;
-	case SPEED_100 + DUPLEX_FULL:
+	case SPEED_100 + FULL_DUPLEX:
 		adapter->hw.forced_speed_duplex = e1000_100_full;
 		break;
-	case SPEED_1000 + DUPLEX_FULL:
+	case SPEED_1000 + FULL_DUPLEX:
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_1000_FULL;
 		break;
-	case SPEED_1000 + DUPLEX_HALF: /* not supported */
+	case SPEED_1000 + HALF_DUPLEX: /* not supported */
 	default:
 		DPRINTK(PROBE, ERR, 
 			"Unsupported Speed/Duplexity configuration\n");

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Paul Rolland]

> it's mangled, tabs have been turned into whitespaces. I fixed 
> it so please
> use the appended one.

Sorry about that, thanks for the fix.

Paul

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Jesper Juhl]

On 2/25/06, Paul Rolland <rol@as2917.net> wrote:
> Hello,
>
> This patch is based on Linux 2.4.32, and I've verified the same problem
> exists on 2.6.15.4.

are you planning a 2.6 patch as well ?

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Paul Rolland]

Hello,

> are you planning a 2.6 patch as well ?
> 
I'm preparing it, and I'll be carefull with Tab/space ;)

Paul

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Jesper Juhl]

On 2/26/06, Paul Rolland <rol@witbe.net> wrote:
> Hello,
>
> > are you planning a 2.6 patch as well ?
> >
> I'm preparing it, and I'll be carefull with Tab/space ;)
>
Ok, great, I was just wondering since I would have made one if you had
no plans to do so.

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Paul Rolland]

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

Hello,

> Ok, great, I was just wondering since I would have made one if you had
> no plans to do so.

Well, I was just waiting to make sure it was interesting for someone ;)

Here is it, verified with tab and not spaces... but attached as my mailer
is likely to cripple anything I try to inline...

Signed-off-by: Paul Rolland <rol@as2917.net>

Cheers,
Paul

[-- Attachment #2: e1000.patch-2.6.15.4 --]
[-- Type: application/octet-stream, Size: 1507 bytes --]

diff -urN linux-2.6.15.4.orig/drivers/net/e1000/e1000_main.c linux-2.6.15.4/drivers/net/e1000/e1000_main.c
--- linux-2.6.15.4.orig/drivers/net/e1000/e1000_main.c	Fri Feb 10 07:22:48 2006
+++ linux-2.6.15.4/drivers/net/e1000/e1000_main.c	Sun Feb 26 15:04:40 2006
@@ -4153,29 +4153,29 @@
 
 	/* Fiber NICs only allow 1000 gbps Full duplex */
 	if((adapter->hw.media_type == e1000_media_type_fiber) &&
-		spddplx != (SPEED_1000 + DUPLEX_FULL)) {
+		spddplx != (SPEED_1000 + FULL_DUPLEX)) {
 		DPRINTK(PROBE, ERR, "Unsupported Speed/Duplex configuration\n");
 		return -EINVAL;
 	}
 
 	switch(spddplx) {
-	case SPEED_10 + DUPLEX_HALF:
+	case SPEED_10 + HALF_DUPLEX:
 		adapter->hw.forced_speed_duplex = e1000_10_half;
 		break;
-	case SPEED_10 + DUPLEX_FULL:
+	case SPEED_10 + FULL_DUPLEX:
 		adapter->hw.forced_speed_duplex = e1000_10_full;
 		break;
-	case SPEED_100 + DUPLEX_HALF:
+	case SPEED_100 + HALF_DUPLEX:
 		adapter->hw.forced_speed_duplex = e1000_100_half;
 		break;
-	case SPEED_100 + DUPLEX_FULL:
+	case SPEED_100 + FULL_DUPLEX:
 		adapter->hw.forced_speed_duplex = e1000_100_full;
 		break;
-	case SPEED_1000 + DUPLEX_FULL:
+	case SPEED_1000 + FULL_DUPLEX:
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_1000_FULL;
 		break;
-	case SPEED_1000 + DUPLEX_HALF: /* not supported */
+	case SPEED_1000 + HALF_DUPLEX: /* not supported */
 	default:
 		DPRINTK(PROBE, ERR, "Unsupported Speed/Duplex configuration\n");
 		return -EINVAL;

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Jesse Brandeburg]

On 2/26/06, Paul Rolland <rol@witbe.net> wrote:
> Hello,
>
> > Ok, great, I was just wondering since I would have made one if you had
> > no plans to do so.
>
> Well, I was just waiting to make sure it was interesting for someone ;)
>
> Here is it, verified with tab and not spaces... but attached as my mailer
> is likely to cripple anything I try to inline...
>
> Signed-off-by: Paul Rolland <rol@as2917.net>

I've got an issue with this, as the same function is called in
e1000_ethtool.c.  I think the correct fix is to fix the caller in the
mii-tool case, but I am working on verifiying my assumptions.

In the meantime can you send the exact command you were having the problem with?

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Jesse Brandeburg]

> From: Paul Rolland <rol@as2917.net>
> 
> Hello,
> 
> This patch is based on Linux 2.4.32, and I've verified the same problem
> exists on 2.6.15.4.
> Working on a machine with a 2.4.32 kernel, I was surprised to see the driver
> complaining when setting the speed to 100FD using mii-tool, but accepting
> the setting with ethtool.
> Digging into the code, I found that there is some confusion with :
>  - DUPLEX_FULL and FULL_DUPLEX,
>  - DUPLEX_HALF and HALF_DUPLEX
> in the code :
> ...
>                            spddplx += (mii_reg & 0x100)
>                                        ? FULL_DUPLEX :
>                                        HALF_DUPLEX;
>                            retval = e1000_set_spd_dplx(adapter,
>                                                        spddplx);

Please try this patch:

e1000: fix mii-tool access to setting speed and duplex

Paul Rolland reported that e1000 was having a hard time using mii-tool to
set speed and duplex.  This patch fixes the issue on both newer hardware as
well as fixing the code issue that originally caused the problem.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Paul Rolland <rol@as2917.net>

---

  drivers/net/e1000/e1000_main.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 31e3329..9730c2e 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -4269,7 +4269,7 @@ e1000_mii_ioctl(struct net_device *netde
  			spin_unlock_irqrestore(&adapter->stats_lock, flags);
  			return -EIO;
  		}
-		if (adapter->hw.phy_type == e1000_phy_m88) {
+		if (adapter->hw.media_type == e1000_media_type_copper) {
  			switch (data->reg_num) {
  			case PHY_CTRL:
  				if (mii_reg & MII_CR_POWER_DOWN)
@@ -4285,8 +4285,8 @@ e1000_mii_ioctl(struct net_device *netde
  					else
  						spddplx = SPEED_10;
  					spddplx += (mii_reg & 0x100)
-						   ? FULL_DUPLEX :
-						   HALF_DUPLEX;
+						   ? DUPLEX_FULL :
+						   DUPLEX_HALF;
  					retval = e1000_set_spd_dplx(adapter,
  								    spddplx);
  					if (retval) {

Re: [2.4.32 - 2.6.15.4] e1000 - Fix mii interface

[Paul Rolland]

Hello Jesse,

>   					spddplx += (mii_reg & 0x100)
> -						   ? FULL_DUPLEX :
> -						   HALF_DUPLEX;
> +						   ? DUPLEX_FULL :
> +						   DUPLEX_HALF;

As I said in my first mail, I didn't want to go that way as one of the
two DUPLEX_FULL or DUPLEX_HALF is defined as being 0, which prevents
detecting incorrect/incomplete initializations.

The problem I had came from :
mii-tool -F 100BaseTx-FD eth0
when at the same time the ethtool interface was OK.

But you are right, the change I made missed the second caller.

The correct change is yours, though for the reason I put above, I think
it'll make it harder to spot other bugs ;)

Well done,
Paul