[PATCH v1] net/core: support runtime PM on net_device

In ioctl path on net_device, the physical deivce is often
touched, but the physical device may have been put into runtime
suspend state already, so cause some utilitis(ifconfig, ethtool,
...) to return failure in this situation.

This patch enables runtime PM on net_device and mark it as
no_callbacks, and resumes the net_device if physical device
is to be accessed, then suspends it after completion of the
access.

This patch fixes the problem above.

Acked-by: Rafael J. Wysocki <redacted>
Signed-off-by: Ming Lei <redacted>
---
v1:
	- fix one undefined local variable compile failure

 net/core/dev.c       |   83 +++++++++++++++++++++++++++++++-------------------
 net/core/ethtool.c   |    9 ++++--
 net/core/net-sysfs.c |    4 +++
 3 files changed, 63 insertions(+), 33 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 09cb3f6..d772ce8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -135,6 +135,7 @@
 #include <linux/net_tstamp.h>
 #include <linux/static_key.h>
 #include <net/flow_keys.h>
+#include <linux/pm_runtime.h>
 
 #include "net-sysfs.h"
 

@@ -4993,39 +4994,24 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
 /*
  *	Perform the SIOCxIFxxx calls, inside rtnl_lock()
  */
-static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
+static int __dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 {
 	int err;
 	struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name);
 	const struct net_device_ops *ops;
 
-	if (!dev)
-		return -ENODEV;
-
 	ops = dev->netdev_ops;
 
 	switch (cmd) {
 	case SIOCSIFFLAGS:	/* Set interface flags */
 		return dev_change_flags(dev, ifr->ifr_flags);
 
-	case SIOCSIFMETRIC:	/* Set the metric on the interface
-				   (currently unused) */
-		return -EOPNOTSUPP;
-
 	case SIOCSIFMTU:	/* Set the MTU of a device */
 		return dev_set_mtu(dev, ifr->ifr_mtu);
 
 	case SIOCSIFHWADDR:
 		return dev_set_mac_address(dev, &ifr->ifr_hwaddr);
 
-	case SIOCSIFHWBROADCAST:
-		if (ifr->ifr_hwaddr.sa_family != dev->type)
-			return -EINVAL;
-		memcpy(dev->broadcast, ifr->ifr_hwaddr.sa_data,
-		       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
-		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
-		return 0;
-
 	case SIOCSIFMAP:
 		if (ops->ndo_set_config) {
 			if (!netif_device_present(dev))

@@ -5049,21 +5035,6 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 		if (!netif_device_present(dev))
 			return -ENODEV;
 		return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
-
-	case SIOCSIFTXQLEN:
-		if (ifr->ifr_qlen < 0)
-			return -EINVAL;
-		dev->tx_queue_len = ifr->ifr_qlen;
-		return 0;
-
-	case SIOCSIFNAME:
-		ifr->ifr_newname[IFNAMSIZ-1] = '\0';
-		return dev_change_name(dev, ifr->ifr_newname);
-
-	case SIOCSHWTSTAMP:
-		err = net_hwtstamp_validate(ifr);
-		if (err)
-			return err;
 		/* fall through */
 
 	/*

@@ -5099,6 +5070,56 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 	return err;
 }
 
+static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
+{
+	int err;
+	struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name);
+	const struct net_device_ops *ops;
+
+	if (!dev)
+		return -ENODEV;
+
+	ops = dev->netdev_ops;
+
+	switch (cmd) {
+	case SIOCSIFMETRIC:	/* Set the metric on the interface
+				   (currently unused) */
+		return -EOPNOTSUPP;
+
+	case SIOCSIFHWBROADCAST:
+		if (ifr->ifr_hwaddr.sa_family != dev->type)
+			return -EINVAL;
+		memcpy(dev->broadcast, ifr->ifr_hwaddr.sa_data,
+		       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
+		return 0;
+
+	case SIOCSIFTXQLEN:
+		if (ifr->ifr_qlen < 0)
+			return -EINVAL;
+		dev->tx_queue_len = ifr->ifr_qlen;
+		return 0;
+
+	case SIOCSIFNAME:
+		ifr->ifr_newname[IFNAMSIZ-1] = '\0';
+		return dev_change_name(dev, ifr->ifr_newname);
+
+	case SIOCSHWTSTAMP:
+		err = net_hwtstamp_validate(ifr);
+		if (err)
+			return err;
+	}
+
+	if (pm_runtime_get_sync(&dev->dev) < 0)
+		return -ENODEV;
+
+	err = __dev_ifsioc(net, ifr, cmd);
+
+	pm_runtime_put(&dev->dev);
+
+	return err;
+}
+
 /*
  *	This function handles all "interface"-type I/O control requests. The actual
  *	'doing' part of this is dev_ifsioc above.

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 4d64cc2..dae4417 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c

@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/rtnetlink.h>
 #include <linux/sched.h>
+#include <linux/pm_runtime.h>
 
 /*
  * Some useful ethtool_ops methods that're device independent.

@@ -1464,10 +1465,13 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 			return -EPERM;
 	}
 
+	if (pm_runtime_get_sync(&dev->dev) < 0)
+		return -ENODEV;
+
 	if (dev->ethtool_ops->begin) {
 		rc = dev->ethtool_ops->begin(dev);
 		if (rc  < 0)
-			return rc;
+			goto exit;
 	}
 	old_features = dev->features;
 

@@ -1648,6 +1652,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 
 	if (old_features != dev->features)
 		netdev_features_change(dev);
-
+exit:
+	pm_runtime_put(&dev->dev);
 	return rc;
 }

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index bcf02f6..c9adb89 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c

@@ -23,6 +23,7 @@
 #include <linux/export.h>
 #include <linux/jiffies.h>
 #include <net/wext.h>
+#include <linux/pm_runtime.h>
 
 #include "net-sysfs.h"
 

@@ -1415,6 +1416,9 @@ int netdev_register_kobject(struct net_device *net)
 	if (error)
 		return error;
 
+	pm_runtime_no_callbacks(dev);
+	pm_runtime_enable(dev);
+
 	error = register_queue_kobjects(net);
 	if (error) {
 		device_del(dev);

-- 
1.7.9.5

Ming Lei [off-list ref] writes:

In ioctl path on net_device, the physical deivce is often
touched, but the physical device may have been put into runtime
suspend state already, so cause some utilitis(ifconfig, ethtool,
...) to return failure in this situation.

I have to as the stupid questions again, sorry...

Just wondering, isn't that really a driver problem?  The driver will
know whether or not hardware access is required, and should wake up the
device if necessary.  Unless I misunderstand something here, this seems
like papering over driver bugs?


Bjørn

On Thu, Oct 18, 2012 at 4:29 PM, Bjørn Mork [off-list ref] wrote:

Ming Lei [off-list ref] writes:

quoted

In ioctl path on net_device, the physical deivce is often
touched, but the physical device may have been put into runtime
suspend state already, so cause some utilitis(ifconfig, ethtool,
...) to return failure in this situation.

I have to as the stupid questions again, sorry...

Just wondering, isn't that really a driver problem?  The driver will

It is or not, :-)

know whether or not hardware access is required, and should wake up the

The netcore knows that first,  doesn't it？

device if necessary.  Unless I misunderstand something here, this seems
like papering over driver bugs?

Suppose it is driver bug, and basically most network drivers don't consider
that, and we can fix that in netcore generally, so why bother all drivers to do
that?


Thanks,
--
Ming Lei

Ming Lei [off-list ref] writes:

On Thu, Oct 18, 2012 at 4:29 PM, Bjørn Mork [off-list ref] wrote:

quoted

Ming Lei [off-list ref] writes:

quoted

In ioctl path on net_device, the physical deivce is often
touched, but the physical device may have been put into runtime
suspend state already, so cause some utilitis(ifconfig, ethtool,
...) to return failure in this situation.

I have to as the stupid questions again, sorry...

Just wondering, isn't that really a driver problem?  The driver will

It is or not, :-)

quoted

know whether or not hardware access is required, and should wake up the

The netcore knows that first,  doesn't it？

Really? Does netcore know which ioctls the driver can handle without
waking the device?  You can of course do an educated guess, but I really
hate guesswork if there is a real answer somewhere else...

quoted

device if necessary.  Unless I misunderstand something here, this seems
like papering over driver bugs?

Suppose it is driver bug, and basically most network drivers don't consider
that, and we can fix that in netcore generally, so why bother all drivers to do
that?

Because bugs are supposed to be fixed and not hidden?

Note that I am not claiming this is a bug.  That is still an open
question as far as I can see.



Bjørn

On Thu, Oct 18, 2012 at 7:40 PM, Bjørn Mork [off-list ref] wrote:

Because bugs are supposed to be fixed and not hidden?

OK, now let's discuss solution for the problem which should be
clear and be solved.

The 1st one is to do it by this patch, or rule out some ioctl cmd
which needn't wakeup device.

The second one is to fix ioctl one by one in each driver.

Any other solution?

Note that I am not claiming this is a bug.  That is still an open
question as far as I can see.

Thanks,
--
Ming Lei

On 10/18/2012 02:55 PM, Ming Lei wrote:

On Thu, Oct 18, 2012 at 7:40 PM, Bjørn Mork [off-list ref] wrote:

quoted

Because bugs are supposed to be fixed and not hidden?

OK, now let's discuss solution for the problem which should be
clear and be solved.

The 1st one is to do it by this patch, or rule out some ioctl cmd
which needn't wakeup device.

The second one is to fix ioctl one by one in each driver.

Any other solution?

quoted

Note that I am not claiming this is a bug.  That is still an open
question as far as I can see.

Hi Ming,

IIUC, the pm_runtime is related to the device drivers, so at the first
glance, we should see invocation of the runtime's functions in drivers/*
and arch/*. Adding these calls in the net core, which makes sense at a
certain point, is a bit weird for me.

From my POV, if the drivers have been modified to support the pm_runtime
and this new functionality brought a regression with the ioctl, that
should be fixed in the drivers and not in the core code.

What happens with your patch if we use ethtool on a virtual device like
veth, macvlan, bridge, ... ?

Shouldn't handle the mac address, mtu, ... changes with the rtnetlink also ?

Thanks
  -- Daniel

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

From: Ming Lei <redacted>
Date: Thu, 18 Oct 2012 20:55:58 +0800

On Thu, Oct 18, 2012 at 7:40 PM, Bjørn Mork [off-list ref] wrote:

quoted

Because bugs are supposed to be fixed and not hidden?

OK, now let's discuss solution for the problem which should be
clear and be solved.

The 1st one is to do it by this patch, or rule out some ioctl cmd
which needn't wakeup device.

The second one is to fix ioctl one by one in each driver.

Any other solution?

The second is clearly the correct way to handle these issues.

Drivers need to be aware when their drivers ops are called whether
they can safely access the device or not.

I'm not applying this patch.

On Fri, Oct 19, 2012 at 12:34 AM, Daniel Lezcano
[off-list ref] wrote:

Hi Ming,

IIUC, the pm_runtime is related to the device drivers, so at the first
glance, we should see invocation of the runtime's functions in drivers/*
and arch/*. Adding these calls in the net core, which makes sense at a
certain point, is a bit weird for me.

From my POV, if the drivers have been modified to support the pm_runtime
and this new functionality brought a regression with the ioctl, that
should be fixed in the drivers and not in the core code.

What happens with your patch if we use ethtool on a virtual device like
veth, macvlan, bridge, ... ?

No any effect no matter if these drivers implement runtime PM or not.


Thanks,
--
Ming Lei