[net-next,3/4] net: ipa: introduce ipa_assert()

Commit Message

Alex Elder March 19, 2021, 4:29 a.m. UTC

Create a new macro ipa_assert() to verify that a condition is true.
This produces a build-time error if the condition can be evaluated
at build time; otherwise __ipa_assert_runtime() is called (described
below).

Another macro, ipa_assert_always() verifies that an expression
yields true at runtime, and if it does not, reports an error
message.

When IPA validation is enabled, __ipa_assert_runtime() becomes a
call to ipa_assert_always(), resulting in runtime verification of
the asserted condition.  Otherwise __ipa_assert_runtime() has no
effect, so such ipa_assert() calls are effectively ignored.

IPA assertion errors will be reported using the IPA device if
possible.

Signed-off-by: Alex Elder <[e-mail]>
---
 drivers/net/ipa/ipa_assert.h | 50 ++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 drivers/net/ipa/ipa_assert.h

Comments

Leon Romanovsky March 19, 2021, 4:55 a.m. UTC | #1

On Thu, Mar 18, 2021 at 11:29:22PM -0500, Alex Elder wrote:
> Create a new macro ipa_assert() to verify that a condition is true.
> This produces a build-time error if the condition can be evaluated
> at build time; otherwise __ipa_assert_runtime() is called (described
> below).
> 
> Another macro, ipa_assert_always() verifies that an expression
> yields true at runtime, and if it does not, reports an error
> message.
> 
> When IPA validation is enabled, __ipa_assert_runtime() becomes a
> call to ipa_assert_always(), resulting in runtime verification of
> the asserted condition.  Otherwise __ipa_assert_runtime() has no
> effect, so such ipa_assert() calls are effectively ignored.
> 
> IPA assertion errors will be reported using the IPA device if
> possible.
> 
> Signed-off-by: Alex Elder <[e-mail]>
> ---
>  drivers/net/ipa/ipa_assert.h | 50 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 drivers/net/ipa/ipa_assert.h
> 
> diff --git a/drivers/net/ipa/ipa_assert.h b/drivers/net/ipa/ipa_assert.h
> new file mode 100644
> index 0000000000000..7e5b9d487f69d
> --- /dev/null
> +++ b/drivers/net/ipa/ipa_assert.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2021 Linaro Ltd.
> + */
> +#ifndef _IPA_ASSERT_H_
> +#define _IPA_ASSERT_H_
> +
> +#include <linux/compiler.h>
> +#include <linux/printk.h>
> +#include <linux/device.h>
> +
> +/* 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.

So any random kernel developer will be able to change the code without
investing too much time to understand this custom logic.

And constant expressions are checked with BUILD_BUG_ON().

If you still feel need to provide assertion like this, it should be done
in general code.

Thanks

> +
> +/* Constant message used when an assertion fails */
> +#define __ipa_failure_msg(expr)	"IPA assertion failed: " #expr "\n"
> +
> +#ifdef IPA_VALIDATION
> +
> +/* Only do runtime checks for "normal" assertions if validating the code */
> +#define __ipa_assert_runtime(dev, expr)	ipa_assert_always(dev, expr)
> +
> +#else /* !IPA_VALIDATION */
> +
> +/* "Normal" assertions aren't checked when validation is disabled */
> +#define __ipa_assert_runtime(dev, expr)	\
> +	do { (void)(dev); (void)(expr); } while (0)
> +
> +#endif /* !IPA_VALIDATION */
> +
> +#endif /* _IPA_ASSERT_H_ */
> -- 
> 2.27.0
>

Alex Elder March 19, 2021, 12:38 p.m. UTC | #2

On 3/18/21 11:55 PM, Leon Romanovsky wrote:
> On Thu, Mar 18, 2021 at 11:29:22PM -0500, Alex Elder wrote:
>> Create a new macro ipa_assert() to verify that a condition is true.
>> This produces a build-time error if the condition can be evaluated
>> at build time; otherwise __ipa_assert_runtime() is called (described
>> below).
>>
>> Another macro, ipa_assert_always() verifies that an expression
>> yields true at runtime, and if it does not, reports an error
>> message.
>>
>> When IPA validation is enabled, __ipa_assert_runtime() becomes a
>> call to ipa_assert_always(), resulting in runtime verification of
>> the asserted condition.  Otherwise __ipa_assert_runtime() has no
>> effect, so such ipa_assert() calls are effectively ignored.
>>
>> IPA assertion errors will be reported using the IPA device if
>> possible.
>>
>> Signed-off-by: Alex Elder <[e-mail]>
>> ---
>>   drivers/net/ipa/ipa_assert.h | 50 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>   create mode 100644 drivers/net/ipa/ipa_assert.h
>>
>> diff --git a/drivers/net/ipa/ipa_assert.h b/drivers/net/ipa/ipa_assert.h
>> new file mode 100644
>> index 0000000000000..7e5b9d487f69d
>> --- /dev/null
>> +++ b/drivers/net/ipa/ipa_assert.h
>> @@ -0,0 +1,50 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2021 Linaro Ltd.
>> + */
>> +#ifndef _IPA_ASSERT_H_
>> +#define _IPA_ASSERT_H_
>> +
>> +#include <linux/compiler.h>
>> +#include <linux/printk.h>
>> +#include <linux/device.h>
>> +
>> +/* 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()).

Existing kernel primitives don't achieve these objectives:
- Don't check things at run time under normal conditions
- Do check things when validation is enabled
- If you can check it at compile time, check it regardless
If there is something that helps me do that, suggest it because
I will be glad to use it.

> So any random kernel developer will be able to change the code without
> investing too much time to understand this custom logic.

There should be almost no need to change the definition of
ipa_assert().  Even so, this custom logic is not all that
complicated in my view; it's broken into a few macros that
are each pretty simple.  It was actuallyl a little simpler
before I added some things to satisfy checkpatch.pl.

> And constant expressions are checked with BUILD_BUG_ON().

BUILD_BUG_ON() is great.  But it doesn't work for
non-constant expressions.

Suppose I try to simplify ipa_assert() as:

    #define ipa_assert(dev, expr) \
	do { \
	    BUILD_BUG_ON(expr); \
	    __ipa_runtime_assert(dev, expr); \
	} while (0)

I can't do that, because BUILD_BUG_ON() evaluates the
expression, so I can't safely do that again in the called
macro.

I explained how I wanted it to work earlier, but the main
reason for doing this is to communicate to the reader
that the asserted properties are guaranteed to be true.
It can be valuable for making things understandable.
You don't have to wonder, "what if the value passed
is out of range?"  It won't be, and the assertion tells
you that.

> If you still feel need to provide assertion like this, it should be done
> in general code.

I could do that but I'm afraid it's more than I intend
to take on, and am not aware of anyone else asking for
a feature like this.  Do you want it?

I honestly appreciate your input, but I don't agree with
it.  I can do without codifying informative assertions
but I'd really like to be able to test them.

					-Alex

> Thanks
> 
>> +
>> +/* Constant message used when an assertion fails */
>> +#define __ipa_failure_msg(expr)	"IPA assertion failed: " #expr "\n"
>> +
>> +#ifdef IPA_VALIDATION
>> +
>> +/* Only do runtime checks for "normal" assertions if validating the code */
>> +#define __ipa_assert_runtime(dev, expr)	ipa_assert_always(dev, expr)
>> +
>> +#else /* !IPA_VALIDATION */
>> +
>> +/* "Normal" assertions aren't checked when validation is disabled */
>> +#define __ipa_assert_runtime(dev, expr)	\
>> +	do { (void)(dev); (void)(expr); } while (0)
>> +
>> +#endif /* !IPA_VALIDATION */
>> +
>> +#endif /* _IPA_ASSERT_H_ */
>> -- 
>> 2.27.0
>>

Leon Romanovsky March 19, 2021, 3:32 p.m. UTC | #3

On Fri, Mar 19, 2021 at 07:38:26AM -0500, Alex Elder wrote:
> On 3/18/21 11:55 PM, Leon Romanovsky wrote:
> > On Thu, Mar 18, 2021 at 11:29:22PM -0500, Alex Elder wrote:
> > > Create a new macro ipa_assert() to verify that a condition is true.
> > > This produces a build-time error if the condition can be evaluated
> > > at build time; otherwise __ipa_assert_runtime() is called (described
> > > below).
> > > 
> > > Another macro, ipa_assert_always() verifies that an expression
> > > yields true at runtime, and if it does not, reports an error
> > > message.
> > > 
> > > When IPA validation is enabled, __ipa_assert_runtime() becomes a
> > > call to ipa_assert_always(), resulting in runtime verification of
> > > the asserted condition.  Otherwise __ipa_assert_runtime() has no
> > > effect, so such ipa_assert() calls are effectively ignored.
> > > 
> > > IPA assertion errors will be reported using the IPA device if
> > > possible.
> > > 
> > > Signed-off-by: Alex Elder <[e-mail]>
> > > ---
> > >   drivers/net/ipa/ipa_assert.h | 50 ++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 50 insertions(+)
> > >   create mode 100644 drivers/net/ipa/ipa_assert.h
> > > 
> > > diff --git a/drivers/net/ipa/ipa_assert.h b/drivers/net/ipa/ipa_assert.h
> > > new file mode 100644
> > > index 0000000000000..7e5b9d487f69d
> > > --- /dev/null
> > > +++ b/drivers/net/ipa/ipa_assert.h
> > > @@ -0,0 +1,50 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (C) 2021 Linaro Ltd.
> > > + */
> > > +#ifndef _IPA_ASSERT_H_
> > > +#define _IPA_ASSERT_H_
> > > +
> > > +#include <linux/compiler.h>
> > > +#include <linux/printk.h>
> > > +#include <linux/device.h>
> > > +
> > > +/* 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.

> 
> Existing kernel primitives don't achieve these objectives:
> - Don't check things at run time under normal conditions
> - Do check things when validation is enabled
> - If you can check it at compile time, check it regardless
> If there is something that helps me do that, suggest it because
> I will be glad to use it.

Separate checks to two flows and it will be natural to achieve what you
want.

> 
> > So any random kernel developer will be able to change the code without
> > investing too much time to understand this custom logic.
> 
> There should be almost no need to change the definition of
> ipa_assert().  Even so, this custom logic is not all that
> complicated in my view; it's broken into a few macros that
> are each pretty simple.  It was actuallyl a little simpler
> before I added some things to satisfy checkpatch.pl.

Every obfuscation is simple, but together it is nightmare for the person
who does some random kernel job and needs to change such obfuscated code.

> 
> > And constant expressions are checked with BUILD_BUG_ON().
> 
> BUILD_BUG_ON() is great.  But it doesn't work for
> non-constant expressions.

Of course, be explicit and use BUILD_BUG_ON() for constants and write
the code that don't need such constructions.

Thanks

Alex Elder March 19, 2021, 4:01 p.m. UTC | #4

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

Andrew Lunn March 19, 2021, 6:20 p.m. UTC | #5

> It will be much better for everyone if you don't obfuscate existing
> kernel primitives and don't hide constant vs. dynamic expressions.
> 
> So any random kernel developer will be able to change the code without
> investing too much time to understand this custom logic.
> 
> And constant expressions are checked with BUILD_BUG_ON().
> 
> If you still feel need to provide assertion like this, it should be done
> in general code.

+1

	Andrew

Patch

diff --git a/drivers/net/ipa/ipa_assert.h b/drivers/net/ipa/ipa_assert.h
new file mode 100644
index 0000000000000..7e5b9d487f69d
--- /dev/null
+++ b/drivers/net/ipa/ipa_assert.h
@@ -0,0 +1,50 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2021 Linaro Ltd.
+ */
+#ifndef _IPA_ASSERT_H_
+#define _IPA_ASSERT_H_
+
+#include <linux/compiler.h>
+#include <linux/printk.h>
+#include <linux/device.h>
+
+/* 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)
+
+/* Constant message used when an assertion fails */
+#define __ipa_failure_msg(expr)	"IPA assertion failed: " #expr "\n"
+
+#ifdef IPA_VALIDATION
+
+/* Only do runtime checks for "normal" assertions if validating the code */
+#define __ipa_assert_runtime(dev, expr)	ipa_assert_always(dev, expr)
+
+#else /* !IPA_VALIDATION */
+
+/* "Normal" assertions aren't checked when validation is disabled */
+#define __ipa_assert_runtime(dev, expr)	\
+	do { (void)(dev); (void)(expr); } while (0)
+
+#endif /* !IPA_VALIDATION */
+
+#endif /* _IPA_ASSERT_H_ */