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;
Subject: Re: [PATCH net-next 4/4] net: ipa: activate some commented assertions
To:     Leon Romanovsky <leon@kernel.org>
Cc:     davem@davemloft.net, kuba@kernel.org, bjorn.andersson@linaro.org,
        evgreen@chromium.org, cpratapa@codeaurora.org, elder@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20210319042923.1584593-1-elder@linaro.org>
 <20210319042923.1584593-5-elder@linaro.org> <YFQwAYL15nEkfNf7@unreal>
 <7520639c-f08b-cb25-1a62-7e3d69981f95@linaro.org> <YFTAn7tHBAG2PO78@unreal>
From:   Alex Elder <elder@linaro.org>
Message-ID: <69647805-7ace-442d-2268-aa7c4800ab6e@linaro.org>
Date:   Fri, 19 Mar 2021 10:32:50 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <YFTAn7tHBAG2PO78@unreal>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/19/21 10:17 AM, Leon Romanovsky wrote:
>>>> @@ -212,7 +213,7 @@ static inline u32 ipa_reg_bcr_val(enum ipa_version version)
>>>>    			BCR_HOLB_DROP_L2_IRQ_FMASK |
>>>>    			BCR_DUAL_TX_FMASK;
>>>> -	/* assert(version != IPA_VERSION_4_5); */
>>>> +	ipa_assert(NULL, version != IPA_VERSION_4_5);
>>> This assert will fire for IPA_VERSION_4_2, I doubt that this is
>>> something you want.
>> No, it will only fail if version == IPA_VERSION_4_5.
>> The logic of an assertion is the opposite of BUG_ON().
>> It fails only if the asserted condition yields false.
> ok, this ipa_reg_bcr_val() is called in only one place and has a
> protection from IPA_VERSION_4_5, why don't you code it at the same
> .c file instead of adding useless assert?

As I mentioned in my other message, the purpose of an
assertion is *communicating with the reader*.  The
fact that an assertion may expand to code that ensures
the assertion is true is secondary.

This particular assertion says that the version will never
be 4.5.  While looking at this function, you don't need to
see if the caller ensures that, the assertion *tells* you.

Whether an assertion is warranted is really subjective.
You may not appreciate the value of that, but I do.

					-Alex