Date: Wed,  7 Feb 2024 10:51:11 +0100
From: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
To: netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Johannes Berg <johannes@...solutions.net>,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	Sasha Neftin <sasha.neftin@...el.com>,
	Dima Ruinskiy <dima.ruinskiy@...el.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	"Rafael J . Wysocki " <rafael@...nel.org>
Subject: [PATCH] net: avoid net core runtime resume for most drivers

Introducing runtime resume before ndo_open and ethtool ops by commits:

d43c65b05b84 ("ethtool: runtime-resume netdev parent in ethnl_ops_begin")
bd869245a3dc ("net: core: try to runtime-resume detached device in __dev_open")

caused rtnl_lock -> rtnl_lock deadlock for igc/igb drivers if user enabled
runtime power management by:

echo auto > /sys/bus/pci/devices/{PCI_ID}/power/control

and then use ethtool or open the link, when device is suspended.

The deadlock was reported at few places before, for example:

https://lore.kernel.org/netdev/20211124144505.31e15716@hermes.local/
https://lore.kernel.org/all/20231202221402.GA11155@merlins.org/

and has fix for igb:

ac8c58f5b535 ("igb: fix deadlock caused by taking RTNL in RPM resume path")

However this solution does not take into account various corner cases.
For example it breaks RTNL lock assertion for netif_set_real_num_queues().
Fixing the deadlock issue properly in race free manner in igb/igc drivers
is not easy.

Additionally for other drivers, that fine tune their pm runtime
implementation, runtime resume by net core cause unnecessary wake-ups.
Various ethtool ops do not require HW access and can be done without
resuming device. On dev_open(), we can error exit before ndo_open(),
then not-used device will stay permanently enabled (if not implemented
runtime pm with autosuspend).

So, remove the runtime resume calls from net core.

However, now seems there are some benefits of the calls for r8169
driver, so keep them for it by introducing IFF_DO_RUNTIME_PM priv flag
and use it for r8169.

Fixes: d43c65b05b84 ("ethtool: runtime-resume netdev parent in ethnl_ops_begin")
Fixes: bd869245a3dc ("net: core: try to runtime-resume detached device in __dev_open")
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 2 +-
 include/linux/netdevice.h                 | 1 +
 net/core/dev.c                            | 2 +-
 net/ethtool/ioctl.c                       | 4 ++--
 net/ethtool/netlink.c                     | 6 +++---
 5 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index dd73df6b17b0..f430df3c9e51 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5287,7 +5287,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
 			   NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
 	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
-	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
+	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_DO_RUNTIME_PM;
 
 	/*
 	 * Pretend we are using VLANs; This bypasses a nasty bug where
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 118c40258d07..b149eca32adc 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1768,6 +1768,7 @@ enum netdev_priv_flags {
 	IFF_TX_SKB_NO_LINEAR		= BIT_ULL(31),
 	IFF_CHANGE_PROTO_DOWN		= BIT_ULL(32),
 	IFF_SEE_ALL_HWTSTAMP_REQUESTS	= BIT_ULL(33),
+	IFF_DO_RUNTIME_PM		= BIT_ULL(34),
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
diff --git a/net/core/dev.c b/net/core/dev.c
index cb2dab0feee0..bd9af0469dfa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1420,7 +1420,7 @@ static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
 
 	if (!netif_device_present(dev)) {
 		/* may be detached because parent is runtime-suspended */
-		if (dev->dev.parent)
+		if (dev->priv_flags & IFF_DO_RUNTIME_PM && dev->dev.parent)
 			pm_runtime_resume(dev->dev.parent);
 		if (!netif_device_present(dev))
 			return -ENODEV;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 7519b0818b91..8e424c08de89 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2875,7 +2875,7 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
 			return -EPERM;
 	}
 
-	if (dev->dev.parent)
+	if (dev->priv_flags & IFF_DO_RUNTIME_PM && dev->dev.parent)
 		pm_runtime_get_sync(dev->dev.parent);
 
 	if (!netif_device_present(dev)) {
@@ -3106,7 +3106,7 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
 	if (old_features != dev->features)
 		netdev_features_change(dev);
 out:
-	if (dev->dev.parent)
+	if (dev->priv_flags & IFF_DO_RUNTIME_PM && dev->dev.parent)
 		pm_runtime_put(dev->dev.parent);
 
 	return rc;
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index fe3553f60bf3..089a88a12f7a 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -37,7 +37,7 @@ int ethnl_ops_begin(struct net_device *dev)
 	if (!dev)
 		return -ENODEV;
 
-	if (dev->dev.parent)
+	if (dev->priv_flags & IFF_DO_RUNTIME_PM && dev->dev.parent)
 		pm_runtime_get_sync(dev->dev.parent);
 
 	if (!netif_device_present(dev) ||
@@ -54,7 +54,7 @@ int ethnl_ops_begin(struct net_device *dev)
 
 	return 0;
 err:
-	if (dev->dev.parent)
+	if (dev->priv_flags & IFF_DO_RUNTIME_PM && dev->dev.parent)
 		pm_runtime_put(dev->dev.parent);
 
 	return ret;
@@ -65,7 +65,7 @@ void ethnl_ops_complete(struct net_device *dev)
 	if (dev->ethtool_ops->complete)
 		dev->ethtool_ops->complete(dev);
 
-	if (dev->dev.parent)
+	if (dev->priv_flags & IFF_DO_RUNTIME_PM  && dev->dev.parent)
 		pm_runtime_put(dev->dev.parent);
 }
 
-- 
2.34.1

Powered by blists - more mailing lists