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

[Stephen Hemminger <shemminger@vyatta.com>]

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;
}



--