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 <nooiwa@meriraclelinux.com>
---
 drivers/net/bnx2.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Naohiro Ooiwa wrote:

> 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 <nooiwa@meriraclelinux.com>

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.

> ---
>  drivers/net/bnx2.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 38f1c33..9eee986 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -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
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Naohiro Ooiwa wrote:
> 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".

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2009-06-24 at 09:43 -0700, Rick Jones wrote:
> Naohiro Ooiwa wrote:
> > 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".
> 
> 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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: "Michael Chan" <mchan@broadcom.com>
Date: Wed, 24 Jun 2009 09:59:20 -0700

> On Wed, 2009-06-24 at 09:43 -0700, Rick Jones wrote:
>> 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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2009-06-24 at 16:42 -0700, David Miller wrote:
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Wed, 24 Jun 2009 09:59:20 -0700
> 
> > On Wed, 2009-06-24 at 09:43 -0700, Rick Jones wrote:
> >> 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?
> 

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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: "Michael Chan" <mchan@broadcom.com>
Date: Wed, 24 Jun 2009 16:48:32 -0700

> 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 :-/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: David Miller <davem@davemloft.net>
Date: Wed, 24 Jun 2009 17:12:46 -0700 (PDT)

> From: "Michael Chan" <mchan@broadcom.com>
> Date: Wed, 24 Jun 2009 16:48:32 -0700
> 
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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


David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 24 Jun 2009 17:12:46 -0700 (PDT)
> 
>> From: "Michael Chan" <mchan@broadcom.com>
>> Date: Wed, 24 Jun 2009 16:48:32 -0700
>>
>>> 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.
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html