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

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

[Ilya Shchepetkov]

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

[PATCH 1/5] sgi-xp: Call netif_carrier_off() after register_netdev()

[Ilya Shchepetkov]

For carrier detection to work properly when binding the driver with a
cable unplugged, netif_carrier_off() should be calle 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 <shchepetkov@ispras.ru>
---
 drivers/misc/sgi-xp/xpnet.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/sgi-xp/xpnet.c b/drivers/misc/sgi-xp/xpnet.c
index 3fac67a..9296c8d 100644
--- a/drivers/misc/sgi-xp/xpnet.c
+++ b/drivers/misc/sgi-xp/xpnet.c
@@ -550,8 +550,6 @@ xpnet_init(void)
 		return -ENOMEM;
 	}
 
-	netif_carrier_off(xpnet_device);
-
 	xpnet_device->netdev_ops = &xpnet_netdev_ops;
 	xpnet_device->mtu = XPNET_DEF_MTU;
 
@@ -584,6 +582,8 @@ xpnet_init(void)
 		kfree(xpnet_broadcast_partitions);
 	}
 
+	netif_carrier_off(xpnet_device);
+
 	return result;
 }
 
-- 
1.7.7

Re: [PATCH 1/5] sgi-xp: Call netif_carrier_off() after register_netdev()

[Robin Holt]

On Tue, Aug 14, 2012 at 02:28:51PM +0400, Ilya Shchepetkov wrote:
> For carrier detection to work properly when binding the driver with a
> cable unplugged, netif_carrier_off() should be calle 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 <shchepetkov@ispras.ru>

Acked-by: Robin Holt <holt@sgi.com>

> ---
>  drivers/misc/sgi-xp/xpnet.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/sgi-xp/xpnet.c b/drivers/misc/sgi-xp/xpnet.c
> index 3fac67a..9296c8d 100644
> --- a/drivers/misc/sgi-xp/xpnet.c
> +++ b/drivers/misc/sgi-xp/xpnet.c
> @@ -550,8 +550,6 @@ xpnet_init(void)
>  		return -ENOMEM;
>  	}
>  
> -	netif_carrier_off(xpnet_device);
> -
>  	xpnet_device->netdev_ops = &xpnet_netdev_ops;
>  	xpnet_device->mtu = XPNET_DEF_MTU;
>  
> @@ -584,6 +582,8 @@ xpnet_init(void)
>  		kfree(xpnet_broadcast_partitions);
>  	}
>  
> +	netif_carrier_off(xpnet_device);
> +
>  	return result;
>  }
>  
> -- 
> 1.7.7

[PATCH 2/5] de2104x: Call netif_carrier_off() after register_netdev()

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

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

Signed-off-by: Ilya Shchepetkov <shchepetkov@ispras.ru>
---
 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

[PATCH 3/5] net/mlx4_en: Call netif_carrier_off() after register_netdev()

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

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

Signed-off-by: Ilya Shchepetkov <shchepetkov@ispras.ru>
---
 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

RE: [PATCH 3/5] net/mlx4_en: Call netif_carrier_off() after register_netdev()

[Sathya.Perla]

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

Re: [PATCH 3/5] net/mlx4_en: Call netif_carrier_off() after register_netdev()

[Ben Hutchings]

On Tue, 2012-08-14 at 12:36 +0000, Sathya.Perla@Emulex.Com wrote:
> >-----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.

[PATCH 4/5] sungem: Call netif_carrier_off() after register_netdev()

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

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

Signed-off-by: Ilya Shchepetkov <shchepetkov@ispras.ru>
---
 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

[PATCH 5/5] net/hyperv: Call netif_carrier_off() after register_netdev()

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

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

Signed-off-by: Ilya Shchepetkov <shchepetkov@ispras.ru>
---
 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

RE: [PATCH 5/5] net/hyperv: Call netif_carrier_off() after register_netdev()

[Haiyang Zhang]

> -----Original Message-----
> From: Ilya Shchepetkov [mailto:shchepetkov@ispras.ru]
> Sent: Tuesday, August 14, 2012 6:29 AM
> To: KY Srinivasan
> Cc: Ilya Shchepetkov; Haiyang Zhang; David S. Miller;
> devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; ldv-project@ispras.ru
> Subject: [PATCH 5/5] net/hyperv: Call netif_carrier_off() after
> register_netdev()
> 
> 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 <shchepetkov@ispras.ru>
> ---
>  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);

Thanks.

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

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

[David Miller]

From: Ilya Shchepetkov <shchepetkov@ispras.ru>
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)
> 
>>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?

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.

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

[Ben Hutchings]

On Tue, 2012-08-14 at 14:00 -0700, David Miller wrote:
> From: Ilya Shchepetkov <shchepetkov@ispras.ru>
> 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)
> > 
> >>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?
> 
> 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.

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

[Bjørn Mork]

Ben Hutchings <bhutchings@solarflare.com> writes:

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

So you may want to add something like

int register_netdev_carrier_off(struct net_device *dev)
{
	int err;

	rtnl_lock();
	err = register_netdevice(dev);
        if (!err)
                set_bit(__LINK_STATE_NOCARRIER, &dev->state)
	rtnl_unlock();
	return err;
}


for these drivers?



Bjørn

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

[Ilya Shchepetkov]

>> Ben Hutchings <bhutchings@solarflare.com> writes:
>>> 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.
>> So you may want to add something like
>>
>> int register_netdev_carrier_off(struct net_device *dev)
>> {
>>      int err;
>>
>>      rtnl_lock();
>>      err = register_netdevice(dev);
>>         if (!err)
>>                 set_bit(__LINK_STATE_NOCARRIER, &dev->state)
>>      rtnl_unlock();
>>      return err;
>> }
>>
>>
>> for these drivers?

t looks like this variant is equivalent to the existing code:

	netif_carrier_off(dev);
	err = register_netdev(dev);
	if (err)
		goto out;

According to explanation in commit 22604c866889c4b2e12b73cbf1683bda1b72a313,
in this case "this causes these drivers to incorrectly report their
link status as IF_OPER_UNKNOWN which can falsely set the IFF_RUNNING
flag when the interface is first brought up".

As far as I understand, to fix the issue it is required to call
netif_carrier_off() itself:

int register_netdev_carrier_off(struct net_device *dev)
{
	int err;

	rtnl_lock();
	err = register_netdevice(dev);
	if (!err)
		netif_carrier_off(dev);
	rtnl_unlock();
	return err;
}

What do you think?

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

[Stephen Hemminger]

On Fri, 17 Aug 2012 11:55:43 +0400
Ilya Shchepetkov <shchepetkov@ispras.ru> wrote:

> >> Ben Hutchings <bhutchings@solarflare.com> writes:
> >>> 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.
> >> So you may want to add something like
> >>
> >> int register_netdev_carrier_off(struct net_device *dev)
> >> {
> >>      int err;
> >>
> >>      rtnl_lock();
> >>      err = register_netdevice(dev);
> >>         if (!err)
> >>                 set_bit(__LINK_STATE_NOCARRIER, &dev->state)
> >>      rtnl_unlock();
> >>      return err;
> >> }
> >>
> >>
> >> for these drivers?
> 
> t looks like this variant is equivalent to the existing code:
> 
> 	netif_carrier_off(dev);
> 	err = register_netdev(dev);
> 	if (err)
> 		goto out;
> 
> According to explanation in commit 22604c866889c4b2e12b73cbf1683bda1b72a313,
> in this case "this causes these drivers to incorrectly report their
> link status as IF_OPER_UNKNOWN which can falsely set the IFF_RUNNING
> flag when the interface is first brought up".
> 
> As far as I understand, to fix the issue it is required to call
> netif_carrier_off() itself:
> 
> int register_netdev_carrier_off(struct net_device *dev)
> {
> 	int err;
> 
> 	rtnl_lock();
> 	err = register_netdevice(dev);
> 	if (!err)
> 		netif_carrier_off(dev);
> 	rtnl_unlock();
> 	return err;
> }
> 
> What do you think?

Does this prevent  multiple link events from being reported to user space?

If the root cause of the problem is the link status
(commit 22604c866889c4b2e12b73cbf1683bda1b72a313), then the kernel
should be fixed to do link status correctly.

>From an application point of view IFF_RUNNING is meaningless unless IFF_UP
is set.