[PATCH net-next 0/3] r8169: improve runtime pm

[PATCH net-next 0/3] r8169: improve runtime pm

[Heiner Kallweit]

On my system with two network ports I found that runtime PM didn't
suspend the unused port. Therefore I checked runtime pm in this driver
in somewhat more detail and this series improves runtime pm in general
and solves the mentioned issue.

Tested on a system with RTL8168evl (MAC version 34).

Heiner Kallweit (3):
  r8169: remove unneeded rpm ops in rtl_shutdown
  r8169: improve runtime pm in rtl8169_check_link_status
  r8169: improve runtime pm in general and suspend unused ports

 drivers/net/ethernet/realtek/r8169.c | 44 ++++++++++++++----------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

-- 
2.15.1

[PATCH net-next 1/3] r8169: remove unneeded rpm ops in rtl_shutdown

[Heiner Kallweit]

This patch reverts commit 2a15cd2ff488 "r8169: runtime resume before
shutdown" from 2012. Few months after this change the underlying issue
was solved in the PCI core with commit 3ff2de9ba1a2 "PCI/PM: Resume
device before shutdown".

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 857f67beb..606fb7698 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -8195,9 +8195,6 @@ static void rtl_shutdown(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct rtl8169_private *tp = netdev_priv(dev);
-	struct device *d = &pdev->dev;
-
-	pm_runtime_get_sync(d);
 
 	rtl8169_net_suspend(dev);
 
@@ -8215,8 +8212,6 @@ static void rtl_shutdown(struct pci_dev *pdev)
 		pci_wake_from_d3(pdev, true);
 		pci_set_power_state(pdev, PCI_D3hot);
 	}
-
-	pm_runtime_put_noidle(d);
 }
 
 static void rtl_remove_one(struct pci_dev *pdev)
-- 
2.15.1

[PATCH net-next 2/3] r8169: improve runtime pm in rtl8169_check_link_status

[Heiner Kallweit]

This patch partially reverts commit e4fbce740f07 "r8169: Fix runtime
power management" from 2010. At that time the suspend delay was 100ms
and therefore suspending happened during initial aneg. Currently
suspend delay is 5s, so suspend starts after aneg and the issue
doesn't exist any longer. On my system aneg takes almost 3s, to be on
the safe side let's increase the suspend delay to 10s.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 606fb7698..29edaa4c6 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1675,33 +1675,24 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
 	}
 }
 
-static void __rtl8169_check_link_status(struct net_device *dev,
-					struct rtl8169_private *tp,
-					void __iomem *ioaddr, bool pm)
+static void rtl8169_check_link_status(struct net_device *dev,
+				      struct rtl8169_private *tp,
+				      void __iomem *ioaddr)
 {
 	if (tp->link_ok(ioaddr)) {
 		rtl_link_chg_patch(tp);
 		/* This is to cancel a scheduled suspend if there's one. */
-		if (pm)
-			pm_request_resume(&tp->pci_dev->dev);
+		pm_request_resume(&tp->pci_dev->dev);
 		netif_carrier_on(dev);
 		if (net_ratelimit())
 			netif_info(tp, ifup, dev, "link up\n");
 	} else {
 		netif_carrier_off(dev);
 		netif_info(tp, ifdown, dev, "link down\n");
-		if (pm)
-			pm_schedule_suspend(&tp->pci_dev->dev, 5000);
+		pm_schedule_suspend(&tp->pci_dev->dev, 10000);
 	}
 }
 
-static void rtl8169_check_link_status(struct net_device *dev,
-				      struct rtl8169_private *tp,
-				      void __iomem *ioaddr)
-{
-	__rtl8169_check_link_status(dev, tp, ioaddr, false);
-}
-
 #define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST)
 
 static u32 __rtl8169_get_wol(struct rtl8169_private *tp)
@@ -7746,7 +7737,7 @@ static void rtl_slow_event_work(struct rtl8169_private *tp)
 		rtl8169_pcierr_interrupt(dev);
 
 	if (status & LinkChg)
-		__rtl8169_check_link_status(dev, tp, tp->mmio_addr, true);
+		rtl8169_check_link_status(dev, tp, tp->mmio_addr);
 
 	rtl_irq_enable_all(tp);
 }
-- 
2.15.1

[PATCH net-next 3/3] r8169: improve runtime pm in general and suspend unused ports

[Heiner Kallweit]

So far rpm doesn't cover cases like unused ports which are never
brought up. If they are active at probe time they remain in this state.
Included in this patch:

- Let the idle notification check whether we can suspend and let it
  schedule the suspend. This way we don't need to have calls to
  pm_schedule_suspend in different places.
  
- At the end of rtl_open and rtl_init_one send an idle notification
  to allow suspending if the link is down. If a cable is plugged in
  aneg is finished before the suspend timer expires and the suspend
  request is cancelled.
  
- Change rtl8169_runtime_suspend to power down the chip if the
  interface is down.

Successfully tested on a RTL8168evl (mac version 34).

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 29edaa4c6..272c5962e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1689,7 +1689,7 @@ static void rtl8169_check_link_status(struct net_device *dev,
 	} else {
 		netif_carrier_off(dev);
 		netif_info(tp, ifdown, dev, "link down\n");
-		pm_schedule_suspend(&tp->pci_dev->dev, 10000);
+		pm_runtime_idle(&tp->pci_dev->dev);
 	}
 }
 
@@ -7950,7 +7950,7 @@ static int rtl_open(struct net_device *dev)
 	rtl_unlock_work(tp);
 
 	tp->saved_wolopts = 0;
-	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_put_sync(&pdev->dev);
 
 	rtl8169_check_link_status(dev, tp, ioaddr);
 out:
@@ -8094,8 +8094,10 @@ static int rtl8169_runtime_suspend(struct device *device)
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct rtl8169_private *tp = netdev_priv(dev);
 
-	if (!tp->TxDescArray)
+	if (!tp->TxDescArray) {
+		rtl_pll_power_down(tp);
 		return 0;
+	}
 
 	rtl_lock_work(tp);
 	tp->saved_wolopts = __rtl8169_get_wol(tp);
@@ -8137,9 +8139,11 @@ static int rtl8169_runtime_idle(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct net_device *dev = pci_get_drvdata(pdev);
-	struct rtl8169_private *tp = netdev_priv(dev);
 
-	return tp->TxDescArray ? -EBUSY : 0;
+	if (!netif_running(dev) || !netif_carrier_ok(dev))
+		pm_schedule_suspend(device, 10000);
+
+	return -EBUSY;
 }
 
 static const struct dev_pm_ops rtl8169_pm_ops = {
@@ -8687,11 +8691,11 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		rtl8168_driver_start(tp);
 	}
 
-	if (pci_dev_run_wake(pdev))
-		pm_runtime_put_noidle(&pdev->dev);
-
 	netif_carrier_off(dev);
 
+	if (pci_dev_run_wake(pdev))
+		pm_runtime_put_sync(&pdev->dev);
+
 	return 0;
 }
 
-- 
2.15.1

Re: [PATCH net-next 0/3] r8169: improve runtime pm

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Mon, 8 Jan 2018 21:34:41 +0100

> On my system with two network ports I found that runtime PM didn't
> suspend the unused port. Therefore I checked runtime pm in this driver
> in somewhat more detail and this series improves runtime pm in general
> and solves the mentioned issue.
> 
> Tested on a system with RTL8168evl (MAC version 34).

Series applied, thank you.

RE: [PATCH net-next 3/3] r8169: improve runtime pm in general and suspend unused ports

[Hayes Wang]

Heiner Kallweit [mailto:hkallweit1@gmail.com]
> Sent: Tuesday, January 09, 2018 4:39 AM
[...]
> - Change rtl8169_runtime_suspend to power down the chip if the
>   interface is down.

The original driver has done it. I don't think you have to do it again in rtl8169_runtime_suspend().

[...]
> -	if (!tp->TxDescArray)
> +	if (!tp->TxDescArray) {
> +		rtl_pll_power_down(tp);
>  		return 0;
> +	}

It is unnecessary.

Best Regards,
Hayes

RE: [PATCH net-next 3/3] r8169: improve runtime pm in general and suspend unused ports

[Hayes Wang]

> Heiner Kallweit [mailto:hkallweit1@gmail.com]
> > Sent: Tuesday, January 09, 2018 4:39 AM
> [...]
> > - Change rtl8169_runtime_suspend to power down the chip if the
> >   interface is down.
> 
> The original driver has done it. I don't think you have to do it again in
> rtl8169_runtime_suspend().

It's my mistake. If ndo_stop() doesn't be called after probe(),
rtl_pll_power_down() wouldn't be called, either.
You are right.

Best Regards,
Hayes