| Message ID | 619e6cc2-9253-fd1e-564a-eec944ee31e1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8169: small improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
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.
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.
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);
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(-)