Patchwork vmxnet3: set carrier state properly on probe

login
register
mail settings
Submitter Neil Horman
Date Jan. 25, 2013, 8:54 p.m.
Message ID <1359147253-7204-1-git-send-email-nhorman@tuxdriver.com>
Download mbox | patch
Permalink /patch/215867/
State Superseded
Delegated to: David Miller
Headers show

Comments

Neil Horman - Jan. 25, 2013, 8:54 p.m.
vmxnet3 fails to set netif_carrier_off on probe, meaning that when an interface
is opened the __LINK_STATE_NOCARRIER bit is already cleared, and so
/sys/class/net/<ifname>/operstate remains in the unknown state.  Correct this by
setting netif_carrier_off on probe, like other drivers do.

Also, while we're at it, lets remove the netif_carrier_ok checks from the
link_state_update function, as that check is atomically contained within the
netif_carrier_[on|off] functions anyway

Tested successfully by myself

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: "VMware, Inc." <pv-drivers@vmware.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Ben Hutchings - Jan. 29, 2013, 5:54 p.m.
On Fri, 2013-01-25 at 15:54 -0500, Neil Horman wrote:
> vmxnet3 fails to set netif_carrier_off on probe, meaning that when an interface
> is opened the __LINK_STATE_NOCARRIER bit is already cleared, and so
> /sys/class/net/<ifname>/operstate remains in the unknown state.  Correct this by
> setting netif_carrier_off on probe, like other drivers do.
> 
> Also, while we're at it, lets remove the netif_carrier_ok checks from the
> link_state_update function, as that check is atomically contained within the
> netif_carrier_[on|off] functions anyway
> 
> Tested successfully by myself
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: "VMware, Inc." <pv-drivers@vmware.com>
> ---
>  drivers/net/vmxnet3/vmxnet3_drv.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
> index b1c90f8..66c26a9 100644
> --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
[...]
> @@ -3067,6 +3065,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
>  		goto err_register;
>  	}
>  
> +	netif_carrier_off(netdev);
>  	vmxnet3_check_link(adapter, false);
>  	return 0;
>  

You should do this before calling register_netdev(), otherwise the link
state change can race with vmxnet3_open().

Ben.
Dmitry Torokhov - Jan. 29, 2013, 6:54 p.m.
On Tuesday, January 29, 2013 05:54:11 PM Ben Hutchings wrote:
> On Fri, 2013-01-25 at 15:54 -0500, Neil Horman wrote:
> > vmxnet3 fails to set netif_carrier_off on probe, meaning that when an
> > interface is opened the __LINK_STATE_NOCARRIER bit is already cleared,
> > and so /sys/class/net/<ifname>/operstate remains in the unknown state. 
> > Correct this by setting netif_carrier_off on probe, like other drivers
> > do.
> > 
> > Also, while we're at it, lets remove the netif_carrier_ok checks from the
> > link_state_update function, as that check is atomically contained within
> > the netif_carrier_[on|off] functions anyway
> > 
> > Tested successfully by myself
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: "VMware, Inc." <pv-drivers@vmware.com>
> > ---
> > 
> >  drivers/net/vmxnet3/vmxnet3_drv.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c
> > b/drivers/net/vmxnet3/vmxnet3_drv.c index b1c90f8..66c26a9 100644
> > --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> > +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> 
> [...]
> 
> > @@ -3067,6 +3065,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
> > 
> >  		goto err_register;
> >  	
> >  	}
> > 
> > +	netif_carrier_off(netdev);
> > 
> >  	vmxnet3_check_link(adapter, false);
> >  	return 0;
> 
> You should do this before calling register_netdev(), otherwise the link
> state change can race with vmxnet3_open().

Hmm, is it safe to do before the network device is registered? The rest
of the drivers (e1000, tg3, etc) seem to be doing netif_carrier_off()
in open() rather than probe() and then do link checks.

Thanks,
Dmitry

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Neil Horman - Jan. 29, 2013, 7:09 p.m.
On Tue, Jan 29, 2013 at 10:54:28AM -0800, Dmitry Torokhov wrote:
> On Tuesday, January 29, 2013 05:54:11 PM Ben Hutchings wrote:
> > On Fri, 2013-01-25 at 15:54 -0500, Neil Horman wrote:
> > > vmxnet3 fails to set netif_carrier_off on probe, meaning that when an
> > > interface is opened the __LINK_STATE_NOCARRIER bit is already cleared,
> > > and so /sys/class/net/<ifname>/operstate remains in the unknown state. 
> > > Correct this by setting netif_carrier_off on probe, like other drivers
> > > do.
> > > 
> > > Also, while we're at it, lets remove the netif_carrier_ok checks from the
> > > link_state_update function, as that check is atomically contained within
> > > the netif_carrier_[on|off] functions anyway
> > > 
> > > Tested successfully by myself
> > > 
> > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > CC: "David S. Miller" <davem@davemloft.net>
> > > CC: "VMware, Inc." <pv-drivers@vmware.com>
> > > ---
> > > 
> > >  drivers/net/vmxnet3/vmxnet3_drv.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c
> > > b/drivers/net/vmxnet3/vmxnet3_drv.c index b1c90f8..66c26a9 100644
> > > --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> > > +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> > 
> > [...]
> > 
> > > @@ -3067,6 +3065,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
> > > 
> > >  		goto err_register;
> > >  	
> > >  	}
> > > 
> > > +	netif_carrier_off(netdev);
> > > 
> > >  	vmxnet3_check_link(adapter, false);
> > >  	return 0;
> > 
> > You should do this before calling register_netdev(), otherwise the link
> > state change can race with vmxnet3_open().
> 
> Hmm, is it safe to do before the network device is registered? The rest
> of the drivers (e1000, tg3, etc) seem to be doing netif_carrier_off()
> in open() rather than probe() and then do link checks.
> 
It appears to be safe to me, at least for the netif_carrier_off case, as theres
a check to effecitvely just set the NOCARRIER bit if the device is in
UNREGISTERED state.
Neil

> Thanks,
> Dmitry
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Jan. 29, 2013, 7:17 p.m.
On Tue, 2013-01-29 at 14:09 -0500, Neil Horman wrote:
> On Tue, Jan 29, 2013 at 10:54:28AM -0800, Dmitry Torokhov wrote:
> > On Tuesday, January 29, 2013 05:54:11 PM Ben Hutchings wrote:
> > > On Fri, 2013-01-25 at 15:54 -0500, Neil Horman wrote:
> > > > vmxnet3 fails to set netif_carrier_off on probe, meaning that when an
> > > > interface is opened the __LINK_STATE_NOCARRIER bit is already cleared,
> > > > and so /sys/class/net/<ifname>/operstate remains in the unknown state. 
> > > > Correct this by setting netif_carrier_off on probe, like other drivers
> > > > do.
> > > > 
> > > > Also, while we're at it, lets remove the netif_carrier_ok checks from the
> > > > link_state_update function, as that check is atomically contained within
> > > > the netif_carrier_[on|off] functions anyway
> > > > 
> > > > Tested successfully by myself
> > > > 
> > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > CC: "David S. Miller" <davem@davemloft.net>
> > > > CC: "VMware, Inc." <pv-drivers@vmware.com>
> > > > ---
> > > > 
> > > >  drivers/net/vmxnet3/vmxnet3_drv.c | 7 +++----
> > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c
> > > > b/drivers/net/vmxnet3/vmxnet3_drv.c index b1c90f8..66c26a9 100644
> > > > --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> > > > +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> > > 
> > > [...]
> > > 
> > > > @@ -3067,6 +3065,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
> > > > 
> > > >  		goto err_register;
> > > >  	
> > > >  	}
> > > > 
> > > > +	netif_carrier_off(netdev);
> > > > 
> > > >  	vmxnet3_check_link(adapter, false);
> > > >  	return 0;
> > > 
> > > You should do this before calling register_netdev(), otherwise the link
> > > state change can race with vmxnet3_open().
> > 
> > Hmm, is it safe to do before the network device is registered? The rest
> > of the drivers (e1000, tg3, etc) seem to be doing netif_carrier_off()
> > in open() rather than probe() and then do link checks.
> > 
> It appears to be safe to me, at least for the netif_carrier_off case, as theres
> a check to effecitvely just set the NOCARRIER bit if the device is in
> UNREGISTERED state.

Right, I made this work properly a little while ago (commit
8f4cccbbd92f).  Previous best practice was to call netif_carrier_off()
after registration, but userland could still see the wrong operstate
because linkwatch updates it asynchronously.

Ben.
Dmitry Torokhov - Jan. 29, 2013, 7:53 p.m.
On Tuesday, January 29, 2013 07:17:10 PM Ben Hutchings wrote:
> On Tue, 2013-01-29 at 14:09 -0500, Neil Horman wrote:
> > On Tue, Jan 29, 2013 at 10:54:28AM -0800, Dmitry Torokhov wrote:
> > > On Tuesday, January 29, 2013 05:54:11 PM Ben Hutchings wrote:
> > > > On Fri, 2013-01-25 at 15:54 -0500, Neil Horman wrote:
> > > > > vmxnet3 fails to set netif_carrier_off on probe, meaning that when
> > > > > an
> > > > > interface is opened the __LINK_STATE_NOCARRIER bit is already
> > > > > cleared,
> > > > > and so /sys/class/net/<ifname>/operstate remains in the unknown
> > > > > state.
> > > > > Correct this by setting netif_carrier_off on probe, like other
> > > > > drivers
> > > > > do.
> > > > > 
> > > > > Also, while we're at it, lets remove the netif_carrier_ok checks
> > > > > from the
> > > > > link_state_update function, as that check is atomically contained
> > > > > within
> > > > > the netif_carrier_[on|off] functions anyway
> > > > > 
> > > > > Tested successfully by myself
> > > > > 
> > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > > CC: "David S. Miller" <davem@davemloft.net>
> > > > > CC: "VMware, Inc." <pv-drivers@vmware.com>
> > > > > ---
> > > > > 
> > > > >  drivers/net/vmxnet3/vmxnet3_drv.c | 7 +++----
> > > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c
> > > > > b/drivers/net/vmxnet3/vmxnet3_drv.c index b1c90f8..66c26a9 100644
> > > > > --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> > > > > +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -3067,6 +3065,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
> > > > > 
> > > > >  		goto err_register;
> > > > >  	
> > > > >  	}
> > > > > 
> > > > > +	netif_carrier_off(netdev);
> > > > > 
> > > > >  	vmxnet3_check_link(adapter, false);
> > > > >  	return 0;
> > > > 
> > > > You should do this before calling register_netdev(), otherwise the
> > > > link
> > > > state change can race with vmxnet3_open().
> > > 
> > > Hmm, is it safe to do before the network device is registered? The rest
> > > of the drivers (e1000, tg3, etc) seem to be doing netif_carrier_off()
> > > in open() rather than probe() and then do link checks.
> > 
> > It appears to be safe to me, at least for the netif_carrier_off case, as
> > theres a check to effecitvely just set the NOCARRIER bit if the device is
> > in UNREGISTERED state.
> 
> Right, I made this work properly a little while ago (commit
> 8f4cccbbd92f).  Previous best practice was to call netif_carrier_off()
> after registration, but userland could still see the wrong operstate
> because linkwatch updates it asynchronously.

Ah, OK I see. Thank you for the explanation.

Thanks,
Dmitry

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index b1c90f8..66c26a9 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -150,8 +150,7 @@  vmxnet3_check_link(struct vmxnet3_adapter *adapter, bool affectTxQueue)
 	if (ret & 1) { /* Link is up. */
 		netdev_info(adapter->netdev, "NIC Link is Up %d Mbps\n",
 			    adapter->link_speed);
-		if (!netif_carrier_ok(adapter->netdev))
-			netif_carrier_on(adapter->netdev);
+		netif_carrier_on(adapter->netdev);
 
 		if (affectTxQueue) {
 			for (i = 0; i < adapter->num_tx_queues; i++)
@@ -160,8 +159,7 @@  vmxnet3_check_link(struct vmxnet3_adapter *adapter, bool affectTxQueue)
 		}
 	} else {
 		netdev_info(adapter->netdev, "NIC Link is Down\n");
-		if (netif_carrier_ok(adapter->netdev))
-			netif_carrier_off(adapter->netdev);
+		netif_carrier_off(adapter->netdev);
 
 		if (affectTxQueue) {
 			for (i = 0; i < adapter->num_tx_queues; i++)
@@ -3067,6 +3065,7 @@  vmxnet3_probe_device(struct pci_dev *pdev,
 		goto err_register;
 	}
 
+	netif_carrier_off(netdev);
 	vmxnet3_check_link(adapter, false);
 	return 0;