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

On 05/08/2021 14:11, Joakim Zhang wrote:

quoted

-----Original Message-----
From: Grygorii Strashko <grygorii.strashko@ti.com>
Sent: 2021年8月5日 16:21
To: Heiner Kallweit <hkallweit1@gmail.com>; Jakub Kicinski
[off-list ref]; David Miller [off-list ref]
Cc: netdev@vger.kernel.org; Linux PM list <redacted>;
Andrew Lunn [off-list ref]; Florian Fainelli [off-list ref]
Subject: Re: [PATCH net-next 1/4] ethtool: runtime-resume netdev parent
before ethtool ioctl ops



On 04/08/2021 22:33, Heiner Kallweit wrote:

quoted

On 04.08.2021 10:43, Grygorii Strashko wrote:

quoted

On 04/08/2021 00:32, Heiner Kallweit wrote:

quoted

On 03.08.2021 22:41, Grygorii Strashko wrote:

quoted

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.

quoted

quoted

quoted

quoted

Not sure if there's any case yet where the netdev-embedded device is

power-managed.

quoted

quoted

quoted

Typically only the parent (e.g. a PCI device) is.

quoted

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.

Right. But if pm_runtime_enable() is added for ndev->dev then PM
runtime will start working for it and should handle parent properly -
from my experience, every time any code need manipulate with "parent" or

smth. else to make PM runtime working it means smth. is wrong.

quoted

quoted

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index

f6197774048b..33b72b788aa2 100644

--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c

@@ -1963,6 +1963,7 @@ int netdev_register_kobject(struct net_device

*ndev)
          }

          pm_runtime_set_memalloc_noio(dev, true);
+       pm_runtime_enable(dev);

          return error;
   }

quoted

quoted

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?

quoted

quoted

quoted

quoted

Where would netdev->dev be runtime-resumed so that

netif_device_present() passes?

quoted

quoted

That's the biggest issues here. Some driver uses
netif_device_detach() in PM runtime and, this way, introduces custom

dependency between Core device PM (runtime) sate and Net core, other driver
does not do.

quoted

quoted

Does it means every driver with PM runtime now have to be updated to

indicate it PM state to Net core with netif_device_detach()?

quoted

No, that's not needed.

quoted

Why? Why return value from pm_runtime_get calls is not enough?

Believe me it's terrible idea to introduce custom PM state dependency
between PM runtime and Net core, for example it took years to sync

properly System wide suspend and PM runtime which are separate framworks.

quoted

quoted

By the way netif_device_detach() during System Wide suspend is looks
perfectly valid, because entering System wide Suspend should prohibit
any access to netdev at some stage. And that's what 99% of network
drivers are doing (actually I can find only ./realtek/r8169_main.c
which abuse netif_device_detach() function and, I assume, it is your
case)

Actually I was inspired by the Intel drivers, see e.g.
__igc_shutdown(). They also detach the netdevice on runtime suspend.
One reason is that several core functions check for device presence
before e.g. calling a ndo callback. Example: dev_set_mtu_ext()

right and also:
- netlink - which you've hacked already
- 8021q:
vlan_dev_ioctl/vlan_dev_neigh_setup/vlan_add_rx_filter_info/vlan_kill_rx_filte
r_info

Yes, there are many place need to do such check. I always face a problem that where I need to
runtime-resume the device, is there any suggestion? I always add it when an issue came out.

What confuse me it that, is there any document describe that which .ndo callback should be called
with interface up, instead .ndo callback _CAN_ be called with interface down? I think this can help
us decide when we need runtime-resume device.

In general, you can assume that any ndo can be called which are not part of data path (xmit, watchdog, irq/napi),
so the only option is to put netif down and go through every ndo testing.

After leaning all your discussion, from my point of view, it seems not a good choice to add RPM
to net core. It had better handled by driver itself.

-- 
Best regards,
grygorii