From bd869245a3dcca1c8a77dfade7d3dc69a68365df Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Sat, 20 Jun 2020 22:35:42 +0200 Subject: [PATCH 1/7] net: core: try to runtime-resume detached device in __dev_open A netdevice may be marked as detached because the parent is runtime-suspended and not accessible whilst interface or link is down. An example are PCI network devices that go into PCI D3hot, see e.g. __igc_shutdown() or rtl8169_net_suspend(). If netdevice is down and marked as detached we can only open it if we runtime-resume it before __dev_open() calls netif_device_present(). Therefore, if netdevice is detached, try to runtime-resume the parent and only return with an error if it's still detached. Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- net/core/dev.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index cea7fc1716caf..c5ec4d50acd14 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -143,6 +143,7 @@ #include #include #include +#include #include "net-sysfs.h" @@ -1492,8 +1493,13 @@ static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack) ASSERT_RTNL(); - if (!netif_device_present(dev)) - return -ENODEV; + if (!netif_device_present(dev)) { + /* may be detached because parent is runtime-suspended */ + if (dev->dev.parent) + pm_runtime_resume(dev->dev.parent); + if (!netif_device_present(dev)) + return -ENODEV; + } /* Block netpoll from trying to do any rx path servicing. * If we don't do this there is a chance ndo_poll_controller From 476c4f5de3689a39a097ad20120ca5653a52dad4 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Sat, 20 Jun 2020 22:36:26 +0200 Subject: [PATCH 2/7] r8169: mark device as not present when in PCI D3 Mark the netdevice as detached whenever we go into PCI D3hot. This allows to remove some checks e.g. from ethtool ops because dev_ethtool() checks for netif_device_present() in the beginning. In this context move waking up the queue out of rtl_reset_work() because in cases where netif_device_attach() is called afterwards the queue should be woken up by the latter function only. Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/ethernet/realtek/r8169_main.c | 25 +++++++++++++---------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index a3c4187d918bc..ec724e69931d5 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -3977,10 +3977,9 @@ static void rtl8169_cleanup(struct rtl8169_private *tp, bool going_down) static void rtl_reset_work(struct rtl8169_private *tp) { - struct net_device *dev = tp->dev; int i; - netif_stop_queue(dev); + netif_stop_queue(tp->dev); rtl8169_cleanup(tp, false); @@ -3989,7 +3988,6 @@ static void rtl_reset_work(struct rtl8169_private *tp) napi_enable(&tp->napi); rtl_hw_start(tp); - netif_wake_queue(dev); } static void rtl8169_tx_timeout(struct net_device *dev, unsigned int txqueue) @@ -4574,8 +4572,10 @@ static void rtl_task(struct work_struct *work) !test_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags)) goto out_unlock; - if (test_and_clear_bit(RTL_FLAG_TASK_RESET_PENDING, tp->wk.flags)) + if (test_and_clear_bit(RTL_FLAG_TASK_RESET_PENDING, tp->wk.flags)) { rtl_reset_work(tp); + netif_wake_queue(tp->dev); + } out_unlock: rtl_unlock_work(tp); } @@ -4820,11 +4820,10 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats) static void rtl8169_net_suspend(struct rtl8169_private *tp) { - if (!netif_running(tp->dev)) - return; - netif_device_detach(tp->dev); - rtl8169_down(tp); + + if (netif_running(tp->dev)) + rtl8169_down(tp); } #ifdef CONFIG_PM @@ -4840,8 +4839,6 @@ static int __maybe_unused rtl8169_suspend(struct device *device) static void __rtl8169_resume(struct rtl8169_private *tp) { - netif_device_attach(tp->dev); - rtl_pll_power_up(tp); rtl8169_init_phy(tp); @@ -4863,6 +4860,8 @@ static int __maybe_unused rtl8169_resume(struct device *device) if (netif_running(tp->dev)) __rtl8169_resume(tp); + netif_device_attach(tp->dev); + return 0; } @@ -4870,8 +4869,10 @@ static int rtl8169_runtime_suspend(struct device *device) { struct rtl8169_private *tp = dev_get_drvdata(device); - if (!tp->TxDescArray) + if (!tp->TxDescArray) { + netif_device_detach(tp->dev); return 0; + } rtl_lock_work(tp); __rtl8169_set_wol(tp, WAKE_PHY); @@ -4895,6 +4896,8 @@ static int rtl8169_runtime_resume(struct device *device) if (tp->TxDescArray) __rtl8169_resume(tp); + netif_device_attach(tp->dev); + return 0; } From ec2f204bddb5f9b7819507b9b5df5ca6197ce912 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Sat, 20 Jun 2020 22:37:01 +0200 Subject: [PATCH 3/7] r8169: remove no longer needed checks for device being runtime-active Because the netdevice is marked as detached now when parent is not accessible we can remove quite some checks. Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/ethernet/realtek/r8169_main.c | 54 +++-------------------- 1 file changed, 6 insertions(+), 48 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index ec724e69931d5..81a1cea0989d6 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -1422,24 +1422,17 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts) static int rtl8169_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { struct rtl8169_private *tp = netdev_priv(dev); - struct device *d = tp_to_dev(tp); if (wol->wolopts & ~WAKE_ANY) return -EINVAL; - pm_runtime_get_noresume(d); - rtl_lock_work(tp); tp->saved_wolopts = wol->wolopts; - - if (pm_runtime_active(d)) - __rtl8169_set_wol(tp, tp->saved_wolopts); + __rtl8169_set_wol(tp, tp->saved_wolopts); rtl_unlock_work(tp); - pm_runtime_put_noidle(d); - return 0; } @@ -1657,17 +1650,10 @@ static void rtl8169_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *stats, u64 *data) { struct rtl8169_private *tp = netdev_priv(dev); - struct device *d = tp_to_dev(tp); - struct rtl8169_counters *counters = tp->counters; - - ASSERT_RTNL(); - - pm_runtime_get_noresume(d); - - if (pm_runtime_active(d)) - rtl8169_update_counters(tp); + struct rtl8169_counters *counters; - pm_runtime_put_noidle(d); + counters = tp->counters; + rtl8169_update_counters(tp); data[0] = le64_to_cpu(counters->tx_packets); data[1] = le64_to_cpu(counters->rx_packets); @@ -1899,48 +1885,26 @@ static int rtl_set_coalesce(struct net_device *dev, struct ethtool_coalesce *ec) static int rtl8169_get_eee(struct net_device *dev, struct ethtool_eee *data) { struct rtl8169_private *tp = netdev_priv(dev); - struct device *d = tp_to_dev(tp); - int ret; if (!rtl_supports_eee(tp)) return -EOPNOTSUPP; - pm_runtime_get_noresume(d); - - if (!pm_runtime_active(d)) { - ret = -EOPNOTSUPP; - } else { - ret = phy_ethtool_get_eee(tp->phydev, data); - } - - pm_runtime_put_noidle(d); - - return ret; + return phy_ethtool_get_eee(tp->phydev, data); } static int rtl8169_set_eee(struct net_device *dev, struct ethtool_eee *data) { struct rtl8169_private *tp = netdev_priv(dev); - struct device *d = tp_to_dev(tp); int ret; if (!rtl_supports_eee(tp)) return -EOPNOTSUPP; - pm_runtime_get_noresume(d); - - if (!pm_runtime_active(d)) { - ret = -EOPNOTSUPP; - goto out; - } - ret = phy_ethtool_set_eee(tp->phydev, data); if (!ret) tp->eee_adv = phy_read_mmd(dev->phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV); -out: - pm_runtime_put_noidle(d); return ret; } @@ -2219,19 +2183,13 @@ static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr) static int rtl_set_mac_address(struct net_device *dev, void *p) { struct rtl8169_private *tp = netdev_priv(dev); - struct device *d = tp_to_dev(tp); int ret; ret = eth_mac_addr(dev, p); if (ret) return ret; - pm_runtime_get_noresume(d); - - if (pm_runtime_active(d)) - rtl_rar_set(tp, dev->dev_addr); - - pm_runtime_put_noidle(d); + rtl_rar_set(tp, dev->dev_addr); return 0; } From 567ca57faa6266da931679fd4c4eb9863855f804 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Sat, 20 Jun 2020 22:37:50 +0200 Subject: [PATCH 4/7] r8169: add rtl8169_up Factor out bringing device up to a new function rtl8169_up(), similar to rtl8169_down() for bringing the device down. Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/ethernet/realtek/r8169_main.c | 48 ++++++++--------------- 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 81a1cea0989d6..4d3e7db94bdd9 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -4613,6 +4613,19 @@ static void rtl8169_down(struct rtl8169_private *tp) rtl_unlock_work(tp); } +static void rtl8169_up(struct rtl8169_private *tp) +{ + rtl_lock_work(tp); + rtl_pll_power_up(tp); + rtl8169_init_phy(tp); + napi_enable(&tp->napi); + set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); + rtl_reset_work(tp); + + phy_start(tp->phydev); + rtl_unlock_work(tp); +} + static int rtl8169_close(struct net_device *dev) { struct rtl8169_private *tp = netdev_priv(dev); @@ -4688,25 +4701,10 @@ static int rtl_open(struct net_device *dev) if (retval) goto err_free_irq; - rtl_lock_work(tp); - - set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); - - napi_enable(&tp->napi); - - rtl8169_init_phy(tp); - - rtl_pll_power_up(tp); - - rtl_hw_start(tp); - + rtl8169_up(tp); rtl8169_init_counter_offsets(tp); - - phy_start(tp->phydev); netif_start_queue(dev); - rtl_unlock_work(tp); - pm_runtime_put_sync(&pdev->dev); out: return retval; @@ -4795,20 +4793,6 @@ static int __maybe_unused rtl8169_suspend(struct device *device) return 0; } -static void __rtl8169_resume(struct rtl8169_private *tp) -{ - rtl_pll_power_up(tp); - rtl8169_init_phy(tp); - - phy_start(tp->phydev); - - rtl_lock_work(tp); - napi_enable(&tp->napi); - set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags); - rtl_reset_work(tp); - rtl_unlock_work(tp); -} - static int __maybe_unused rtl8169_resume(struct device *device) { struct rtl8169_private *tp = dev_get_drvdata(device); @@ -4816,7 +4800,7 @@ static int __maybe_unused rtl8169_resume(struct device *device) rtl_rar_set(tp, tp->dev->dev_addr); if (netif_running(tp->dev)) - __rtl8169_resume(tp); + rtl8169_up(tp); netif_device_attach(tp->dev); @@ -4852,7 +4836,7 @@ static int rtl8169_runtime_resume(struct device *device) rtl_unlock_work(tp); if (tp->TxDescArray) - __rtl8169_resume(tp); + rtl8169_up(tp); netif_device_attach(tp->dev); From abe5fc42f9ce942c96d50bf6b44886b70d5759ec Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Sat, 20 Jun 2020 22:38:24 +0200 Subject: [PATCH 5/7] r8169: use RTNL to protect critical sections Most relevant ops (open, close, ethtool ops) are protected with RTNL lock by net core. Make sure that such ops can't be interrupted by e.g. (runtime-)suspending by taking the RTNL lock in suspend ops and the PCI error handler. Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/ethernet/realtek/r8169_main.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 4d3e7db94bdd9..5a6d53fe8f86c 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -4524,6 +4524,7 @@ static void rtl_task(struct work_struct *work) struct rtl8169_private *tp = container_of(work, struct rtl8169_private, wk.work); + rtnl_lock(); rtl_lock_work(tp); if (!netif_running(tp->dev) || @@ -4536,6 +4537,7 @@ static void rtl_task(struct work_struct *work) } out_unlock: rtl_unlock_work(tp); + rtnl_unlock(); } static int rtl8169_poll(struct napi_struct *napi, int budget) @@ -4788,7 +4790,9 @@ static int __maybe_unused rtl8169_suspend(struct device *device) { struct rtl8169_private *tp = dev_get_drvdata(device); + rtnl_lock(); rtl8169_net_suspend(tp); + rtnl_unlock(); return 0; } @@ -4816,11 +4820,13 @@ static int rtl8169_runtime_suspend(struct device *device) return 0; } + rtnl_lock(); rtl_lock_work(tp); __rtl8169_set_wol(tp, WAKE_PHY); rtl_unlock_work(tp); rtl8169_net_suspend(tp); + rtnl_unlock(); return 0; } @@ -4882,7 +4888,9 @@ static void rtl_shutdown(struct pci_dev *pdev) { struct rtl8169_private *tp = pci_get_drvdata(pdev); + rtnl_lock(); rtl8169_net_suspend(tp); + rtnl_unlock(); /* Restore original MAC address */ rtl_rar_set(tp, tp->dev->perm_addr); From 06a14ab852fbd33b237b1bbe6515e9cbd8e3800e Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Sat, 20 Jun 2020 22:38:55 +0200 Subject: [PATCH 6/7] r8169: remove driver-specific mutex Now that the critical sections are protected with RTNL lock, we don't need a separate mutex any longer. Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/ethernet/realtek/r8169_main.c | 45 ----------------------- 1 file changed, 45 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 5a6d53fe8f86c..154488202f0f6 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -611,7 +611,6 @@ struct rtl8169_private { struct { DECLARE_BITMAP(flags, RTL_FLAG_MAX); - struct mutex mutex; struct work_struct work; } wk; @@ -663,16 +662,6 @@ static inline struct device *tp_to_dev(struct rtl8169_private *tp) return &tp->pci_dev->dev; } -static void rtl_lock_work(struct rtl8169_private *tp) -{ - mutex_lock(&tp->wk.mutex); -} - -static void rtl_unlock_work(struct rtl8169_private *tp) -{ - mutex_unlock(&tp->wk.mutex); -} - static void rtl_lock_config_regs(struct rtl8169_private *tp) { RTL_W8(tp, Cfg9346, Cfg9346_Lock); @@ -1348,10 +1337,8 @@ static void rtl8169_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { struct rtl8169_private *tp = netdev_priv(dev); - rtl_lock_work(tp); wol->supported = WAKE_ANY; wol->wolopts = tp->saved_wolopts; - rtl_unlock_work(tp); } static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts) @@ -1426,13 +1413,9 @@ static int rtl8169_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) if (wol->wolopts & ~WAKE_ANY) return -EINVAL; - rtl_lock_work(tp); - tp->saved_wolopts = wol->wolopts; __rtl8169_set_wol(tp, tp->saved_wolopts); - rtl_unlock_work(tp); - return 0; } @@ -1495,8 +1478,6 @@ static int rtl8169_set_features(struct net_device *dev, { struct rtl8169_private *tp = netdev_priv(dev); - rtl_lock_work(tp); - rtl_set_rx_config_features(tp, features); if (features & NETIF_F_RXCSUM) @@ -1514,8 +1495,6 @@ static int rtl8169_set_features(struct net_device *dev, RTL_W16(tp, CPlusCmd, tp->cp_cmd); rtl_pci_commit(tp); - rtl_unlock_work(tp); - return 0; } @@ -1541,10 +1520,8 @@ static void rtl8169_get_regs(struct net_device *dev, struct ethtool_regs *regs, u32 *dw = p; int i; - rtl_lock_work(tp); for (i = 0; i < R8169_REGS_SIZE; i += 4) memcpy_fromio(dw++, data++, 4); - rtl_unlock_work(tp); } static const char rtl8169_gstrings[][ETH_GSTRING_LEN] = { @@ -1860,8 +1837,6 @@ static int rtl_set_coalesce(struct net_device *dev, struct ethtool_coalesce *ec) units = DIV_ROUND_UP(ec->rx_coalesce_usecs * 1000U, scale); w |= FIELD_PREP(RTL_COALESCE_RX_USECS, units); - rtl_lock_work(tp); - RTL_W16(tp, IntrMitigate, w); /* Meaning of PktCntrDisable bit changed from RTL8168e-vl */ @@ -1877,8 +1852,6 @@ static int rtl_set_coalesce(struct net_device *dev, struct ethtool_coalesce *ec) RTL_W16(tp, CPlusCmd, tp->cp_cmd); rtl_pci_commit(tp); - rtl_unlock_work(tp); - return 0; } @@ -2162,8 +2135,6 @@ static void rtl8169_init_phy(struct rtl8169_private *tp) static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr) { - rtl_lock_work(tp); - rtl_unlock_config_regs(tp); RTL_W32(tp, MAC4, addr[4] | addr[5] << 8); @@ -2176,8 +2147,6 @@ static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr) rtl_rar_exgmac_set(tp, addr); rtl_lock_config_regs(tp); - - rtl_unlock_work(tp); } static int rtl_set_mac_address(struct net_device *dev, void *p) @@ -4525,7 +4494,6 @@ static void rtl_task(struct work_struct *work) container_of(work, struct rtl8169_private, wk.work); rtnl_lock(); - rtl_lock_work(tp); if (!netif_running(tp->dev) || !test_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags)) @@ -4536,7 +4504,6 @@ static void rtl_task(struct work_struct *work) netif_wake_queue(tp->dev); } out_unlock: - rtl_unlock_work(tp); rtnl_unlock(); } @@ -4599,8 +4566,6 @@ static int r8169_phy_connect(struct rtl8169_private *tp) static void rtl8169_down(struct rtl8169_private *tp) { - rtl_lock_work(tp); - /* Clear all task flags */ bitmap_zero(tp->wk.flags, RTL_FLAG_MAX); @@ -4611,13 +4576,10 @@ static void rtl8169_down(struct rtl8169_private *tp) rtl8169_cleanup(tp, true); rtl_pll_power_down(tp); - - rtl_unlock_work(tp); } static void rtl8169_up(struct rtl8169_private *tp) { - rtl_lock_work(tp); rtl_pll_power_up(tp); rtl8169_init_phy(tp); napi_enable(&tp->napi); @@ -4625,7 +4587,6 @@ static void rtl8169_up(struct rtl8169_private *tp) rtl_reset_work(tp); phy_start(tp->phydev); - rtl_unlock_work(tp); } static int rtl8169_close(struct net_device *dev) @@ -4821,10 +4782,7 @@ static int rtl8169_runtime_suspend(struct device *device) } rtnl_lock(); - rtl_lock_work(tp); __rtl8169_set_wol(tp, WAKE_PHY); - rtl_unlock_work(tp); - rtl8169_net_suspend(tp); rtnl_unlock(); @@ -4837,9 +4795,7 @@ static int rtl8169_runtime_resume(struct device *device) rtl_rar_set(tp, tp->dev->dev_addr); - rtl_lock_work(tp); __rtl8169_set_wol(tp, tp->saved_wolopts); - rtl_unlock_work(tp); if (tp->TxDescArray) rtl8169_up(tp); @@ -5296,7 +5252,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) return rc; } - mutex_init(&tp->wk.mutex); INIT_WORK(&tp->wk.work, rtl_task); u64_stats_init(&tp->rx_stats.syncp); u64_stats_init(&tp->tx_stats.syncp); From 288302dab34ece4993ea3dd011299f406b78f237 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Sat, 20 Jun 2020 22:39:35 +0200 Subject: [PATCH 7/7] r8169: improve rtl8169_runtime_resume Simplify rtl8169_runtime_resume() by calling rtl8169_resume(). Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/ethernet/realtek/r8169_main.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 154488202f0f6..7e2a62a9ed619 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -4758,13 +4758,13 @@ static int __maybe_unused rtl8169_suspend(struct device *device) return 0; } -static int __maybe_unused rtl8169_resume(struct device *device) +static int rtl8169_resume(struct device *device) { struct rtl8169_private *tp = dev_get_drvdata(device); rtl_rar_set(tp, tp->dev->dev_addr); - if (netif_running(tp->dev)) + if (tp->TxDescArray) rtl8169_up(tp); netif_device_attach(tp->dev); @@ -4793,16 +4793,9 @@ static int rtl8169_runtime_resume(struct device *device) { struct rtl8169_private *tp = dev_get_drvdata(device); - rtl_rar_set(tp, tp->dev->dev_addr); - __rtl8169_set_wol(tp, tp->saved_wolopts); - if (tp->TxDescArray) - rtl8169_up(tp); - - netif_device_attach(tp->dev); - - return 0; + return rtl8169_resume(device); } static int rtl8169_runtime_idle(struct device *device)