RFC: netdev: allow ethtool physical id to drop rtnl_lock

RFC: netdev: allow ethtool physical id to drop rtnl_lock

[Stephen Hemminger]

The ethtool operation to blink LED can take an indeterminately long time,
blocking out other operations (via rtnl_lock). This patch is an attempt
to work around the problem.

It does need more discussion, because it will mean that drivers that formerly
were protected from changes during blink aren't.  For example, user could
start device blinking, and then plug in cable causing change netlink event
to change state or pull cable and have device come down.

The other possibility is to do this on a driver by driver basis
which is more effort.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/core/ethtool.c	2009-10-30 10:27:23.621917624 -0700
+++ b/net/core/ethtool.c	2009-10-30 10:35:53.787670774 -0700
@@ -17,6 +17,7 @@
 #include <linux/errno.h>
 #include <linux/ethtool.h>
 #include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
 #include <asm/uaccess.h>
 
 /*
@@ -781,6 +782,8 @@ static int ethtool_get_strings(struct ne
 static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_value id;
+	int err;
+	static int busy;
 
 	if (!dev->ethtool_ops->phys_id)
 		return -EOPNOTSUPP;
@@ -788,7 +791,21 @@ static int ethtool_phys_id(struct net_de
 	if (copy_from_user(&id, useraddr, sizeof(id)))
 		return -EFAULT;
 
-	return dev->ethtool_ops->phys_id(dev, id.data);
+	if (busy)
+		return -EBUSY;
+
+	/* This operation may take a long time, drop lock */
+	busy = 1;
+	dev_hold(dev);
+	rtnl_unlock();
+
+	err = dev->ethtool_ops->phys_id(dev, id.data);
+
+	rtnl_lock();
+	dev_put(dev);
+	busy = 0;
+
+	return err;
 }
 
 static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)

Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock

[Eric Dumazet]

Stephen Hemminger a écrit :
> The ethtool operation to blink LED can take an indeterminately long time,
> blocking out other operations (via rtnl_lock). This patch is an attempt
> to work around the problem.
> 
> It does need more discussion, because it will mean that drivers that formerly
> were protected from changes during blink aren't.  For example, user could
> start device blinking, and then plug in cable causing change netlink event
> to change state or pull cable and have device come down.
> 
> The other possibility is to do this on a driver by driver basis
> which is more effort.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> --- a/net/core/ethtool.c	2009-10-30 10:27:23.621917624 -0700
> +++ b/net/core/ethtool.c	2009-10-30 10:35:53.787670774 -0700
> @@ -17,6 +17,7 @@
>  #include <linux/errno.h>
>  #include <linux/ethtool.h>
>  #include <linux/netdevice.h>
> +#include <linux/rtnetlink.h>
>  #include <asm/uaccess.h>
>  
>  /*
> @@ -781,6 +782,8 @@ static int ethtool_get_strings(struct ne
>  static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
>  {
>  	struct ethtool_value id;
> +	int err;
> +	static int busy;
>  
>  	if (!dev->ethtool_ops->phys_id)
>  		return -EOPNOTSUPP;
> @@ -788,7 +791,21 @@ static int ethtool_phys_id(struct net_de
>  	if (copy_from_user(&id, useraddr, sizeof(id)))
>  		return -EFAULT;
>  
> -	return dev->ethtool_ops->phys_id(dev, id.data);
> +	if (busy)
> +		return -EBUSY;
> +
> +	/* This operation may take a long time, drop lock */
> +	busy = 1;
> +	dev_hold(dev);
> +	rtnl_unlock();
> +
> +	err = dev->ethtool_ops->phys_id(dev, id.data);
> +
> +	rtnl_lock();
> +	dev_put(dev);
> +	busy = 0;
> +
> +	return err;
>  }
>  

It seems reasonable, but why have a global 'busy' flag, and not
private to each netdev ?

Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock

[Stephen Hemminger]

On Fri, 30 Oct 2009 18:47:39 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Stephen Hemminger a écrit :
> > The ethtool operation to blink LED can take an indeterminately long time,
> > blocking out other operations (via rtnl_lock). This patch is an attempt
> > to work around the problem.
> > 
> > It does need more discussion, because it will mean that drivers that formerly
> > were protected from changes during blink aren't.  For example, user could
> > start device blinking, and then plug in cable causing change netlink event
> > to change state or pull cable and have device come down.
> > 
> > The other possibility is to do this on a driver by driver basis
> > which is more effort.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> > 
> > 
> > --- a/net/core/ethtool.c	2009-10-30 10:27:23.621917624 -0700
> > +++ b/net/core/ethtool.c	2009-10-30 10:35:53.787670774 -0700
> > @@ -17,6 +17,7 @@
> >  #include <linux/errno.h>
> >  #include <linux/ethtool.h>
> >  #include <linux/netdevice.h>
> > +#include <linux/rtnetlink.h>
> >  #include <asm/uaccess.h>
> >  
> >  /*
> > @@ -781,6 +782,8 @@ static int ethtool_get_strings(struct ne
> >  static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
> >  {
> >  	struct ethtool_value id;
> > +	int err;
> > +	static int busy;
> >  
> >  	if (!dev->ethtool_ops->phys_id)
> >  		return -EOPNOTSUPP;
> > @@ -788,7 +791,21 @@ static int ethtool_phys_id(struct net_de
> >  	if (copy_from_user(&id, useraddr, sizeof(id)))
> >  		return -EFAULT;
> >  
> > -	return dev->ethtool_ops->phys_id(dev, id.data);
> > +	if (busy)
> > +		return -EBUSY;
> > +
> > +	/* This operation may take a long time, drop lock */
> > +	busy = 1;
> > +	dev_hold(dev);
> > +	rtnl_unlock();
> > +
> > +	err = dev->ethtool_ops->phys_id(dev, id.data);
> > +
> > +	rtnl_lock();
> > +	dev_put(dev);
> > +	busy = 0;
> > +
> > +	return err;
> >  }
> >  
> 
> It seems reasonable, but why have a global 'busy' flag, and not
> private to each netdev ?
> 

Because that is what old behaviour was (one blink at a time).


-- 

Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock

[Michael Chan]

On Fri, 2009-10-30 at 10:42 -0700, Stephen Hemminger wrote:
> The ethtool operation to blink LED can take an indeterminately long time,
> blocking out other operations (via rtnl_lock). This patch is an attempt
> to work around the problem.
> 
> It does need more discussion, because it will mean that drivers that formerly
> were protected from changes during blink aren't.  For example, user could
> start device blinking, and then plug in cable causing change netlink event
> to change state or pull cable and have device come down.

Yeah, the biggest concern is shutting down the device while it is still
blinking.  During shutdown, some devices are brought to low power state
and the chip will no longer respond to I/Os to blink the LEDs.  On some
systems, this can cause bus hang or NMI.

> 
> The other possibility is to do this on a driver by driver basis
> which is more effort.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> --- a/net/core/ethtool.c	2009-10-30 10:27:23.621917624 -0700
> +++ b/net/core/ethtool.c	2009-10-30 10:35:53.787670774 -0700
> @@ -17,6 +17,7 @@
>  #include <linux/errno.h>
>  #include <linux/ethtool.h>
>  #include <linux/netdevice.h>
> +#include <linux/rtnetlink.h>
>  #include <asm/uaccess.h>
>  
>  /*
> @@ -781,6 +782,8 @@ static int ethtool_get_strings(struct ne
>  static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
>  {
>  	struct ethtool_value id;
> +	int err;
> +	static int busy;
>  
>  	if (!dev->ethtool_ops->phys_id)
>  		return -EOPNOTSUPP;
> @@ -788,7 +791,21 @@ static int ethtool_phys_id(struct net_de
>  	if (copy_from_user(&id, useraddr, sizeof(id)))
>  		return -EFAULT;
>  
> -	return dev->ethtool_ops->phys_id(dev, id.data);
> +	if (busy)
> +		return -EBUSY;
> +
> +	/* This operation may take a long time, drop lock */
> +	busy = 1;
> +	dev_hold(dev);
> +	rtnl_unlock();
> +
> +	err = dev->ethtool_ops->phys_id(dev, id.data);
> +
> +	rtnl_lock();
> +	dev_put(dev);
> +	busy = 0;
> +
> +	return err;
>  }
>  
>  static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
> --
> 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
> 

Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Sat, 31 Oct 2009 09:44:22 -0700

> 
> On Fri, 2009-10-30 at 10:42 -0700, Stephen Hemminger wrote:
>> The ethtool operation to blink LED can take an indeterminately long time,
>> blocking out other operations (via rtnl_lock). This patch is an attempt
>> to work around the problem.
>> 
>> It does need more discussion, because it will mean that drivers that formerly
>> were protected from changes during blink aren't.  For example, user could
>> start device blinking, and then plug in cable causing change netlink event
>> to change state or pull cable and have device come down.
> 
> Yeah, the biggest concern is shutting down the device while it is still
> blinking.  During shutdown, some devices are brought to low power state
> and the chip will no longer respond to I/Os to blink the LEDs.  On some
> systems, this can cause bus hang or NMI.

Right, and for this reason we'll either need find some way to stop
the LED blinking when the device is brought down.

We can deal with this in a way such that we'll never need to bug
the drivers again if we want to mess with the implementation again.

Create a "netif_phys_id_loop_iter()" that, along with a netdev
pointer, takes a "u32 data" which is whatever was passed in to
ethtool_ops->id().

The drivers then structure their loops like:

	while (1) {
		blink_it_baby();
		data = netif_phys_id_loop_iter(dev, data);
		if (!data)
			break;
	}

Next, we take that:

	if (data == 0)
		data = UINT_MAX / 2;

That every driver seems to do, and stick it in the ethtool op dispatch
in net/core/ethtool.c so it doesn't need to be duplicated (and
potentially forgotten) in every implementation.

Finally, in netif_phys_id_loop_iter() we put something like:

u32 netif_phys_id_loop_iter(struct netdev *dev, u32 data)
{
	if (dev->reg_state == NETREG_UNREGISTERING)
		return 0;
	if (msleep_interruptible(500))
		return 0;
	return data - 2;
}

Then, unregister somehow blocks on the ->phys_id() hitting that
NETREG_UNREGISTERING check and returning.

Anyways, you get the idea.

Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock

[Stephen Hemminger]

On Mon, 02 Nov 2009 00:17:10 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: "Michael Chan" <mchan@broadcom.com>
> Date: Sat, 31 Oct 2009 09:44:22 -0700
> 
> > 
> > On Fri, 2009-10-30 at 10:42 -0700, Stephen Hemminger wrote:
> >> The ethtool operation to blink LED can take an indeterminately long time,
> >> blocking out other operations (via rtnl_lock). This patch is an attempt
> >> to work around the problem.
> >> 
> >> It does need more discussion, because it will mean that drivers that formerly
> >> were protected from changes during blink aren't.  For example, user could
> >> start device blinking, and then plug in cable causing change netlink event
> >> to change state or pull cable and have device come down.
> > 
> > Yeah, the biggest concern is shutting down the device while it is still
> > blinking.  During shutdown, some devices are brought to low power state
> > and the chip will no longer respond to I/Os to blink the LEDs.  On some
> > systems, this can cause bus hang or NMI.
> 
> Right, and for this reason we'll either need find some way to stop
> the LED blinking when the device is brought down.
> 
> We can deal with this in a way such that we'll never need to bug
> the drivers again if we want to mess with the implementation again.
> 
> Create a "netif_phys_id_loop_iter()" that, along with a netdev
> pointer, takes a "u32 data" which is whatever was passed in to
> ethtool_ops->id().
> 
> The drivers then structure their loops like:
> 
> 	while (1) {
> 		blink_it_baby();
> 		data = netif_phys_id_loop_iter(dev, data);
> 		if (!data)
> 			break;
> 	}
> 
> Next, we take that:
> 
> 	if (data == 0)
> 		data = UINT_MAX / 2;
> 
> That every driver seems to do, and stick it in the ethtool op dispatch
> in net/core/ethtool.c so it doesn't need to be duplicated (and
> potentially forgotten) in every implementation.
> 
> Finally, in netif_phys_id_loop_iter() we put something like:
> 
> u32 netif_phys_id_loop_iter(struct netdev *dev, u32 data)
> {
> 	if (dev->reg_state == NETREG_UNREGISTERING)
> 		return 0;
> 	if (msleep_interruptible(500))
> 		return 0;
> 	return data - 2;
> }
> 
> Then, unregister somehow blocks on the ->phys_id() hitting that
> NETREG_UNREGISTERING check and returning.
> 
> Anyways, you get the idea.

For compatibility, I was thinking of adding a new ethtool hook that
moves the blinking loop into ethtool.


static int ethtool_phys_blink(struct net_device *dev, u32 secs)
{
	while (secs > 0) {
		dev->ethtool_ps->phys_led(dev, ETH_LED_ON);
...
		dev->ethtool_ps->phys_led(dev, ETH_LED_OFF);
	}
	dev->ethtool_ops->phys_led(dev, ETH_LED_NORMAL);
}


static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
{
	struct ethtool_value id;

	if (copy_from_user(&id, useraddr, sizeof(id)))
		return -EFAULT;

	if (dev->ethtool_ops->phys_led)
		return ethtool_phys_blink(dev, id.data);

	if (dev->ethtool_ops->phys_id)
		return dev->ethtool_ops->phys_id(dev, id.data);
	else
		return -EOPNOTSUPP;
}



-- 

Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock

[Ben Hutchings]

On Mon, 2009-11-02 at 08:51 -0800, Stephen Hemminger wrote:
[...]
> For compatibility, I was thinking of adding a new ethtool hook that
> moves the blinking loop into ethtool.
>
> static int ethtool_phys_blink(struct net_device *dev, u32 secs)
> {
> 	while (secs > 0) {
> 		dev->ethtool_ps->phys_led(dev, ETH_LED_ON);
> ...
> 		dev->ethtool_ps->phys_led(dev, ETH_LED_OFF);
> 	}
> 	dev->ethtool_ops->phys_led(dev, ETH_LED_NORMAL);
> }
[...]

We could even force this on unconverted drivers, but using a longer
loop period to allow for slow blinking:

static int ethtool_phys_id_loop(struct net_device *dev, u32 secs)
{
	u32 timeout;
	int rc;

	for (;;) {
		timeout = min_t(u32, secs, 5);
		secs -= timeout;
		rc = dev->ethtool_ops->phys_id(dev, timeout);
		if (rc || secs == 0)
			return rc;
		...
	}
}

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.