netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vmxnet3: set carrier state properly on probe
@ 2013-01-25 20:54 Neil Horman
  2013-01-29 17:54 ` Ben Hutchings
  2013-01-29 18:36 ` [PATCH v2] " Neil Horman
  0 siblings, 2 replies; 12+ messages in thread
From: Neil Horman @ 2013-01-25 20:54 UTC (permalink / raw)
  To: netdev; +Cc: Neil Horman, David S. Miller, VMware, Inc.

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
@@ -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;
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] vmxnet3: set carrier state properly on probe
  2013-01-25 20:54 [PATCH] vmxnet3: set carrier state properly on probe Neil Horman
@ 2013-01-29 17:54 ` Ben Hutchings
  2013-01-29 18:54   ` [Pv-drivers] " Dmitry Torokhov
  2013-01-29 18:36 ` [PATCH v2] " Neil Horman
  1 sibling, 1 reply; 12+ messages in thread
From: Ben Hutchings @ 2013-01-29 17:54 UTC (permalink / raw)
  To: Neil Horman; +Cc: netdev, David S. Miller, VMware, Inc.

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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] vmxnet3: set carrier state properly on probe
  2013-01-25 20:54 [PATCH] vmxnet3: set carrier state properly on probe Neil Horman
  2013-01-29 17:54 ` Ben Hutchings
@ 2013-01-29 18:36 ` Neil Horman
  2013-01-29 19:54   ` [Pv-drivers] " Dmitry Torokhov
  2013-01-29 20:44   ` David Miller
  1 sibling, 2 replies; 12+ messages in thread
From: Neil Horman @ 2013-01-29 18:36 UTC (permalink / raw)
  To: netdev; +Cc: Neil Horman, David S. Miller, VMware, Inc., Ben Hutchings

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>
CC: Ben Hutchings <bhutchings@solarflare.com>

---
Change notes:

v2) Moved netif_carrier_off above register_netdev to prevent race with dev_open
as per Ben H.
---
 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..ffb97b2 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++)
@@ -3060,6 +3058,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	netif_set_real_num_tx_queues(adapter->netdev, adapter->num_tx_queues);
 	netif_set_real_num_rx_queues(adapter->netdev, adapter->num_rx_queues);
 
+	netif_carrier_off(netdev);
 	err = register_netdev(netdev);
 
 	if (err) {
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Pv-drivers] [PATCH] vmxnet3: set carrier state properly on probe
  2013-01-29 17:54 ` Ben Hutchings
@ 2013-01-29 18:54   ` Dmitry Torokhov
  2013-01-29 19:09     ` Neil Horman
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2013-01-29 18:54 UTC (permalink / raw)
  To: pv-drivers; +Cc: Ben Hutchings, Neil Horman, netdev, David S. Miller

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Pv-drivers] [PATCH] vmxnet3: set carrier state properly on probe
  2013-01-29 18:54   ` [Pv-drivers] " Dmitry Torokhov
@ 2013-01-29 19:09     ` Neil Horman
  2013-01-29 19:17       ` Ben Hutchings
  0 siblings, 1 reply; 12+ messages in thread
From: Neil Horman @ 2013-01-29 19:09 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: pv-drivers, Ben Hutchings, netdev, David S. Miller

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Pv-drivers] [PATCH] vmxnet3: set carrier state properly on probe
  2013-01-29 19:09     ` Neil Horman
@ 2013-01-29 19:17       ` Ben Hutchings
  2013-01-29 19:53         ` Dmitry Torokhov
  0 siblings, 1 reply; 12+ messages in thread
From: Ben Hutchings @ 2013-01-29 19:17 UTC (permalink / raw)
  To: Neil Horman; +Cc: Dmitry Torokhov, pv-drivers, netdev, David S. Miller

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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Pv-drivers] [PATCH] vmxnet3: set carrier state properly on probe
  2013-01-29 19:17       ` Ben Hutchings
@ 2013-01-29 19:53         ` Dmitry Torokhov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2013-01-29 19:53 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Neil Horman, pv-drivers, netdev, David S. Miller

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Pv-drivers] [PATCH v2] vmxnet3: set carrier state properly on probe
  2013-01-29 18:36 ` [PATCH v2] " Neil Horman
@ 2013-01-29 19:54   ` Dmitry Torokhov
  2013-01-29 20:44   ` David Miller
  1 sibling, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2013-01-29 19:54 UTC (permalink / raw)
  To: pv-drivers; +Cc: Neil Horman, netdev, Ben Hutchings, David S. Miller

On Tuesday, January 29, 2013 01:36:51 PM 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>

Acked-by: Dmitry Torokhov <dtor@vmware.com>

> CC: Ben Hutchings <bhutchings@solarflare.com>
> 
> ---
> Change notes:
> 
> v2) Moved netif_carrier_off above register_netdev to prevent race with
> dev_open as per Ben H.
> ---
>  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..ffb97b2 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++)
> @@ -3060,6 +3058,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
>  	netif_set_real_num_tx_queues(adapter->netdev, adapter->num_tx_queues);
>  	netif_set_real_num_rx_queues(adapter->netdev, adapter->num_rx_queues);
> 
> +	netif_carrier_off(netdev);
>  	err = register_netdev(netdev);
> 
>  	if (err) {

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] vmxnet3: set carrier state properly on probe
  2013-01-29 18:36 ` [PATCH v2] " Neil Horman
  2013-01-29 19:54   ` [Pv-drivers] " Dmitry Torokhov
@ 2013-01-29 20:44   ` David Miller
  2013-01-29 21:08     ` Neil Horman
  2013-01-29 21:15     ` [PATCH v3] " Neil Horman
  1 sibling, 2 replies; 12+ messages in thread
From: David Miller @ 2013-01-29 20:44 UTC (permalink / raw)
  To: nhorman; +Cc: netdev, pv-drivers, bhutchings

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 29 Jan 2013 13:36:51 -0500

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

This doesn't apply to the current 'net' tree.  Please respin.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] vmxnet3: set carrier state properly on probe
  2013-01-29 20:44   ` David Miller
@ 2013-01-29 21:08     ` Neil Horman
  2013-01-29 21:15     ` [PATCH v3] " Neil Horman
  1 sibling, 0 replies; 12+ messages in thread
From: Neil Horman @ 2013-01-29 21:08 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, pv-drivers, bhutchings

On Tue, Jan 29, 2013 at 03:44:30PM -0500, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Tue, 29 Jan 2013 13:36:51 -0500
> 
> > 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>
> 
> This doesn't apply to the current 'net' tree.  Please respin.
I based it off net-next.  I'll respin for net
Neil

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] vmxnet3: set carrier state properly on probe
  2013-01-29 20:44   ` David Miller
  2013-01-29 21:08     ` Neil Horman
@ 2013-01-29 21:15     ` Neil Horman
  2013-01-29 21:29       ` David Miller
  1 sibling, 1 reply; 12+ messages in thread
From: Neil Horman @ 2013-01-29 21:15 UTC (permalink / raw)
  To: netdev; +Cc: Neil Horman, David S. Miller, VMware, Inc., Ben Hutchings

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>
CC: Ben Hutchings <bhutchings@solarflare.com>

---
Change notes:

v2) Moved netif_carrier_off above register_netdev to prevent race with dev_open
as per Ben H.

v3) Respin for net tree
---
 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 dc8913c..12c6440 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -154,8 +154,7 @@ vmxnet3_check_link(struct vmxnet3_adapter *adapter, bool affectTxQueue)
 	if (ret & 1) { /* Link is up. */
 		printk(KERN_INFO "%s: NIC Link is Up %d Mbps\n",
 		       adapter->netdev->name, 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++)
@@ -165,8 +164,7 @@ vmxnet3_check_link(struct vmxnet3_adapter *adapter, bool affectTxQueue)
 	} else {
 		printk(KERN_INFO "%s: NIC Link is Down\n",
 		       adapter->netdev->name);
-		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++)
@@ -3061,6 +3059,7 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	netif_set_real_num_tx_queues(adapter->netdev, adapter->num_tx_queues);
 	netif_set_real_num_rx_queues(adapter->netdev, adapter->num_rx_queues);
 
+	netif_carrier_off(netdev);
 	err = register_netdev(netdev);
 
 	if (err) {
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] vmxnet3: set carrier state properly on probe
  2013-01-29 21:15     ` [PATCH v3] " Neil Horman
@ 2013-01-29 21:29       ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2013-01-29 21:29 UTC (permalink / raw)
  To: nhorman; +Cc: netdev, pv-drivers, bhutchings

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 29 Jan 2013 16:15:45 -0500

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

Applied, thanks Neil.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-01-29 21:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-25 20:54 [PATCH] vmxnet3: set carrier state properly on probe Neil Horman
2013-01-29 17:54 ` Ben Hutchings
2013-01-29 18:54   ` [Pv-drivers] " Dmitry Torokhov
2013-01-29 19:09     ` Neil Horman
2013-01-29 19:17       ` Ben Hutchings
2013-01-29 19:53         ` Dmitry Torokhov
2013-01-29 18:36 ` [PATCH v2] " Neil Horman
2013-01-29 19:54   ` [Pv-drivers] " Dmitry Torokhov
2013-01-29 20:44   ` David Miller
2013-01-29 21:08     ` Neil Horman
2013-01-29 21:15     ` [PATCH v3] " Neil Horman
2013-01-29 21:29       ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).