[PATCH net-next 0/3] r8169: small improvements

This series includes a number of smaller improvements.

Heiner Kallweit (3):
  r8169: replace BUG_ON with WARN in _rtl_eri_write
  r8169: improve rtl_ocp_reg_failure
  r8169: don't wakeup-enable device on shutdown if WOL is disabled

 drivers/net/ethernet/realtek/r8169_main.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

-- 
2.30.0

Use WARN here to avoid stopping the system. In addition print the addr
and mask values that triggered the warning.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 024042f37..9af048ad0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -763,7 +763,7 @@ static void _rtl_eri_write(struct rtl8169_private *tp, int addr, u32 mask,
 {
 	u32 cmd = ERIAR_WRITE_CMD | type | mask | addr;
 
-	BUG_ON((addr & 3) || (mask == 0));
+	WARN(addr & 3 || !mask, "addr: 0x%x, mask: 0x%08x\n", addr, mask);
 	RTL_W32(tp, ERIDR, val);
 	r8168fp_adjust_ocp_cmd(tp, &cmd, type);
 	RTL_W32(tp, ERIAR, cmd);

-- 
2.30.0

If WOL isn't enabled, then there's no need to enable wakeup from D3.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 6005d37b6..982e6b2f0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -4876,7 +4876,7 @@ static void rtl_shutdown(struct pci_dev *pdev)
 			rtl_wol_shutdown_quirk(tp);
 		}
 
-		pci_wake_from_d3(pdev, true);
+		pci_wake_from_d3(pdev, tp->saved_wolopts);
 		pci_set_power_state(pdev, PCI_D3hot);
 	}
 }

-- 
2.30.0

Use WARN_ONCE here to get a call trace in case of a problem.
This facilitates finding the offending code part.

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

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 9af048ad0..6005d37b6 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -810,14 +810,9 @@ static void rtl_eri_clear_bits(struct rtl8169_private *tp, int addr, u32 m)
 	rtl_w0w1_eri(tp, addr, 0, m);
 }
 
-static bool rtl_ocp_reg_failure(struct rtl8169_private *tp, u32 reg)
+static bool rtl_ocp_reg_failure(u32 reg)
 {
-	if (reg & 0xffff0001) {
-		if (net_ratelimit())
-			netdev_err(tp->dev, "Invalid ocp reg %x!\n", reg);
-		return true;
-	}
-	return false;
+	return WARN_ONCE(reg & 0xffff0001, "Invalid ocp reg %x!\n", reg);
 }
 
 DECLARE_RTL_COND(rtl_ocp_gphy_cond)

@@ -827,7 +822,7 @@ DECLARE_RTL_COND(rtl_ocp_gphy_cond)
 
 static void r8168_phy_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
 {
-	if (rtl_ocp_reg_failure(tp, reg))
+	if (rtl_ocp_reg_failure(reg))
 		return;
 
 	RTL_W32(tp, GPHY_OCP, OCPAR_FLAG | (reg << 15) | data);

@@ -837,7 +832,7 @@ static void r8168_phy_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
 
 static int r8168_phy_ocp_read(struct rtl8169_private *tp, u32 reg)
 {
-	if (rtl_ocp_reg_failure(tp, reg))
+	if (rtl_ocp_reg_failure(reg))
 		return 0;
 
 	RTL_W32(tp, GPHY_OCP, reg << 15);

@@ -848,7 +843,7 @@ static int r8168_phy_ocp_read(struct rtl8169_private *tp, u32 reg)
 
 static void r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
 {
-	if (rtl_ocp_reg_failure(tp, reg))
+	if (rtl_ocp_reg_failure(reg))
 		return;
 
 	RTL_W32(tp, OCPDR, OCPAR_FLAG | (reg << 15) | data);

@@ -856,7 +851,7 @@ static void r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
 
 static u16 r8168_mac_ocp_read(struct rtl8169_private *tp, u32 reg)
 {
-	if (rtl_ocp_reg_failure(tp, reg))
+	if (rtl_ocp_reg_failure(reg))
 		return 0;
 
 	RTL_W32(tp, OCPDR, reg << 15);

-- 
2.30.0

On 06.01.2021 14:26, Heiner Kallweit wrote:

This series includes a number of smaller improvements.

Heiner Kallweit (3):
  r8169: replace BUG_ON with WARN in _rtl_eri_write
  r8169: improve rtl_ocp_reg_failure
  r8169: don't wakeup-enable device on shutdown if WOL is disabled

 drivers/net/ethernet/realtek/r8169_main.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

Just saw that this series is marked "doesn't apply on net-next" in
patchwork. I checked and indeed there's a dependency on prior,
not yet applied series 
[PATCH net-next 0/2] r8169: improve RTL8168g PHY suspend quirk
Shall I resend once the first series is applied?

On Wed, Jan 6, 2021 at 5:32 AM Heiner Kallweit [off-list ref] wrote:

Would it make more sense to perhaps just catch the case via an if
statement, display the warning, and then return instead of proceeding
with the write with the bad values?

I'm just wondering if this could make things worse by putting the
device in a bad state in some way resulting in either a timeout
waiting for a response or a hang.

On 06.01.2021 23:18, Alexander Duyck wrote:

On Wed, Jan 6, 2021 at 5:32 AM Heiner Kallweit [off-list ref] wrote:

quoted

Use WARN here to avoid stopping the system. In addition print the addr
and mask values that triggered the warning.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 024042f37..9af048ad0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -763,7 +763,7 @@ static void _rtl_eri_write(struct rtl8169_private *tp, int addr, u32 mask,
 {
        u32 cmd = ERIAR_WRITE_CMD | type | mask | addr;

-       BUG_ON((addr & 3) || (mask == 0));
+       WARN(addr & 3 || !mask, "addr: 0x%x, mask: 0x%08x\n", addr, mask);
        RTL_W32(tp, ERIDR, val);
        r8168fp_adjust_ocp_cmd(tp, &cmd, type);
        RTL_W32(tp, ERIAR, cmd);

Would it make more sense to perhaps just catch the case via an if
statement, display the warning, and then return instead of proceeding
with the write with the bad values?

I'm just wondering if this could make things worse by putting the
device in a bad state in some way resulting in either a timeout
waiting for a response or a hang.

I tend to agree. Typically I'd expect that this warning is triggered
during development only, but yes: Returning instead of writing to a
wrong register is better.