* Re: [PATCH] Make virtio_net support carrier detection [not found] <1236772642-12705-1-git-send-email-pktoss@gmail.com> @ 2009-03-12 7:29 ` Rusty Russell 2009-03-12 7:44 ` Pantelis Koukousoulas 0 siblings, 1 reply; 28+ messages in thread From: Rusty Russell @ 2009-03-12 7:29 UTC (permalink / raw) To: Pantelis Koukousoulas; +Cc: netdev 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; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 7:29 ` [PATCH] Make virtio_net support carrier detection Rusty Russell @ 2009-03-12 7:44 ` Pantelis Koukousoulas 2009-03-12 9:16 ` Rusty Russell 0 siblings, 1 reply; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-12 7:44 UTC (permalink / raw) To: Rusty Russell, netdev 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 7:44 ` Pantelis Koukousoulas @ 2009-03-12 9:16 ` Rusty Russell 2009-03-12 10:23 ` Pantelis Koukousoulas 2009-03-12 11:43 ` Dan Williams 0 siblings, 2 replies; 28+ messages in thread From: Rusty Russell @ 2009-03-12 9:16 UTC (permalink / raw) To: Pantelis Koukousoulas; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 9:16 ` Rusty Russell @ 2009-03-12 10:23 ` Pantelis Koukousoulas 2009-03-12 11:05 ` Pantelis Koukousoulas 2009-03-12 11:43 ` Dan Williams 1 sibling, 1 reply; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-12 10:23 UTC (permalink / raw) To: Rusty Russell; +Cc: netdev > 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 10:23 ` Pantelis Koukousoulas @ 2009-03-12 11:05 ` Pantelis Koukousoulas 0 siblings, 0 replies; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-12 11:05 UTC (permalink / raw) To: Rusty Russell; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 9:16 ` Rusty Russell 2009-03-12 10:23 ` Pantelis Koukousoulas @ 2009-03-12 11:43 ` Dan Williams 2009-03-12 12:47 ` Pantelis Koukousoulas 1 sibling, 1 reply; 28+ messages in thread From: Dan Williams @ 2009-03-12 11:43 UTC (permalink / raw) To: Rusty Russell; +Cc: Pantelis Koukousoulas, netdev 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 11:43 ` Dan Williams @ 2009-03-12 12:47 ` Pantelis Koukousoulas 2009-03-12 12:52 ` David Miller 0 siblings, 1 reply; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-12 12:47 UTC (permalink / raw) To: Dan Williams; +Cc: Rusty Russell, netdev > 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 12:47 ` Pantelis Koukousoulas @ 2009-03-12 12:52 ` David Miller 2009-03-12 12:58 ` Pantelis Koukousoulas ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: David Miller @ 2009-03-12 12:52 UTC (permalink / raw) To: pktoss; +Cc: dcbw, rusty, netdev 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 12:52 ` David Miller @ 2009-03-12 12:58 ` Pantelis Koukousoulas 2009-03-12 13:03 ` David Miller 2009-03-12 13:41 ` Dan Williams 2009-03-12 21:39 ` Rusty Russell 2 siblings, 1 reply; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-12 12:58 UTC (permalink / raw) To: David Miller; +Cc: dcbw, rusty, netdev > 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 12:58 ` Pantelis Koukousoulas @ 2009-03-12 13:03 ` David Miller 2009-03-12 13:10 ` Pantelis Koukousoulas 0 siblings, 1 reply; 28+ messages in thread From: David Miller @ 2009-03-12 13:03 UTC (permalink / raw) To: pktoss; +Cc: dcbw, rusty, netdev 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 13:03 ` David Miller @ 2009-03-12 13:10 ` Pantelis Koukousoulas 0 siblings, 0 replies; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-12 13:10 UTC (permalink / raw) To: David Miller; +Cc: dcbw, rusty, netdev 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 12:52 ` David Miller 2009-03-12 12:58 ` Pantelis Koukousoulas @ 2009-03-12 13:41 ` Dan Williams 2009-03-12 13:55 ` Dan Williams 2009-03-12 21:39 ` Rusty Russell 2 siblings, 1 reply; 28+ messages in thread From: Dan Williams @ 2009-03-12 13:41 UTC (permalink / raw) To: David Miller; +Cc: pktoss, rusty, netdev 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 13:41 ` Dan Williams @ 2009-03-12 13:55 ` Dan Williams 0 siblings, 0 replies; 28+ messages in thread From: Dan Williams @ 2009-03-12 13:55 UTC (permalink / raw) To: David Miller; +Cc: pktoss, rusty, netdev 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 12:52 ` David Miller 2009-03-12 12:58 ` Pantelis Koukousoulas 2009-03-12 13:41 ` Dan Williams @ 2009-03-12 21:39 ` Rusty Russell 2009-03-12 23:47 ` Rusty Russell 2 siblings, 1 reply; 28+ messages in thread From: Rusty Russell @ 2009-03-12 21:39 UTC (permalink / raw) To: David Miller; +Cc: pktoss, dcbw, netdev 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 21:39 ` Rusty Russell @ 2009-03-12 23:47 ` Rusty Russell 2009-03-12 23:55 ` Stephen Hemminger ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Rusty Russell @ 2009-03-12 23:47 UTC (permalink / raw) To: David Miller; +Cc: pktoss, dcbw, netdev 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); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 23:47 ` Rusty Russell @ 2009-03-12 23:55 ` Stephen Hemminger 2009-03-13 5:04 ` Pantelis Koukousoulas 2009-03-13 19:01 ` David Miller 2 siblings, 0 replies; 28+ messages in thread From: Stephen Hemminger @ 2009-03-12 23:55 UTC (permalink / raw) To: Rusty Russell; +Cc: David Miller, pktoss, dcbw, netdev 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 23:47 ` Rusty Russell 2009-03-12 23:55 ` Stephen Hemminger @ 2009-03-13 5:04 ` Pantelis Koukousoulas 2009-03-13 19:01 ` David Miller 2 siblings, 0 replies; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-13 5:04 UTC (permalink / raw) To: Rusty Russell; +Cc: David Miller, dcbw, netdev 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-12 23:47 ` Rusty Russell 2009-03-12 23:55 ` Stephen Hemminger 2009-03-13 5:04 ` Pantelis Koukousoulas @ 2009-03-13 19:01 ` David Miller 2009-03-14 0:19 ` Rusty Russell 2 siblings, 1 reply; 28+ messages in thread From: David Miller @ 2009-03-13 19:01 UTC (permalink / raw) To: rusty; +Cc: pktoss, dcbw, netdev 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 :-)) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-13 19:01 ` David Miller @ 2009-03-14 0:19 ` Rusty Russell 2009-03-14 10:40 ` Pantelis Koukousoulas 2009-03-16 2:58 ` David Miller 0 siblings, 2 replies; 28+ messages in thread From: Rusty Russell @ 2009-03-14 0:19 UTC (permalink / raw) To: David Miller; +Cc: pktoss, dcbw, netdev 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-14 0:19 ` Rusty Russell @ 2009-03-14 10:40 ` Pantelis Koukousoulas 2009-03-14 11:33 ` Pantelis Koukousoulas 2009-03-15 22:39 ` Rusty Russell 2009-03-16 2:58 ` David Miller 1 sibling, 2 replies; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-14 10:40 UTC (permalink / raw) To: Rusty Russell; +Cc: David Miller, dcbw, netdev, markmc > 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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-14 10:40 ` Pantelis Koukousoulas @ 2009-03-14 11:33 ` Pantelis Koukousoulas 2009-03-15 22:39 ` Rusty Russell 1 sibling, 0 replies; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-14 11:33 UTC (permalink / raw) To: Rusty Russell; +Cc: David Miller, dcbw, netdev, markmc 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-14 10:40 ` Pantelis Koukousoulas 2009-03-14 11:33 ` Pantelis Koukousoulas @ 2009-03-15 22:39 ` Rusty Russell 2009-03-16 2:05 ` Pantelis Koukousoulas 1 sibling, 1 reply; 28+ messages in thread From: Rusty Russell @ 2009-03-15 22:39 UTC (permalink / raw) To: Pantelis Koukousoulas; +Cc: David Miller, dcbw, netdev, markmc 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-15 22:39 ` Rusty Russell @ 2009-03-16 2:05 ` Pantelis Koukousoulas 0 siblings, 0 replies; 28+ messages in thread From: Pantelis Koukousoulas @ 2009-03-16 2:05 UTC (permalink / raw) To: Rusty Russell; +Cc: David Miller, netdev 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-14 0:19 ` Rusty Russell 2009-03-14 10:40 ` Pantelis Koukousoulas @ 2009-03-16 2:58 ` David Miller 2009-03-16 3:13 ` Rusty Russell 1 sibling, 1 reply; 28+ messages in thread From: David Miller @ 2009-03-16 2:58 UTC (permalink / raw) To: rusty; +Cc: pktoss, dcbw, netdev 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-16 2:58 ` David Miller @ 2009-03-16 3:13 ` Rusty Russell 2009-03-16 3:15 ` David Miller 0 siblings, 1 reply; 28+ messages in thread From: Rusty Russell @ 2009-03-16 3:13 UTC (permalink / raw) To: David Miller; +Cc: pktoss, dcbw, netdev 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-16 3:13 ` Rusty Russell @ 2009-03-16 3:15 ` David Miller 0 siblings, 0 replies; 28+ messages in thread From: David Miller @ 2009-03-16 3:15 UTC (permalink / raw) To: rusty; +Cc: pktoss, dcbw, netdev 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" :-) ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Make virtio_net support carrier detection @ 2009-03-14 0:23 Rusty Russell 2009-03-19 1:40 ` David Miller 0 siblings, 1 reply; 28+ messages in thread From: Rusty Russell @ 2009-03-14 0:23 UTC (permalink / raw) To: Linus Torvalds; +Cc: netdev, stable 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] Make virtio_net support carrier detection 2009-03-14 0:23 Rusty Russell @ 2009-03-19 1:40 ` David Miller 0 siblings, 0 replies; 28+ messages in thread From: David Miller @ 2009-03-19 1:40 UTC (permalink / raw) To: rusty; +Cc: torvalds, netdev, stable 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2009-03-19 1:40 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1236772642-12705-1-git-send-email-pktoss@gmail.com> 2009-03-12 7:29 ` [PATCH] Make virtio_net support carrier detection Rusty Russell 2009-03-12 7:44 ` Pantelis Koukousoulas 2009-03-12 9:16 ` Rusty Russell 2009-03-12 10:23 ` Pantelis Koukousoulas 2009-03-12 11:05 ` Pantelis Koukousoulas 2009-03-12 11:43 ` Dan Williams 2009-03-12 12:47 ` Pantelis Koukousoulas 2009-03-12 12:52 ` David Miller 2009-03-12 12:58 ` Pantelis Koukousoulas 2009-03-12 13:03 ` David Miller 2009-03-12 13:10 ` Pantelis Koukousoulas 2009-03-12 13:41 ` Dan Williams 2009-03-12 13:55 ` Dan Williams 2009-03-12 21:39 ` Rusty Russell 2009-03-12 23:47 ` Rusty Russell 2009-03-12 23:55 ` Stephen Hemminger 2009-03-13 5:04 ` Pantelis Koukousoulas 2009-03-13 19:01 ` David Miller 2009-03-14 0:19 ` Rusty Russell 2009-03-14 10:40 ` Pantelis Koukousoulas 2009-03-14 11:33 ` Pantelis Koukousoulas 2009-03-15 22:39 ` Rusty Russell 2009-03-16 2:05 ` Pantelis Koukousoulas 2009-03-16 2:58 ` David Miller 2009-03-16 3:13 ` Rusty Russell 2009-03-16 3:15 ` David Miller 2009-03-14 0:23 Rusty Russell 2009-03-19 1:40 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.