[PATCH] bnx2: turn off the network statck during initialization

[PATCH] bnx2: turn off the network statck during initialization

[Jiang Wang]

The initialization state of bnx2 driver is wrong. It does not turn
of the Linux network stack using netif_carrier_off. This may lead to
inconsistent report from ethtool as the link is up but speed is
unknown when the cable is not plugged in.

E.g.
        Speed: Unknown! (0)<--------------------------------------
        Duplex: Half       <--------------------------------------
        MDI: Unknown! (0)
        Port: Twisted Pair
        PHYAD: 1
        Transceiver: internal
        Auto-negotiation: on
        Supports Wake-on: g
        Wake-on: d
        Link detected: yes <---------------------------------------

This patches fixed the problem by turning off the network stack
during initialization.

Signed-off-by: Jiang Wang <jwang@riverbed.com>
---
 drivers/net/ethernet/broadcom/bnx2.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index ac7b744..ce4548d 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -8463,6 +8463,10 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->features |= dev->hw_features;
 	dev->priv_flags |= IFF_UNICAST_FLT;
 
+	/* tell the stack to leave us alone until bnx2_open() is called */
+	netif_carrier_off(dev);
+	netif_stop_queue(dev);
+
 	if ((rc = register_netdev(dev))) {
 		dev_err(&pdev->dev, "Cannot register net device\n");
 		goto error;
-- 
1.7.1

Re: [PATCH] bnx2: turn off the network statck during initialization

[Michael Chan]

On Thu, 2012-08-16 at 11:21 -0700, Jiang Wang wrote: 
> The initialization state of bnx2 driver is wrong. It does not turn
> of the Linux network stack using netif_carrier_off. This may lead to
> inconsistent report from ethtool as the link is up but speed is
> unknown when the cable is not plugged in.
> 
> E.g.
>         Speed: Unknown! (0)<--------------------------------------
>         Duplex: Half       <--------------------------------------
>         MDI: Unknown! (0)
>         Port: Twisted Pair
>         PHYAD: 1
>         Transceiver: internal
>         Auto-negotiation: on
>         Supports Wake-on: g
>         Wake-on: d
>         Link detected: yes <---------------------------------------
> 
> This patches fixed the problem by turning off the network stack
> during initialization.
> 
> Signed-off-by: Jiang Wang <jwang@riverbed.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
> index ac7b744..ce4548d 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -8463,6 +8463,10 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	dev->features |= dev->hw_features;
>  	dev->priv_flags |= IFF_UNICAST_FLT;
>  
> +	/* tell the stack to leave us alone until bnx2_open() is called */
> +	netif_carrier_off(dev);

We have tried this before and this didn't work.  netif_carrier_off()
calls linkwatch_fire_event() to schedule the event.  If
register_netdev() fails for whatever reason, the netdev will be freed
but the link_watch event may still be scheduled.

Calling netif_carrier_off() after register_netdev() returns successfully
also may not work as it will race with bnx2_open() which enables IRQ.
An linkup IRQ can happen at time and we may end up with the wrong
carrier state because of the race condition.

> +	netif_stop_queue(dev);
> +
>  	if ((rc = register_netdev(dev))) {
>  		dev_err(&pdev->dev, "Cannot register net device\n");
>  		goto error;

RE: [PATCH] bnx2: turn off the network statck during initialization

[Jiang Wang]

Hi Michael,

I just checked the code for netif_carrier_off(), and it will return if the dev is not initialized (registered) as follows:

void netif_carrier_off(struct net_device *dev)
{
        if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
                if (dev->reg_state == NETREG_UNINITIALIZED)
                        return;
                linkwatch_fire_event(dev);
        }
}

So linkwatch_fire_event should not fire in this case, right? Thanks for the quick response.

Regards,

Jiang

-------------------------------------
Jiang Wang
Member of Technical Staff
Riverbed Technology
Tel: (408) 522-5109
Email: Jiang.Wang@riverbed.com
www.riverbed.com


-----Original Message-----
From: Michael Chan [mailto:mchan@broadcom.com] 
Sent: Thursday, August 16, 2012 11:57 AM
To: Jiang Wang
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Chaitanya Lala; Francis St. Amant; Jiang Wang
Subject: Re: [PATCH] bnx2: turn off the network statck during initialization

On Thu, 2012-08-16 at 11:21 -0700, Jiang Wang wrote: 
> The initialization state of bnx2 driver is wrong. It does not turn of 
> the Linux network stack using netif_carrier_off. This may lead to 
> inconsistent report from ethtool as the link is up but speed is 
> unknown when the cable is not plugged in.
> 
> E.g.
>         Speed: Unknown! (0)<--------------------------------------
>         Duplex: Half       <--------------------------------------
>         MDI: Unknown! (0)
>         Port: Twisted Pair
>         PHYAD: 1
>         Transceiver: internal
>         Auto-negotiation: on
>         Supports Wake-on: g
>         Wake-on: d
>         Link detected: yes <---------------------------------------
> 
> This patches fixed the problem by turning off the network stack during 
> initialization.
> 
> Signed-off-by: Jiang Wang <jwang@riverbed.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
> b/drivers/net/ethernet/broadcom/bnx2.c
> index ac7b744..ce4548d 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -8463,6 +8463,10 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	dev->features |= dev->hw_features;
>  	dev->priv_flags |= IFF_UNICAST_FLT;
>  
> +	/* tell the stack to leave us alone until bnx2_open() is called */
> +	netif_carrier_off(dev);

We have tried this before and this didn't work.  netif_carrier_off() calls linkwatch_fire_event() to schedule the event.  If
register_netdev() fails for whatever reason, the netdev will be freed but the link_watch event may still be scheduled.

Calling netif_carrier_off() after register_netdev() returns successfully also may not work as it will race with bnx2_open() which enables IRQ.
An linkup IRQ can happen at time and we may end up with the wrong carrier state because of the race condition.

> +	netif_stop_queue(dev);
> +
>  	if ((rc = register_netdev(dev))) {
>  		dev_err(&pdev->dev, "Cannot register net device\n");
>  		goto error;

RE: [PATCH] bnx2: turn off the network statck during initialization

[Michael Chan]

On Thu, 2012-08-16 at 19:15 +0000, Jiang Wang wrote: 
> Hi Michael,
> 
> I just checked the code for netif_carrier_off(), and it will return if
> the dev is not initialized (registered) as follows:
> 
> void netif_carrier_off(struct net_device *dev)
> {
>         if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
>                 if (dev->reg_state == NETREG_UNINITIALIZED)
>                         return;
>                 linkwatch_fire_event(dev);
>         }
> }
> 
> So linkwatch_fire_event should not fire in this case, right? Thanks
> for the quick response.
> 
Yes, you're right.  Your patch should not cause any problem then.

But I still don't understand what problem you're trying to solve.  The
link status from ethtool is determined by bnx2_get_link() and it should
always return link down before bnx2_open() is called.  Can you please
elborate?  Thanks.

> Regards,
> 
> Jiang
> 
> -------------------------------------
> Jiang Wang
> Member of Technical Staff
> Riverbed Technology
> Tel: (408) 522-5109
> Email: Jiang.Wang@riverbed.com
> www.riverbed.com
> 
> 
> -----Original Message-----
> From: Michael Chan [mailto:mchan@broadcom.com] 
> Sent: Thursday, August 16, 2012 11:57 AM
> To: Jiang Wang
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Chaitanya Lala; Francis St. Amant; Jiang Wang
> Subject: Re: [PATCH] bnx2: turn off the network statck during initialization
> 
> On Thu, 2012-08-16 at 11:21 -0700, Jiang Wang wrote: 
> > The initialization state of bnx2 driver is wrong. It does not turn of 
> > the Linux network stack using netif_carrier_off. This may lead to 
> > inconsistent report from ethtool as the link is up but speed is 
> > unknown when the cable is not plugged in.
> > 
> > E.g.
> >         Speed: Unknown! (0)<--------------------------------------
> >         Duplex: Half       <--------------------------------------
> >         MDI: Unknown! (0)
> >         Port: Twisted Pair
> >         PHYAD: 1
> >         Transceiver: internal
> >         Auto-negotiation: on
> >         Supports Wake-on: g
> >         Wake-on: d
> >         Link detected: yes <---------------------------------------
> > 
> > This patches fixed the problem by turning off the network stack during 
> > initialization.
> > 
> > Signed-off-by: Jiang Wang <jwang@riverbed.com>
> > ---
> >  drivers/net/ethernet/broadcom/bnx2.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
> > b/drivers/net/ethernet/broadcom/bnx2.c
> > index ac7b744..ce4548d 100644
> > --- a/drivers/net/ethernet/broadcom/bnx2.c
> > +++ b/drivers/net/ethernet/broadcom/bnx2.c
> > @@ -8463,6 +8463,10 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	dev->features |= dev->hw_features;
> >  	dev->priv_flags |= IFF_UNICAST_FLT;
> >  
> > +	/* tell the stack to leave us alone until bnx2_open() is called */
> > +	netif_carrier_off(dev);
> 
> We have tried this before and this didn't work.  netif_carrier_off() calls linkwatch_fire_event() to schedule the event.  If
> register_netdev() fails for whatever reason, the netdev will be freed but the link_watch event may still be scheduled.
> 
> Calling netif_carrier_off() after register_netdev() returns successfully also may not work as it will race with bnx2_open() which enables IRQ.
> An linkup IRQ can happen at time and we may end up with the wrong carrier state because of the race condition.
> 
> > +	netif_stop_queue(dev);
> > +
> >  	if ((rc = register_netdev(dev))) {
> >  		dev_err(&pdev->dev, "Cannot register net device\n");
> >  		goto error;
> 
> 
> 
> 

RE: [PATCH] bnx2: turn off the network statck during initialization

[Jiang Wang]

OK. I see what you mean. In fact, I am working on an old RHEL based kernel and it still reports the link via ethtool_op_get_link. That is why I got the inconsistent report from the ethtool as I described in the patch. (the ethtool showed the link is up, but speed is unknown, duplex is half).

I guess I can just port in the patch for bnx2_get_link() to solve this problem.

Also, I have another comment related to link state.

Right now, the bnx2 driver powers up the device in bnx2_init_board(), regardless the netif_carrier is on or off. This may introduce following inconsistent behaviors:
1) suppose the cable is plugged in to the NIC and the other end is connected to a switch
2) user powers up the box
3) the Linux does not bring up the interface; i.e, ifconfig ethx shows it is down
4) ethtool ethx will show no link
5) if the user goes to check the light on the physical NIC, he will see the green link light is ON. That means the link is up, right? 

I think it is better to power down the device until bnx2_open is called. In this way, ethtool report and the physical link light will be consistent. 

Any suggestions? Thanks.

Regards,

Jiang

-----Original Message-----
From: Michael Chan [mailto:mchan@broadcom.com] 
Sent: Thursday, August 16, 2012 12:29 PM
To: Jiang Wang
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Chaitanya Lala; Francis St. Amant
Subject: RE: [PATCH] bnx2: turn off the network statck during initialization

On Thu, 2012-08-16 at 19:15 +0000, Jiang Wang wrote: 
> Hi Michael,
> 
> I just checked the code for netif_carrier_off(), and it will return if 
> the dev is not initialized (registered) as follows:
> 
> void netif_carrier_off(struct net_device *dev) {
>         if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
>                 if (dev->reg_state == NETREG_UNINITIALIZED)
>                         return;
>                 linkwatch_fire_event(dev);
>         }
> }
> 
> So linkwatch_fire_event should not fire in this case, right? Thanks 
> for the quick response.
> 
Yes, you're right.  Your patch should not cause any problem then.

But I still don't understand what problem you're trying to solve.  The link status from ethtool is determined by bnx2_get_link() and it should always return link down before bnx2_open() is called.  Can you please elborate?  Thanks.

> Regards,
> 
> Jiang
> 
> -------------------------------------
> Jiang Wang
> Member of Technical Staff
> Riverbed Technology
> Tel: (408) 522-5109
> Email: Jiang.Wang@riverbed.com
> www.riverbed.com
> 
> 
> -----Original Message-----
> From: Michael Chan [mailto:mchan@broadcom.com]
> Sent: Thursday, August 16, 2012 11:57 AM
> To: Jiang Wang
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Chaitanya 
> Lala; Francis St. Amant; Jiang Wang
> Subject: Re: [PATCH] bnx2: turn off the network statck during 
> initialization
> 
> On Thu, 2012-08-16 at 11:21 -0700, Jiang Wang wrote: 
> > The initialization state of bnx2 driver is wrong. It does not turn 
> > of the Linux network stack using netif_carrier_off. This may lead to 
> > inconsistent report from ethtool as the link is up but speed is 
> > unknown when the cable is not plugged in.
> > 
> > E.g.
> >         Speed: Unknown! (0)<--------------------------------------
> >         Duplex: Half       <--------------------------------------
> >         MDI: Unknown! (0)
> >         Port: Twisted Pair
> >         PHYAD: 1
> >         Transceiver: internal
> >         Auto-negotiation: on
> >         Supports Wake-on: g
> >         Wake-on: d
> >         Link detected: yes <---------------------------------------
> > 
> > This patches fixed the problem by turning off the network stack 
> > during initialization.
> > 
> > Signed-off-by: Jiang Wang <jwang@riverbed.com>
> > ---
> >  drivers/net/ethernet/broadcom/bnx2.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/broadcom/bnx2.c
> > b/drivers/net/ethernet/broadcom/bnx2.c
> > index ac7b744..ce4548d 100644
> > --- a/drivers/net/ethernet/broadcom/bnx2.c
> > +++ b/drivers/net/ethernet/broadcom/bnx2.c
> > @@ -8463,6 +8463,10 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	dev->features |= dev->hw_features;
> >  	dev->priv_flags |= IFF_UNICAST_FLT;
> >  
> > +	/* tell the stack to leave us alone until bnx2_open() is called */
> > +	netif_carrier_off(dev);
> 
> We have tried this before and this didn't work.  netif_carrier_off() 
> calls linkwatch_fire_event() to schedule the event.  If
> register_netdev() fails for whatever reason, the netdev will be freed but the link_watch event may still be scheduled.
> 
> Calling netif_carrier_off() after register_netdev() returns successfully also may not work as it will race with bnx2_open() which enables IRQ.
> An linkup IRQ can happen at time and we may end up with the wrong carrier state because of the race condition.
> 
> > +	netif_stop_queue(dev);
> > +
> >  	if ((rc = register_netdev(dev))) {
> >  		dev_err(&pdev->dev, "Cannot register net device\n");
> >  		goto error;
> 
> 
> 
> 

RE: [PATCH] bnx2: turn off the network statck during initialization

[Michael Chan]

On Thu, 2012-08-16 at 20:28 +0000, Jiang Wang wrote:
> Also, I have another comment related to link state.
> 
> Right now, the bnx2 driver powers up the device in bnx2_init_board(),
> regardless the netif_carrier is on or off.

We actually don't power up the device.  bnx2_init_board() just probes
the device.  If link is already up, it will stay up.  If it is down, it
will stay down.

> This may introduce following inconsistent behaviors:
> 1) suppose the cable is plugged in to the NIC and the other end is
> connected to a switch
> 2) user powers up the box

The link may already be up before or when you power up the box because
of management firmware (iLO, etc) or WoL.

> 3) the Linux does not bring up the interface; i.e, ifconfig ethx shows
> it is down
> 4) ethtool ethx will show no link
> 5) if the user goes to check the light on the physical NIC, he will
> see the green link light is ON. That means the link is up, right? 
> 
> I think it is better to power down the device until bnx2_open is
> called. In this way, ethtool report and the physical link light will
> be consistent. 

We cannot power it down.  If link is up, it is up for a reason (e.g. it
is an iLO port, etc).

Thanks.

RE: [PATCH] bnx2: turn off the network statck during initialization

[Ben Hutchings]

On Thu, 2012-08-16 at 20:28 +0000, Jiang Wang wrote:
[...]
> Also, I have another comment related to link state.
> 
> Right now, the bnx2 driver powers up the device in bnx2_init_board(),
> regardless the netif_carrier is on or off. This may introduce
> following inconsistent behaviors:
> 1) suppose the cable is plugged in to the NIC and the other end is
> connected to a switch
> 2) user powers up the box
> 3) the Linux does not bring up the interface; i.e, ifconfig ethx shows
> it is down

Most distributions will bring up all interfaces at boot, by default.

> 4) ethtool ethx will show no link
> 5) if the user goes to check the light on the physical NIC, he will
> see the green link light is ON. That means the link is up, right? 
> 
> I think it is better to power down the device until bnx2_open is
> called. In this way, ethtool report and the physical link light will
> be consistent. 
[...]

In general, a physical network interface may need to be enabled to allow
management traffic to a BMC, even when the interface is in the 'down'
state on the host.

The link state should be considered unknown whenever the interface is
down, and /sys/class/net/$IFACE/carrier is not readable then.  However
ETHTOOL_GLINK is not expected to fail in this way, and must always
return 0 (down) or 1 (up).  The convention (which is now enforced in
ethtool_get_link()) is that when the interface is down it should always
return 0.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [PATCH] bnx2: turn off the network statck during initialization

[Jiang Wang]

On Thu, 2012-08-16 at 20:28 +0000, Jiang Wang wrote:
> Also, I have another comment related to link state.
> 
> Right now, the bnx2 driver powers up the device in bnx2_init_board(), 
> regardless the netif_carrier is on or off.

We actually don't power up the device.  bnx2_init_board() just probes the device.  If link is already up, it will stay up.  If it is down, it will stay down.

---- I see. I was confused by the name of bnx2_set_power_state()  and I think the PHY is actually powered up by bnx2_reset_nic, right?

> This may introduce following inconsistent behaviors:
> 1) suppose the cable is plugged in to the NIC and the other end is 
> connected to a switch
> 2) user powers up the box

The link may already be up before or when you power up the box because of management firmware (iLO, etc) or WoL.

---- OK.

> 3) the Linux does not bring up the interface; i.e, ifconfig ethx shows 
> it is down
> 4) ethtool ethx will show no link
> 5) if the user goes to check the light on the physical NIC, he will 
> see the green link light is ON. That means the link is up, right?
> 
> I think it is better to power down the device until bnx2_open is 
> called. In this way, ethtool report and the physical link light will 
> be consistent.

We cannot power it down.  If link is up, it is up for a reason (e.g. it is an iLO port, etc).

---- OK. 


Thanks.

RE: [PATCH] bnx2: turn off the network statck during initialization

[Michael Chan]

On Thu, 2012-08-16 at 21:48 +0000, Jiang Wang wrote:
> ---- I see. I was confused by the name of bnx2_set_power_state()  and
> I think the PHY is actually powered up by bnx2_reset_nic, right?

bnx2_init_phy() in bnx2_init_nic() will bring up the PHY if it is down.

> 
> 

RE: [PATCH] bnx2: turn off the network statck during initialization

[Jiang Wang]

Got it. Thank you very much.

Regards,

Jiang

-------------------------------------
Jiang Wang
Member of Technical Staff
Riverbed Technology
Tel: (408) 522-5109
Email: Jiang.Wang@riverbed.com
www.riverbed.com


-----Original Message-----
From: Michael Chan [mailto:mchan@broadcom.com] 
Sent: Thursday, August 16, 2012 3:28 PM
To: Jiang Wang
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Chaitanya Lala; Francis St. Amant
Subject: RE: [PATCH] bnx2: turn off the network statck during initialization

On Thu, 2012-08-16 at 21:48 +0000, Jiang Wang wrote:
> ---- I see. I was confused by the name of bnx2_set_power_state()  and 
> I think the PHY is actually powered up by bnx2_reset_nic, right?

bnx2_init_phy() in bnx2_init_nic() will bring up the PHY if it is down.

> 
>