[PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[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


Signed-off-by: Naohiro Ooiwa <nooiwa@meriraclelinux.com>
---
 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

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Michael Chan]

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
>
>
>

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[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.

How do you feel about this idea.


Best Regards,
Naohiro Ooiwa


Signed-off-by: Ooiwa Naohiro <nooiwa@miraclelinux.com>
---
 bnx2.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 9eee986..89cd235 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -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,




Michael Chan 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".
>>
>> 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
> 

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Michael Chan]

Naohiro Ooiwa wrote:

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

This patch is fine.  Thanks.

Acked-by: Michael Chan <mchan@broadcom.com>

> ---
>  bnx2.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 9eee986..89cd235 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -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,
>
>
>
>
> Michael Chan 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".
> >>
> >> 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
> >
>
>
>

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Naohiro Ooiwa]

Hi Michael

Thank you for your reply.

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

Thanks you.
Naohiro Ooiwa


Michael Chan wrote:
> Naohiro Ooiwa wrote:
> 
>> 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 <nooiwa@miraclelinux.com>
> 
> This patch is fine.  Thanks.
> 
> Acked-by: Michael Chan <mchan@broadcom.com>
> 
>> ---
>>  bnx2.c |   10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
>> index 9eee986..89cd235 100644
>> --- a/drivers/net/bnx2.c
>> +++ b/drivers/net/bnx2.c
>> @@ -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,
>>
>>
>>
>>
>> Michael Chan 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".
>>>>
>>>> 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
>>>
>>
>>
> 
> --
> 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
> 

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[David Miller]

From: Naohiro Ooiwa <nooiwa@miraclelinux.com>
Date: Wed, 24 Jun 2009 14:45:42 +0900

> 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.

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 24 Jun 2009 00:08:52 -0700 (PDT)

> From: Naohiro Ooiwa <nooiwa@miraclelinux.com>
> Date: Wed, 24 Jun 2009 14:45:42 +0900
> 
>> 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.

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Naohiro Ooiwa]

Hi David

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

Best Regards,
Naohiro Ooiwa

David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 24 Jun 2009 00:08:52 -0700 (PDT)
> 
>> From: Naohiro Ooiwa <nooiwa@miraclelinux.com>
>> Date: Wed, 24 Jun 2009 14:45:42 +0900
>>
>>> 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.

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Rick Jones]

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

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Michael Chan]

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.

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[David Miller]

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?

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Michael Chan]

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.

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[David Miller]

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 :-/

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[David Miller]

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.

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Naohiro Ooiwa]

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.
> 

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[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.

Best Regards,
Naohiro Ooiwa


Signed-off-by: Naohiro Ooiwa <nooiwa@miraclelinux.com>
---
 drivers/net/bnx2x_main.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index fbf1352..951714a 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -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,
-- 
1.5.4.1

Michael Chan wrote:
> 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.
> 
> 
> 

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Eilon Greenstein]

On Sun, 2009-06-28 at 17:49 -0700, Naohiro Ooiwa wrote:
> 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!

> 
> Signed-off-by: Naohiro Ooiwa <nooiwa@miraclelinux.com>
Acked-by: Eilon Greenstein <eilong@broadcom.com>

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[David Miller]

From: "Eilon Greenstein" <eilong@broadcom.com>
Date: Tue, 30 Jun 2009 17:50:16 +0300

> On Sun, 2009-06-28 at 17:49 -0700, Naohiro Ooiwa wrote:
>> 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!
> 
>> 
>> Signed-off-by: Naohiro Ooiwa <nooiwa@miraclelinux.com>
> Acked-by: Eilon Greenstein <eilong@broadcom.com>

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 <nooiwa@miraclelinux.com>
Acked-by: Eilon Greenstein <eilong@broadcom.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] bnx2: Fix the behavior of ethtool when ONBOOT=no

[Naohiro Ooiwa]

Hi David

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

Best Regards,
Naohiro Ooiwa

David Miller wrote:
> From: "Eilon Greenstein" <eilong@broadcom.com>
> Date: Tue, 30 Jun 2009 17:50:16 +0300
> 
>> On Sun, 2009-06-28 at 17:49 -0700, Naohiro Ooiwa wrote:
>>> 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!
>>
>>> Signed-off-by: Naohiro Ooiwa <nooiwa@miraclelinux.com>
>> Acked-by: Eilon Greenstein <eilong@broadcom.com>
> 
> 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 <nooiwa@miraclelinux.com>
> Acked-by: Eilon Greenstein <eilong@broadcom.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>