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

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

[Chunhao Lin]

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

Chunhao Lin (3):
  r8169:fix nic sometimes doesn't work after changing the mac address.
  r8169:eliminate error message in using ethtool -S when nic is down.
  r8169: Enable RX_MULTI_EN for RTL_GIGA_MAC_VER_41~48

 drivers/net/ethernet/realtek/r8169.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH net 1/3] r8169:fix nic sometimes doesn't work after changing the mac address.

[Chunhao Lin]

When there is no AC power, NIC doesn't work after changing 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 be put in runtime suspend state and
device will in D3 state. In 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 so the network will not
work normally.

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

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

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 4caeacb..432b278 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4457,6 +4457,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))
@@ -4464,7 +4465,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;
 }
@@ -7872,6 +7878,8 @@ static int rtl8169_runtime_resume(struct device *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

Re: [PATCH net 1/3] r8169:fix nic sometimes doesn't work after changing the mac address.

[Francois Romieu]

Chunhao Lin <hau@realtek.com> :
> When there is no AC power, NIC doesn't work after changing 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 be put in runtime suspend state and
> device will in D3 state. In 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 so the network will not
> work normally.
> 
> In this patch I add detecting runtime pm state when setting mac address. If
> driver is in runtime suspend, I will skip setting mac address and  set the new
> mac address during runtime resume.

Instead of taking the device out of suspended mode to perform the required
action, the driver is moving to a model where 1) said action may be scheduled
to a later time - or result from past time work - and 2) rpm handler must
handle a lot of pm unrelated work.

rtl8169_ethtool_ops.{get_wol, get_regs, get_settings} aren't even fixed
yet (what about the .set_xyz handlers ?).

I can't help thinking that the driver should return to a state where it
stupidly does what it is asked to. No software caching, plain device
access, resume when needed, suspend as "suspend" instead of suspend as
"anticipate whatever may happen to avoid waking up".

-- 
Ueimor

RE: [PATCH net 1/3] r8169:fix nic sometimes doesn't work after changing the mac address.

[Hau]

 > Instead of taking the device out of suspended mode to perform the required
> action, the driver is moving to a model where 1) said action may be
> scheduled to a later time - or result from past time work - and 2) rpm handler
> must handle a lot of pm unrelated work.
> 
> rtl8169_ethtool_ops.{get_wol, get_regs, get_settings} aren't even fixed yet
> (what about the .set_xyz handlers ?).
> 
> I can't help thinking that the driver should return to a state where it stupidly
> does what it is asked to. No software caching, plain device access, resume
> when needed, suspend as "suspend" instead of suspend as "anticipate
> whatever may happen to avoid waking up".
>

This rpm related patches just the workaround for the issues reported by end users.  As you say, the Linux kernel should handle these events when driver is in runtime suspend state.
 
------Please consider the environment before printing this e-mail.

[PATCH net 2/3] r8169:eliminate error message in using ethtool -S when nic is down.

[Chunhao Lin]

This issue is caused by runtime power management. When NIC is down (ifconfig down)
the NIC may be put into runtime suspend state, that cause driver cannot dump
tally counter successfully and incur error message "rtl_counters_cond == 1
(loop: 1000, delay: 10)"

In this patch I add deceting driver runtime pm state. If driver is in runtime
suspend state, I will skip dump tall counter.

Signed-off-by: Chunhao Lin <hau@realtek.com>
---
 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 432b278..3e9eb64 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2291,11 +2291,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

[PATCH net 3/3] r8169: Enable RX_MULTI_EN for RTL_GIGA_MAC_VER_41~48

[Chunhao Lin]

For RTL8168G/RTL8168H/RTL8411B/RTL8107E, enable this flag to eliminate message
"AMD-Vi: Event logged [IO_PAGE_FAULT device=01:00.0 domain=0x0002
address=0x0000000000003000 flags=0x0050] in dmesg.

Signed-off-by: Chunhao Lin <hau@realtek.com>
---
 drivers/net/ethernet/realtek/r8169.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 3e9eb64..c8f0077 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4945,8 +4945,6 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
 		RTL_W32(RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST);
 		break;
 	case RTL_GIGA_MAC_VER_40:
-		RTL_W32(RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST | RX_EARLY_OFF);
-		break;
 	case RTL_GIGA_MAC_VER_41:
 	case RTL_GIGA_MAC_VER_42:
 	case RTL_GIGA_MAC_VER_43:
@@ -4955,8 +4953,6 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
 	case RTL_GIGA_MAC_VER_46:
 	case RTL_GIGA_MAC_VER_47:
 	case RTL_GIGA_MAC_VER_48:
-		RTL_W32(RxConfig, RX128_INT_EN | RX_DMA_BURST | RX_EARLY_OFF);
-		break;
 	case RTL_GIGA_MAC_VER_49:
 	case RTL_GIGA_MAC_VER_50:
 	case RTL_GIGA_MAC_VER_51:
-- 
1.9.1

Re: [PATCH net 0/3] r8169:issues fix.

[David Miller]

From: Chunhao Lin <hau@realtek.com>
Date: Fri, 26 Feb 2016 16:40:43 +0800

> This series of patches fix 3 issues that are listed below.
> 
> Chunhao Lin (3):
>   r8169:fix nic sometimes doesn't work after changing the mac address.
>   r8169:eliminate error message in using ethtool -S when nic is down.
>   r8169: Enable RX_MULTI_EN for RTL_GIGA_MAC_VER_41~48

I don't agree with changes #1 and #2.

If you are going to go to a model where every single configuration
operation is recorded in software and performed at resume time, then
really do it and fix it in the whole driver.  As currently coded you
are leaving lots of known bugs in the driver.

#2 is even a worse situation.  If you are going to handle things this
way you must sync the counters when the suspend happens, so that the
statistics get call receives up to date values.

RE: [PATCH net 0/3] r8169:issues fix.

[Hau]

> I don't agree with changes #1 and #2.
> 
> If you are going to go to a model where every single configuration operation
> is recorded in software and performed at resume time, then really do it and
> fix it in the whole driver.  As currently coded you are leaving lots of known
> bugs in the driver.
> 
> #2 is even a worse situation.  If you are going to handle things this way you
> must sync the counters when the suspend happens, so that the statistics get
> call receives up to date values.

Thanks for your advice. I will send #3 patch first. Then modify #1 and #2 patch and resend these two patches.

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