[patch net-next-2.6 0/2] net: allow to change carrier via sysfs

[patch net-next-2.6 0/2] net: allow to change carrier via sysfs

[Jiri Pirko]

As suggested by Ben Hutchings I made "carrier" attribute writeable.
With this patchset one can set carrier on/off on dummy. Might by also handy
to implement this for other devices.

Jiri Pirko (2):
  net: allow to change carrier via sysfs
  dummy: implement carrier change

 drivers/net/dummy.c       |   10 ++++++++++
 include/linux/netdevice.h |    4 ++++
 net/core/dev.c            |   19 +++++++++++++++++++
 net/core/net-sysfs.c      |   26 +++++++++++++++++++++++++-
 4 files changed, 58 insertions(+), 1 deletions(-)

-- 
1.7.6

[patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Jiri Pirko]

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(-)

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),
-- 
1.7.6

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Ben Hutchings]

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.

-- 
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.

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Jiri Pirko]

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.
>

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Michał Mirosław]

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

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Stephen Hemminger]

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.

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Jiri Pirko]

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
>

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Jiri Pirko]

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

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Michał Mirosław]

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

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Jiri Pirko]

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

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Nicolas de Pesloüan]

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.

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Ben Hutchings]

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.

-- 
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.

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Jiri Pirko]

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.
>

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Nicolas de Pesloüan]

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.

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Ben Hutchings]

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.

-- 
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.

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Ben Hutchings]

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.

-- 
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.

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Stephen Hemminger]

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.

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Jiri Pirko]

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).

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [patch net-next-2.6 1/2] net: allow to change carrier via sysfs

[Jiri Pirko]

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

[patch net-next-2.6 2/2] dummy: implement carrier change

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/dummy.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index a7c5e88..1e32e14 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -112,6 +112,15 @@ static void dummy_dev_free(struct net_device *dev)
 	free_netdev(dev);
 }
 
+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;
+}
+
 static const struct net_device_ops dummy_netdev_ops = {
 	.ndo_init		= dummy_dev_init,
 	.ndo_start_xmit		= dummy_xmit,
@@ -119,6 +128,7 @@ static const struct net_device_ops dummy_netdev_ops = {
 	.ndo_set_rx_mode	= set_multicast_list,
 	.ndo_set_mac_address	= dummy_set_address,
 	.ndo_get_stats64	= dummy_get_stats64,
+	.ndo_change_carrier	= dummy_change_carrier,
 };
 
 static void dummy_setup(struct net_device *dev)
-- 
1.7.6