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

[Heiner Kallweit <hkallweit1@gmail.com>]

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