* [PATCH net-next 0/3] r8169: small improvements @ 2021-01-06 13:26 Heiner Kallweit 2021-01-06 13:28 ` [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write Heiner Kallweit ` (3 more replies) 0 siblings, 4 replies; 7+ messages in thread From: Heiner Kallweit @ 2021-01-06 13:26 UTC (permalink / raw) To: Jakub Kicinski, David Miller, Realtek linux nic maintainers; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write 2021-01-06 13:26 [PATCH net-next 0/3] r8169: small improvements Heiner Kallweit @ 2021-01-06 13:28 ` Heiner Kallweit 2021-01-06 22:18 ` Alexander Duyck 2021-01-06 13:28 ` [PATCH net-next 2/3] r8169: improve rtl_ocp_reg_failure Heiner Kallweit ` (2 subsequent siblings) 3 siblings, 1 reply; 7+ messages in thread From: Heiner Kallweit @ 2021-01-06 13:28 UTC (permalink / raw) To: Jakub Kicinski, David Miller, Realtek linux nic maintainers; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write 2021-01-06 13:28 ` [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write Heiner Kallweit @ 2021-01-06 22:18 ` Alexander Duyck 2021-01-06 23:02 ` Heiner Kallweit 0 siblings, 1 reply; 7+ messages in thread From: Alexander Duyck @ 2021-01-06 22:18 UTC (permalink / raw) To: Heiner Kallweit Cc: Jakub Kicinski, David Miller, Realtek linux nic maintainers, netdev On Wed, Jan 6, 2021 at 5:32 AM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write 2021-01-06 22:18 ` Alexander Duyck @ 2021-01-06 23:02 ` Heiner Kallweit 0 siblings, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2021-01-06 23:02 UTC (permalink / raw) To: Alexander Duyck Cc: Jakub Kicinski, David Miller, Realtek linux nic maintainers, netdev On 06.01.2021 23:18, Alexander Duyck wrote: > On Wed, Jan 6, 2021 at 5:32 AM Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next 2/3] r8169: improve rtl_ocp_reg_failure 2021-01-06 13:26 [PATCH net-next 0/3] r8169: small improvements Heiner Kallweit 2021-01-06 13:28 ` [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write Heiner Kallweit @ 2021-01-06 13:28 ` Heiner Kallweit 2021-01-06 13:29 ` [PATCH net-next 3/3] r8169: don't wakeup-enable device on shutdown if WOL is disabled Heiner Kallweit 2021-01-06 18:10 ` [PATCH net-next 0/3] r8169: small improvements Heiner Kallweit 3 siblings, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2021-01-06 13:28 UTC (permalink / raw) To: Jakub Kicinski, David Miller, Realtek linux nic maintainers; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 3/3] r8169: don't wakeup-enable device on shutdown if WOL is disabled 2021-01-06 13:26 [PATCH net-next 0/3] r8169: small improvements Heiner Kallweit 2021-01-06 13:28 ` [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write Heiner Kallweit 2021-01-06 13:28 ` [PATCH net-next 2/3] r8169: improve rtl_ocp_reg_failure Heiner Kallweit @ 2021-01-06 13:29 ` Heiner Kallweit 2021-01-06 18:10 ` [PATCH net-next 0/3] r8169: small improvements Heiner Kallweit 3 siblings, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2021-01-06 13:29 UTC (permalink / raw) To: Jakub Kicinski, David Miller, Realtek linux nic maintainers; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 0/3] r8169: small improvements 2021-01-06 13:26 [PATCH net-next 0/3] r8169: small improvements Heiner Kallweit ` (2 preceding siblings ...) 2021-01-06 13:29 ` [PATCH net-next 3/3] r8169: don't wakeup-enable device on shutdown if WOL is disabled Heiner Kallweit @ 2021-01-06 18:10 ` Heiner Kallweit 3 siblings, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2021-01-06 18:10 UTC (permalink / raw) To: Jakub Kicinski, David Miller, Realtek linux nic maintainers; +Cc: netdev 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-06 23:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-06 13:26 [PATCH net-next 0/3] r8169: small improvements Heiner Kallweit 2021-01-06 13:28 ` [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write Heiner Kallweit 2021-01-06 22:18 ` Alexander Duyck 2021-01-06 23:02 ` Heiner Kallweit 2021-01-06 13:28 ` [PATCH net-next 2/3] r8169: improve rtl_ocp_reg_failure Heiner Kallweit 2021-01-06 13:29 ` [PATCH net-next 3/3] r8169: don't wakeup-enable device on shutdown if WOL is disabled Heiner Kallweit 2021-01-06 18:10 ` [PATCH net-next 0/3] r8169: small improvements Heiner Kallweit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).