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
ethtool ops in kernel.
This patch series covers the typical case that the netdev parent is power-
managed, e.g. a PCI device. Not sure whether cases exist where the netdev
itself is power-managed. If yes then we may need an extension for this.
But the series as-is at least shouldn't cause problems in that case.
Heiner Kallweit (4):
ethtool: runtime-resume netdev parent before ethtool ioctl ops
ethtool: move implementation of ethnl_ops_begin/complete to netlink.c
ethtool: move netif_device_present check from
ethnl_parse_header_dev_get to ethnl_ops_begin
ethtool: runtime-resume netdev parent in ethnl_ops_begin
net/ethtool/ioctl.c | 18 ++++++++++++++---
net/ethtool/netlink.c | 45 +++++++++++++++++++++++++++++++++++++------
net/ethtool/netlink.h | 15 ++-------------
3 files changed, 56 insertions(+), 22 deletions(-)
--
2.32.0
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(-)
In preparation of subsequent extensions to both functions move the
implementations from netlink.h to netlink.c.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
net/ethtool/netlink.c | 14 ++++++++++++++
net/ethtool/netlink.h | 15 ++-------------
2 files changed, 16 insertions(+), 13 deletions(-)
If device is runtime-suspended and not accessible then it may be
flagged as not present. If checking whether device is present is
done too early then we may bail out before we have the chance to
runtime-resume the device. Therefore move this check to
ethnl_ops_begin(). This is in preparation of a follow-up patch
that tries to runtime-resume the device before executing ethtool
ops.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
net/ethtool/netlink.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
@@ -115,12 +121,6 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,return-EINVAL;}-if(dev&&!netif_device_present(dev)){-dev_put(dev);-NL_SET_ERR_MSG(extack,"device not present");-return-ENODEV;-}-req_info->dev=dev;req_info->flags=flags;return0;
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/netlink.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
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
ethtool ops in kernel.
This patch series covers the typical case that the netdev parent is power-
managed, e.g. a PCI device. Not sure whether cases exist where the netdev
itself is power-managed. If yes then we may need an extension for this.
But the series as-is at least shouldn't cause problems in that case.
Heiner Kallweit (4):
ethtool: runtime-resume netdev parent before ethtool ioctl ops
ethtool: move implementation of ethnl_ops_begin/complete to netlink.c
ethtool: move netif_device_present check from
ethnl_parse_header_dev_get to ethnl_ops_begin
ethtool: runtime-resume netdev parent in ethnl_ops_begin
net/ethtool/ioctl.c | 18 ++++++++++++++---
net/ethtool/netlink.c | 45 +++++++++++++++++++++++++++++++++++++------
net/ethtool/netlink.h | 15 ++-------------
3 files changed, 56 insertions(+), 22 deletions(-)
Patchwork is showing the following warning for all patches in the series.
netdev/cc_maintainers warning 7 maintainers not CCed: ecree@solarflare.com andrew@lunn.ch magnus.karlsson@intel.com danieller@nvidia.com arnd@arndb.de irusskikh@marvell.com alexanderduyck@fb.com
This seems to be a false positive, e.g. address ecree@solarflare.com
doesn't exist at all in MAINTAINERS file.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-02 14:17:20
On Sun, 1 Aug 2021 18:25:52 +0200 Heiner Kallweit wrote:
Patchwork is showing the following warning for all patches in the series.
netdev/cc_maintainers warning 7 maintainers not CCed: ecree@solarflare.com andrew@lunn.ch magnus.karlsson@intel.com danieller@nvidia.com arnd@arndb.de irusskikh@marvell.com alexanderduyck@fb.com
This seems to be a false positive, e.g. address ecree@solarflare.com
doesn't exist at all in MAINTAINERS file.
It gets the list from the get_maintainers script. It's one of the less
reliable tests, but I feel like efforts should be made primarily
towards improving get_maintainers rather than improving the test itself.
On Sun, 1 Aug 2021 18:25:52 +0200 Heiner Kallweit wrote:
quoted
Patchwork is showing the following warning for all patches in the series.
netdev/cc_maintainers warning 7 maintainers not CCed: ecree@solarflare.com andrew@lunn.ch magnus.karlsson@intel.com danieller@nvidia.com arnd@arndb.de irusskikh@marvell.com alexanderduyck@fb.com
This seems to be a false positive, e.g. address ecree@solarflare.com
doesn't exist at all in MAINTAINERS file.
It gets the list from the get_maintainers script. It's one of the less
reliable tests, but I feel like efforts should be made primarily
towards improving get_maintainers rather than improving the test itself.
When running get_maintainers.pl for any of the patches in the series
I don't get these addresses. I run get_maintainers w/o options, maybe
you set some special option? That's what I get when running get_maintainers:
"David S. Miller" [off-list ref] (maintainer:NETWORKING [GENERAL])
Jakub Kicinski [off-list ref] (maintainer:NETWORKING [GENERAL])
Stephen Rothwell [off-list ref] (commit_signer:1/2=50%,authored:1/2=50%,added_lines:3144/3159=100%)
Heiner Kallweit [off-list ref] (commit_signer:1/2=50%,authored:1/2=50%,removed_lines:3/3=100%)
netdev@vger.kernel.org (open list:NETWORKING [GENERAL])
linux-kernel@vger.kernel.org (open list)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-02 16:54:50
On Mon, 2 Aug 2021 18:42:28 +0200 Heiner Kallweit wrote:
On 02.08.2021 16:15, Jakub Kicinski wrote:
quoted
On Sun, 1 Aug 2021 18:25:52 +0200 Heiner Kallweit wrote:
quoted
Patchwork is showing the following warning for all patches in the series.
netdev/cc_maintainers warning 7 maintainers not CCed: ecree@solarflare.com andrew@lunn.ch magnus.karlsson@intel.com danieller@nvidia.com arnd@arndb.de irusskikh@marvell.com alexanderduyck@fb.com
This seems to be a false positive, e.g. address ecree@solarflare.com
doesn't exist at all in MAINTAINERS file.
It gets the list from the get_maintainers script. It's one of the less
reliable tests, but I feel like efforts should be made primarily
towards improving get_maintainers rather than improving the test itself.
When running get_maintainers.pl for any of the patches in the series
I don't get these addresses. I run get_maintainers w/o options, maybe
you set some special option? That's what I get when running get_maintainers:
"David S. Miller" [off-list ref] (maintainer:NETWORKING [GENERAL])
Jakub Kicinski [off-list ref] (maintainer:NETWORKING [GENERAL])
Stephen Rothwell [off-list ref] (commit_signer:1/2=50%,authored:1/2=50%,added_lines:3144/3159=100%)
Heiner Kallweit [off-list ref] (commit_signer:1/2=50%,authored:1/2=50%,removed_lines:3/3=100%)
netdev@vger.kernel.org (open list:NETWORKING [GENERAL])
linux-kernel@vger.kernel.org (open list)
Mm. Maybe your system doesn't have some perl module? Not sure what it
may be. With tip of net-next/master:
$ ./scripts/get_maintainer.pl /tmp/te/0002-ethtool-move-implementation-of-ethnl_ops_begin-compl.patch
"David S. Miller" [off-list ref] (maintainer:NETWORKING [GENERAL],commit_signer:12/16=75%,commit_signer:15/18=83%)
Jakub Kicinski [off-list ref] (maintainer:NETWORKING [GENERAL],commit_signer:11/16=69%,authored:9/16=56%,added_lines:127/198=64%,removed_lines:41/57=72%,commit_signer:14/18=78%,authored:11/18=61%,added_lines:74/84=88%,removed_lines:35/52=67%)
Heiner Kallweit [off-list ref] (commit_signer:3/16=19%,authored:3/16=19%,added_lines:46/198=23%,removed_lines:13/57=23%,authored:1/18=6%,removed_lines:13/52=25%)
Fernando Fernandez Mancera [off-list ref] (commit_signer:1/16=6%,authored:1/16=6%)
Vladyslav Tarasiuk [off-list ref] (commit_signer:1/16=6%,added_lines:11/198=6%,commit_signer:1/18=6%)
Yangbo Lu [off-list ref] (authored:1/16=6%,added_lines:10/198=5%,authored:1/18=6%)
Johannes Berg [off-list ref] (authored:1/16=6%)
Zheng Yongjun [off-list ref] (commit_signer:1/18=6%)
Andrew Lunn [off-list ref] (commit_signer:1/18=6%)
Danielle Ratson [off-list ref] (authored:1/18=6%)
Ido Schimmel [off-list ref] (authored:1/18=6%)
netdev@vger.kernel.org (open list:NETWORKING [GENERAL])
linux-kernel@vger.kernel.org (open list)
On Mon, 2 Aug 2021 18:42:28 +0200 Heiner Kallweit wrote:
quoted
On 02.08.2021 16:15, Jakub Kicinski wrote:
quoted
On Sun, 1 Aug 2021 18:25:52 +0200 Heiner Kallweit wrote:
quoted
Patchwork is showing the following warning for all patches in the series.
netdev/cc_maintainers warning 7 maintainers not CCed: ecree@solarflare.com andrew@lunn.ch magnus.karlsson@intel.com danieller@nvidia.com arnd@arndb.de irusskikh@marvell.com alexanderduyck@fb.com
This seems to be a false positive, e.g. address ecree@solarflare.com
doesn't exist at all in MAINTAINERS file.
It gets the list from the get_maintainers script. It's one of the less
reliable tests, but I feel like efforts should be made primarily
towards improving get_maintainers rather than improving the test itself.
When running get_maintainers.pl for any of the patches in the series
I don't get these addresses. I run get_maintainers w/o options, maybe
you set some special option? That's what I get when running get_maintainers:
"David S. Miller" [off-list ref] (maintainer:NETWORKING [GENERAL])
Jakub Kicinski [off-list ref] (maintainer:NETWORKING [GENERAL])
Stephen Rothwell [off-list ref] (commit_signer:1/2=50%,authored:1/2=50%,added_lines:3144/3159=100%)
Heiner Kallweit [off-list ref] (commit_signer:1/2=50%,authored:1/2=50%,removed_lines:3/3=100%)
netdev@vger.kernel.org (open list:NETWORKING [GENERAL])
linux-kernel@vger.kernel.org (open list)
Mm. Maybe your system doesn't have some perl module? Not sure what it
may be. With tip of net-next/master:
$ ./scripts/get_maintainer.pl /tmp/te/0002-ethtool-move-implementation-of-ethnl_ops_begin-compl.patch
"David S. Miller" [off-list ref] (maintainer:NETWORKING [GENERAL],commit_signer:12/16=75%,commit_signer:15/18=83%)
Jakub Kicinski [off-list ref] (maintainer:NETWORKING [GENERAL],commit_signer:11/16=69%,authored:9/16=56%,added_lines:127/198=64%,removed_lines:41/57=72%,commit_signer:14/18=78%,authored:11/18=61%,added_lines:74/84=88%,removed_lines:35/52=67%)
Heiner Kallweit [off-list ref] (commit_signer:3/16=19%,authored:3/16=19%,added_lines:46/198=23%,removed_lines:13/57=23%,authored:1/18=6%,removed_lines:13/52=25%)
Fernando Fernandez Mancera [off-list ref] (commit_signer:1/16=6%,authored:1/16=6%)
Vladyslav Tarasiuk [off-list ref] (commit_signer:1/16=6%,added_lines:11/198=6%,commit_signer:1/18=6%)
Yangbo Lu [off-list ref] (authored:1/16=6%,added_lines:10/198=5%,authored:1/18=6%)
Johannes Berg [off-list ref] (authored:1/16=6%)
Zheng Yongjun [off-list ref] (commit_signer:1/18=6%)
Andrew Lunn [off-list ref] (commit_signer:1/18=6%)
Danielle Ratson [off-list ref] (authored:1/18=6%)
Ido Schimmel [off-list ref] (authored:1/18=6%)
netdev@vger.kernel.org (open list:NETWORKING [GENERAL])
linux-kernel@vger.kernel.org (open list)
Ah, maybe it's because I typically don't work with the full git repo
but just do a "git clone --depth 1".
Hello:
This series was applied to netdev/net-next.git (refs/heads/master):
On Sun, 1 Aug 2021 12:35:18 +0200 you 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)
[...]
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(-)
@@ -2589,7 +2590,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)intrc;netdev_features_told_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...
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(-)
@@ -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.
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;
}
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(-)
@@ -2589,7 +2590,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)intrc;netdev_features_told_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.
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.
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.
@@ -1963,6 +1963,7 @@ int netdev_register_kobject(struct net_device *ndev)}pm_runtime_set_memalloc_noio(dev,true);+pm_runtime_enable(dev);returnerror;}
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?
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()?
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)
Wouldn't we then need RPM ops for the parent (e.g. PCI) and for netdev->dev?
No. as I know - netdev->dev can be declared as pm_runtime_no_callbacks(&adap->dev);
I2C adapter might be a good example to check.
E.g. the parent runtime-resume can be triggered by a PCI PME, then it would
have to resume netdev->dev.
quoted
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.
I'm not against code refactoring and optimization, but in my opinion it has to be done right from the beginning or
not done at all.
quoted
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.
And if 'parent' is not supporting PM runtime - it, as i see, should be handled by PM runtime core properly.
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.
huh. you can't 'try' pm_runtime_get_sync() and then align on netif_device_present() :(
might be, some how, it will work for r8169_main, but will not work for others.
- no checking pm_runtime_get_sync() err code will cause PM runtime 'usage_count' leak
- no checking pm_runtime_get_sync() err may cause to continue( for TI CPSW for example) with
device in undefined PM state ("disabled" or "half-enabled") and so crash later.
quoted
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 ..
Again, netif_device_detach() seems correct for System wide suspend, but in my opinion - it's
not correct for PM runtime.
Sry, with all do respect, first corresponding driver has to be fixed and not Net core hacked to support it.
Further decisions is up to maintainers.
quoted
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;
}
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(-)
@@ -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.
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.
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.
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()
Same applies for __dev_set_rx_mode(). Therefore I wondered whether cpsw_ndo_set_rx_mode()
- that does not include runtime-resuming the device - may be called when device is
runtime-suspended, e.g. if interface is up, but link is down.
quoted
Wouldn't we then need RPM ops for the parent (e.g. PCI) and for netdev->dev?
No. as I know - netdev->dev can be declared as pm_runtime_no_callbacks(&adap->dev);
I2C adapter might be a good example to check.
quoted
E.g. the parent runtime-resume can be triggered by a PCI PME, then it would
have to resume netdev->dev.
quoted
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.
I'm not against code refactoring and optimization, but in my opinion it has to be done right from the beginning or
not done at all.
quoted
quoted
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.
And if 'parent' is not supporting PM runtime - it, as i see, should be handled by PM runtime core properly.
I agree that checking the return code could make sense, but then we would
quoted
have to be careful which error codes we consider as failed.
huh. you can't 'try' pm_runtime_get_sync() and then align on netif_device_present() :(
might be, some how, it will work for r8169_main, but will not work for others.
- no checking pm_runtime_get_sync() err code will cause PM runtime 'usage_count' leak
- no checking pm_runtime_get_sync() err may cause to continue( for TI CPSW for example) with
device in undefined PM state ("disabled" or "half-enabled") and so crash later.
I'd say 95% of rpm callers don't check the return value. I'm not saying this is a good thing,
but obviously it doesn't cause relevant harm.
quoted
quoted
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 ..
Again, netif_device_detach() seems correct for System wide suspend, but in my opinion - it's
not correct for PM runtime.
Sry, with all do respect, first corresponding driver has to be fixed and not Net core hacked to support it.
Further decisions is up to maintainers.
quoted
quoted
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;
}
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(-)
@@ -2589,7 +2590,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)intrc;netdev_features_told_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.
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.
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.
@@ -1963,6 +1963,7 @@ int netdev_register_kobject(struct net_device *ndev)}pm_runtime_set_memalloc_noio(dev,true);+pm_runtime_enable(dev);returnerror;}
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?
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.
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.
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_filter_info
Same applies for __dev_set_rx_mode(). Therefore I wondered whether cpsw_ndo_set_rx_mode()
- that does not include runtime-resuming the device - may be called when device is
runtime-suspended, e.g. if interface is up, but link is down.
CPSW doesn't manage PM runtime in link status handler, as it has only on/off state and off state can cause full
context loss restore of which is expensive and hard to implement. And for most of netdev drivers no aggressive PM runtime
is implemented exactly because of that (mac/vlan/fdb/mdb/...). Common patterns:
(a)
.probe
-get
.remove
-put
(b)
.probe
-get
-put
.open
-get
.close
-put
.protect places which may be called when netif is down
The CPSW follows (b) and so cpsw_ndo_set_rx_mode() can't be called when when device is
runtime-suspended.
I assume, some hw like PCI, can have more PM states and in some of them keep HW context intact.
quoted
quoted
Wouldn't we then need RPM ops for the parent (e.g. PCI) and for netdev->dev?
No. as I know - netdev->dev can be declared as pm_runtime_no_callbacks(&adap->dev);
I2C adapter might be a good example to check.
quoted
E.g. the parent runtime-resume can be triggered by a PCI PME, then it would
have to resume netdev->dev.
quoted
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.
I'm not against code refactoring and optimization, but in my opinion it has to be done right from the beginning or
not done at all.
quoted
quoted
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.
And if 'parent' is not supporting PM runtime - it, as i see, should be handled by PM runtime core properly.
I agree that checking the return code could make sense, but then we would
quoted
have to be careful which error codes we consider as failed.
huh. you can't 'try' pm_runtime_get_sync() and then align on netif_device_present() :(
might be, some how, it will work for r8169_main, but will not work for others.
- no checking pm_runtime_get_sync() err code will cause PM runtime 'usage_count' leak
No. pm_runtime_get_sync() always bumps the usage count, no matter whether it fails or not.
- no checking pm_runtime_get_sync() err may cause to continue( for TI CPSW for example) with
device in undefined PM state ("disabled" or "half-enabled") and so crash later.
I'd say 95% of rpm callers don't check the return value. I'm not saying this is a good thing,
but obviously it doesn't cause relevant harm.
this is completely wrong assumption as PM errors cause silent stuck, undefined behavior or dumps (sometimes delayed)
which is terribly hard to root cause.
yes. many drivers do not check, but over last few years more and more strict policies applied to avoid that and
in many case no checking return code - is red flag and patch reject.
Don't like that phrase ;), but "It doesn't mean that incorrect code has to be copy-pasted all over the places"
this is correct get pattern for get:
ret = pm_runtime_get_sync(&pdev->dev);
if (ret < 0) {
pm_runtime_put_noidle(&pdev->dev);
return ret;
}
My strong opinion
- PM runtime return code must be checked.
- get rid of netif_device_detach() in r8169
by the way, have you tried below test with your driver (not sure how it works for you):
.rtl_open
- pm_runtime_get_sync
- pm_runtime_put_sync - usage_count == 0
.r8169_phylink_handler
- pm_request_resume - why async? still usage_count == 0
.some ethtool request to go through dev_ethtool()
- pm_runtime_get_sync
- pm_runtime_put - async, usage_count == 0
^ would not it put r8169 in runtime-suspended state while link is still UP?
quoted
quoted
quoted
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 ..
Again, netif_device_detach() seems correct for System wide suspend, but in my opinion - it's
not correct for PM runtime.
Sry, with all do respect, first corresponding driver has to be fixed and not Net core hacked to support it.
Further decisions is up to maintainers.
quoted
quoted
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;
}
-----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
@@ -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.
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
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.
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.
quoted
Same applies for __dev_set_rx_mode(). Therefore I wondered whether
cpsw_ndo_set_rx_mode()
- that does not include runtime-resuming the device - may be called
when device is runtime-suspended, e.g. if interface is up, but link is down.
CPSW doesn't manage PM runtime in link status handler, as it has only on/off
state and off state can cause full context loss restore of which is expensive and
hard to implement. And for most of netdev drivers no aggressive PM runtime is
implemented exactly because of that (mac/vlan/fdb/mdb/...). Common
patterns:
(a)
.probe
-get
.remove
-put
(b)
.probe
-get
-put
.open
-get
.close
-put
.protect places which may be called when netif is down
The CPSW follows (b) and so cpsw_ndo_set_rx_mode() can't be called when
when device is runtime-suspended.
I assume, some hw like PCI, can have more PM states and in some of them
keep HW context intact.
quoted
quoted
quoted
Wouldn't we then need RPM ops for the parent (e.g. PCI) and for
netdev->dev?
quoted
quoted
No. as I know - netdev->dev can be declared as
pm_runtime_no_callbacks(&adap->dev);
I2C adapter might be a good example to check.
quoted
E.g. the parent runtime-resume can be triggered by a PCI PME, then
it would have to resume netdev->dev.
quoted
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.
I'm not against code refactoring and optimization, but in my opinion
it has to be done right from the beginning or not done at all.
quoted
quoted
at minimum it might be tricky and required very careful approach
(especially in err path).
quoted
quoted
quoted
quoted
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.
quoted
quoted
quoted
The pm_runtime_get_sync() calls are attempts here. We don't want to
bail out if a device doesn't support RPM.
And if 'parent' is not supporting PM runtime - it, as i see, should be handled
by PM runtime core properly.
quoted
quoted
I agree that checking the return code could make sense, but then we
would
quoted
have to be careful which error codes we consider as failed.
huh. you can't 'try' pm_runtime_get_sync() and then align on
netif_device_present() :(
might be, some how, it will work for r8169_main, but will not work for
others.
quoted
quoted
- no checking pm_runtime_get_sync() err code will cause PM runtime
'usage_count' leak
No. pm_runtime_get_sync() always bumps the usage count, no matter
whether it fails or not.
quoted
This makes it easy to deal with this. The problem you describe exists
with pm_runtime_resume_and_get(). That's why I wondered whether we
should annotate this function as __must_check. See here:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
.kernel.org%2Flinux-pm%2FCAJZ5v0gps0C2923VqM8876npvhcETsyN%2BajAk
- no checking pm_runtime_get_sync() err may cause to continue( for TI
CPSW for example) with
device in undefined PM state ("disabled" or "half-enabled") and so crash
later.
quoted
quoted
I'd say 95% of rpm callers don't check the return value. I'm not
saying this is a good thing, but obviously it doesn't cause relevant harm.
this is completely wrong assumption as PM errors cause silent stuck, undefined
behavior or dumps (sometimes delayed) which is terribly hard to root cause.
yes. many drivers do not check, but over last few years more and more strict
policies applied to avoid that and in many case no checking return code - is red
flag and patch reject.
Don't like that phrase ;), but "It doesn't mean that incorrect code has to be
copy-pasted all over the places"
this is correct get pattern for get:
ret = pm_runtime_get_sync(&pdev->dev);
if (ret < 0) {
pm_runtime_put_noidle(&pdev->dev);
return ret;
}
Yes, it’s the correct usage for pm_runtime_get_sync.
Best Regards,
Joakim Zhang
My strong opinion
- PM runtime return code must be checked.
- get rid of netif_device_detach() in r8169
by the way, have you tried below test with your driver (not sure how it works
for you):
.rtl_open
- pm_runtime_get_sync
- pm_runtime_put_sync - usage_count == 0 .r8169_phylink_handler
- pm_request_resume - why async? still usage_count == 0 .some ethtool
request to go through dev_ethtool()
- pm_runtime_get_sync
- pm_runtime_put - async, usage_count == 0
^ would not it put r8169 in runtime-suspended state while link is still UP?
quoted
quoted
quoted
quoted
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.
quoted
quoted
quoted
quoted
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 ..
quoted
quoted
Again, netif_device_detach() seems correct for System wide suspend,
but in my opinion - it's not correct for PM runtime.
Sry, with all do respect, first corresponding driver has to be fixed and not
Net core hacked to support it.
quoted
quoted
Further decisions is up to maintainers.
quoted
quoted
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;
}
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/netlink.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
[...]
void ethnl_ops_complete(struct net_device *dev)
{
if (dev && dev->ethtool_ops->complete)
dev->ethtool_ops->complete(dev);
+
+ if (dev->dev.parent)
+ pm_runtime_put(dev->dev.parent);
}
/**
Hello Heiner,
Coverity complains that we checked dev != NULL earlier but now
unconditionally dereference it:
*** CID 1506213: Null pointer dereferences (FORWARD_NULL)
/net/ethtool/netlink.c: 67 in ethnl_ops_complete()
61
62 void ethnl_ops_complete(struct net_device *dev)
63 {
64 if (dev && dev->ethtool_ops->complete)
65 dev->ethtool_ops->complete(dev);
66
-----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
@@ -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.
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
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.
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/netlink.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
[...]
quoted
void ethnl_ops_complete(struct net_device *dev)
{
if (dev && dev->ethtool_ops->complete)
dev->ethtool_ops->complete(dev);
+
+ if (dev->dev.parent)
+ pm_runtime_put(dev->dev.parent);
}
/**
Hello Heiner,
Coverity complains that we checked dev != NULL earlier but now
unconditionally dereference it:
Thanks for the hint. I wonder whether we have any valid case where
dev could be NULL. There are several places where dev is dereferenced
after the call to ethnl_ops_begin(). Just one example:
linkmodes_prepare_data()
Only ethnl_request_ops where allow_nodev_do is true is
ethnl_strset_request_ops. However in strset_prepare_data()
ethnl_ops_begin() is called only if dev isn't NULL.
Supposedly we should return an error from ethnl_ops_begin()
if dev is NULL.
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(-)
@@ -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.
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.
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.
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.
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.
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_filter_info
quoted
Same applies for __dev_set_rx_mode(). Therefore I wondered whether cpsw_ndo_set_rx_mode()
- that does not include runtime-resuming the device - may be called when device is
runtime-suspended, e.g. if interface is up, but link is down.
CPSW doesn't manage PM runtime in link status handler, as it has only on/off state and off state can cause full
context loss restore of which is expensive and hard to implement. And for most of netdev drivers no aggressive PM runtime
is implemented exactly because of that (mac/vlan/fdb/mdb/...). Common patterns:
(a)
.probe
-get
.remove
-put
(b)
.probe
-get
-put
.open
-get
.close
-put
.protect places which may be called when netif is down
The CPSW follows (b) and so cpsw_ndo_set_rx_mode() can't be called when when device is
runtime-suspended.
I assume, some hw like PCI, can have more PM states and in some of them keep HW context intact.
Exactly, there's no reason to keep PCI in D0 if link is down. Once NIC detects a cable was
plugged in it triggers a PCI PME and PCI core sets PCI bus from D3cold/D3hot to D0 and
runtime-resumes device.
quoted
quoted
quoted
Wouldn't we then need RPM ops for the parent (e.g. PCI) and for netdev->dev?
No. as I know - netdev->dev can be declared as pm_runtime_no_callbacks(&adap->dev);
I2C adapter might be a good example to check.
quoted
E.g. the parent runtime-resume can be triggered by a PCI PME, then it would
have to resume netdev->dev.
quoted
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.
I'm not against code refactoring and optimization, but in my opinion it has to be done right from the beginning or
not done at all.
quoted
quoted
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.
And if 'parent' is not supporting PM runtime - it, as i see, should be handled by PM runtime core properly.
I agree that checking the return code could make sense, but then we would
quoted
have to be careful which error codes we consider as failed.
huh. you can't 'try' pm_runtime_get_sync() and then align on netif_device_present() :(
might be, some how, it will work for r8169_main, but will not work for others.
- no checking pm_runtime_get_sync() err code will cause PM runtime 'usage_count' leak
No. pm_runtime_get_sync() always bumps the usage count, no matter whether it fails or not.
- no checking pm_runtime_get_sync() err may cause to continue( for TI CPSW for example) with
device in undefined PM state ("disabled" or "half-enabled") and so crash later.
I'd say 95% of rpm callers don't check the return value. I'm not saying this is a good thing,
but obviously it doesn't cause relevant harm.
this is completely wrong assumption as PM errors cause silent stuck, undefined behavior or dumps (sometimes delayed)
which is terribly hard to root cause.
yes. many drivers do not check, but over last few years more and more strict policies applied to avoid that and
in many case no checking return code - is red flag and patch reject.
Don't like that phrase ;), but "It doesn't mean that incorrect code has to be copy-pasted all over the places"
this is correct get pattern for get:
ret = pm_runtime_get_sync(&pdev->dev);
if (ret < 0) {
pm_runtime_put_noidle(&pdev->dev);
return ret;
}
That's exactly what pm_runtime_resume_and_get() does. IIRC this helper hasn't been
part of the API from the beginning and was added later.
My strong opinion
- PM runtime return code must be checked.
- get rid of netif_device_detach() in r8169
by the way, have you tried below test with your driver (not sure how it works for you):
.rtl_open
- pm_runtime_get_sync
- pm_runtime_put_sync - usage_count == 0
.r8169_phylink_handler
- pm_request_resume - why async? still usage_count == 0
pm_request_resume() is only meant to cancel a potentially scheduled runtime-suspend
if link has a short drop. In such a case link would be up again after ~ 3-4s,
timeout for runtime-suspending device after link drop is 10s.
.some ethtool request to go through dev_ethtool()
- pm_runtime_get_sync
- pm_runtime_put - async, usage_count == 0
^ would not it put r8169 in runtime-suspended state while link is still UP?
No, see rtl8169_runtime_idle(). If link is up no runtime suspend is scheduled.
quoted
quoted
quoted
quoted
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 ..
Again, netif_device_detach() seems correct for System wide suspend, but in my opinion - it's
not correct for PM runtime.
Sry, with all do respect, first corresponding driver has to be fixed and not Net core hacked to support it.
Further decisions is up to maintainers.
quoted
quoted
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;
}
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(-)
@@ -2589,7 +2590,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)intrc;netdev_features_told_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.
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.
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.
@@ -1963,6 +1963,7 @@ int netdev_register_kobject(struct net_device *ndev)}pm_runtime_set_memalloc_noio(dev,true);+pm_runtime_enable(dev);returnerror;}
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?
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.
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.
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_filter_info
quoted
Same applies for __dev_set_rx_mode(). Therefore I wondered whether cpsw_ndo_set_rx_mode()
- that does not include runtime-resuming the device - may be called when device is
runtime-suspended, e.g. if interface is up, but link is down.
CPSW doesn't manage PM runtime in link status handler, as it has only on/off state and off state can cause full
context loss restore of which is expensive and hard to implement. And for most of netdev drivers no aggressive PM runtime
is implemented exactly because of that (mac/vlan/fdb/mdb/...). Common patterns:
(a)
.probe
-get
.remove
-put
(b)
.probe
-get
-put
.open
-get
.close
-put
.protect places which may be called when netif is down
The CPSW follows (b) and so cpsw_ndo_set_rx_mode() can't be called when when device is
runtime-suspended.
I assume, some hw like PCI, can have more PM states and in some of them keep HW context intact.
Exactly, there's no reason to keep PCI in D0 if link is down. Once NIC detects a cable was
plugged in it triggers a PCI PME and PCI core sets PCI bus from D3cold/D3hot to D0 and
runtime-resumes device.
quoted
quoted
quoted
quoted
Wouldn't we then need RPM ops for the parent (e.g. PCI) and for netdev->dev?
No. as I know - netdev->dev can be declared as pm_runtime_no_callbacks(&adap->dev);
I2C adapter might be a good example to check.
quoted
E.g. the parent runtime-resume can be triggered by a PCI PME, then it would
have to resume netdev->dev.
quoted
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.
I'm not against code refactoring and optimization, but in my opinion it has to be done right from the beginning or
not done at all.
quoted
quoted
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.
And if 'parent' is not supporting PM runtime - it, as i see, should be handled by PM runtime core properly.
I agree that checking the return code could make sense, but then we would
quoted
have to be careful which error codes we consider as failed.
huh. you can't 'try' pm_runtime_get_sync() and then align on netif_device_present() :(
might be, some how, it will work for r8169_main, but will not work for others.
- no checking pm_runtime_get_sync() err code will cause PM runtime 'usage_count' leak
No. pm_runtime_get_sync() always bumps the usage count, no matter whether it fails or not.
- no checking pm_runtime_get_sync() err may cause to continue( for TI CPSW for example) with
device in undefined PM state ("disabled" or "half-enabled") and so crash later.
I'd say 95% of rpm callers don't check the return value. I'm not saying this is a good thing,
but obviously it doesn't cause relevant harm.
this is completely wrong assumption as PM errors cause silent stuck, undefined behavior or dumps (sometimes delayed)
which is terribly hard to root cause.
yes. many drivers do not check, but over last few years more and more strict policies applied to avoid that and
in many case no checking return code - is red flag and patch reject.
Don't like that phrase ;), but "It doesn't mean that incorrect code has to be copy-pasted all over the places"
this is correct get pattern for get:
ret = pm_runtime_get_sync(&pdev->dev);
if (ret < 0) {
pm_runtime_put_noidle(&pdev->dev);
return ret;
}
That's exactly what pm_runtime_resume_and_get() does. IIRC this helper hasn't been
part of the API from the beginning and was added later.
quoted
My strong opinion
- PM runtime return code must be checked.
- get rid of netif_device_detach() in r8169
by the way, have you tried below test with your driver (not sure how it works for you):
.rtl_open
- pm_runtime_get_sync
- pm_runtime_put_sync - usage_count == 0
.r8169_phylink_handler
- pm_request_resume - why async? still usage_count == 0
pm_request_resume() is only meant to cancel a potentially scheduled runtime-suspend
if link has a short drop. In such a case link would be up again after ~ 3-4s,
timeout for runtime-suspending device after link drop is 10s.
quoted
.some ethtool request to go through dev_ethtool()
- pm_runtime_get_sync
- pm_runtime_put - async, usage_count == 0
^ would not it put r8169 in runtime-suspended state while link is still UP?
No, see rtl8169_runtime_idle(). If link is up no runtime suspend is scheduled.
really :) This one
static int rtl8169_runtime_idle(struct device *device)
{
struct rtl8169_private *tp = dev_get_drvdata(device);
if (!netif_running(tp->dev) || !netif_carrier_ok(tp->dev))
pm_schedule_suspend(device, 10000);
return -EBUSY;
}
Sry, but you really need to take pause and rump up on PM runtime, with all do respect.
Pay attention on PM runtime autosupend, so you can do properly get_sync on link up and use autosuspend on Link down.
Hope PM people can comment here also.
--
Best regards,
grygorii