[PATCH net 0/3] r8169:fix 3 runtime pm related issues.

This series of patches fix 3 runtime pm related issues that are listed below.

Chunhao Lin (3):
  r8169:fix kernel log spam when set or get hardware wol setting.
  r8169:add checking driver's runtime pm status in rtl8169_get_ethtool_stats()
  r8169:fix nic may not work after changing the mac address.

 drivers/net/ethernet/realtek/r8169.c | 37 ++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

-- 
1.9.1

Not to call rtl8169_update_counters() to dump tally counter when driver is in
runtime suspend state.

Signed-off-by: Chunhao Lin <redacted>
---
 drivers/net/ethernet/realtek/r8169.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f07604f..4a17342 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c

@@ -2308,11 +2308,17 @@ static void rtl8169_get_ethtool_stats(struct net_device *dev,
 				      struct ethtool_stats *stats, u64 *data)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
+	struct pci_dev *pdev = tp->pci_dev;
 	struct rtl8169_counters *counters = tp->counters;
 
 	ASSERT_RTNL();
 
-	rtl8169_update_counters(dev);
+	pm_runtime_get_noresume(&pdev->dev);
+
+	if (pm_runtime_active(&pdev->dev))
+		rtl8169_update_counters(dev);
+
+	pm_runtime_put_noidle(&pdev->dev);
 
 	data[0] = le64_to_cpu(counters->tx_packets);
 	data[1] = le64_to_cpu(counters->rx_packets);

-- 
1.9.1

When there is no AC power, NIC may not work after changing the mac address.
Please refer to following link.
http://www.spinics.net/lists/netdev/msg356572.html

This issue is caused by runtime power management. When there is no AC power,
if we put NIC down (ifconfig down), the driver will in runtime suspend state
and hardware will be put into D3 state. During this time, driver cannot access
hardware regisers. So if you set new mac address during this time, it will not
work. And then, after resume, the NIC will keep using the old mac address and
the network will not work normally.

In this patch I add detecting runtime pm status when setting mac address.
If driver is in runtime suspend state, it will skip setting mac address and
set the new mac address during runtime resume.

Signed-off-by: Chunhao Lin <redacted>
---
 drivers/net/ethernet/realtek/r8169.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 4a17342..ff1611e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c

@@ -4480,6 +4480,7 @@ 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 pci_dev *pdev = tp->pci_dev;
 	struct sockaddr *addr = p;
 
 	if (!is_valid_ether_addr(addr->sa_data))

@@ -4487,7 +4488,12 @@ static int rtl_set_mac_address(struct net_device *dev, void *p)
 
 	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
 
-	rtl_rar_set(tp, dev->dev_addr);
+	pm_runtime_get_noresume(&pdev->dev);
+
+	if (pm_runtime_active(&pdev->dev))
+		rtl_rar_set(tp, dev->dev_addr);
+
+	pm_runtime_put_noidle(&pdev->dev);
 
 	return 0;
 }

@@ -7890,6 +7896,7 @@ static int rtl8169_runtime_resume(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);
+	rtl_rar_set(tp, dev->dev_addr);
 
 	if (!tp->TxDescArray)
 		return 0;

-- 
1.9.1

NIC will be put into D3 state during runtime suspend state.
When set or get hardware wol setting, driver will write or read hardware
register. If we set or get hardware wol setting in runtime suspend state,
because NIC will in D3 state, the register value read by driver will return all
0xff. That will let driver thinking register flag is not toggled and then prints
the warning message "rtl_counters_cond == 1 (loop: 1000, delay: 10)" to kernel
log.

For fixing this issue, add checking driver's pm runtime status in
rtl8169_get_wol() and rtl8169_set_wol().

Signed-off-by: Chunhao Lin <redacted>
---
 drivers/net/ethernet/realtek/r8169.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0e62d74..f07604f 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c

@@ -1749,13 +1749,21 @@ static u32 __rtl8169_get_wol(struct rtl8169_private *tp)
 static void rtl8169_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
+	struct pci_dev *pdev = tp->pci_dev;
+
+	pm_runtime_get_noresume(&pdev->dev);
 
 	rtl_lock_work(tp);
 
 	wol->supported = WAKE_ANY;
-	wol->wolopts = __rtl8169_get_wol(tp);
+	if (pm_runtime_active(&pdev->dev))
+		wol->wolopts = __rtl8169_get_wol(tp);
+	else
+		wol->wolopts = tp->saved_wolopts;
 
 	rtl_unlock_work(tp);
+
+	pm_runtime_put_noidle(&pdev->dev);
 }
 
 static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)

@@ -1845,6 +1853,9 @@ 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 pci_dev *pdev = tp->pci_dev;
+
+	pm_runtime_get_noresume(&pdev->dev);
 
 	rtl_lock_work(tp);
 

@@ -1852,12 +1863,17 @@ static int rtl8169_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 		tp->features |= RTL_FEATURE_WOL;
 	else
 		tp->features &= ~RTL_FEATURE_WOL;
-	__rtl8169_set_wol(tp, wol->wolopts);
+	if (pm_runtime_active(&pdev->dev))
+		__rtl8169_set_wol(tp, wol->wolopts);
+	else
+		tp->saved_wolopts = wol->wolopts;
 
 	rtl_unlock_work(tp);
 
 	device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts);
 
+	pm_runtime_put_noidle(&pdev->dev);
+
 	return 0;
 }
 

-- 
1.9.1

Chunhao Lin [off-list ref] :
[...]

[...]

Either the driver resumes the device so that it can perform requested
operation or it signals .set_wol failure when the device is suspended.

If the driver does something else, "spam removal" translates to
"silent failure".

-- 
Ueimor

Chunhao Lin [off-list ref] :
[...]

Nit: you may directly use "struct device *d = &tp->pci_dev->dev;"

-- 
Ueimor

[...]

quoted

@@ -1852,12 +1863,17 @@ static int rtl8169_set_wol(struct net_device

*dev, struct ethtool_wolinfo *wol)

quoted

 		tp->features |= RTL_FEATURE_WOL;
 	else
 		tp->features &= ~RTL_FEATURE_WOL;
-	__rtl8169_set_wol(tp, wol->wolopts);
+	if (pm_runtime_active(&pdev->dev))
+		__rtl8169_set_wol(tp, wol->wolopts);
+	else
+		tp->saved_wolopts = wol->wolopts;

 	rtl_unlock_work(tp);

 	device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts);

+	pm_runtime_put_noidle(&pdev->dev);
+
 	return 0;

Either the driver resumes the device so that it can perform requested
operation or it signals .set_wol failure when the device is suspended.

If the driver does something else, "spam removal" translates to "silent
failure".

Because "tp->saved_wolopts" will be used to set hardware wol capability in rtl8169_runtime_resume().  So I prefer to keep "wol->wolopts" to " tp->saved_wolopts " in runtime suspend state and set this to this "wol->wolopts" to hardware in in rtl8169_runtime_resume(). 

Thanks.

------Please consider the environment before printing this e-mail.

[...]

Nit: you may directly use "struct device *d = &tp->pci_dev->dev;"

I will do that on my next version patch.

Thanks.
------Please consider the environment before printing this e-mail.

Hau [off-list ref] :
[...]

quoted

Either the driver resumes the device so that it can perform requested
operation or it signals .set_wol failure when the device is suspended.

If the driver does something else, "spam removal" translates to "silent
failure".

Because "tp->saved_wolopts" will be used to set hardware wol capability in
rtl8169_runtime_resume().  So I prefer to keep "wol->wolopts" to
" tp->saved_wolopts " in runtime suspend state and set this to this
"wol->wolopts" to hardware in in rtl8169_runtime_resume(). 

It would be fine if it could be proven that rtl8169_runtime_resume() will
always be run before software state is lost.

-- 
Ueimor