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

[Heiner Kallweit <hkallweit1@gmail.com>]

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.