Re: [PATCH net-next 1/3] r8169: replace BUG_ON with WARN in _rtl_eri_write

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.