[Patch] netxen_nic: Bugfix for wrong RUNNING status of NX3031 NICs with no link

[Patch] netxen_nic: Bugfix for wrong RUNNING status of NX3031 NICs with no link

[jiang.biao2]

From: Li Fengmao <li.fengmao@zte.com.cn>

Ifconfig command displays RUNNING status when there is no link on
NX3031 NICs. The problem is caused by the wrong calling order of
netif_carrier_off() and register_netdev() in netxen_setup_netdev(),
dev->reg_state is initialized to NETREG_UNINITIALIZED before
registering network device, so the linkwatch_fire_event() will not
be called to notify the link change event in netif_carrier_off(),
and the operational status of network device will be set to
IF_OPER_UNKNOWN. In that case, the IFF_RUNNING flags will be set,
which result in the wrong RUNNING status in the result of ifconfig
command.

It can be solved by calling netif_carrier_off() after
register_netdev() in netxen_setup_netdev() to ensure the
notification of the link change event. And then the operational
status of device will be set to IF_OPER_DOWN correctly.

Steps to reproduce the bug:
1. Prepare a NX3031 NIC with no network cable on it.
2. Reboot the system. (e.g exec reboot command)
3. Activate the interface. (e.g ifconfig eth0 up)
4. The operstatus of the NX3031 NIC will be RUNNING in the result of
ifconfig command.

Signed-off-by: Li Fengmao <li.fengmao@zte.com.cn>
Reviewed-by: Long Chun <long.chun@zte.com.cn>
Reviewed-by: Wang Liang <wang.liang82@zte.com.cn>
Reviewed-by: Cai Qu <cai.qu@zte.com.cn>
Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>

--- drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c	2013-09-26 09:51:39.115903504 +0800
+++ drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c	2013-09-26 09:53:13.868903522 +0800
@@ -1404,13 +1404,13 @@ netxen_setup_netdev(struct netxen_adapte
 	if (netxen_read_mac_addr(adapter))
 		dev_warn(&pdev->dev, "failed to read mac addr\n");

-	netif_carrier_off(netdev);
-
 	err = register_netdev(netdev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to register net device\n");
 		return err;
 	}
+
+	netif_carrier_off(netdev);

 	return 0;
 }

Re: [Patch] netxen_nic: Bugfix for wrong RUNNING status of NX3031 NICs with no link

[Ben Hutchings]

On Thu, 2014-01-02 at 11:37 +0800, jiang.biao2@zte.com.cn wrote:
> From: Li Fengmao <li.fengmao@zte.com.cn>
> 
> Ifconfig command displays RUNNING status when there is no link on
> NX3031 NICs. The problem is caused by the wrong calling order of
> netif_carrier_off() and register_netdev() in netxen_setup_netdev(),
> dev->reg_state is initialized to NETREG_UNINITIALIZED before
> registering network device, so the linkwatch_fire_event() will not
> be called to notify the link change event in netif_carrier_off(),
> and the operational status of network device will be set to
> IF_OPER_UNKNOWN. In that case, the IFF_RUNNING flags will be set,
> which result in the wrong RUNNING status in the result of ifconfig
> command.
[...]

This was fixed in Linux 3.7.  The driver should not be changed.

(This change may be worthwhile in an out-of-tree version for older
kernel versions, but beware of races with opening of the device.)

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.

[Patch] netxen_nic: Bugfix for wrong RUNNING status of NX3031 NICs with no link

[jiang.biao2]

From: Li Fengmao <li.fengmao@zte.com.cn>

Ifconfig command displays RUNNING status when there is no link on
NX3031 NICs. The problem is caused by the wrong calling order of
netif_carrier_off() and register_netdev() in netxen_setup_netdev(),
dev->reg_state is initialized to NETREG_UNINITIALIZED before
registering network device, so the linkwatch_fire_event() will not
be called to notify the link change event in netif_carrier_off(),
and the operational status of network device will be set to
IF_OPER_UNKNOWN. In that case, the IFF_RUNNING flags will be set,
which result in the wrong RUNNING status in the result of ifconfig
command.

It can be solved by calling netif_carrier_off() after
register_netdev() in netxen_setup_netdev() to ensure the
notification of the link change event. And then the operational
status of device will be set to IF_OPER_DOWN correctly.

Steps to reproduce the bug:
1. Prepare a NX3031 NIC with no network cable on it.
2. Reboot the system. (e.g exec reboot command)
3. Activate the interface. (e.g ifconfig eth0 up)
4. The operstatus of the NX3031 NIC will be RUNNING in the result of
ifconfig command.

Signed-off-by: Li Fengmao <li.fengmao@zte.com.cn>
Reviewed-by: Long Chun <long.chun@zte.com.cn>
Reviewed-by: Wang Liang <wang.liang82@zte.com.cn>
Reviewed-by: Cai Qu <cai.qu@zte.com.cn>
Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>

--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -1404,13 +1404,13 @@ netxen_setup_netdev(struct netxen_adapte
 	if (netxen_read_mac_addr(adapter))
 		dev_warn(&pdev->dev, "failed to read mac addr\n");

-	netif_carrier_off(netdev);
-
 	err = register_netdev(netdev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to register net device\n");
 		return err;
 	}
+
+	netif_carrier_off(netdev);

 	return 0;
 }

Re: [Patch] netxen_nic: Bugfix for wrong RUNNING status of NX3031 NICs with no link

[David Miller]

From: jiang.biao2@zte.com.cn
Date: Thu, 2 Jan 2014 14:32:32 +0800

> From: Li Fengmao <li.fengmao@zte.com.cn>
> 
> Ifconfig command displays RUNNING status when there is no link on
> NX3031 NICs. The problem is caused by the wrong calling order of
> netif_carrier_off() and register_netdev() in netxen_setup_netdev(),
> dev->reg_state is initialized to NETREG_UNINITIALIZED before
> registering network device, so the linkwatch_fire_event() will not
> be called to notify the link change event in netif_carrier_off(),
> and the operational status of network device will be set to
> IF_OPER_UNKNOWN. In that case, the IFF_RUNNING flags will be set,
> which result in the wrong RUNNING status in the result of ifconfig
> command.
> 
> It can be solved by calling netif_carrier_off() after
> register_netdev() in netxen_setup_netdev() to ensure the
> notification of the link change event. And then the operational
> status of device will be set to IF_OPER_DOWN correctly.
> 
> Steps to reproduce the bug:
> 1. Prepare a NX3031 NIC with no network cable on it.
> 2. Reboot the system. (e.g exec reboot command)
> 3. Activate the interface. (e.g ifconfig eth0 up)
> 4. The operstatus of the NX3031 NIC will be RUNNING in the result of
> ifconfig command.
> 
> Signed-off-by: Li Fengmao <li.fengmao@zte.com.cn>
> Reviewed-by: Long Chun <long.chun@zte.com.cn>
> Reviewed-by: Wang Liang <wang.liang82@zte.com.cn>
> Reviewed-by: Cai Qu <cai.qu@zte.com.cn>
> Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>

You cannot do this, because at the very exact moment you call
register_netdevice() the device can be brought up and once the
device is up the link can be brought up.

Therefore if you invoke netif_carrier_off() right after
register_netdevice(), it can cancel out a legitimate
netif_carrier_on() call.

Re: [Patch] netxen_nic: Bugfix for wrong RUNNING status of NX3031 NICs with no link

[jiang.biao2]

> You cannot do this, because at the very exact moment you call
> register_netdevice() the device can be brought up and once the
> device is up the link can be brought up.

> Therefore if you invoke netif_carrier_off() right after
> register_netdevice(), it can cancel out a legitimate
> netif_carrier_on() call.

register_netdevice() should be call in probe(), at that time, is
it impossible to bring up device because the driver loading has
not finished?

Even that, the netif_carrier_off() will be called before
netif_carrier_on(), and that will not be a problem.

Besides, the other net drivers(e.g igb, e1000, e1000e) I saw all
call netif_carrier_off() after register_netdev(). There is no such
problems in these drivers.

Re: [Patch] netxen_nic: Bugfix for wrong RUNNING status of NX3031 NICs with no link

[Ben Hutchings]

On Thu, 2014-01-02 at 17:16 +0800, jiang.biao2@zte.com.cn wrote:
> > You cannot do this, because at the very exact moment you call
> > register_netdevice() the device can be brought up and once the
> > device is up the link can be brought up.
> 
> > Therefore if you invoke netif_carrier_off() right after
> > register_netdevice(), it can cancel out a legitimate
> > netif_carrier_on() call.
> 
> register_netdevice() should be call in probe(), at that time, is
> it impossible to bring up device because the driver loading has
> not finished?

So far as the networking core is concerned, when register_netdevice() is
called the device is ready to use.  But you can do:

rtnl_lock();
register_netdevice();
netif_carrier_off();
rtnl_unlock();

However, as I said before, the current order in netxen_nic works and
only older kernel versions require you to call netif_carrier_off() after
registering the device.

> Even that, the netif_carrier_off() will be called before
> netif_carrier_on(), and that will not be a problem.
> 
> Besides, the other net drivers(e.g igb, e1000, e1000e) I saw all
> call netif_carrier_off() after register_netdev(). There is no such
> problems in these drivers.

It is not easy to hit the race condition but it is real.

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.