Re: [PATCH net-next 1/4] ethtool: runtime-resume netdev parent before ethtool ioctl ops

On 03.08.2021 22:41, Grygorii Strashko wrote:

On 01/08/2021 13:36, Heiner Kallweit wrote:

quoted

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(&ethcmd, 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.

Not sure if there's any case yet where the netdev-embedded device is power-managed.
Typically only the parent (e.g. a PCI device) is.

rpm_resume()
...
    if (!parent && dev->parent) {
 --> here

Currently we don't get that far because we will bail out here already:

else if (dev->power.disable_depth > 0)
		retval = -EACCES;

If netdev-embedded device isn't power-managed then disable_depth is 1.

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?

Where would netdev->dev be runtime-resumed so that netif_device_present() passes?
Wouldn't we then need RPM ops for the parent (e.g. PCI) and for netdev->dev?
E.g. the parent runtime-resume can be triggered by a PCI PME, then it would
have to resume netdev->dev.

But, to be honest, I'm not sure adding PM runtime manipulation to the net core is a good idea -

The TI CPSW driver runtime-resumes the device in begin ethtool op and suspends
it in complete. This pattern is used in more than one driver and may be worth
being moved to the core.

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 pm_runtime_get_sync() calls are attempts here. We don't want to bail out if a device
doesn't support RPM. I agree that checking the return code could make sense, but then we would
have to be careful which error codes we consider as failed.

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

That's not a rare pattern when suspending or runtime-suspending to prevent different types
of access to a not accessible device. But yes, it's relatively big hammer ..

huh, see it's merged already, so...

quoted

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