Date:   Thu, 5 Aug 2021 14:58:24 +0300
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Joakim Zhang <qiangqing.zhang@....com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Linux PM list <linux-pm@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next 1/4] ethtool: runtime-resume netdev parent before
 ethtool ioctl ops



On 05/08/2021 14:11, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Grygorii Strashko <grygorii.strashko@...com>
>> Sent: 2021年8月5日 16:21
>> To: Heiner Kallweit <hkallweit1@...il.com>; Jakub Kicinski
>> <kuba@...nel.org>; David Miller <davem@...emloft.net>
>> Cc: netdev@...r.kernel.org; Linux PM list <linux-pm@...r.kernel.org>;
>> Andrew Lunn <andrew@...n.ch>; Florian Fainelli <f.fainelli@...il.com>
>> 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:
>>> On 04.08.2021 10:43, Grygorii Strashko wrote:
>>>>
>>>>
>>>> On 04/08/2021 00:32, Heiner Kallweit wrote:
>>>>> On 03.08.2021 22:41, Grygorii Strashko wrote:
>>>>>>
>>>>>>
>>>>>> On 01/08/2021 13:36, Heiner Kallweit wrote:
>>>>>>> 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@...il.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.
>>>>
>>>> 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.
>>>>
>>>> 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;
>>>>    }
>>>>
>>>>
>>>>>
>>>>>> 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?
>>>>
>>>> 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.
>>>> 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()?
>>>
>>> No, that's not needed.
>>>
>>>> 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.
>>>>
>>>> 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

Powered by blists - more mailing lists