Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 27 Jul 2021 14:16:39 +0300
From:   Leon Romanovsky <leon@kernel.org>
To:     Alex Elder <elder@linaro.org>
Cc:     davem@davemloft.net, kuba@kernel.org, bjorn.andersson@linaro.org,
        evgreen@chromium.org, cpratapa@codeaurora.org,
        subashab@codeaurora.org, elder@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 0/4] net: ipa: kill IPA_VALIDATION
Message-ID: <YP/rFwvIHOvIwMNO@unreal>
References: <20210726174010.396765-1-elder@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210726174010.396765-1-elder@linaro.org>
Precedence: bulk

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@linaro.org/
> 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 ...

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