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

Subsystems: networking [general], the rest

STALE5102d

5 messages, 3 authors, 2012-10-17 · open the first message on its own page

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

From: Ming Lei <hidden>
Date: 2012-10-10 12:58:59

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.

Signed-off-by: Ming Lei <redacted>
---
 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 de2bad7..d46d4ed 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"
 
@@ -4966,39 +4967,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))
@@ -5022,21 +5008,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 */
 
 	/*
@@ -5072,6 +5043,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..2dc43da 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 ((ret = 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

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

From: Oliver Neukum <hidden>
Date: 2012-10-10 13:19:15

On Wednesday 10 October 2012 20:58:33 Ming Lei wrote:
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.
[..]
+	if (pm_runtime_get_sync(&dev->dev) < 0)
+		return -ENODEV;
-EIO would be appropriate.
quoted hunk
+	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..2dc43da 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 ((ret = pm_runtime_get_sync(&dev->dev)) < 0)
+		return -ENODEV;
+
 	if (dev->ethtool_ops->begin) {
Perhaps you should check that first.
quoted hunk
 		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);
Why?

	Regards
		Oliver

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

From: Ming Lei <hidden>
Date: 2012-10-11 02:41:14

Oliver, sorry for not CC list.

On Wed, Oct 10, 2012 at 9:17 PM, Oliver Neukum [off-list ref] wrote:
On Wednesday 10 October 2012 20:58:33 Ming Lei wrote:
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.

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.
[..]
quoted
+     if (pm_runtime_get_sync(&dev->dev) < 0)
+             return -ENODEV;
-EIO would be appropriate.
-ENODEV should be fine, at least some current drivers
return it in this situation.
quoted
+     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..2dc43da 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 ((ret = pm_runtime_get_sync(&dev->dev)) < 0)
+             return -ENODEV;
+
      if (dev->ethtool_ops->begin) {
Perhaps you should check that first.
No, ->begin() may access physical device too, also no matter
->begin is defined or not, the 'cmd' will be sent to device driver.
quoted
              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);
Why?
Firstly, 'net_device' is a 'class' device, and its class of 'net_class' doesn't
define runtime pm callback. Also no drivers define its runtime pm callback
in its device_type, so I mark it as no_callback.

Also if not enabling runtime pm for net_device, pm_runtime_get_sync()
will return -EACCESS.

Thanks,
--
Ming Lei

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

From: Rafael J. Wysocki <hidden>
Date: 2012-10-16 17:03:34

On Wednesday 10 of October 2012 20:58:33 Ming Lei wrote:
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.

Signed-off-by: Ming Lei <redacted>
Well, it looks good in principle.

Acked-by: Rafael J. Wysocki <redacted>

quoted hunk
---
 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 de2bad7..d46d4ed 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"
 
@@ -4966,39 +4967,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))
@@ -5022,21 +5008,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 */
 
 	/*
@@ -5072,6 +5043,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..2dc43da 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 ((ret = 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);
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

From: Ming Lei <hidden>
Date: 2012-10-17 14:17:50

On Wed, Oct 17, 2012 at 1:07 AM, Rafael J. Wysocki [off-list ref] wrote:
On Wednesday 10 of October 2012 20:58:33 Ming Lei wrote:
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.

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.

Signed-off-by: Ming Lei <redacted>
Well, it looks good in principle.

Acked-by: Rafael J. Wysocki <redacted>
OK, I will prepare -v1 for merge later if no one has objection on
the patch, which has a compile error.('ret' is not defined in
dev_ethtool)

Thanks,
--
Ming Lei
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help