| Message ID | 1314715608-978-2-git-send-email-jpirko@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2011-08-30 at 16:46 +0200, Jiri Pirko wrote: > Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier > callback to allow changing carrier from userspace. [...] > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c > index 56e42ab..c8f2ca4 100644 > --- a/net/core/net-sysfs.c > +++ b/net/core/net-sysfs.c > @@ -126,6 +126,30 @@ static ssize_t show_broadcast(struct device *dev, > return -EINVAL; > } > > +static ssize_t store_carrier(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct net_device *netdev = to_net_dev(dev); > + ssize_t ret; > + int new_value; > + > + if (!capable(CAP_NET_ADMIN)) > + return -EPERM; > + > + if (!netif_running(netdev)) > + return -EINVAL; Not sure that's the right error code. > + if (sscanf(buf, "%d", &new_value) != 1) > + return -EINVAL; kstrtoint() (Or maybe we should have kstrobool().) > + if (!rtnl_trylock()) > + return restart_syscall(); [...] This is consistent with other attributes, but it seems like a really bad practice as it will generally result in the process busy-waiting on the RTNL lock! I think it would be better to add and use an rtnl_lock_interruptible(). Ben.
Tue, Aug 30, 2011 at 05:14:22PM CEST, bhutchings@solarflare.com wrote: >On Tue, 2011-08-30 at 16:46 +0200, Jiri Pirko wrote: >> Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier >> callback to allow changing carrier from userspace. >[...] >> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c >> index 56e42ab..c8f2ca4 100644 >> --- a/net/core/net-sysfs.c >> +++ b/net/core/net-sysfs.c >> @@ -126,6 +126,30 @@ static ssize_t show_broadcast(struct device *dev, >> return -EINVAL; >> } >> >> +static ssize_t store_carrier(struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t len) >> +{ >> + struct net_device *netdev = to_net_dev(dev); >> + ssize_t ret; >> + int new_value; >> + >> + if (!capable(CAP_NET_ADMIN)) >> + return -EPERM; >> + >> + if (!netif_running(netdev)) >> + return -EINVAL; > >Not sure that's the right error code. I stayed consistent with show_carrier in this. > >> + if (sscanf(buf, "%d", &new_value) != 1) >> + return -EINVAL; > >kstrtoint() Okay. > >(Or maybe we should have kstrobool().) > >> + if (!rtnl_trylock()) >> + return restart_syscall(); >[...] > >This is consistent with other attributes, but it seems like a really bad >practice as it will generally result in the process busy-waiting on the >RTNL lock! I think it would be better to add and use an >rtnl_lock_interruptible(). Right. But as you said, this is the same as with others. I would replace this in another patch. > >Ben. > >-- >Ben Hutchings, Staff Engineer, Solarflare >Not speaking for my employer; that's the marketing department's job. >They asked us to note that Solarflare product names are trademarked. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2011/8/30 Jiri Pirko <jpirko@redhat.com>: > Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier > callback to allow changing carrier from userspace. Do you expect drivers using implementation different than just calling netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? BTW, I like this feature! Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 30 Aug 2011 20:11:37 +0200 Michał Mirosław <mirqus@gmail.com> wrote: > 2011/8/30 Jiri Pirko <jpirko@redhat.com>: > > Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier > > callback to allow changing carrier from userspace. > > Do you expect drivers using implementation different than just calling > netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? > > BTW, I like this feature! Ok for virtual devices, but please don't implement it in real hardware. There is already enough breakage in carrier management in applications. It also overlaps with operstate perhaps that is a more more complete solution. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tue, Aug 30, 2011 at 08:11:37PM CEST, mirqus@gmail.com wrote: >2011/8/30 Jiri Pirko <jpirko@redhat.com>: >> Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier >> callback to allow changing carrier from userspace. > >Do you expect drivers using implementation different than just calling >netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? Yes, generally it can be used also for en/disable phy, for testing purposes if hw and driver would support it. > >BTW, I like this feature! > >Best Regards, >Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tue, Aug 30, 2011 at 08:25:53PM CEST, shemminger@vyatta.com wrote: >On Tue, 30 Aug 2011 20:11:37 +0200 >Michał Mirosław <mirqus@gmail.com> wrote: > >> 2011/8/30 Jiri Pirko <jpirko@redhat.com>: >> > Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier >> > callback to allow changing carrier from userspace. >> >> Do you expect drivers using implementation different than just calling >> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >> >> BTW, I like this feature! > >Ok for virtual devices, but please don't implement it in real hardware. >There is already enough breakage in carrier management in applications. >It also overlaps with operstate perhaps that is a more more complete >solution. Looking at operstate doc, I'm not sure what exactly do you mean by "overlapping". The main purpose of my patch is to give certain virt devices the opportunity to emulate carrier loss. But I do not see reason why this can't be implemented by real hw. Or course there should be explicitelly documented the purpose of this feature. Jirka > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
W dniu 31 sierpnia 2011 10:26 użytkownik Jiri Pirko <jpirko@redhat.com> napisał: > Tue, Aug 30, 2011 at 08:11:37PM CEST, mirqus@gmail.com wrote: >>2011/8/30 Jiri Pirko <jpirko@redhat.com>: >>> Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier >>> callback to allow changing carrier from userspace. >>Do you expect drivers using implementation different than just calling >>netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? > Yes, generally it can be used also for en/disable phy, for testing > purposes if hw and driver would support it. I'd like to see this working for GRE tunnel devices (for keepalive daemon to be able to indicate to routing daemons whether tunnel is really working) - implementation would be identical to dummy's case. Should I prepare a patch or can I leave it to you? Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wed, Aug 31, 2011 at 10:33:50AM CEST, mirqus@gmail.com wrote: >W dniu 31 sierpnia 2011 10:26 użytkownik Jiri Pirko <jpirko@redhat.com> napisał: >> Tue, Aug 30, 2011 at 08:11:37PM CEST, mirqus@gmail.com wrote: >>>2011/8/30 Jiri Pirko <jpirko@redhat.com>: >>>> Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier >>>> callback to allow changing carrier from userspace. >>>Do you expect drivers using implementation different than just calling >>>netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >> Yes, generally it can be used also for en/disable phy, for testing >> purposes if hw and driver would support it. > >I'd like to see this working for GRE tunnel devices (for keepalive >daemon to be able to indicate to routing daemons whether tunnel is >really working) - implementation would be identical to dummy's case. >Should I prepare a patch or can I leave it to you? Ok, I can include it to this patchset (I'm going to repost first patch anyway) > >Best Regards, >Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 31/08/2011 10:45, Jiri Pirko a écrit : >>>> Do you expect drivers using implementation different than just calling >>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >>> Yes, generally it can be used also for en/disable phy, for testing >>> purposes if hw and driver would support it. >> >> I'd like to see this working for GRE tunnel devices (for keepalive >> daemon to be able to indicate to routing daemons whether tunnel is >> really working) - implementation would be identical to dummy's case. >> Should I prepare a patch or can I leave it to you? > > Ok, I can include it to this patchset (I'm going to repost first patch > anyway) Can't we assume that the dummy's case is the default behavior and register this default ndo_change_carrier callback for every device ? Device drivers willing to do something different can install a different callback if appropriate. This would avoid duplicating the following code in most drivers, that don't need something special. static int dummy_change_carrier(struct net_device *dev, bool new_carrier) { if (new_carrier) netif_carrier_on(dev); else netif_carrier_off(dev); return 0; } If someone is not confident with this default callback registered for all device, at least, we can put this code in a common place, so that a driver willing to use it doesn't need to have its own version of it. Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: > Le 31/08/2011 10:45, Jiri Pirko a écrit : > > >>>> Do you expect drivers using implementation different than just calling > >>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? > >>> Yes, generally it can be used also for en/disable phy, for testing > >>> purposes if hw and driver would support it. > >> > >> I'd like to see this working for GRE tunnel devices (for keepalive > >> daemon to be able to indicate to routing daemons whether tunnel is > >> really working) - implementation would be identical to dummy's case. > >> Should I prepare a patch or can I leave it to you? > > > > Ok, I can include it to this patchset (I'm going to repost first patch > > anyway) > > Can't we assume that the dummy's case is the default behavior and > register this default > ndo_change_carrier callback for every device ? You have got to be joking. No device driver that has real link monitoring should use this implementation. [...] > If someone is not confident with this default callback registered for > all device, at least, we can > put this code in a common place, so that a driver willing to use it > doesn't need to have its own > version of it. I agree that this might be useful in some other software devices, though. Ben.
Wed, Aug 31, 2011 at 10:12:17PM CEST, bhutchings@solarflare.com wrote: >On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: >> Le 31/08/2011 10:45, Jiri Pirko a écrit : >> >> >>>> Do you expect drivers using implementation different than just calling >> >>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >> >>> Yes, generally it can be used also for en/disable phy, for testing >> >>> purposes if hw and driver would support it. >> >> >> >> I'd like to see this working for GRE tunnel devices (for keepalive >> >> daemon to be able to indicate to routing daemons whether tunnel is >> >> really working) - implementation would be identical to dummy's case. >> >> Should I prepare a patch or can I leave it to you? >> > >> > Ok, I can include it to this patchset (I'm going to repost first patch >> > anyway) >> >> Can't we assume that the dummy's case is the default behavior and >> register this default >> ndo_change_carrier callback for every device ? > >You have got to be joking. No device driver that has real link >monitoring should use this implementation. > >[...] >> If someone is not confident with this default callback registered for >> all device, at least, we can >> put this code in a common place, so that a driver willing to use it >> doesn't need to have its own >> version of it. > >I agree that this might be useful in some other software devices, >though. Maybe it would be better to just add priv_flag for this instead of introducing netdev_op. Then do on/off is this flag is set. Not sure though... Jirka > >Ben. > >-- >Ben Hutchings, Staff Engineer, Solarflare >Not speaking for my employer; that's the marketing department's job. >They asked us to note that Solarflare product names are trademarked. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 31/08/2011 22:12, Ben Hutchings a écrit : > On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: >> Le 31/08/2011 10:45, Jiri Pirko a écrit : >> >>>>>> Do you expect drivers using implementation different than just calling >>>>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >>>>> Yes, generally it can be used also for en/disable phy, for testing >>>>> purposes if hw and driver would support it. >>>> >>>> I'd like to see this working for GRE tunnel devices (for keepalive >>>> daemon to be able to indicate to routing daemons whether tunnel is >>>> really working) - implementation would be identical to dummy's case. >>>> Should I prepare a patch or can I leave it to you? >>> >>> Ok, I can include it to this patchset (I'm going to repost first patch >>> anyway) >> >> Can't we assume that the dummy's case is the default behavior and >> register this default >> ndo_change_carrier callback for every device ? > > You have got to be joking. No device driver that has real link > monitoring should use this implementation. Well, why not? Arguably, this is probably not the feature one would use every day, but... Testing a cluster reaction to a link down event would be easier if one doesn't need to unplug the cable for the test. I understand that one can turn off the switch port (physical or virtual), but echo 0 > /sys/class/net/eth0/carrier would be nice too. Of course, I assume that netif_carrier_on and netif_carrier_off are only called on real link status change. So the value written into /sys/class/net/eth0/carrier would stay until the link revert it, due to a double status change (up-down-up or down-up-down). But I may miss totally this point. Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-08-31 at 22:31 +0200, Nicolas de Pesloüan wrote: > Le 31/08/2011 22:12, Ben Hutchings a écrit : > > On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: > >> Le 31/08/2011 10:45, Jiri Pirko a écrit : > >> > >>>>>> Do you expect drivers using implementation different than just calling > >>>>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? > >>>>> Yes, generally it can be used also for en/disable phy, for testing > >>>>> purposes if hw and driver would support it. > >>>> > >>>> I'd like to see this working for GRE tunnel devices (for keepalive > >>>> daemon to be able to indicate to routing daemons whether tunnel is > >>>> really working) - implementation would be identical to dummy's case. > >>>> Should I prepare a patch or can I leave it to you? > >>> > >>> Ok, I can include it to this patchset (I'm going to repost first patch > >>> anyway) > >> > >> Can't we assume that the dummy's case is the default behavior and > >> register this default > >> ndo_change_carrier callback for every device ? > > > > You have got to be joking. No device driver that has real link > > monitoring should use this implementation. > > Well, why not? Arguably, this is probably not the feature one would > use every day, but... > > Testing a cluster reaction to a link down event would be easier if one > doesn't need to unplug the > cable for the test. I understand that one can turn off the switch port > (physical or virtual), but > echo 0 > /sys/class/net/eth0/carrier would be nice too. > > Of course, I assume that netif_carrier_on and netif_carrier_off are > only called on real link status > change. So the value written into /sys/class/net/eth0/carrier would > stay until the link revert it, > due to a double status change (up-down-up or down-up-down). But I may > miss totally this point. Think what happens if the user sets carrier on, when the link is actually down. Ben.
On 08/31/2011 01:31 PM, Nicolas de Pesloüan wrote: > Le 31/08/2011 22:12, Ben Hutchings a écrit : >> On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: >>> Le 31/08/2011 10:45, Jiri Pirko a écrit : >>> >>>>>>> Do you expect drivers using implementation different than just calling >>>>>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >>>>>> Yes, generally it can be used also for en/disable phy, for testing >>>>>> purposes if hw and driver would support it. >>>>> >>>>> I'd like to see this working for GRE tunnel devices (for keepalive >>>>> daemon to be able to indicate to routing daemons whether tunnel is >>>>> really working) - implementation would be identical to dummy's case. >>>>> Should I prepare a patch or can I leave it to you? >>>> >>>> Ok, I can include it to this patchset (I'm going to repost first patch >>>> anyway) >>> >>> Can't we assume that the dummy's case is the default behavior and >>> register this default >>> ndo_change_carrier callback for every device ? >> >> You have got to be joking. No device driver that has real link >> monitoring should use this implementation. > > Well, why not? Arguably, this is probably not the feature one would use every day, but... > > Testing a cluster reaction to a link down event would be easier if one doesn't need to unplug the cable for the test. I understand that one can turn off the > switch port (physical or virtual), but echo 0 > /sys/class/net/eth0/carrier would be nice too. There is special hardware out there that can do bypass, and often it also has a mode that will programatically cut link by throwing some relays. We use this for our testing equipment... If there is some way to twiddle standard-ish hardware to actually drop link, that would be neat. I'd think it should be an ethtool type of thing, however. Actually dropping link, and letting that naturally propagate up the stack seems more reasonable than lying about the status half way up the stack. Thanks, Ben
On Wed, 2011-08-31 at 13:48 -0700, Ben Greear wrote: > On 08/31/2011 01:31 PM, Nicolas de Pesloüan wrote: > > Le 31/08/2011 22:12, Ben Hutchings a écrit : > >> On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: > >>> Le 31/08/2011 10:45, Jiri Pirko a écrit : > >>> > >>>>>>> Do you expect drivers using implementation different than just calling > >>>>>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? > >>>>>> Yes, generally it can be used also for en/disable phy, for testing > >>>>>> purposes if hw and driver would support it. > >>>>> > >>>>> I'd like to see this working for GRE tunnel devices (for keepalive > >>>>> daemon to be able to indicate to routing daemons whether tunnel is > >>>>> really working) - implementation would be identical to dummy's case. > >>>>> Should I prepare a patch or can I leave it to you? > >>>> > >>>> Ok, I can include it to this patchset (I'm going to repost first patch > >>>> anyway) > >>> > >>> Can't we assume that the dummy's case is the default behavior and > >>> register this default > >>> ndo_change_carrier callback for every device ? > >> > >> You have got to be joking. No device driver that has real link > >> monitoring should use this implementation. > > > > Well, why not? Arguably, this is probably not the feature one would use every day, but... > > > > Testing a cluster reaction to a link down event would be easier if one doesn't need to unplug the cable for the test. I understand that one can turn off the > > switch port (physical or virtual), but echo 0 > /sys/class/net/eth0/carrier would be nice too. > > There is special hardware out there that can do bypass, and often it also has a mode > that will programatically cut link by throwing some relays. We use this for our > testing equipment... > > If there is some way to twiddle standard-ish hardware to actually drop link, that > would be neat. I'd think it should be an ethtool type of thing, however. We need to be able to control this as part of our driver test suite (on the peer, not the device under test). There are various MDIO bits that look like they should do this but unfortunately they don't have consistent effects. Besides that, many PHYs are not MDIO-manageable. So this would have to be a device-specific operation, whether it's exposed through ethtool or sysfs. > Actually dropping link, and letting that naturally propagate up the stack seems > more reasonable than lying about the status half way up the stack. Right. Ben.
On Wed, 31 Aug 2011 22:36:45 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote: > On Wed, 2011-08-31 at 13:48 -0700, Ben Greear wrote: > > On 08/31/2011 01:31 PM, Nicolas de Pesloüan wrote: > > > Le 31/08/2011 22:12, Ben Hutchings a écrit : > > >> On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: > > >>> Le 31/08/2011 10:45, Jiri Pirko a écrit : > > >>> > > >>>>>>> Do you expect drivers using implementation different than just calling > > >>>>>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? > > >>>>>> Yes, generally it can be used also for en/disable phy, for testing > > >>>>>> purposes if hw and driver would support it. > > >>>>> > > >>>>> I'd like to see this working for GRE tunnel devices (for keepalive > > >>>>> daemon to be able to indicate to routing daemons whether tunnel is > > >>>>> really working) - implementation would be identical to dummy's case. > > >>>>> Should I prepare a patch or can I leave it to you? > > >>>> > > >>>> Ok, I can include it to this patchset (I'm going to repost first patch > > >>>> anyway) > > >>> > > >>> Can't we assume that the dummy's case is the default behavior and > > >>> register this default > > >>> ndo_change_carrier callback for every device ? > > >> > > >> You have got to be joking. No device driver that has real link > > >> monitoring should use this implementation. > > > > > > Well, why not? Arguably, this is probably not the feature one would use every day, but... > > > > > > Testing a cluster reaction to a link down event would be easier if one doesn't need to unplug the cable for the test. I understand that one can turn off the > > > switch port (physical or virtual), but echo 0 > /sys/class/net/eth0/carrier would be nice too. > > > > There is special hardware out there that can do bypass, and often it also has a mode > > that will programatically cut link by throwing some relays. We use this for our > > testing equipment... > > > > If there is some way to twiddle standard-ish hardware to actually drop link, that > > would be neat. I'd think it should be an ethtool type of thing, however. > > We need to be able to control this as part of our driver test suite (on > the peer, not the device under test). There are various MDIO bits that > look like they should do this but unfortunately they don't have > consistent effects. Besides that, many PHYs are not MDIO-manageable. > So this would have to be a device-specific operation, whether it's > exposed through ethtool or sysfs. > > > Actually dropping link, and letting that naturally propagate up the stack seems > > more reasonable than lying about the status half way up the stack. > For testing clustering, there are hooks in vmware and QEMU/KVM to allow dropping carrier on the VM side. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/31/2011 02:36 PM, Ben Hutchings wrote: > On Wed, 2011-08-31 at 13:48 -0700, Ben Greear wrote: >> On 08/31/2011 01:31 PM, Nicolas de Pesloüan wrote: >>> Le 31/08/2011 22:12, Ben Hutchings a écrit : >>>> On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: >>>>> Le 31/08/2011 10:45, Jiri Pirko a écrit : >>>>> >>>>>>>>> Do you expect drivers using implementation different than just calling >>>>>>>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >>>>>>>> Yes, generally it can be used also for en/disable phy, for testing >>>>>>>> purposes if hw and driver would support it. >>>>>>> >>>>>>> I'd like to see this working for GRE tunnel devices (for keepalive >>>>>>> daemon to be able to indicate to routing daemons whether tunnel is >>>>>>> really working) - implementation would be identical to dummy's case. >>>>>>> Should I prepare a patch or can I leave it to you? >>>>>> >>>>>> Ok, I can include it to this patchset (I'm going to repost first patch >>>>>> anyway) >>>>> >>>>> Can't we assume that the dummy's case is the default behavior and >>>>> register this default >>>>> ndo_change_carrier callback for every device ? >>>> >>>> You have got to be joking. No device driver that has real link >>>> monitoring should use this implementation. >>> >>> Well, why not? Arguably, this is probably not the feature one would use every day, but... >>> >>> Testing a cluster reaction to a link down event would be easier if one doesn't need to unplug the cable for the test. I understand that one can turn off the >>> switch port (physical or virtual), but echo 0> /sys/class/net/eth0/carrier would be nice too. >> >> There is special hardware out there that can do bypass, and often it also has a mode >> that will programatically cut link by throwing some relays. We use this for our >> testing equipment... >> >> If there is some way to twiddle standard-ish hardware to actually drop link, that >> would be neat. I'd think it should be an ethtool type of thing, however. > > We need to be able to control this as part of our driver test suite (on > the peer, not the device under test). There are various MDIO bits that > look like they should do this but unfortunately they don't have > consistent effects. Besides that, many PHYs are not MDIO-manageable. > So this would have to be a device-specific operation, whether it's > exposed through ethtool or sysfs. Well, I have boxes that can act as the peer. Nics are intel chipset 1G, the bypass/link-drop feature is some separate hardware on the NIC, and there is a hack of a driver & user-space tools that controls those bits. But, if there is a way to fiddle normal NICs w/out the special bypass/disconnect hardware, that would be welcome. Even if it's different for each driver, and not supported by all, ethtool can deal with that sort of thing. Here's a link to the hardware we use...it comes with drivers, though we've hacked on ours a bit. http://silicom-usa.com/upload/Downloads/Product_102.pdf Thanks, Ben
Wed, Aug 31, 2011 at 10:48:22PM CEST, greearb@candelatech.com wrote: >On 08/31/2011 01:31 PM, Nicolas de Pesloüan wrote: >>Le 31/08/2011 22:12, Ben Hutchings a écrit : >>>On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: >>>>Le 31/08/2011 10:45, Jiri Pirko a écrit : >>>> >>>>>>>>Do you expect drivers using implementation different than just calling >>>>>>>>netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >>>>>>>Yes, generally it can be used also for en/disable phy, for testing >>>>>>>purposes if hw and driver would support it. >>>>>> >>>>>>I'd like to see this working for GRE tunnel devices (for keepalive >>>>>>daemon to be able to indicate to routing daemons whether tunnel is >>>>>>really working) - implementation would be identical to dummy's case. >>>>>>Should I prepare a patch or can I leave it to you? >>>>> >>>>>Ok, I can include it to this patchset (I'm going to repost first patch >>>>>anyway) >>>> >>>>Can't we assume that the dummy's case is the default behavior and >>>>register this default >>>>ndo_change_carrier callback for every device ? >>> >>>You have got to be joking. No device driver that has real link >>>monitoring should use this implementation. >> >>Well, why not? Arguably, this is probably not the feature one would use every day, but... >> >>Testing a cluster reaction to a link down event would be easier if one doesn't need to unplug the cable for the test. I understand that one can turn off the >>switch port (physical or virtual), but echo 0 > /sys/class/net/eth0/carrier would be nice too. > >There is special hardware out there that can do bypass, and often it also has a mode >that will programatically cut link by throwing some relays. We use this for our >testing equipment... > >If there is some way to twiddle standard-ish hardware to actually drop link, that >would be neat. I'd think it should be an ethtool type of thing, however. Ethtool can implement this eventually by calling the same ndo. > >Actually dropping link, and letting that naturally propagate up the stack seems >more reasonable than lying about the status half way up the stack. Yes, that is really the intension of the proposed ndo. Real hw driver should implement that as you say, not directly setting carrier_on/off > >Thanks, >Ben > >-- >Ben Greear <greearb@candelatech.com> >Candela Technologies Inc http://www.candelatech.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wed, Aug 31, 2011 at 11:40:23PM CEST, shemminger@vyatta.com wrote: >On Wed, 31 Aug 2011 22:36:45 +0100 >Ben Hutchings <bhutchings@solarflare.com> wrote: > >> On Wed, 2011-08-31 at 13:48 -0700, Ben Greear wrote: >> > On 08/31/2011 01:31 PM, Nicolas de Pesloüan wrote: >> > > Le 31/08/2011 22:12, Ben Hutchings a écrit : >> > >> On Wed, 2011-08-31 at 22:03 +0200, Nicolas de Pesloüan wrote: >> > >>> Le 31/08/2011 10:45, Jiri Pirko a écrit : >> > >>> >> > >>>>>>> Do you expect drivers using implementation different than just calling >> > >>>>>>> netif_carrier_on/off? Or is it supposed to also e.g. power down PHYs? >> > >>>>>> Yes, generally it can be used also for en/disable phy, for testing >> > >>>>>> purposes if hw and driver would support it. >> > >>>>> >> > >>>>> I'd like to see this working for GRE tunnel devices (for keepalive >> > >>>>> daemon to be able to indicate to routing daemons whether tunnel is >> > >>>>> really working) - implementation would be identical to dummy's case. >> > >>>>> Should I prepare a patch or can I leave it to you? >> > >>>> >> > >>>> Ok, I can include it to this patchset (I'm going to repost first patch >> > >>>> anyway) >> > >>> >> > >>> Can't we assume that the dummy's case is the default behavior and >> > >>> register this default >> > >>> ndo_change_carrier callback for every device ? >> > >> >> > >> You have got to be joking. No device driver that has real link >> > >> monitoring should use this implementation. >> > > >> > > Well, why not? Arguably, this is probably not the feature one would use every day, but... >> > > >> > > Testing a cluster reaction to a link down event would be easier if one doesn't need to unplug the cable for the test. I understand that one can turn off the >> > > switch port (physical or virtual), but echo 0 > /sys/class/net/eth0/carrier would be nice too. >> > >> > There is special hardware out there that can do bypass, and often it also has a mode >> > that will programatically cut link by throwing some relays. We use this for our >> > testing equipment... >> > >> > If there is some way to twiddle standard-ish hardware to actually drop link, that >> > would be neat. I'd think it should be an ethtool type of thing, however. >> >> We need to be able to control this as part of our driver test suite (on >> the peer, not the device under test). There are various MDIO bits that >> look like they should do this but unfortunately they don't have >> consistent effects. Besides that, many PHYs are not MDIO-manageable. >> So this would have to be a device-specific operation, whether it's >> exposed through ethtool or sysfs. >> >> > Actually dropping link, and letting that naturally propagate up the stack seems >> > more reasonable than lying about the status half way up the stack. >> > >For testing clustering, there are hooks in vmware and QEMU/KVM to allow >dropping carrier on the VM side. Afaik in kvm this is only possible on emulated e1000 (last time I checked). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 0a7f619..6bca5b7 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -945,6 +945,8 @@ struct net_device_ops { u32 features); int (*ndo_set_features)(struct net_device *dev, u32 features); + int (*ndo_change_carrier)(struct net_device *dev, + bool new_carrier); }; /* @@ -2093,6 +2095,8 @@ extern int dev_set_mtu(struct net_device *, int); extern void dev_set_group(struct net_device *, int); extern int dev_set_mac_address(struct net_device *, struct sockaddr *); +extern int dev_change_carrier(struct net_device *, + bool new_carrier); extern int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, struct netdev_queue *txq); diff --git a/net/core/dev.c b/net/core/dev.c index b2e262e..11b0fc7 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4793,6 +4793,25 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa) } EXPORT_SYMBOL(dev_set_mac_address); +/** + * dev_change_carrier - Change device carrier + * @dev: device + * @new_carries: new value + * + * Change device carrier + */ +int dev_change_carrier(struct net_device *dev, bool new_carrier) +{ + const struct net_device_ops *ops = dev->netdev_ops; + + if (!ops->ndo_change_carrier) + return -EOPNOTSUPP; + if (!netif_device_present(dev)) + return -ENODEV; + return ops->ndo_change_carrier(dev, new_carrier); +} +EXPORT_SYMBOL(dev_change_carrier); + /* * Perform the SIOCxIFxxx calls, inside rcu_read_lock() */ diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 56e42ab..c8f2ca4 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -126,6 +126,30 @@ static ssize_t show_broadcast(struct device *dev, return -EINVAL; } +static ssize_t store_carrier(struct device *dev, struct device_attribute *attr, + const char *buf, size_t len) +{ + struct net_device *netdev = to_net_dev(dev); + ssize_t ret; + int new_value; + + if (!capable(CAP_NET_ADMIN)) + return -EPERM; + + if (!netif_running(netdev)) + return -EINVAL; + + if (sscanf(buf, "%d", &new_value) != 1) + return -EINVAL; + + if (!rtnl_trylock()) + return restart_syscall(); + ret = dev_change_carrier(netdev, new_value ? true: false); + rtnl_unlock(); + + return ret < 0 ? ret : len; +} + static ssize_t show_carrier(struct device *dev, struct device_attribute *attr, char *buf) { @@ -315,7 +339,7 @@ static struct device_attribute net_class_attributes[] = { __ATTR(link_mode, S_IRUGO, show_link_mode, NULL), __ATTR(address, S_IRUGO, show_address, NULL), __ATTR(broadcast, S_IRUGO, show_broadcast, NULL), - __ATTR(carrier, S_IRUGO, show_carrier, NULL), + __ATTR(carrier, S_IRUGO | S_IWUSR, show_carrier, store_carrier), __ATTR(speed, S_IRUGO, show_speed, NULL), __ATTR(duplex, S_IRUGO, show_duplex, NULL), __ATTR(dormant, S_IRUGO, show_dormant, NULL),
Allow to write to "carrier" attribute. Devices may implement ndo_change_carrier callback to allow changing carrier from userspace. Signed-off-by: Jiri Pirko <jpirko@redhat.com> --- include/linux/netdevice.h | 4 ++++ net/core/dev.c | 19 +++++++++++++++++++ net/core/net-sysfs.c | 26 +++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 1 deletions(-)