Re: [PATCH net-next 0/4] net: ipa: kill IPA_VALIDATION

[Alex Elder]

On 7/27/21 6:16 AM, Leon Romanovsky wrote:
> On Mon, Jul 26, 2021 at 12:40:06PM -0500, Alex Elder wrote:
>> A few months ago I proposed cleaning up some code that validates
>> certain things conditionally, arguing that doing so once is enough,
>> thus doing so always should not be necessary.
>>   https://lore.kernel.org/netdev/20210320141729.1956732-1-elder@xxxxxxxxxx/
>> Leon Romanovsky felt strongly that this was a mistake, and in the
>> end I agreed to change my plans.
> 
> <...>
> 
>> The second patch fixes a bug that wasn't normally exposed because of
>> the conditional compilation (a reason Leon was right about this).
> 
> Thanks Alex,
> 
> If you want another anti pattern that is very popular in netdev, the following pattern is
> wrong by definition :):
> if (WARN_ON(...))
>   return ...

I understand this reasoning.

I had it return an error if the WARN_ON() condition was true in cases
where the function returned a value and callers already handled errors.
I looked back at the patch and here is one of those cases:

gsi_channel_trans_alloc()
- If too many TREs are requested we do not want to allocate them
  from the pool, or it will cause further breakage.  By returning
  early, no transaction will be filled or committed, and an error
  message will (often) be reported, which will indicate the source
  of the error.  If any error occurs during initialization, we fail
  that whole process and everything should be cleaned up.  So in
  this case at least, returning if this ever occurred is better
  than allowing control to continue into the function.

In any case I take your point.  I will now add to my task list
a review of these spots.  I'd like to be sure an error message
*is* reported at an appropriate level up the chain of callers so
I can always identify the culprit in the a WARN_ON() fires (even
though it should never
 happen).  And in each case I'll evaluate
whether returning is better than not.

Thanks.

					-Alex

> The WARN_*() macros are intended catch impossible flows, something that
> shouldn't exist. The idea that printed stack to dmesg and return to the
> caller will fix the situation is a very naive one. That stack already
> says that something very wrong in the system.
> 
> If such flow can be valid use "if(...) return ..", if not use plain
> WARN_ON(...).
> 
> Thanks
>