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

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

Re: =?iso-2022-jp-3?B?GyRCRXobJChQJTsbKEI6?= Re: [Patch] netxen_nic: Bugfix for wrong RUNNING status of NX3031 NICs with no link

[David Miller]

From: jiang.biao2@zte.com.cn
Date: Fri, 3 Jan 2014 15:07:27 +0800

> netif_carrier_off();
> netdev->operstate=IF_OPER_DOWN;
> register_netdevice();

This is too gross for words.

Stop fighting the way that device registraction and initial carrier
state works, please.

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

[Ben Hutchings]

On Fri, 2014-01-03 at 15:07 +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.
> 
> But the problem I depicted really exists in the current netxen_nic driver,
> you can easily reproduce it following the reproduction steps.

'Current' as in Linux 3.12/3.13-rc7?

I don't know where I would find a Netxen card now.  So I can't test
netxen_nic myself, but I can assure you the sfc driver does things in
the same order and it works correctly.

> Can the following modification fix both the problem I faced and the race
> you worried?
> 
> netif_carrier_off();
> netdev->operstate=IF_OPER_DOWN;
> register_netdevice();

register_netdevice() calls linkwatch_init_dev() which sets the operstate
if necessary.  You should not set it directly.

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: =?iso-2022-jp-3?B?GyRCRXobJChQJTsbKEI6?= 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 16:53:17 +0800

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

Scripts in userspace listen for device events, such as registry, and
in response can immediately open the device.

That can occur before register_netdevice() returns, it is completely
and %100 asynchronous.