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 3/4] net: ipa: introduce ipa_assert()
To:     Leon Romanovsky <leon@kernel.org>, Alex Elder <elder@linaro.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-4-elder@linaro.org> <YFQurZjWYaolHGvR@unreal>
 <edb7ab60-f0e2-8fc4-ca73-9614bb547ab5@linaro.org> <YFTD/TZ2tFX/X3dD@unreal>
From:   Alex Elder <elder@ieee.org>
Message-ID: <5b5d3f17-e647-ca1c-1ec0-fdc2396fa168@ieee.org>
Date:   Fri, 19 Mar 2021 11:01:10 -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: <YFTD/TZ2tFX/X3dD@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:32 AM, Leon Romanovsky wrote:
>>>> +/* Verify the expression yields true, and fail at build time if possible */
>>>> +#define ipa_assert(dev, expr) \
>>>> +	do { \
>>>> +		if (__builtin_constant_p(expr)) \
>>>> +			compiletime_assert(expr, __ipa_failure_msg(expr)); \
>>>> +		else \
>>>> +			__ipa_assert_runtime(dev, expr); \
>>>> +	} while (0)
>>>> +
>>>> +/* Report an error if the given expression evaluates to false at runtime */
>>>> +#define ipa_assert_always(dev, expr) \
>>>> +	do { \
>>>> +		if (unlikely(!(expr))) { \
>>>> +			struct device *__dev = (dev); \
>>>> +			\
>>>> +			if (__dev) \
>>>> +				dev_err(__dev, __ipa_failure_msg(expr)); \
>>>> +			else  \
>>>> +				pr_err(__ipa_failure_msg(expr)); \
>>>> +		} \
>>>> +	} while (0)
>>> It will be much better for everyone if you don't obfuscate existing
>>> kernel primitives and don't hide constant vs. dynamic expressions.
>> I don't agree with this characterization.
>>
>> Yes, there is some complexity in this one source file, where
>> ipa_assert() is defined.  But as a result, callers are simple
>> one-line statements (similar to WARN_ON()).
> It is not complexity but being explicit vs. implicit. The coding
> style that has explicit flows will be always better than implicit
> one. By adding your custom assert, you are hiding the flows and
> makes unclear what can be evaluated at compilation and what can't.
Assertions like this are a tool.  They aid readability
by communicating conditions that can be assumed to hold
at the time code is executed.  They are *not* part of
the normal code function.  They are optional, and code
*must* operate correctly even if all assertions are
removed.

Whether a condition that is asserted can be determined
at compile time or build time is irrelevant.  But as
an optimization, if it can be checked at compile time,
I want to do that, so we can catch the problem as early
as possible.

I understand your point about separating compile-time
versus runtime code.  I mean, it's a piece of really
understanding what's going on when your code is
executing.  But what I'm trying to do here is more
like a "functional comment," i.e., a comment about
the state of things that can be optionally verified.
I find them to be concise and useful:
	assert(count <= MAX_COUNT);
versus
	/* Caller must ensure count is in range */

We might just disagree on this, and that's OK.  I'm
interested to hear whether others think.

					-Alex