| 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 | expand |
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!
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; >
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; >
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;