| Message ID | f89bf2f3-5324-b635-4253-8a8be316c15b@gmail.com |
|---|---|
| Headers | show |
| Series | r8169: small improvements | expand |
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 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.