Re: [PATCH net-next 1/4] ethtool: runtime-resume netdev parent before ethtool ioctl ops
From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: 2021-08-03 20:41:34
Also in:
linux-pm
On 01/08/2021 13:36, Heiner Kallweit wrote:
quoted hunk ↗ jump to hunk
If a network device is runtime-suspended then: - network device may be flagged as detached and all ethtool ops (even if not accessing the device) will fail because netif_device_present() returns false - ethtool ops may fail because device is not accessible (e.g. because being in D3 in case of a PCI device) It may not be desirable that userspace can't use even simple ethtool ops that not access the device if interface or link is down. To be more friendly to userspace let's ensure that device is runtime-resumed when executing the respective ethtool op in kernel. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- net/ethtool/ioctl.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index baa5d1004..b7ff9abe7 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c@@ -23,6 +23,7 @@ #include <linux/rtnetlink.h> #include <linux/sched/signal.h> #include <linux/net.h> +#include <linux/pm_runtime.h> #include <net/devlink.h> #include <net/xdp_sock_drv.h> #include <net/flow_offload.h>@@ -2589,7 +2590,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) int rc; netdev_features_t old_features; - if (!dev || !netif_device_present(dev)) + if (!dev) return -ENODEV; if (copy_from_user(ðcmd, useraddr, sizeof(ethcmd)))@@ -2645,10 +2646,18 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) return -EPERM; } + if (dev->dev.parent) + pm_runtime_get_sync(dev->dev.parent);
the PM Runtime should allow to wake up parent when child is resumed if everything is configured properly.
rpm_resume()
...
if (!parent && dev->parent) {
--> here
So, hence PM runtime calls are moved to from drivers to net_core wouldn't be more correct approach to
enable PM runtime for netdev->dev and lets PM runtime do the job?
But, to be honest, I'm not sure adding PM runtime manipulation to the net core is a good idea -
at minimum it might be tricky and required very careful approach (especially in err path).
For example, even in this patch you do not check return value of pm_runtime_get_sync() and in
commit bd869245a3dc ("net: core: try to runtime-resume detached device in __dev_open") also actualy.
The TI CPSW driver may also be placed in non reachable state when netdev is closed (and even lose context),
but we do not use netif_device_detach() (so netdev is accessible through netdev_ops/ethtool_ops),
but instead wake up device by runtime PM for allowed operations or just save requested configuration which
is applied at netdev->open() time then.
I feel that using netif_device_detach() in PM runtime sounds like a too heavy approach ;)
huh, see it's merged already, so...
quoted hunk ↗ jump to hunk
+ + if (!netif_device_present(dev)) { + rc = -ENODEV; + goto out; + } + if (dev->ethtool_ops->begin) { rc = dev->ethtool_ops->begin(dev); - if (rc < 0) - return rc; + if (rc < 0) + goto out; } old_features = dev->features;@@ -2867,6 +2876,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) if (old_features != dev->features) netdev_features_change(dev); +out: + if (dev->dev.parent) + pm_runtime_put(dev->dev.parent); return rc; }
-- Best regards, grygorii