[SRU,B,A,X,T] virtio-net: Fix operstate for virtio when no VIRTIO_NET_F_STATUS

Message ID 1522951942-23734-1-git-send-email-eric.desrochers@canonical.com
State New
Headers show
Series
  • [SRU,B,A,X,T] virtio-net: Fix operstate for virtio when no VIRTIO_NET_F_STATUS
Related show

Commit Message

Eric Desrochers April 5, 2018, 6:12 p.m.
From: Jay Vosburgh <jay.vosburgh@canonical.com>

BugLink: https://bugs.launchpad.net/bugs/1761534

The operstate update logic will leave an interface in the
default UNKNOWN operstate if the interface carrier state never changes
from the default carrier up state set at creation.  This includes the
case of an explicit call to netif_carrier_on, as the carrier on to on
transition has no effect on operstate.

	This affects virtio-net for the case that the virtio peer does
not support VIRTIO_NET_F_STATUS (the feature that provides carrier state
updates).  Without this feature, the virtio specification states that
"the link should be assumed active," so, logically, the operstate should
be UP instead of UNKNOWN.  This has impact on user space applications
that use the operstate to make availability decisions for the interface.

	Resolve this by changing the virtio probe logic slightly to call
netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS
cases, and then the existing call to netif_carrier_on for the "without"
case will cause an operstate transition.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
(cherry-picked from commit bda7fab54828bbef2164bb23c0f6b1a7d05cc718)
Signed-off-by: Eric Desrochers <eric.desrochers@canonical.com>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Seth Forshee April 9, 2018, 8:58 p.m. | #1
On Thu, Apr 05, 2018 at 02:12:22PM -0400, Eric Desrochers wrote:
> From: Jay Vosburgh <jay.vosburgh@canonical.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1761534
> 
> The operstate update logic will leave an interface in the
> default UNKNOWN operstate if the interface carrier state never changes
> from the default carrier up state set at creation.  This includes the
> case of an explicit call to netif_carrier_on, as the carrier on to on
> transition has no effect on operstate.
> 
> 	This affects virtio-net for the case that the virtio peer does
> not support VIRTIO_NET_F_STATUS (the feature that provides carrier state
> updates).  Without this feature, the virtio specification states that
> "the link should be assumed active," so, logically, the operstate should
> be UP instead of UNKNOWN.  This has impact on user space applications
> that use the operstate to make availability decisions for the interface.
> 
> 	Resolve this by changing the virtio probe logic slightly to call
> netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS
> cases, and then the existing call to netif_carrier_on for the "without"
> case will cause an operstate transition.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (cherry-picked from commit bda7fab54828bbef2164bb23c0f6b1a7d05cc718)
> Signed-off-by: Eric Desrochers <eric.desrochers@canonical.com>

Acked-by: Seth Forshee <seth.forshee@canonical.com>

Applied to bionic/master-next, thanks!
Kleber Sacilotto de Souza April 18, 2018, 7:24 a.m. | #2
On 04/05/18 20:12, Eric Desrochers wrote:
> From: Jay Vosburgh <jay.vosburgh@canonical.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1761534
> 
> The operstate update logic will leave an interface in the
> default UNKNOWN operstate if the interface carrier state never changes
> from the default carrier up state set at creation.  This includes the
> case of an explicit call to netif_carrier_on, as the carrier on to on
> transition has no effect on operstate.
> 
> 	This affects virtio-net for the case that the virtio peer does
> not support VIRTIO_NET_F_STATUS (the feature that provides carrier state
> updates).  Without this feature, the virtio specification states that
> "the link should be assumed active," so, logically, the operstate should
> be UP instead of UNKNOWN.  This has impact on user space applications
> that use the operstate to make availability decisions for the interface.
> 
> 	Resolve this by changing the virtio probe logic slightly to call
> netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS
> cases, and then the existing call to netif_carrier_on for the "without"
> case will cause an operstate transition.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (cherry-picked from commit bda7fab54828bbef2164bb23c0f6b1a7d05cc718)
> Signed-off-by: Eric Desrochers <eric.desrochers@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f42ee452072b..11cb54dacdab 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2692,8 +2692,8 @@ static int virtnet_probe(struct virtio_device *vdev)
>  
>  	/* Assume link up if device can't report link status,
>  	   otherwise get link status from config. */
> +	netif_carrier_off(dev);
>  	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> -		netif_carrier_off(dev);
>  		schedule_work(&vi->config_work);
>  	} else {
>  		vi->status = VIRTIO_NET_S_LINK_UP;
>
Stefan Bader April 19, 2018, 4:42 p.m. | #3
On 05.04.2018 20:12, Eric Desrochers wrote:
> From: Jay Vosburgh <jay.vosburgh@canonical.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1761534
> 
> The operstate update logic will leave an interface in the
> default UNKNOWN operstate if the interface carrier state never changes
> from the default carrier up state set at creation.  This includes the
> case of an explicit call to netif_carrier_on, as the carrier on to on
> transition has no effect on operstate.
> 
> 	This affects virtio-net for the case that the virtio peer does
> not support VIRTIO_NET_F_STATUS (the feature that provides carrier state
> updates).  Without this feature, the virtio specification states that
> "the link should be assumed active," so, logically, the operstate should
> be UP instead of UNKNOWN.  This has impact on user space applications
> that use the operstate to make availability decisions for the interface.
> 
> 	Resolve this by changing the virtio probe logic slightly to call
> netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS
> cases, and then the existing call to netif_carrier_on for the "without"
> case will cause an operstate transition.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (cherry-picked from commit bda7fab54828bbef2164bb23c0f6b1a7d05cc718)
> Signed-off-by: Eric Desrochers <eric.desrochers@canonical.com>
> ---

Applied to Artful, Xenial, and Trusty master-next.

-Stefan

>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f42ee452072b..11cb54dacdab 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2692,8 +2692,8 @@ static int virtnet_probe(struct virtio_device *vdev)
>  
>  	/* Assume link up if device can't report link status,
>  	   otherwise get link status from config. */
> +	netif_carrier_off(dev);
>  	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> -		netif_carrier_off(dev);
>  		schedule_work(&vi->config_work);
>  	} else {
>  		vi->status = VIRTIO_NET_S_LINK_UP;
>

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f42ee452072b..11cb54dacdab 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2692,8 +2692,8 @@  static int virtnet_probe(struct virtio_device *vdev)
 
 	/* Assume link up if device can't report link status,
 	   otherwise get link status from config. */
+	netif_carrier_off(dev);
 	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
-		netif_carrier_off(dev);
 		schedule_work(&vi->config_work);
 	} else {
 		vi->status = VIRTIO_NET_S_LINK_UP;