[PATCH] Make virtio_net support carrier detection

[PATCH] Make virtio_net support carrier detection

[Rusty Russell]

Impact: Make NetworkManager work with virtio_net

For now the semantics are simple: There is always carrier.

This allows a seamless experience with e.g., qemu/kvm
where NetworkManager just configures and sets up
everything automagically.

If/when a generally agreed-upon way to control
carrier on/off in the emulator/hypervisor level
emerges, it will be trivial to extend the driver
to support that too, but for now even this 2-liner
makes user experience that much better.

Signed-off-by: Pantelis Koukousoulas <pktoss@gmail.com>
Acked-by: David Miller <davem@davemloft.net>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c688083..e67d16c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -612,6 +612,7 @@ static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tx_csum = virtnet_set_tx_csum,
 	.set_sg = ethtool_op_set_sg,
 	.set_tso = ethtool_op_set_tso,
+	.get_link = ethtool_op_get_link,
 };
 
 #define MIN_MTU 68
@@ -739,6 +740,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 		goto unregister;
 	}
 
+	netif_carrier_on(dev);
+
 	pr_debug("virtnet: registered device %s\n", dev->name);
 	return 0;
 
-- 
1.5.6.3

Re: [PATCH] Make virtio_net support carrier detection

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Sat, 14 Mar 2009 10:53:51 +1030

> Impact: Make NetworkManager work with virtio_net
> 
> For now the semantics are simple: There is always carrier.
> 
> This allows a seamless experience with e.g., qemu/kvm
> where NetworkManager just configures and sets up
> everything automagically.
> 
> If/when a generally agreed-upon way to control
> carrier on/off in the emulator/hypervisor level
> emerges, it will be trivial to extend the driver
> to support that too, but for now even this 2-liner
> makes user experience that much better.
> 
> Signed-off-by: Pantelis Koukousoulas <pktoss@gmail.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Applied, thanks Rusty.

Re: [PATCH] Make virtio_net support carrier detection

[Rusty Russell]

On Wednesday 11 March 2009 22:27:22 Pantelis Koukousoulas wrote:
> For now the semantics are simple: There is always carrier.
> 
> This allows a seamless experience with e.g., qemu/kvm
> where NetworkManager just configures and sets up
> everything automagically.

So, NetworkManager ignores the device because it doesn't support
carrier detection?

That seems weird, but I have nothing against this patch in general.

Thanks,
Rusty.

> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -612,6 +612,7 @@ static struct ethtool_ops virtnet_ethtool_ops = {
>  	.set_tx_csum = virtnet_set_tx_csum,
>  	.set_sg = ethtool_op_set_sg,
>  	.set_tso = ethtool_op_set_tso,
> +	.get_link = ethtool_op_get_link,
>  };
>  
>  #define MIN_MTU 68
> @@ -739,6 +740,8 @@ static int virtnet_probe(struct virtio_device *vdev)
>  		goto unregister;
>  	}
>  
> +	netif_carrier_on(dev);
> +
>  	pr_debug("virtnet: registered device %s\n", dev->name);
>  	return 0;

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

On Thu, Mar 12, 2009 at 9:29 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Wednesday 11 March 2009 22:27:22 Pantelis Koukousoulas wrote:
>> For now the semantics are simple: There is always carrier.
>>
>> This allows a seamless experience with e.g., qemu/kvm
>> where NetworkManager just configures and sets up
>> everything automagically.
>
> So, NetworkManager ignores the device because it doesn't support
> carrier detection?

It doesn't ignore it, it just doesn't enable it by default, like it does for
those that do report carrier.

But the user experience is that on real hardware you just boot a livecd
and see your network "just working" (and the familiar icon with the 2
computers indicating that), while under e.g., KVM/virtio you get
non-working network / an icon with a red X and you have to
figure out to left click on it and select eth0 to get things to work.

Some newer versions of NetworkManager seem to assume that
if no carrier detection is supported then carrier is on by default
so it should work for those, but that assumption seems like
a worse hack than this patch :)

(And in any case this patch helps all those who don't run
the latest unstable branch of their distro, while causing
no harm as far as I can see)

Pantelis

Re: [PATCH] Make virtio_net support carrier detection

[Rusty Russell]

On Thursday 12 March 2009 18:14:44 Pantelis Koukousoulas wrote:
> On Thu, Mar 12, 2009 at 9:29 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > On Wednesday 11 March 2009 22:27:22 Pantelis Koukousoulas wrote:
> >> For now the semantics are simple: There is always carrier.
> >>
> >> This allows a seamless experience with e.g., qemu/kvm
> >> where NetworkManager just configures and sets up
> >> everything automagically.
> >
> > So, NetworkManager ignores the device because it doesn't support
> > carrier detection?
> 
> It doesn't ignore it, it just doesn't enable it by default, like it does for
> those that do report carrier.
> 
> But the user experience is that on real hardware you just boot a livecd
> and see your network "just working" (and the familiar icon with the 2
> computers indicating that), while under e.g., KVM/virtio you get
> non-working network / an icon with a red X and you have to
> figure out to left click on it and select eth0 to get things to work.
> 
> Some newer versions of NetworkManager seem to assume that
> if no carrier detection is supported then carrier is on by default
> so it should work for those, but that assumption seems like
> a worse hack than this patch :)

No, that's absolutely sane behavior, the previous was buggy.  If a network
doesn't support carrier, you shouldn't look for it.

Since they've fixed this in Network Manager, I'm not tempted to lie about it
in the driver (though the distributions might choose to).

Rusty.

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

> No, that's absolutely sane behavior, the previous was buggy.  If a network
> doesn't support carrier, you shouldn't look for it.
>
> Since they've fixed this in Network Manager, I'm not tempted to lie about it
> in the driver (though the distributions might choose to).
>
> Rusty.
>

Ok, if "lying" is reporting that the driver supports carrier
detection, this looks
like an innocent lie :). Because the carrier *is* on, this is the truth and you
could just as well claim that the virtual hardware supports detection
as that it doesn't :)

(Plus there is no technical reason why virtio_net cannot report carrier status,
the only argument against it is saving 2 lines of code)

IMHO, if someone has such an ancient card whose hardware cannot report
carrier status (even my ne2k isa can do this iirc) they would be trained
to set the network up manually.

NetworkManager 's purpose is to automatically select the 'best' network
to connect to. By default it prefers wired networks from wireless.

If a wired eth card that does not report carrier status has no cable connected,
then (with the 'default ON' hack) NM will prefer that (useless) card even if
there is a possibility for a successful wireless connection.

That would upset me as a user and that is why I think this hack is wrong.
If the hardware cannot report carrier, then only the user can know whether
this card should be preferred or not, it has to be explicit.

Anyway, that is as much arguing as I can afford for  a 2-liner :)
If you still don't like it, that 's fine with me, I doubt I 'll have
too much trouble
carrying it forward myself :)

Thanks a lot for taking the time to review :)

Pantelis

P.s., as an added bonus, ethtool eth0 shows a nice "Link detected: yes"
with this patch, instead of the scary "Operation not supported".

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

On Thu, Mar 12, 2009 at 12:23 PM, Pantelis Koukousoulas
<pktoss@gmail.com> wrote:
>> No, that's absolutely sane behavior, the previous was buggy.  If a network
>> doesn't support carrier, you shouldn't look for it.
>>
>> Since they've fixed this in Network Manager, I'm not tempted to lie about it
>> in the driver (though the distributions might choose to).
>>
>> Rusty.
>>
>
> Ok, if "lying" is reporting that the driver supports carrier
> detection, this looks
> like an innocent lie :). Because the carrier *is* on, this is the truth and you
> could just as well claim that the virtual hardware supports detection
> as that it doesn't :)

After reading this one more time, it doesn't make much sense, so let me try
to explain my point of view more clearly:
For the wired case, carrier status == "is cable connected?"
(NetworkManager is asking that)

So,

* virtio saying "Hey, I have no idea if cable is connected or not. I
have no way to know that".
  (aka carrier detect not supported)

I think this is closer to lying than:

* "Hey, I know that the cable is connected". == carrier detection
supported. carrier is on

Because unlike prehistoric hardware / incomplete drivers, virtio *does* know :)

Even if someone comes up with a patch at the hypervisor / emulator
level that allows
the user to set carrier status to on or off, then pressure should be
applied on them
to implement this change in a way that makes this known to the virtio_net driver
for the same reasons that actual hardware does it (i.e., users like / want this
functionality :)

Hope this is clearer,
Pantelis

Re: [PATCH] Make virtio_net support carrier detection

[Dan Williams]

On Thu, 2009-03-12 at 19:46 +1030, Rusty Russell wrote:
> On Thursday 12 March 2009 18:14:44 Pantelis Koukousoulas wrote:
> > On Thu, Mar 12, 2009 at 9:29 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > On Wednesday 11 March 2009 22:27:22 Pantelis Koukousoulas wrote:
> > >> For now the semantics are simple: There is always carrier.
> > >>
> > >> This allows a seamless experience with e.g., qemu/kvm
> > >> where NetworkManager just configures and sets up
> > >> everything automagically.
> > >
> > > So, NetworkManager ignores the device because it doesn't support
> > > carrier detection?
> > 
> > It doesn't ignore it, it just doesn't enable it by default, like it does for
> > those that do report carrier.
> > 
> > But the user experience is that on real hardware you just boot a livecd
> > and see your network "just working" (and the familiar icon with the 2
> > computers indicating that), while under e.g., KVM/virtio you get
> > non-working network / an icon with a red X and you have to
> > figure out to left click on it and select eth0 to get things to work.
> > 
> > Some newer versions of NetworkManager seem to assume that
> > if no carrier detection is supported then carrier is on by default
> > so it should work for those, but that assumption seems like
> > a worse hack than this patch :)
> 
> No, that's absolutely sane behavior, the previous was buggy.  If a network
> doesn't support carrier, you shouldn't look for it.
> 
> Since they've fixed this in Network Manager, I'm not tempted to lie about it
> in the driver (though the distributions might choose to).

The problem is that there's no "my carrier detection is accurate" flag
for drivers.  Drivers that don't support carrier detection (say, my
Belkin PCMCIA NE2k card) always report "carrier on", but of course don't
support carrier detection.  So when NetworkManager looks at the device
and sees "hey, there's a carrier!" it will activate it, because a
carrier means the cable is plugged in or the PHY *thinks* a cable is
plugged in.

So NetworkManager checks whether the device supports ethtool get_link or
MII register carrier status in lieu of a general kernel driver flag for
"I support carrier detection".

Carrier is not on/off, it needs to be tristate, like on/off/unknown.  If
it was unknown, NM could make an intelligent decision about this without
resorting to ethtool/MII checks.  But we don't have that.

Dan

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

> The problem is that there's no "my carrier detection is accurate" flag
> for drivers.  Drivers that don't support carrier detection (say, my
> Belkin PCMCIA NE2k card) always report "carrier on", but of course don't
> support carrier detection.  So when NetworkManager looks at the device
> and sees "hey, there's a carrier!" it will activate it, because a
> carrier means the cable is plugged in or the PHY *thinks* a cable is
> plugged in.
>
> So NetworkManager checks whether the device supports ethtool get_link or
> MII register carrier status in lieu of a general kernel driver flag for
> "I support carrier detection".
>
> Carrier is not on/off, it needs to be tristate, like on/off/unknown.  If
> it was unknown, NM could make an intelligent decision about this without
> resorting to ethtool/MII checks.  But we don't have that.

This looks like an independent problem imho, one of kernel<->userspace
API. The current 'defacto' way of finding out if detection is supported or
not (ethtool) seems to give the needed information even if somewhat ugly.

The issue discussed here as I understand it is whether virtio should or
should not support carrier status reporting. IMHO it should, since
it is useful functionality and doesn't cost much.

Dan, What is your opinion on that?

Pantelis

Re: [PATCH] Make virtio_net support carrier detection

[David Miller]

From: Pantelis Koukousoulas <pktoss@gmail.com>
Date: Thu, 12 Mar 2009 14:47:09 +0200

> > The problem is that there's no "my carrier detection is accurate" flag
> > for drivers.  Drivers that don't support carrier detection (say, my
> > Belkin PCMCIA NE2k card) always report "carrier on", but of course don't
> > support carrier detection.  So when NetworkManager looks at the device
> > and sees "hey, there's a carrier!" it will activate it, because a
> > carrier means the cable is plugged in or the PHY *thinks* a cable is
> > plugged in.
> >
> > So NetworkManager checks whether the device supports ethtool get_link or
> > MII register carrier status in lieu of a general kernel driver flag for
> > "I support carrier detection".
> >
> > Carrier is not on/off, it needs to be tristate, like on/off/unknown.  If
> > it was unknown, NM could make an intelligent decision about this without
> > resorting to ethtool/MII checks.  But we don't have that.
> 
> This looks like an independent problem imho, one of kernel<->userspace
> API. The current 'defacto' way of finding out if detection is supported or
> not (ethtool) seems to give the needed information even if somewhat ugly.
> 
> The issue discussed here as I understand it is whether virtio should or
> should not support carrier status reporting. IMHO it should, since
> it is useful functionality and doesn't cost much.
> 
> Dan, What is your opinion on that?

I think Dan is right.

If the driver doesn't provide an explicit carrier, there is no way to
know for sure that it is on of off.  If NetworkManager decides that
this means on, and it's really off, that isn't so nice.

And lots of very old drivers in the tree don't have a link indication
handler, so this is a very real issue.

Not adding a link state handler to virtio_net for pompous reasons like
some theoretical "clean design" claim is idiotic, and in the end bad
for users who are using existing versions of NetworkManager.

I also think the NetworkManager change is wrong, lack of link
indication support does not mean the link is always on, not by a
country mile.  Tell that to all the ancient ethernet drivers in
our tree.

If the link is always on, you should make that explicit by providing
a link state handler, and making sure it always returns true.

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

> If the link is always on, you should make that explicit by providing
> a link state handler, and making sure it always returns true.

So, you agree that my patch that does exactly that is correct ?

Pantelis

Re: [PATCH] Make virtio_net support carrier detection

[David Miller]

From: Pantelis Koukousoulas <pktoss@gmail.com>
Date: Thu, 12 Mar 2009 14:58:28 +0200

> > If the link is always on, you should make that explicit by providing
> > a link state handler, and making sure it always returns true.
> 
> So, you agree that my patch that does exactly that is correct ?

Yes, absolutely.

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

On Thu, Mar 12, 2009 at 3:03 PM, David Miller <davem@davemloft.net> wrote:
> From: Pantelis Koukousoulas <pktoss@gmail.com>
> Date: Thu, 12 Mar 2009 14:58:28 +0200
>
>> > If the link is always on, you should make that explicit by providing
>> > a link state handler, and making sure it always returns true.
>>
>> So, you agree that my patch that does exactly that is correct ?
>
> Yes, absolutely.
>

Great, because the point I was arguing about was exactly what you
wrote on the previous mail :)

Thanks,
Pantelis

Re: [PATCH] Make virtio_net support carrier detection

[Dan Williams]

On Thu, 2009-03-12 at 05:52 -0700, David Miller wrote:
> From: Pantelis Koukousoulas <pktoss@gmail.com>
> Date: Thu, 12 Mar 2009 14:47:09 +0200
> 
> > > The problem is that there's no "my carrier detection is accurate" flag
> > > for drivers.  Drivers that don't support carrier detection (say, my
> > > Belkin PCMCIA NE2k card) always report "carrier on", but of course don't
> > > support carrier detection.  So when NetworkManager looks at the device
> > > and sees "hey, there's a carrier!" it will activate it, because a
> > > carrier means the cable is plugged in or the PHY *thinks* a cable is
> > > plugged in.
> > >
> > > So NetworkManager checks whether the device supports ethtool get_link or
> > > MII register carrier status in lieu of a general kernel driver flag for
> > > "I support carrier detection".
> > >
> > > Carrier is not on/off, it needs to be tristate, like on/off/unknown.  If
> > > it was unknown, NM could make an intelligent decision about this without
> > > resorting to ethtool/MII checks.  But we don't have that.
> > 
> > This looks like an independent problem imho, one of kernel<->userspace
> > API. The current 'defacto' way of finding out if detection is supported or
> > not (ethtool) seems to give the needed information even if somewhat ugly.
> > 
> > The issue discussed here as I understand it is whether virtio should or
> > should not support carrier status reporting. IMHO it should, since
> > it is useful functionality and doesn't cost much.
> > 
> > Dan, What is your opinion on that?
> 
> I think Dan is right.
> 
> If the driver doesn't provide an explicit carrier, there is no way to
> know for sure that it is on of off.  If NetworkManager decides that
> this means on, and it's really off, that isn't so nice.
> 
> And lots of very old drivers in the tree don't have a link indication
> handler, so this is a very real issue.
> 
> Not adding a link state handler to virtio_net for pompous reasons like
> some theoretical "clean design" claim is idiotic, and in the end bad
> for users who are using existing versions of NetworkManager.
> 
> I also think the NetworkManager change is wrong, lack of link
> indication support does not mean the link is always on, not by a
> country mile.  Tell that to all the ancient ethernet drivers in
> our tree.

Checking up more this morning in NetworkManager, here's the current
behavior with 0.7.x.

If the device doesn't support carrier detection (determined by checking
for ethtool or MII ioctl support), NetworkManager will *not* listen for
netlink carrier events.  These are triggered by
netif_carrier_on()/netif_carrier_off(), which even drivers that don't
support carrier detection will still sometimes send.

However, NetworkManager will assume carrier=on, because NM is designed
such that if the carrier is not on, the nothing can be done with the
device because there's no cable connected.

It will then try to automatically bring up any valid 'autoconnect'
ethernet connection.  This will obviously fail, and when NM runs out of
valid 'autoconnect' ethernet configs, the device will be dormant until
the user explicitly chooses to activate it from the UI.

This shouldn't be an issue, because NM supports multiple active devices,
and thus if you have a wifi connection as well, that will successfully
activate and wifi will get the default route while the ethernet device
is still trying its hardest to DHCP or whatever.

Of course, if you've configured a static IP address for your ethernet
interface, NM will happily bring up the non-carrier-capable ethernet
interface with the static IP, because there's no way to know whether or
not there's a cable plugged in when the driver reports carrier=on

Of course, if you've *not* selected "Connect automatically", then NM
will wait for you to do something before configuring the interface, as
expected.

This matches the behavior of most distro network systems where, if the
card didn't support carrier detection and since it would then report
IFF_UP/IFF_LOWER_UP/IFF_RUNNING/etc, the distro scripts would configure
the device anyway, because the driver reports carrier=on.

Dan

Re: [PATCH] Make virtio_net support carrier detection

[Dan Williams]

On Thu, 2009-03-12 at 09:41 -0400, Dan Williams wrote:
> On Thu, 2009-03-12 at 05:52 -0700, David Miller wrote:
> > From: Pantelis Koukousoulas <pktoss@gmail.com>
> > Date: Thu, 12 Mar 2009 14:47:09 +0200
> > 
> > > > The problem is that there's no "my carrier detection is accurate" flag
> > > > for drivers.  Drivers that don't support carrier detection (say, my
> > > > Belkin PCMCIA NE2k card) always report "carrier on", but of course don't
> > > > support carrier detection.  So when NetworkManager looks at the device
> > > > and sees "hey, there's a carrier!" it will activate it, because a
> > > > carrier means the cable is plugged in or the PHY *thinks* a cable is
> > > > plugged in.
> > > >
> > > > So NetworkManager checks whether the device supports ethtool get_link or
> > > > MII register carrier status in lieu of a general kernel driver flag for
> > > > "I support carrier detection".
> > > >
> > > > Carrier is not on/off, it needs to be tristate, like on/off/unknown.  If
> > > > it was unknown, NM could make an intelligent decision about this without
> > > > resorting to ethtool/MII checks.  But we don't have that.
> > > 
> > > This looks like an independent problem imho, one of kernel<->userspace
> > > API. The current 'defacto' way of finding out if detection is supported or
> > > not (ethtool) seems to give the needed information even if somewhat ugly.
> > > 
> > > The issue discussed here as I understand it is whether virtio should or
> > > should not support carrier status reporting. IMHO it should, since
> > > it is useful functionality and doesn't cost much.
> > > 
> > > Dan, What is your opinion on that?
> > 
> > I think Dan is right.
> > 
> > If the driver doesn't provide an explicit carrier, there is no way to
> > know for sure that it is on of off.  If NetworkManager decides that
> > this means on, and it's really off, that isn't so nice.
> > 
> > And lots of very old drivers in the tree don't have a link indication
> > handler, so this is a very real issue.
> > 
> > Not adding a link state handler to virtio_net for pompous reasons like
> > some theoretical "clean design" claim is idiotic, and in the end bad
> > for users who are using existing versions of NetworkManager.
> > 
> > I also think the NetworkManager change is wrong, lack of link
> > indication support does not mean the link is always on, not by a
> > country mile.  Tell that to all the ancient ethernet drivers in
> > our tree.
> 
> Checking up more this morning in NetworkManager, here's the current
> behavior with 0.7.x.
> 
> If the device doesn't support carrier detection (determined by checking
> for ethtool or MII ioctl support), NetworkManager will *not* listen for
> netlink carrier events.  These are triggered by
> netif_carrier_on()/netif_carrier_off(), which even drivers that don't
> support carrier detection will still sometimes send.
> 
> However, NetworkManager will assume carrier=on, because NM is designed
> such that if the carrier is not on, the nothing can be done with the
> device because there's no cable connected.
> 
> It will then try to automatically bring up any valid 'autoconnect'
> ethernet connection.  This will obviously fail, and when NM runs out of
> valid 'autoconnect' ethernet configs, the device will be dormant until
> the user explicitly chooses to activate it from the UI.
> 
> This shouldn't be an issue, because NM supports multiple active devices,
> and thus if you have a wifi connection as well, that will successfully
> activate and wifi will get the default route while the ethernet device
> is still trying its hardest to DHCP or whatever.
> 
> Of course, if you've configured a static IP address for your ethernet
> interface, NM will happily bring up the non-carrier-capable ethernet
> interface with the static IP, because there's no way to know whether or
> not there's a cable plugged in when the driver reports carrier=on
> 
> Of course, if you've *not* selected "Connect automatically", then NM
> will wait for you to do something before configuring the interface, as
> expected.
> 
> This matches the behavior of most distro network systems where, if the
> card didn't support carrier detection and since it would then report
> IFF_UP/IFF_LOWER_UP/IFF_RUNNING/etc, the distro scripts would configure
> the device anyway, because the driver reports carrier=on.

(to be 100% clear, I believe that Pantelis' patch to add minimal ethtool
support is *correct*, and it will fix the issue with NetworkManager)

Dan

Re: [PATCH] Make virtio_net support carrier detection

[Rusty Russell]

On Thursday 12 March 2009 23:22:35 David Miller wrote:
> Not adding a link state handler to virtio_net for pompous reasons like
> some theoretical "clean design" claim is idiotic, and in the end bad
> for users who are using existing versions of NetworkManager.

We really don't know if there's a carrier: virtio_net doesn't support it
(yet).

Dan said we can't return "don't know", but he's wrong: you get
-EOPNOTSUPP from ETHTOOL_GLINK.  That surely gives userspace the best
possible information.

> If the link is always on, you should make that explicit by providing
> a link state handler, and making sure it always returns true.

"If".  We've discussed adding a virtio_net feature to indicate link status,
which implies that it's *not* always on.

Rusty.

Re: [PATCH] Make virtio_net support carrier detection

[Rusty Russell]

On Friday 13 March 2009 08:09:49 Rusty Russell wrote:
> On Thursday 12 March 2009 23:22:35 David Miller wrote:
> > If the link is always on, you should make that explicit by providing
> > a link state handler, and making sure it always returns true.
> 
> "If".  We've discussed adding a virtio_net feature to indicate link status,
> which implies that it's *not* always on.

Actually, I've changed my mind.

Unlike a device which *has* a carrier which we can't detect, there's no
virtio_net "device" which can turn off link (not kvm/qemu, not lguest) without
the pending VIRTIO_NET_S_LINK feature.

Here's the patch for Dave's tree; the q. is do we want to put Pantelis' patch
in 2.6.29 and stable?

Rusty.

Subject: virtio_net: set carrier on by default.

Impact: fix carrier detection, older NetworkManager

This is actually two fixes:
1) If the virtio_net device doesn't support carrier, the answer is
   "yes".  This is because before the status feature there was no way
   of turning the link off in any host implementation, and it also helps
   (older) NetworkManager versions to see the device.

2) We should start with carrier on: virtnet_update_status() does nothing
   if the status hasn't changed (ie. doesn't call netif_carrier_on()).

Reported-by: Pantelis Koukousoulas <pktoss@gmail.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -795,7 +795,10 @@ static int virtnet_probe(struct virtio_d
 		goto unregister;
 	}
 
+	/* If we have no carrier info, the answer is "always on". */
 	vi->status = VIRTIO_NET_S_LINK_UP;
+	netif_carrier_on(vi->dev);
+
 	virtnet_update_status(vi);
 
 	pr_debug("virtnet: registered device %s\n", dev->name);

Re: [PATCH] Make virtio_net support carrier detection

[Stephen Hemminger]

On Fri, 13 Mar 2009 10:17:11 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Friday 13 March 2009 08:09:49 Rusty Russell wrote:
> > On Thursday 12 March 2009 23:22:35 David Miller wrote:
> > > If the link is always on, you should make that explicit by providing
> > > a link state handler, and making sure it always returns true.
> > 
> > "If".  We've discussed adding a virtio_net feature to indicate link status,
> > which implies that it's *not* always on.
> 
> Actually, I've changed my mind.
> 
> Unlike a device which *has* a carrier which we can't detect, there's no
> virtio_net "device" which can turn off link (not kvm/qemu, not lguest) without
> the pending VIRTIO_NET_S_LINK feature.
> 

Yes, need that feature, it is really useful for testing.
It is about the only reason I hold onto using VMware.

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

On Fri, Mar 13, 2009 at 1:47 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Friday 13 March 2009 08:09:49 Rusty Russell wrote:
>> On Thursday 12 March 2009 23:22:35 David Miller wrote:
>> > If the link is always on, you should make that explicit by providing
>> > a link state handler, and making sure it always returns true.
>>
>> "If".  We've discussed adding a virtio_net feature to indicate link status,
>> which implies that it's *not* always on.
>
> Actually, I've changed my mind.
>
> Unlike a device which *has* a carrier which we can't detect, there's no
> virtio_net "device" which can turn off link (not kvm/qemu, not lguest) without
> the pending VIRTIO_NET_S_LINK feature.

Exactly :)

>
> Here's the patch for Dave's tree; the q. is do we want to put Pantelis' patch
> in 2.6.29 and stable?

It would be cool to put my patch in 2.6.29 since it is not intrusive
(just a quickfix)
and then drop it in favor of the proper carrier detection / control +
your patch.

Thanks,
Pantelis

Re: [PATCH] Make virtio_net support carrier detection

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Fri, 13 Mar 2009 10:17:11 +1030

> Subject: virtio_net: set carrier on by default.
> 
> Impact: fix carrier detection, older NetworkManager
> 
> This is actually two fixes:
> 1) If the virtio_net device doesn't support carrier, the answer is
>    "yes".  This is because before the status feature there was no way
>    of turning the link off in any host implementation, and it also helps
>    (older) NetworkManager versions to see the device.
> 
> 2) We should start with carrier on: virtnet_update_status() does nothing
>    if the status hasn't changed (ie. doesn't call netif_carrier_on()).
> 
> Reported-by: Pantelis Koukousoulas <pktoss@gmail.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

You can set netif_carrier_on() until you are blue in the face,
but until you hook up the ethtool link indication operation
NetworkManager won't see it.

I don't understand what all of this hob-knobbing is about,
Pantelis's patch was perfect, appropriate, and should have
gone straight in to net-2.6 and probably -stable too.

Is this some kind of control issue Rusty?  It's the only
explanation I can come up with :-))

Re: [PATCH] Make virtio_net support carrier detection

[Rusty Russell]

On Saturday 14 March 2009 05:31:00 David Miller wrote:
> You can set netif_carrier_on() until you are blue in the face,
> but until you hook up the ethtool link indication operation
> NetworkManager won't see it.

Um, dude, you already have that patch in your net-next tree.  See below.
This one goes on top.

> I don't understand what all of this hob-knobbing is about,
> Pantelis's patch was perfect, appropriate, and should have
> gone straight in to net-2.6 and probably -stable too.
> 
> Is this some kind of control issue Rusty?  It's the only
> explanation I can come up with :-))

I don't think it was straightforward at all.  Current virtio_net "cards"
don't support carrier detection.  We reported that (correctly) to userspace,
like every other driver which doesn't support carrier detect.

So why is virtio_net different?  Or should all devices which can't detect
carrier set it to on, so older NetworkManagers work?

This may be obvious to you.  It's not to me.
Rusty.

commit 9f4d26d0f3016cf8813977d624751b94465fa317
Author: Mark McLoughlin <markmc@redhat.com>
Date:   Mon Jan 19 17:09:49 2009 -0800

    virtio_net: add link status handling
    
    Allow the host to inform us that the link is down by adding
    a VIRTIO_NET_F_STATUS which indicates that device status is
    available in virtio_net config.
    
    This is currently useful for simulating link down conditions
    (e.g. using proposed qemu 'set_link' monitor command) but
    would also be needed if we were to support device assignment
    via virtio.
    
    Signed-off-by: Mark McLoughlin <markmc@redhat.com>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (added future masking)
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 30ae6d9..9b33d6e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -42,6 +42,7 @@ struct virtnet_info
 	struct virtqueue *rvq, *svq;
 	struct net_device *dev;
 	struct napi_struct napi;
+	unsigned int status;
 
 	/* The skb we couldn't send because buffers were full. */
 	struct sk_buff *last_xmit_skb;
@@ -611,6 +612,7 @@ static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tx_csum = virtnet_set_tx_csum,
 	.set_sg = ethtool_op_set_sg,
 	.set_tso = ethtool_op_set_tso,
+	.get_link = ethtool_op_get_link,
 };
 
 #define MIN_MTU 68
@@ -636,6 +638,41 @@ static const struct net_device_ops virtnet_netdev = {
 #endif
 };
 
+static void virtnet_update_status(struct virtnet_info *vi)
+{
+	u16 v;
+
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
+		return;
+
+	vi->vdev->config->get(vi->vdev,
+			      offsetof(struct virtio_net_config, status),
+			      &v, sizeof(v));
+
+	/* Ignore unknown (future) status bits */
+	v &= VIRTIO_NET_S_LINK_UP;
+
+	if (vi->status == v)
+		return;
+
+	vi->status = v;
+
+	if (vi->status & VIRTIO_NET_S_LINK_UP) {
+		netif_carrier_on(vi->dev);
+		netif_wake_queue(vi->dev);
+	} else {
+		netif_carrier_off(vi->dev);
+		netif_stop_queue(vi->dev);
+	}
+}
+
+static void virtnet_config_changed(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	virtnet_update_status(vi);
+}
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int err;
@@ -738,6 +775,9 @@ static int virtnet_probe(struct virtio_device *vdev)
 		goto unregister;
 	}
 
+	vi->status = VIRTIO_NET_S_LINK_UP;
+	virtnet_update_status(vi);
+
 	pr_debug("virtnet: registered device %s\n", dev->name);
 	return 0;
 
@@ -793,7 +833,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
-	VIRTIO_NET_F_MRG_RXBUF,
+	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS,
 	VIRTIO_F_NOTIFY_ON_EMPTY,
 };
 
@@ -805,6 +845,7 @@ static struct virtio_driver virtio_net = {
 	.id_table =	id_table,
 	.probe =	virtnet_probe,
 	.remove =	__devexit_p(virtnet_remove),
+	.config_changed = virtnet_config_changed,
 };
 
 static int __init init(void)
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 5cdd0aa..f76bd4a 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -21,11 +21,16 @@
 #define VIRTIO_NET_F_HOST_ECN	13	/* Host can handle TSO[6] w/ ECN in. */
 #define VIRTIO_NET_F_HOST_UFO	14	/* Host can handle UFO in. */
 #define VIRTIO_NET_F_MRG_RXBUF	15	/* Host can merge receive buffers. */
+#define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
+
+#define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 
 struct virtio_net_config
 {
 	/* The config defining mac address (if VIRTIO_NET_F_MAC) */
 	__u8 mac[6];
+	/* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
+	__u16 status;
 } __attribute__((packed));
 
 /* This is the first element of the scatter-gather list.  If you don't

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

> I don't think it was straightforward at all.  Current virtio_net "cards"
> don't support carrier detection.  We reported that (correctly) to userspace,
> like every other driver which doesn't support carrier detect.
>
> So why is virtio_net different?  Or should all devices which can't detect
> carrier set it to on, so older NetworkManagers work?
>

"The difference between theory and practice is not important in theory
but very important in practice"

The practice is:

1) In real wired network cards, carrier is "off by default" (until you plug
a cable) and for laptops it could well be off most of the time (this
is the case here).
So if a "real hardware" driver reports "on" by default it could be wrong
often enough to cause serious frustration to the user (having to wait for
a timeout possibly).

So, real hardware drivers cannot afford to lie that the carrier is "on".
Only the user can know.

In the server case where carrier is normally "on", you either don't want to
use NetworkManager, or you would configure the link explicitly in NM anyway.


2) In virtio_net, network is "on by default", so the only problem by
reporting "on"
instead of "I have no idea" would be with a future qemu version, *if* the user
does "set_link off" in qemu monitor while using NetworkManager (but using
this feature would require a kernel update anyway comparing to what is
there now). For the extremely common case of someone testing a livecd
of a new distro release in existing qemu/kvm, they get the expected results.


3) There is a real cost of reporting "I have no idea" when the truth is
"it is on unless you do something to turn it off explicitly, oh and you 'll
need a future version of qemu (i.e., the "virtual hardware") for that too".


4) Not supporting carrier detection / reporting is arguably a "virtual
hardware bug".
Just try to sell a real hardware card these days without this feature :)
Not to mention that unlike real hardware, adding such small features to
virtio_net has a near-zero cost.

Working around this "virtual hardware bug" (my patch) while it is being fixed
properly (in both the driver and the hardware at the same time), is reasonable
imho because reporting "on" will be correct 100% of the time with
current versions
of qemu and most of the time with future versions as well.

Mark's commit alone achieves the desired effect of reporting carrier
status correctly
but only in the case of newer (yet unreleased?) qemu because of the

+       if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
+               return;

inside virtnet_update_status(). So, your patch adds the explicit
netif_carrier_on
in the probe function which is essentially replicating the behavior you get with
my patch for current versions of qemu / virtio "hardware", you just have to wait
another 3 months for 2.6.30 (!!) to get this.

So, I still propose my version for 2.6.29 / -stable and then mark's
commit applies
on top of that just fine (just don't delete the netif_carrier_on)  and
adds the feature
of the user being able to set the status to on / off explicitly if
they have a new version
of qemu / virtio while still doing the "right thing" for current versions.

Therefore, I guess I should resend my patch to netdev with reworked comments
to reflect this discussion. Hope that is ok with you too.

Pantelis

>
> commit 9f4d26d0f3016cf8813977d624751b94465fa317
> Author: Mark McLoughlin <markmc@redhat.com>
> Date:   Mon Jan 19 17:09:49 2009 -0800
>
>    virtio_net: add link status handling
>
>    Allow the host to inform us that the link is down by adding
>    a VIRTIO_NET_F_STATUS which indicates that device status is
>    available in virtio_net config.
>
>    This is currently useful for simulating link down conditions
>    (e.g. using proposed qemu 'set_link' monitor command) but
>    would also be needed if we were to support device assignment
>    via virtio.
>
>    Signed-off-by: Mark McLoughlin <markmc@redhat.com>
>    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (added future masking)
>    Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 30ae6d9..9b33d6e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -42,6 +42,7 @@ struct virtnet_info
>        struct virtqueue *rvq, *svq;
>        struct net_device *dev;
>        struct napi_struct napi;
> +       unsigned int status;
>
>        /* The skb we couldn't send because buffers were full. */
>        struct sk_buff *last_xmit_skb;
> @@ -611,6 +612,7 @@ static struct ethtool_ops virtnet_ethtool_ops = {
>        .set_tx_csum = virtnet_set_tx_csum,
>        .set_sg = ethtool_op_set_sg,
>        .set_tso = ethtool_op_set_tso,
> +       .get_link = ethtool_op_get_link,
>  };
>
>  #define MIN_MTU 68
> @@ -636,6 +638,41 @@ static const struct net_device_ops virtnet_netdev = {
>  #endif
>  };
>
> +static void virtnet_update_status(struct virtnet_info *vi)
> +{
> +       u16 v;
> +
> +       if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
> +               return;
> +
> +       vi->vdev->config->get(vi->vdev,
> +                             offsetof(struct virtio_net_config, status),
> +                             &v, sizeof(v));
> +
> +       /* Ignore unknown (future) status bits */
> +       v &= VIRTIO_NET_S_LINK_UP;
> +
> +       if (vi->status == v)
> +               return;
> +
> +       vi->status = v;
> +
> +       if (vi->status & VIRTIO_NET_S_LINK_UP) {
> +               netif_carrier_on(vi->dev);
> +               netif_wake_queue(vi->dev);
> +       } else {
> +               netif_carrier_off(vi->dev);
> +               netif_stop_queue(vi->dev);
> +       }
> +}
> +
> +static void virtnet_config_changed(struct virtio_device *vdev)
> +{
> +       struct virtnet_info *vi = vdev->priv;
> +
> +       virtnet_update_status(vi);
> +}
> +
>  static int virtnet_probe(struct virtio_device *vdev)
>  {
>        int err;
> @@ -738,6 +775,9 @@ static int virtnet_probe(struct virtio_device *vdev)
>                goto unregister;
>        }
>
> +       vi->status = VIRTIO_NET_S_LINK_UP;
> +       virtnet_update_status(vi);
> +
>        pr_debug("virtnet: registered device %s\n", dev->name);
>        return 0;
>
> @@ -793,7 +833,7 @@ static unsigned int features[] = {
>        VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
>        VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
>        VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
> -       VIRTIO_NET_F_MRG_RXBUF,
> +       VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS,
>        VIRTIO_F_NOTIFY_ON_EMPTY,
>  };
>
> @@ -805,6 +845,7 @@ static struct virtio_driver virtio_net = {
>        .id_table =     id_table,
>        .probe =        virtnet_probe,
>        .remove =       __devexit_p(virtnet_remove),
> +       .config_changed = virtnet_config_changed,
>  };
>
>  static int __init init(void)
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 5cdd0aa..f76bd4a 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -21,11 +21,16 @@
>  #define VIRTIO_NET_F_HOST_ECN  13      /* Host can handle TSO[6] w/ ECN in. */
>  #define VIRTIO_NET_F_HOST_UFO  14      /* Host can handle UFO in. */
>  #define VIRTIO_NET_F_MRG_RXBUF 15      /* Host can merge receive buffers. */
> +#define VIRTIO_NET_F_STATUS    16      /* virtio_net_config.status available */
> +
> +#define VIRTIO_NET_S_LINK_UP   1       /* Link is up */
>
>  struct virtio_net_config
>  {
>        /* The config defining mac address (if VIRTIO_NET_F_MAC) */
>        __u8 mac[6];
> +       /* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
> +       __u16 status;
>  } __attribute__((packed));
>
>  /* This is the first element of the scatter-gather list.  If you don't
>

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

On Sat, Mar 14, 2009 at 12:40 PM, Pantelis Koukousoulas
<pktoss@gmail.com> wrote:
>> I don't think it was straightforward at all.  Current virtio_net "cards"
>> don't support carrier detection.  We reported that (correctly) to userspace,
>> like every other driver which doesn't support carrier detect.
>>
>> So why is virtio_net different?  Or should all devices which can't detect
>> carrier set it to on, so older NetworkManagers work?
>>
>
[snip]
>
> So, I still propose my version for 2.6.29 / -stable and then mark's
> commit applies
> on top of that just fine (just don't delete the netif_carrier_on)  and
> adds the feature
> of the user being able to set the status to on / off explicitly if
> they have a new version
> of qemu / virtio while still doing the "right thing" for current versions.
>
> Therefore, I guess I should resend my patch to netdev with reworked comments
> to reflect this discussion. Hope that is ok with you too.
>
> Pantelis

Or, we can merge the extra netif_carrier_on (what your patch did) to
mark's commit
and just add that full thing to 2.6.29 (as 2 patches probably because
of git, but
one next to the other please for bisection / readability reasons).

I just tested this solution with a self-made livecd, it applies
cleanly and works fine :)
(it might be ~42 lines instead of 2 but it is still trivial enough to
be confident that
nothing will break).

This way we get the expected result for every combination of current and future
versions of qemu / kernel without ever having one "lie" to the other or innocent
users becoming frustrated.

Again:
 (1) My version: smallest, only takes into account current "hardware",
might lead
   to a surprise for someone that uses a future qemu with 2.6.29, does
set_link off
   and expects to see "operation not supported" in ethtool instead of
"Link detected: yes".

 (2) Mark's commit: proper implementation (does both the hardware and
driver part),
       but will only work right for future qemu versions.

 (3) The 2 merged: work for all combinations of current and future
kernel / qemu.
   42 lines instead of 2 but just as unlikely to cause breakage.


I 'd be happy with either (3) in 2.6.29, or (1) in 2.6.29 and (2) in 2.6.30.

Just please don't select to "do nothing". That would be wrong :)

Pantelis

Re: [PATCH] Make virtio_net support carrier detection

[Rusty Russell]

On Saturday 14 March 2009 21:10:43 Pantelis Koukousoulas wrote:
> Therefore, I guess I should resend my patch to netdev with reworked comments
> to reflect this discussion. Hope that is ok with you too.

Already sent to Linus and -stable, with Dave's Ack.

Tho he hasn't applied it (yet?)

Thanks,
Rusty.

Re: [PATCH] Make virtio_net support carrier detection

[Pantelis Koukousoulas]

On Mon, Mar 16, 2009 at 12:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Saturday 14 March 2009 21:10:43 Pantelis Koukousoulas wrote:
>> Therefore, I guess I should resend my patch to netdev with reworked comments
>> to reflect this discussion. Hope that is ok with you too.
>
> Already sent to Linus and -stable, with Dave's Ack.
>
> Tho he hasn't applied it (yet?)
>
> Thanks,
> Rusty.
>

Great, I 'll just wait then :)

Thanks,
Pantelis

Re: [PATCH] Make virtio_net support carrier detection

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Sat, 14 Mar 2009 10:49:42 +1030

> On Saturday 14 March 2009 05:31:00 David Miller wrote:
> > You can set netif_carrier_on() until you are blue in the face,
> > but until you hook up the ethtool link indication operation
> > NetworkManager won't see it.
> 
> Um, dude, you already have that patch in your net-next tree.  See below.
> This one goes on top.

Ahh, I didn't comprehend this bit.

I overreacted, that's for sure, sorry.

> Or should all devices which can't detect carrier set it to on, so
> older NetworkManagers work?

It is my opinion that this is the only correct behavior.

If you don't know, it's on.  Because "on" means "usable".  And you
can't fault NetworkManager or any other tool for making that kind of
decision.

Re: [PATCH] Make virtio_net support carrier detection

[Rusty Russell]

On Monday 16 March 2009 13:28:06 David Miller wrote:
> > Or should all devices which can't detect carrier set it to on, so
> > older NetworkManagers work?
> 
> It is my opinion that this is the only correct behavior.
> 
> If you don't know, it's on.  Because "on" means "usable".  And you
> can't fault NetworkManager or any other tool for making that kind of
> decision.

Yes, I agree that NM has to assume unknown == on.  But I still think the
kernel is right as it is to return -EOPNOTSUPP and let userspace deal with it.

"Let's fix broken userspace in the kernel" arguments make me nervous ;)

Cheers,
Rusty.

Re: [PATCH] Make virtio_net support carrier detection

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 16 Mar 2009 13:43:13 +1030

> On Monday 16 March 2009 13:28:06 David Miller wrote:
> > > Or should all devices which can't detect carrier set it to on, so
> > > older NetworkManagers work?
> > 
> > It is my opinion that this is the only correct behavior.
> > 
> > If you don't know, it's on.  Because "on" means "usable".  And you
> > can't fault NetworkManager or any other tool for making that kind of
> > decision.
> 
> Yes, I agree that NM has to assume unknown == on.  But I still think the
> kernel is right as it is to return -EOPNOTSUPP and let userspace deal with it.
> 
> "Let's fix broken userspace in the kernel" arguments make me nervous ;)

This is right from one perspective.

But from another, -EOPNOTSUPP means "half-written driver, kick maintainer"
:-)