[PATCH 0/5] Call netif_carrier_off() after register_netdev()

Hi,

There are several patches on the subject:
	
	31bde1ceaa873bcaecd49e829bfabceacc4c512d
	c55ad8e56b983f03589b38b4504b5d1f41161ff8
	e826eafa65c6f1f7c8db5a237556cebac57ebcc5
	0d672e9f8ac320c6d1ea9103db6df7f99ea20361
	6a3c869a6021f4abcd69aa5fbb15c63f69eb36fe

In 2008, David Miller wrote in his commit:
(b47300168e770b60ab96c8924854c3b0eb4260eb)

net: Do not fire linkwatch events until the device is registered.

Several device drivers try to do things like netif_carrier_off()
before register_netdev() is invoked.  This is bogus, but too many
drivers do this to fix them all up in one go.

But I don't understand what will happen in this case?

Thanks,
Ilya

For carrier detection to work properly when binding the driver with a
cable unplugged, netif_carrier_off() should be called after
register_netdev(), not before.

Calling netif_carrier_off() before register_netdev() was causing the
network interface to miss a linkwatch pending event leading to an
inconsistent state if the link is not up when interface is initialized.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Ilya Shchepetkov <redacted>
---
 drivers/net/ethernet/sun/sungem.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
index 9ae12d0..c57c9f6 100644
--- a/drivers/net/ethernet/sun/sungem.c
+++ b/drivers/net/ethernet/sun/sungem.c

@@ -2909,7 +2909,6 @@ static int __devinit gem_init_one(struct pci_dev *pdev,
 
 	gp->lstate = link_down;
 	gp->timer_ticks = 0;
-	netif_carrier_off(dev);
 
 	gp->regs = ioremap(gemreg_base, gemreg_len);
 	if (!gp->regs) {

@@ -2988,6 +2987,9 @@ static int __devinit gem_init_one(struct pci_dev *pdev,
 		goto err_out_free_consistent;
 	}
 
+	/* Turn off carrier */
+	netif_carrier_off(dev);
+
 	/* Undo the get_cell with appropriate locking (we could use
 	 * ndo_init/uninit but that would be even more clumsy imho)
 	 */

-- 
1.7.7

For carrier detection to work properly when binding the driver with a
cable unplugged, netif_carrier_off() should be called after
register_netdev(), not before.

Calling netif_carrier_off() before register_netdev() was causing the
network interface to miss a linkwatch pending event leading to an
inconsistent state if the link is not up when interface is initialized.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Ilya Shchepetkov <redacted>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index edd9cb8..7bf2923 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c

@@ -1676,13 +1676,13 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 
 	mdev->pndev[port] = dev;
 
-	netif_carrier_off(dev);
 	err = register_netdev(dev);
 	if (err) {
 		en_err(priv, "Netdev registration failed for port %d\n", port);
 		goto out;
 	}
 	priv->registered = 1;
+	netif_carrier_off(dev);
 
 	en_warn(priv, "Using %d TX rings\n", prof->tx_ring_num);
 	en_warn(priv, "Using %d RX rings\n", prof->rx_ring_num);

-- 
1.7.7

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
Behalf Of Ilya Shchepetkov

For carrier detection to work properly when binding the driver with a
cable unplugged, netif_carrier_off() should be called after
register_netdev(), not before.

Calling netif_carrier_off() before register_netdev() was causing the
network interface to miss a linkwatch pending event leading to an
inconsistent state if the link is not up when interface is initialized.

ndo_open() may be called as soon register_netdev() completes...
When netif_carrier_off() is called *after* register_netdev(), isn't there
a possibility of a ndo_open()->netif_carrier_on() call racing this call, causing
incorrect results?

On Tue, 2012-08-14 at 12:36 +0000, Sathya.Perla@Emulex.Com wrote:

quoted

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
Behalf Of Ilya Shchepetkov

For carrier detection to work properly when binding the driver with a
cable unplugged, netif_carrier_off() should be called after
register_netdev(), not before.

Calling netif_carrier_off() before register_netdev() was causing the
network interface to miss a linkwatch pending event leading to an
inconsistent state if the link is not up when interface is initialized.

ndo_open() may be called as soon register_netdev() completes...
When netif_carrier_off() is called *after* register_netdev(), isn't there
a possibility of a ndo_open()->netif_carrier_on() call racing this call, causing
incorrect results?

Yes, you should use something like:

	rtnl_lock();
	rc = register_netdevice(dev);
	if (rc)
		goto out_unlock;
	netif_carrier_off(dev);
	rtnl_unlock();

(Why do we even have register_netdev() when it's such an invitation to
races?)

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.

For carrier detection to work properly when binding the driver with a
cable unplugged, netif_carrier_off() should be called after
register_netdev(), not before.

Calling netif_carrier_off() before register_netdev() was causing the
network interface to miss a linkwatch pending event leading to an
inconsistent state if the link is not up when interface is initialized.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Ilya Shchepetkov <redacted>
---
 drivers/net/ethernet/dec/tulip/de2104x.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/dec/tulip/de2104x.c b/drivers/net/ethernet/dec/tulip/de2104x.c
index 61cc093..237f254 100644
--- a/drivers/net/ethernet/dec/tulip/de2104x.c
+++ b/drivers/net/ethernet/dec/tulip/de2104x.c

@@ -2005,8 +2005,6 @@ static int __devinit de_init_one (struct pci_dev *pdev,
 		de->media_timer.function = de21041_media_timer;
 	de->media_timer.data = (unsigned long) de;
 
-	netif_carrier_off(dev);
-
 	/* wake up device, assign resources */
 	rc = pci_enable_device(pdev);
 	if (rc)

@@ -2074,6 +2072,9 @@ static int __devinit de_init_one (struct pci_dev *pdev,
 	rc = register_netdev(dev);
 	if (rc)
 		goto err_out_iomap;
+
+	/* turn off carrier */
+	netif_carrier_off(dev);
 
 	/* print info about board and interface just registered */
 	netdev_info(dev, "%s at %p, %pM, IRQ %d\n",

-- 
1.7.7

For carrier detection to work properly when binding the driver with a
cable unplugged, netif_carrier_off() should be called after
register_netdev(), not before.

Calling netif_carrier_off() before register_netdev() was causing the
network interface to miss a linkwatch pending event leading to an
inconsistent state if the link is not up when interface is initialized.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Ilya Shchepetkov <redacted>
---
 drivers/net/hyperv/netvsc_drv.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 8c5a1c4..5734ad0 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c

@@ -416,9 +416,6 @@ static int netvsc_probe(struct hv_device *dev,
 	if (!net)
 		return -ENOMEM;
 
-	/* Set initial state */
-	netif_carrier_off(net);
-
 	net_device_ctx = netdev_priv(net);
 	net_device_ctx->device_ctx = dev;
 	hv_set_drvdata(dev, net);

@@ -441,6 +438,9 @@ static int netvsc_probe(struct hv_device *dev,
 		goto out;
 	}
 
+	/* Set initial state */
+	netif_carrier_off(net);
+
 	/* Notify the netvsc driver of the new device */
 	device_info.ring_size = ring_size;
 	ret = rndis_filter_device_add(dev, &device_info);

-- 
1.7.7

Thanks.

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

From: Ilya Shchepetkov <redacted>
Date: Tue, 14 Aug 2012 14:28:50 +0400

Hi,

There are several patches on the subject:
	
	31bde1ceaa873bcaecd49e829bfabceacc4c512d
	c55ad8e56b983f03589b38b4504b5d1f41161ff8
	e826eafa65c6f1f7c8db5a237556cebac57ebcc5
	0d672e9f8ac320c6d1ea9103db6df7f99ea20361
	6a3c869a6021f4abcd69aa5fbb15c63f69eb36fe

In 2008, David Miller wrote in his commit:
(b47300168e770b60ab96c8924854c3b0eb4260eb)

quoted

net: Do not fire linkwatch events until the device is registered.

quoted

Several device drivers try to do things like netif_carrier_off()
before register_netdev() is invoked.  This is bogus, but too many
drivers do this to fix them all up in one go.

But I don't understand what will happen in this case?

Sigh... I would strongly suggest that when you don't understand
something you leave it alone until you do.

You can't do the netif_carrier_off() after the device register because
at the precise moment the device is registered it can be openned in
parallel on another cpu and thus cause the entire carrier state
to be changed.

Therefore if you do the netif_carrier_off() afterwards, it might
be overwriting state changes made in another context.

Please just leave this code alone.

On Tue, 2012-08-14 at 14:00 -0700, David Miller wrote:

From: Ilya Shchepetkov <redacted>
Date: Tue, 14 Aug 2012 14:28:50 +0400

quoted

Hi,

There are several patches on the subject:
	
	31bde1ceaa873bcaecd49e829bfabceacc4c512d
	c55ad8e56b983f03589b38b4504b5d1f41161ff8
	e826eafa65c6f1f7c8db5a237556cebac57ebcc5
	0d672e9f8ac320c6d1ea9103db6df7f99ea20361
	6a3c869a6021f4abcd69aa5fbb15c63f69eb36fe

In 2008, David Miller wrote in his commit:
(b47300168e770b60ab96c8924854c3b0eb4260eb)

quoted

net: Do not fire linkwatch events until the device is registered.

quoted

Several device drivers try to do things like netif_carrier_off()
before register_netdev() is invoked.  This is bogus, but too many
drivers do this to fix them all up in one go.

But I don't understand what will happen in this case?

Sigh... I would strongly suggest that when you don't understand
something you leave it alone until you do.

You can't do the netif_carrier_off() after the device register because
at the precise moment the device is registered it can be openned in
parallel on another cpu and thus cause the entire carrier state
to be changed.

Therefore if you do the netif_carrier_off() afterwards, it might
be overwriting state changes made in another context.

Please just leave this code alone.

But if you do it beforehand then it doesn't have the intended effect.
(Supposed to be fixed by 22604c866889c4b2e12b73cbf1683bda1b72a313, which
had to be reverted: c276e098d3ee33059b4a1c747354226cec58487c.)

So you have to do it after, but without dropping the RTNL lock in
between.

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.