bnx2: Fix the behavior of ethtool when ONBOOT=no

PATCH Applied Naohiro Ooiwa 5 participants 19 messages Jun 24, 2009 – Jul 2, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Naohiro Ooiwa

author

2009-06-24 02:57 UTC #0 lore raw

PATCH 1/1 bnx2: Fix the behavior of ethtool when ONBOOT=no

Hi Michael

I found a little bug.

When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.

    # grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2
    ONBOOT=no
    # ethtool eth2 | tail -n1
            Link detected: yes

I think "Link detected" should be "no".

The bnx2 driver should run netif_carrier_off() in initialization until bnx2_open() is called.

The following is my patch.
It move netif_carrier_off() to bnx2_init_one().
I had already tested this patch. The result is good for me.

Could you please check the my patch ?

Best Regards,
Naohiro Ooiwa

Signed-off-by Naohiro Ooiwa <[email protected]>

---
 drivers/net/bnx2.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

drivers/net/bnx2.c +3 -2

index 38f1c33..9eee986 100644
@@ -6151,8 +6151,6 @@ bnx2_open(struct net_device *dev)
 	struct bnx2 *bp = netdev_priv(dev);
 	int rc;
 
-	netif_carrier_off(dev);
-
 	bnx2_set_power_state(bp, PCI_D0);
 	bnx2_disable_int(bp);

@@ -8066,6 +8064,9 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (CHIP_NUM(bp) == CHIP_NUM_5709)
 		dev->features |= NETIF_F_TSO6;
 
+	/* Should set nocarrier until bnx2_open() is called */
+	netif_carrier_off(dev);
+
 	if ((rc = register_netdev(dev))) {
 		dev_err(&pdev->dev, "Cannot register net device\n");
 		goto error;

Signature · 1 line

1.5.4.1

Michael Chan

2009-06-24 03:48 UTC #1 lore raw

Quoted 21 lines · Naohiro Ooiwa

Hi Michael
I found a little bug.
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.
    # grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2
    ONBOOT=no
    # ethtool eth2 | tail -n1
            Link detected: yes
I think "Link detected" should be "no".
The bnx2 driver should run netif_carrier_off() in initialization until bnx2_open() is called.
The following is my patch.
It move netif_carrier_off() to bnx2_init_one().
I had already tested this patch. The result is good for me.

Quoted 8 lines

Could you please check the my patch ?
Best Regards,
Naohiro Ooiwa
Signed-off-by Naohiro Ooiwa <[email protected]>

This patch will not work. Calling netif_carrier_off() will schedule a link_watch event. If register_netdev() fails for any reason, the netdev will be freed but the link_watch event can still be scheduled.

Calling netif_carrier_off() after register_netdev() returns also will not work because it can then race with ->open() and the interrupt that indicates link up.

Thanks.

Quoted 33 lines

---
 drivers/net/bnx2.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

drivers/net/bnx2.c +3 -2

index 38f1c33..9eee986 100644
@@ -6151,8 +6151,6 @@ bnx2_open(struct net_device *dev)
      struct bnx2 *bp = netdev_priv(dev);
      int rc;
 
-     netif_carrier_off(dev);
-
      bnx2_set_power_state(bp, PCI_D0);
      bnx2_disable_int(bp);

@@ -8066,6 +8064,9 @@ bnx2_init_one(struct pci_dev *pdev,
 const struct pci_device_id *ent)
      if (CHIP_NUM(bp) == CHIP_NUM_5709)
              dev->features |= NETIF_F_TSO6;
 
+     /* Should set nocarrier until bnx2_open() is called */
+     netif_carrier_off(dev);
+
      if ((rc = register_netdev(dev))) {
              dev_err(&pdev->dev, "Cannot register net device\n");

              goto error;

--
1.5.4.1

Naohiro Ooiwa

author

2009-06-24 04:43 UTC #2 lore raw

Hi Michael

Thank you for quick reply and checking my patch.

You're right.
So, can we change the get_link method in ethtool ops. The following is image.

How do you feel about this idea.

Best Regards,
Naohiro Ooiwa

Signed-off-by Ooiwa Naohiro <[email protected]>

---
 bnx2.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

drivers/net/bnx2.c +9 -1

index 9eee986..89cd235 100644
@@ -6824,6 +6824,14 @@ bnx2_nway_reset(struct net_device *dev)
 }
 
 static int
+bnx2_get_link(struct net_device *dev)
+{
+	struct bnx2 *bp = netdev_priv(dev);
+
+	return bp->link_up;
+}
+
+static int
 bnx2_get_eeprom_len(struct net_device *dev)
 {
 	struct bnx2 *bp = netdev_priv(dev);

@@ -7390,7 +7398,7 @@ static const struct ethtool_ops bnx2_ethtool_ops = {
 	.get_wol		= bnx2_get_wol,
 	.set_wol		= bnx2_set_wol,
 	.nway_reset		= bnx2_nway_reset,
-	.get_link		= ethtool_op_get_link,
+	.get_link		= bnx2_get_link,
 	.get_eeprom_len		= bnx2_get_eeprom_len,
 	.get_eeprom		= bnx2_get_eeprom,
 	.set_eeprom		= bnx2_set_eeprom,

Quoted 81 lines · Michael Chan

Quoted 21 lines · Naohiro Ooiwa
Hi Michael
I found a little bug.
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.
    # grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2
    ONBOOT=no
    # ethtool eth2 | tail -n1
            Link detected: yes
I think "Link detected" should be "no".
The bnx2 driver should run netif_carrier_off() in initialization until bnx2_open() is called.
The following is my patch.
It move netif_carrier_off() to bnx2_init_one().
I had already tested this patch. The result is good for me.
Quoted 8 lines
Could you please check the my patch ?
Best Regards,
Naohiro Ooiwa
Signed-off-by Naohiro Ooiwa <[email protected]>
This patch will not work. Calling netif_carrier_off() will schedule a link_watch event. If register_netdev() fails for any reason, the netdev will be freed but the link_watch event can still be scheduled.
Calling netif_carrier_off() after register_netdev() returns also will not work because it can then race with ->open() and the interrupt that indicates link up.
Thanks.
Quoted 28 lines
---
 drivers/net/bnx2.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/bnx2.c +3 -2
index 38f1c33..9eee986 100644
@@ -6151,8 +6151,6 @@ bnx2_open(struct net_device *dev)
      struct bnx2 *bp = netdev_priv(dev);
      int rc;
 
-     netif_carrier_off(dev);
-
      bnx2_set_power_state(bp, PCI_D0);
      bnx2_disable_int(bp);
 
@@ -8066,6 +8064,9 @@ bnx2_init_one(struct pci_dev *pdev,
 const struct pci_device_id *ent)
      if (CHIP_NUM(bp) == CHIP_NUM_5709)
              dev->features |= NETIF_F_TSO6;
 
+     /* Should set nocarrier until bnx2_open() is called */
+     netif_carrier_off(dev);
+
      if ((rc = register_netdev(dev))) {
              dev_err(&pdev->dev, "Cannot register net device\n");
              goto error;
Quoted 5 lines
--
1.5.4.1
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael Chan

Acked-by

2009-06-24 05:05 UTC #3 lore raw

Quoted 8 lines · Naohiro Ooiwa

Hi Michael
Thank you for quick reply and checking my patch.
You're right.
So, can we change the get_link method in ethtool ops. The following is image.

Quoted 8 lines

How do you feel about this idea.
Best Regards,
Naohiro Ooiwa
Signed-off-by Ooiwa Naohiro <[email protected]>

This patch is fine. Thanks.

Acked-by Michael Chan <[email protected]>

Quoted 124 lines

---
 bnx2.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/net/bnx2.c +9 -1
index 9eee986..89cd235 100644
@@ -6824,6 +6824,14 @@ bnx2_nway_reset(struct net_device *dev)
 }
 
 static int
+bnx2_get_link(struct net_device *dev)
+{
+     struct bnx2 *bp = netdev_priv(dev);
+
+     return bp->link_up;
+}
+
+static int
 bnx2_get_eeprom_len(struct net_device *dev)
 {
      struct bnx2 *bp = netdev_priv(dev);
@@ -7390,7 +7398,7 @@ static const struct ethtool_ops
 bnx2_ethtool_ops = {
      .get_wol                = bnx2_get_wol,
      .set_wol                = bnx2_set_wol,
      .nway_reset             = bnx2_nway_reset,
-     .get_link               = ethtool_op_get_link,
+     .get_link               = bnx2_get_link,
      .get_eeprom_len         = bnx2_get_eeprom_len,
      .get_eeprom             = bnx2_get_eeprom,
.set_eeprom = bnx2_set_eeprom,
Quoted 30 lines · Michael Chan
Quoted 28 lines · Naohiro Ooiwa
Hi Michael
I found a little bug.
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.
    # grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2
    ONBOOT=no
    # ethtool eth2 | tail -n1
            Link detected: yes
I think "Link detected" should be "no".
The bnx2 driver should run netif_carrier_off() in initialization until bnx2_open() is called.
The following is my patch.
It move netif_carrier_off() to bnx2_init_one().
I had already tested this patch. The result is good for me.
Could you please check the my patch ?
Best Regards,
Naohiro Ooiwa
Quoted 5 lines
Quoted 1 line
Signed-off-by Naohiro Ooiwa <[email protected]>
This patch will not work. Calling netif_carrier_off() will schedule a link_watch event. If register_netdev() fails for any reason, the netdev will be freed but the link_watch event can still be
scheduled.
Quoted 3 lines
Calling netif_carrier_off() after register_netdev() returns also will not work because it can then race with ->open() and
the interrupt
Quoted 43 lines
that indicates link up.
Thanks.
Quoted 28 lines
---
 drivers/net/bnx2.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/bnx2.c +3 -2
index 38f1c33..9eee986 100644
@@ -6151,8 +6151,6 @@ bnx2_open(struct net_device *dev)
      struct bnx2 *bp = netdev_priv(dev);
      int rc;
 
-     netif_carrier_off(dev);
-
      bnx2_set_power_state(bp, PCI_D0);
      bnx2_disable_int(bp);
 
@@ -8066,6 +8064,9 @@ bnx2_init_one(struct pci_dev *pdev,
 const struct pci_device_id *ent)
      if (CHIP_NUM(bp) == CHIP_NUM_5709)
              dev->features |= NETIF_F_TSO6;
 
+     /* Should set nocarrier until bnx2_open() is called */
+     netif_carrier_off(dev);
+
      if ((rc = register_netdev(dev))) {
              dev_err(&pdev->dev, "Cannot register net device\n");
              goto error;
Quoted 5 lines
--
1.5.4.1
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Naohiro Ooiwa

author

2009-06-24 05:45 UTC #4 lore raw

Hi Michael

Thank you for your reply.

I tested this patch now.
The result was good for me.

Thanks you.
Naohiro Ooiwa

Quoted 149 lines · Michael Chan

Quoted 8 lines · Naohiro Ooiwa
Hi Michael
Thank you for quick reply and checking my patch.
You're right.
So, can we change the get_link method in ethtool ops. The following is image.
Quoted 8 lines
How do you feel about this idea.
Best Regards,
Naohiro Ooiwa
Signed-off-by Ooiwa Naohiro <[email protected]>
This patch is fine. Thanks.
Acked-by Michael Chan <[email protected]>
Quoted 68 lines
---
 bnx2.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/net/bnx2.c +9 -1
index 9eee986..89cd235 100644
@@ -6824,6 +6824,14 @@ bnx2_nway_reset(struct net_device *dev)
 }
 
 static int
+bnx2_get_link(struct net_device *dev)
+{
+     struct bnx2 *bp = netdev_priv(dev);
+
+     return bp->link_up;
+}
+
+static int
 bnx2_get_eeprom_len(struct net_device *dev)
 {
      struct bnx2 *bp = netdev_priv(dev);
@@ -7390,7 +7398,7 @@ static const struct ethtool_ops
 bnx2_ethtool_ops = {
      .get_wol                = bnx2_get_wol,
      .set_wol                = bnx2_set_wol,
      .nway_reset             = bnx2_nway_reset,
-     .get_link               = ethtool_op_get_link,
+     .get_link               = bnx2_get_link,
      .get_eeprom_len         = bnx2_get_eeprom_len,
      .get_eeprom             = bnx2_get_eeprom,
.set_eeprom = bnx2_set_eeprom,
Quoted 30 lines · Michael Chan
Quoted 28 lines · Naohiro Ooiwa
Hi Michael
I found a little bug.
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.
    # grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2
    ONBOOT=no
    # ethtool eth2 | tail -n1
            Link detected: yes
I think "Link detected" should be "no".
The bnx2 driver should run netif_carrier_off() in initialization until bnx2_open() is called.
The following is my patch.
It move netif_carrier_off() to bnx2_init_one().
I had already tested this patch. The result is good for me.
Could you please check the my patch ?
Best Regards,
Naohiro Ooiwa
Quoted 52 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Naohiro Ooiwa <[email protected]>
This patch will not work. Calling netif_carrier_off() will schedule a link_watch event. If register_netdev() fails for any reason, the netdev will be freed but the link_watch event can still be
scheduled.
Quoted 2 lines
Calling netif_carrier_off() after register_netdev() returns also will not work because it can then race with ->open() and
the interrupt
Quoted 42 lines
that indicates link up.
Thanks.
Quoted 28 lines
---
 drivers/net/bnx2.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/bnx2.c +3 -2
index 38f1c33..9eee986 100644
@@ -6151,8 +6151,6 @@ bnx2_open(struct net_device *dev)
      struct bnx2 *bp = netdev_priv(dev);
      int rc;
 
-     netif_carrier_off(dev);
-
      bnx2_set_power_state(bp, PCI_D0);
      bnx2_disable_int(bp);
 
@@ -8066,6 +8064,9 @@ bnx2_init_one(struct pci_dev *pdev,
 const struct pci_device_id *ent)
      if (CHIP_NUM(bp) == CHIP_NUM_5709)
              dev->features |= NETIF_F_TSO6;
 
+     /* Should set nocarrier until bnx2_open() is called */
+     netif_carrier_off(dev);
+
      if ((rc = register_netdev(dev))) {
              dev_err(&pdev->dev, "Cannot register net device\n");
              goto error;
Quoted 5 lines
--
1.5.4.1
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer Applied

2009-06-24 07:08 UTC #5 lore raw

From: Naohiro Ooiwa <[email protected]>
Date: Wed, 24 Jun 2009 14:45:42 +0900

Quoted 6 lines

Hi Michael
Thank you for your reply.
I tested this patch now.
The result was good for me.

I've applied your patch, thank you.

David Miller

maintainer

2009-06-24 07:16 UTC #6 lore raw

From: David Miller <[email protected]>
Date: Wed, 24 Jun 2009 00:08:52 -0700 (PDT)

Quoted 11 lines

From: Naohiro Ooiwa <[email protected]>
Date: Wed, 24 Jun 2009 14:45:42 +0900
Quoted 6 lines
Hi Michael
Thank you for your reply.
I tested this patch now.
The result was good for me.
I've applied your patch, thank you.

Actually, you change gives the following build warning.

drivers/net/bnx2.c:7403: warning: initialization from incompatible pointer type

I'll fix it up, but please take care of such trivial issues before you submit your patch.

Thanks.

Naohiro Ooiwa

author

2009-06-24 08:39 UTC #7 lore raw

Hi David

I'm sorry to trouble you.
I will check it in the next time.

Best Regards,
Naohiro Ooiwa

Quoted 22 lines · David Miller

From: David Miller <[email protected]>
Date: Wed, 24 Jun 2009 00:08:52 -0700 (PDT)
Quoted 10 lines
From: Naohiro Ooiwa <[email protected]>
Date: Wed, 24 Jun 2009 14:45:42 +0900
Quoted 6 lines
Hi Michael
Thank you for your reply.
I tested this patch now.
The result was good for me.
I've applied your patch, thank you.
Actually, you change gives the following build warning.
drivers/net/bnx2.c:7403: warning: initialization from incompatible pointer type
I'll fix it up, but please take care of such trivial issues before you submit your patch.
Thanks.

Rick Jones

2009-06-24 16:43 UTC #8 lore raw

In reply to Naohiro Ooiwa

Quoted 7 lines · Naohiro Ooiwa

Hi Michael
I found a little bug.
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.

Quoted 6 lines

    # grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2
    ONBOOT=no
    # ethtool eth2 | tail -n1
            Link detected: yes

I think "Link detected" should be "no".

Why? Sure, there is no IP on the link, but does that mean the link is otherwise unusable? Is ethtool only about IP status?

rick jones

Michael Chan

2009-06-24 17:10 UTC #9 lore raw

Quoted 13 lines · Rick Jones

Quoted 12 lines · Naohiro Ooiwa
Hi Michael
I found a little bug.
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.
    # grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2
    ONBOOT=no
    # ethtool eth2 | tail -n1
            Link detected: yes

Quoted 5 lines

Quoted 1 line
I think "Link detected" should be "no".
Why? Sure, there is no IP on the link, but does that mean the link is otherwise unusable? Is ethtool only about IP status?

Once the device is closed, we no longer keep track of the link state and no longer have register access to determine the link state. So we assume it is down. In reality, it may still be up if WoL is enabled or management firmware is running, but the driver can no longer keep track of it. If we have to assume one or the other, I think it is more correct to assume it is down.

David Miller

maintainer

2009-06-24 23:42 UTC #10 lore raw

From: "Michael Chan" <[email protected]>
Date: Wed, 24 Jun 2009 09:59:20 -0700

Quoted 11 lines · Rick Jones

Quoted 3 lines
Why? Sure, there is no IP on the link, but does that mean the link is otherwise unusable? Is ethtool only about IP status?
Once the device is closed, we no longer keep track of the link state and no longer have register access to determine the link state. So we assume it is down. In reality, it may still be up if WoL is enabled or management firmware is running, but the driver can no longer keep track of it. If we have to assume one or the other, I think it is more correct to assume it is down.

This is a big problem. I misread this situation when I decided to apply the patch yesterday, sorry. We're going to have to revert it.

Applications like NetworkManager decide which devices to bring up and attempt DHCP etc. on based upon the link status.

So if we report link down, the interface won't even be tried even if a cable is plugged in.

Actually I wonder, does NM bring the interface "up" before checking link state? Does anyone know?

Michael Chan

2009-06-24 23:58 UTC #11 lore raw

Quoted 18 lines · David Miller

From: "Michael Chan" <[email protected]>
Date: Wed, 24 Jun 2009 09:59:20 -0700
Quoted 11 lines · Rick Jones
Quoted 3 lines
Why? Sure, there is no IP on the link, but does that mean the link is otherwise unusable? Is ethtool only about IP status?
Once the device is closed, we no longer keep track of the link state and no longer have register access to determine the link state. So we assume it is down. In reality, it may still be up if WoL is enabled or management firmware is running, but the driver can no longer keep track of it. If we have to assume one or the other, I think it is more correct to assume it is down.
This is a big problem. I misread this situation when I decided to apply the patch yesterday, sorry. We're going to have to revert it.

Quoted 9 lines

Applications like NetworkManager decide which devices to bring up and attempt DHCP etc. on based upon the link status.
So if we report link down, the interface won't even be tried even if a cable is plugged in.
Actually I wonder, does NM bring the interface "up" before checking link state? Does anyone know?

I don't know about NetworkManager, but the old ifup script will bring up the device, wait up to 5 seconds for link up, and then do DHCP.

David Miller

maintainer

2009-06-25 00:12 UTC #12 lore raw

From: "Michael Chan" <[email protected]>
Date: Wed, 24 Jun 2009 16:48:32 -0700

Quoted 2 lines

I don't know about NetworkManager, but the old ifup script will bring up the device, wait up to 5 seconds for link up, and then do DHCP.

I'll swallow a handful of antacid tablets and take a look at the NM sources :-/

David Miller

maintainer

2009-06-25 00:25 UTC #13 lore raw

From: David Miller <[email protected]>
Date: Wed, 24 Jun 2009 17:12:46 -0700 (PDT)

Quoted 8 lines

From: "Michael Chan" <[email protected]>
Date: Wed, 24 Jun 2009 16:48:32 -0700
Quoted 2 lines
I don't know about NetworkManager, but the old ifup script will bring up the device, wait up to 5 seconds for link up, and then do DHCP.
I'll swallow a handful of antacid tablets and take a look at the NM sources :-/

Ok, NM first checks if the device supports either ethtool or MII based link status.

It checks for these capabilities by openning the device, trying the ethtool/ioctl op, then closing the device.

If link status reporting is found to be supported, it records the initial link state and listens for netlink events. (these are generated by netif_carrier_{on,off}() calls in the kernel)

When a link-up status netlink event is received, it brings wired devices reporting such events up.

And most importantly, it seems to bring the device UP during all of this stuff.

So I guess we're OK.

Naohiro Ooiwa

author

2009-06-26 04:30 UTC #14 lore raw

Hi David

Thank you for taking a look at the NM sources.

FYI, I confirmed the behavior of e1000e driver. The condition is ONBOOT=no, and with or without LAN cable. bnx2 is included this patch.

       | LAN cable |    ethtool
       |           | Link detected
-------+-----------+----------------
e1000e |  plugged  |    yes
e1000e | unplugged |     no
bnx2   |  plugged  |    yes
bnx2   | unplugged |     no (previously "yes")

The behavior of bnx2 became congruent with e1000e. The looks is natural for me.

Best Regards,
Naohiro Ooiwa

Quoted 29 lines · David Miller

From: David Miller <[email protected]>
Date: Wed, 24 Jun 2009 17:12:46 -0700 (PDT)
Quoted 7 lines
From: "Michael Chan" <[email protected]>
Date: Wed, 24 Jun 2009 16:48:32 -0700
Quoted 2 lines
I don't know about NetworkManager, but the old ifup script will bring up the device, wait up to 5 seconds for link up, and then do DHCP.
I'll swallow a handful of antacid tablets and take a look at the NM sources :-/
Ok, NM first checks if the device supports either ethtool or MII based link status.
It checks for these capabilities by openning the device, trying the ethtool/ioctl op, then closing the device.
If link status reporting is found to be supported, it records the initial link state and listens for netlink events. (these are generated by netif_carrier_{on,off}() calls in the kernel)
When a link-up status netlink event is received, it brings wired devices reporting such events up.
And most importantly, it seems to bring the device UP during all of this stuff.
So I guess we're OK.

Naohiro Ooiwa

author

2009-06-29 00:49 UTC #15 lore raw

In reply to Michael Chan

Hi Michael

Thank you for your comment.

I tried to create a patch of bnx2x driver.
It's the same fix as bnx2 driver.
Could you please check the my patch ?

Of cource, I made sure of compile.

Best Regards,
Naohiro Ooiwa

Signed-off-by Naohiro Ooiwa <[email protected]>

---
 drivers/net/bnx2x_main.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

drivers/net/bnx2x_main.c +9 -1

index fbf1352..951714a 100644
@@ -8637,6 +8637,14 @@ static int bnx2x_nway_reset(struct net_device *dev)
 	return 0;
 }
 
+static u32
+bnx2x_get_link(struct net_device *dev)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	return bp->link_vars.link_up;
+}
+
 static int bnx2x_get_eeprom_len(struct net_device *dev)
 {
 	struct bnx2x *bp = netdev_priv(dev);

@@ -10034,7 +10042,7 @@ static struct ethtool_ops bnx2x_ethtool_ops = {
 	.get_msglevel		= bnx2x_get_msglevel,
 	.set_msglevel		= bnx2x_set_msglevel,
 	.nway_reset		= bnx2x_nway_reset,
-	.get_link		= ethtool_op_get_link,
+	.get_link		= bnx2x_get_link,
 	.get_eeprom_len		= bnx2x_get_eeprom_len,
 	.get_eeprom		= bnx2x_get_eeprom,
 	.set_eeprom		= bnx2x_set_eeprom,

Signature · 1 line

1.5.4.1

Quoted 28 lines · Michael Chan

Quoted 8 lines · Rick Jones
Quoted 7 lines · Naohiro Ooiwa
Hi Michael
I found a little bug.
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.
Quoted 9 lines
Quoted 6 lines
    # grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2
    ONBOOT=no
    # ethtool eth2 | tail -n1
            Link detected: yes
I think "Link detected" should be "no".
Why? Sure, there is no IP on the link, but does that mean the link is otherwise unusable? Is ethtool only about IP status?
Once the device is closed, we no longer keep track of the link state and no longer have register access to determine the link state. So we assume it is down. In reality, it may still be up if WoL is enabled or management firmware is running, but the driver can no longer keep track of it. If we have to assume one or the other, I think it is more correct to assume it is down.

Eilon Greenstein

Acked-by

2009-06-30 14:50 UTC #16 lore raw

Quoted 9 lines · Naohiro Ooiwa

Hi Michael
Thank you for your comment.
I tried to create a patch of bnx2x driver.
It's the same fix as bnx2 driver.
Could you please check the my patch ?
Of cource, I made sure of compile.

It is also working as expected. I already made a similar patch based on your bnx2 fix and it looks exactly the same...

Thanks!

Quoted 2 lines

Signed-off-by Naohiro Ooiwa <[email protected]>

Acked-by Eilon Greenstein <[email protected]>

David Miller

maintainer Acked-by Eilon Greenstein

2009-06-30 19:46 UTC #17 lore raw

From: "Eilon Greenstein" <[email protected]>
Date: Tue, 30 Jun 2009 17:50:16 +0300

Quoted 11 lines · Naohiro Ooiwa

Quoted 9 lines
Hi Michael
Thank you for your comment.
I tried to create a patch of bnx2x driver.
It's the same fix as bnx2 driver.
Could you please check the my patch ?
Of cource, I made sure of compile.

Quoted 8 lines

It is also working as expected. I already made a similar patch based on your bnx2 fix and it looks exactly the same...
Thanks!
Quoted 2 lines
Signed-off-by Naohiro Ooiwa <[email protected]>
Acked-by Eilon Greenstein <[email protected]>

Applied, but you really need to begin writing proper changelog entries when you send patches out. Here is the one I wrote for you below. Otherwise a random person looking at this change will have no idea the reason for it, nor what it is related to.

==================================================
bnx2x: Fix the behavior of ethtool when ONBOOT=no

This is the same fix as commit 7959ea254ed18faee41160b1c50b3c9664735967 ("bnx2: Fix the behavior of ethtool when ONBOOT=no"), but for bnx2x:

--------------------
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.

# grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2 ONBOOT=no
# ethtool eth2 | tail -n1
Link detected: yes

I think "Link detected" should be "no".
--------------------

Signed-off-by Naohiro Ooiwa <[email protected]>

Acked-by Eilon Greenstein <[email protected]>

Signed-off-by David S. Miller <[email protected]>

Naohiro Ooiwa

author

2009-07-02 04:47 UTC #18 lore raw

Hi David

I'm sorry for my tactless mail.
I see your comment.

Best Regards,
Naohiro Ooiwa

Quoted 49 lines · David Miller

From: "Eilon Greenstein" <[email protected]>
Date: Tue, 30 Jun 2009 17:50:16 +0300
Quoted 9 lines · Naohiro Ooiwa
Quoted 8 lines
Hi Michael
Thank you for your comment.
I tried to create a patch of bnx2x driver.
It's the same fix as bnx2 driver.
Could you please check the my patch ?
Quoted 8 lines
Quoted 1 line
Of cource, I made sure of compile.
It is also working as expected. I already made a similar patch based on your bnx2 fix and it looks exactly the same...
Thanks!
Quoted 1 line
Signed-off-by Naohiro Ooiwa <[email protected]>
Acked-by Eilon Greenstein <[email protected]>
Applied, but you really need to begin writing proper changelog entries when you send patches out. Here is the one I wrote for you below. Otherwise a random person looking at this change will have no idea the reason for it, nor what it is related to.
==================================================
bnx2x: Fix the behavior of ethtool when ONBOOT=no
This is the same fix as commit 7959ea254ed18faee41160b1c50b3c9664735967 ("bnx2: Fix the behavior of ethtool when ONBOOT=no"), but for bnx2x:
--------------------
When configure in ifcfg-eth* is ONBOOT=no,
the behavior of ethtool command is wrong.
# grep ONBOOT /etc/sysconfig/network-scripts/ifcfg-eth2 ONBOOT=no
# ethtool eth2 | tail -n1
Link detected: yes
I think "Link detected" should be "no".
--------------------
Signed-off-by Naohiro Ooiwa <[email protected]>
Acked-by Eilon Greenstein <[email protected]>
Signed-off-by David S. Miller <[email protected]>