[PATCH net-next 0/4] net: ipa: fix validation

[PATCH net-next 0/4] net: ipa: fix validation

[Alex Elder]

There is sanity checking code in the IPA driver that's meant to be
enabled only during development.  This allows the driver to make
certain assumptions, but not have to verify those assumptions are
true at (operational) runtime.  This code is built conditional on
IPA_VALIDATION, set (if desired) inside the IPA makefile.

Unfortunately, this validation code has some errors.  First, there
are some mismatched arguments supplied to some dev_err() calls in
ipa_cmd_table_valid() and ipa_cmd_header_valid(), and these are
exposed if validation is enabled.  Second, the tag that enables
this conditional code isn't used consistently (it's IPA_VALIDATE
in some spots and IPA_VALIDATION in others).

This series fixes those two problems with the conditional validation
code.

In addition, this series introduces some new assertion macros.  I
have been meaning to add this for a long time.  There are comments
indicating places where assertions could be checked throughout the
code.

The macros are designed so that any asserted condition will be
checked at compile time if possible.  Otherwise, the condition
will be checked at runtime *only* if IPA_VALIDATION is enabled,
and ignored otherwise.

NOTE:  The third patch produces two bogus (but understandable)
warnings from checkpatch.pl.  It does not recognize that the "expr"
argument passed to those macros aren't actually evaluated more than
once.  In both cases, all but one reference is consumed by the
preprocessor or compiler.

A final patch converts a handful of commented assertions into
"real" ones.  Some existing validation code can done more simply
with assertions, so over time such cases will be converted.  For
now though, this series adds this assertion capability.

					-Alex

Alex Elder (4):
  net: ipa: fix init header command validation
  net: ipa: fix IPA validation
  net: ipa: introduce ipa_assert()
  net: ipa: activate some commented assertions

 drivers/net/ipa/Makefile       |  2 +-
 drivers/net/ipa/gsi_trans.c    |  8 ++---
 drivers/net/ipa/ipa_assert.h   | 50 ++++++++++++++++++++++++++++++++
 drivers/net/ipa/ipa_cmd.c      | 53 ++++++++++++++++++++++------------
 drivers/net/ipa/ipa_cmd.h      |  6 ++--
 drivers/net/ipa/ipa_endpoint.c |  6 ++--
 drivers/net/ipa/ipa_main.c     |  6 ++--
 drivers/net/ipa/ipa_mem.c      |  6 ++--
 drivers/net/ipa/ipa_reg.h      |  7 +++--
 drivers/net/ipa/ipa_table.c    | 11 ++++---
 drivers/net/ipa/ipa_table.h    |  6 ++--
 11 files changed, 115 insertions(+), 46 deletions(-)
 create mode 100644 drivers/net/ipa/ipa_assert.h

-- 
2.27.0

[PATCH net-next 1/4] net: ipa: fix init header command validation

[Alex Elder]

We use ipa_cmd_header_valid() to ensure certain values we will
program into hardware are within range, well in advance of when we
actually program them.  This way we avoid having to check for errors
when we actually program the hardware.

Unfortunately the dev_err() call for a bad offset value does not
supply the arguments to match the format specifiers properly.
Fix this.

There was also supposed to be a check to ensure the size to be
programmed fits in the field that holds it.  Add this missing check.

Rearrange the way we ensure the header table fits in overall IPA
memory range.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_cmd.c | 49 +++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ipa/ipa_cmd.c b/drivers/net/ipa/ipa_cmd.c
index 35e35852c25c5..b40f031a905a7 100644
--- a/drivers/net/ipa/ipa_cmd.c
+++ b/drivers/net/ipa/ipa_cmd.c
@@ -175,21 +175,23 @@ bool ipa_cmd_table_valid(struct ipa *ipa, const struct ipa_mem *mem,
 			    : field_max(IP_FLTRT_FLAGS_NHASH_ADDR_FMASK);
 	if (mem->offset > offset_max ||
 	    ipa->mem_offset > offset_max - mem->offset) {
-		dev_err(dev, "IPv%c %s%s table region offset too large "
-			      "(0x%04x + 0x%04x > 0x%04x)\n",
-			      ipv6 ? '6' : '4', hashed ? "hashed " : "",
-			      route ? "route" : "filter",
-			      ipa->mem_offset, mem->offset, offset_max);
+		dev_err(dev, "IPv%c %s%s table region offset too large\n",
+			ipv6 ? '6' : '4', hashed ? "hashed " : "",
+			route ? "route" : "filter");
+		dev_err(dev, "    (0x%04x + 0x%04x > 0x%04x)\n",
+			ipa->mem_offset, mem->offset, offset_max);
+
 		return false;
 	}
 
 	if (mem->offset > ipa->mem_size ||
 	    mem->size > ipa->mem_size - mem->offset) {
-		dev_err(dev, "IPv%c %s%s table region out of range "
-			      "(0x%04x + 0x%04x > 0x%04x)\n",
-			      ipv6 ? '6' : '4', hashed ? "hashed " : "",
-			      route ? "route" : "filter",
-			      mem->offset, mem->size, ipa->mem_size);
+		dev_err(dev, "IPv%c %s%s table region out of range\n",
+			ipv6 ? '6' : '4', hashed ? "hashed " : "",
+			route ? "route" : "filter");
+		dev_err(dev, "    (0x%04x + 0x%04x > 0x%04x)\n",
+			mem->offset, mem->size, ipa->mem_size);
+
 		return false;
 	}
 
@@ -205,22 +207,35 @@ static bool ipa_cmd_header_valid(struct ipa *ipa)
 	u32 size_max;
 	u32 size;
 
+	/* In ipa_cmd_hdr_init_local_add() we record the offset and size
+	 * of the header table memory area.  Make sure the offset and size
+	 * fit in the fields that need to hold them, and that the entire
+	 * range is within the overall IPA memory range.
+	 */
 	offset_max = field_max(HDR_INIT_LOCAL_FLAGS_HDR_ADDR_FMASK);
 	if (mem->offset > offset_max ||
 	    ipa->mem_offset > offset_max - mem->offset) {
-		dev_err(dev, "header table region offset too large "
-			      "(0x%04x + 0x%04x > 0x%04x)\n",
-			      ipa->mem_offset + mem->offset, offset_max);
+		dev_err(dev, "header table region offset too large\n");
+		dev_err(dev, "    (0x%04x + 0x%04x > 0x%04x)\n",
+			ipa->mem_offset, mem->offset, offset_max);
+
 		return false;
 	}
 
 	size_max = field_max(HDR_INIT_LOCAL_FLAGS_TABLE_SIZE_FMASK);
 	size = ipa->mem[IPA_MEM_MODEM_HEADER].size;
 	size += ipa->mem[IPA_MEM_AP_HEADER].size;
-	if (mem->offset > ipa->mem_size || size > ipa->mem_size - mem->offset) {
-		dev_err(dev, "header table region out of range "
-			      "(0x%04x + 0x%04x > 0x%04x)\n",
-			      mem->offset, size, ipa->mem_size);
+	if (size > size_max) {
+		dev_err(dev, "header table region too large\n");
+		dev_err(dev, "    (0x%04x > 0x%04x)\n", size, size_max);
+
+		return false;
+	}
+	if (size > ipa->mem_size || mem->offset > ipa->mem_size - size) {
+		dev_err(dev, "header table region out of range\n");
+		dev_err(dev, "    (0x%04x + 0x%04x > 0x%04x)\n",
+			mem->offset, size, ipa->mem_size);
+
 		return false;
 	}
 
-- 
2.27.0

[PATCH net-next 2/4] net: ipa: fix IPA validation

[Alex Elder]

There are blocks of IPA code that sanity-check various values, at
compile time where possible.  Most of these checks can be done once
during development but skipped for normal operation.  These checks
permit the driver to make certain assumptions, thereby avoiding the
need for runtime error checking.

The checks are defined conditionally, but not consistently.  In
some cases IPA_VALIDATION enables the optional checks, while in
others IPA_VALIDATE is used.

Fix this by using IPA_VALIDATION consistently.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/Makefile       | 2 +-
 drivers/net/ipa/gsi_trans.c    | 8 ++++----
 drivers/net/ipa/ipa_cmd.c      | 4 ++--
 drivers/net/ipa/ipa_cmd.h      | 6 +++---
 drivers/net/ipa/ipa_endpoint.c | 6 +++---
 drivers/net/ipa/ipa_main.c     | 6 +++---
 drivers/net/ipa/ipa_mem.c      | 6 +++---
 drivers/net/ipa/ipa_table.c    | 6 +++---
 drivers/net/ipa/ipa_table.h    | 6 +++---
 9 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ipa/Makefile b/drivers/net/ipa/Makefile
index afe5df1e6eeee..014ae36ac6004 100644
--- a/drivers/net/ipa/Makefile
+++ b/drivers/net/ipa/Makefile
@@ -1,5 +1,5 @@
 # Un-comment the next line if you want to validate configuration data
-#ccflags-y		+=	-DIPA_VALIDATE
+# ccflags-y		+=	-DIPA_VALIDATION
 
 obj-$(CONFIG_QCOM_IPA)	+=	ipa.o
 
diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
index 6c3ed5b17b80c..284063b39b33c 100644
--- a/drivers/net/ipa/gsi_trans.c
+++ b/drivers/net/ipa/gsi_trans.c
@@ -90,14 +90,14 @@ int gsi_trans_pool_init(struct gsi_trans_pool *pool, size_t size, u32 count,
 {
 	void *virt;
 
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 	if (!size || size % 8)
 		return -EINVAL;
 	if (count < max_alloc)
 		return -EINVAL;
 	if (!max_alloc)
 		return -EINVAL;
-#endif /* IPA_VALIDATE */
+#endif /* IPA_VALIDATION */
 
 	/* By allocating a few extra entries in our pool (one less
 	 * than the maximum number that will be requested in a
@@ -140,14 +140,14 @@ int gsi_trans_pool_init_dma(struct device *dev, struct gsi_trans_pool *pool,
 	dma_addr_t addr;
 	void *virt;
 
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 	if (!size || size % 8)
 		return -EINVAL;
 	if (count < max_alloc)
 		return -EINVAL;
 	if (!max_alloc)
 		return -EINVAL;
-#endif /* IPA_VALIDATE */
+#endif /* IPA_VALIDATION */
 
 	/* Don't let allocations cross a power-of-two boundary */
 	size = __roundup_pow_of_two(size);
diff --git a/drivers/net/ipa/ipa_cmd.c b/drivers/net/ipa/ipa_cmd.c
index b40f031a905a7..87e1ca2e27106 100644
--- a/drivers/net/ipa/ipa_cmd.c
+++ b/drivers/net/ipa/ipa_cmd.c
@@ -162,7 +162,7 @@ static void ipa_cmd_validate_build(void)
 #undef TABLE_SIZE
 }
 
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 
 /* Validate a memory region holding a table */
 bool ipa_cmd_table_valid(struct ipa *ipa, const struct ipa_mem *mem,
@@ -316,7 +316,7 @@ bool ipa_cmd_data_valid(struct ipa *ipa)
 	return true;
 }
 
-#endif /* IPA_VALIDATE */
+#endif /* IPA_VALIDATION */
 
 int ipa_cmd_pool_init(struct gsi_channel *channel, u32 tre_max)
 {
diff --git a/drivers/net/ipa/ipa_cmd.h b/drivers/net/ipa/ipa_cmd.h
index 6dd3d35cf315d..429245f075122 100644
--- a/drivers/net/ipa/ipa_cmd.h
+++ b/drivers/net/ipa/ipa_cmd.h
@@ -50,7 +50,7 @@ struct ipa_cmd_info {
 	enum dma_data_direction direction;
 };
 
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 
 /**
  * ipa_cmd_table_valid() - Validate a memory region holding a table
@@ -73,7 +73,7 @@ bool ipa_cmd_table_valid(struct ipa *ipa, const struct ipa_mem *mem,
  */
 bool ipa_cmd_data_valid(struct ipa *ipa);
 
-#else /* !IPA_VALIDATE */
+#else /* !IPA_VALIDATION */
 
 static inline bool ipa_cmd_table_valid(struct ipa *ipa,
 				       const struct ipa_mem *mem, bool route,
@@ -87,7 +87,7 @@ static inline bool ipa_cmd_data_valid(struct ipa *ipa)
 	return true;
 }
 
-#endif /* !IPA_VALIDATE */
+#endif /* !IPA_VALIDATION */
 
 /**
  * ipa_cmd_pool_init() - initialize command channel pools
diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 7209ee3c31244..1a4de4e9eafcd 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -75,7 +75,7 @@ struct ipa_status {
 #define IPA_STATUS_FLAGS1_RT_RULE_ID_FMASK	GENMASK(31, 22)
 #define IPA_STATUS_FLAGS2_TAG_FMASK		GENMASK_ULL(63, 16)
 
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 
 static bool ipa_endpoint_data_valid_one(struct ipa *ipa, u32 count,
 			    const struct ipa_gsi_endpoint_data *all_data,
@@ -225,7 +225,7 @@ static bool ipa_endpoint_data_valid(struct ipa *ipa, u32 count,
 	return true;
 }
 
-#else /* !IPA_VALIDATE */
+#else /* !IPA_VALIDATION */
 
 static bool ipa_endpoint_data_valid(struct ipa *ipa, u32 count,
 				    const struct ipa_gsi_endpoint_data *data)
@@ -233,7 +233,7 @@ static bool ipa_endpoint_data_valid(struct ipa *ipa, u32 count,
 	return true;
 }
 
-#endif /* !IPA_VALIDATE */
+#endif /* !IPA_VALIDATION */
 
 /* Allocate a transaction to use on a non-command endpoint */
 static struct gsi_trans *ipa_endpoint_trans_alloc(struct ipa_endpoint *endpoint,
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index d354e3e65ec50..d95909a4aef4c 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -734,7 +734,7 @@ MODULE_DEVICE_TABLE(of, ipa_match);
  * */
 static void ipa_validate_build(void)
 {
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 	/* At one time we assumed a 64-bit build, allowing some do_div()
 	 * calls to be replaced by simple division or modulo operations.
 	 * We currently only perform divide and modulo operations on u32,
@@ -768,7 +768,7 @@ static void ipa_validate_build(void)
 	BUILD_BUG_ON(!ipa_aggr_granularity_val(IPA_AGGR_GRANULARITY));
 	BUILD_BUG_ON(ipa_aggr_granularity_val(IPA_AGGR_GRANULARITY) >
 			field_max(AGGR_GRANULARITY_FMASK));
-#endif /* IPA_VALIDATE */
+#endif /* IPA_VALIDATION */
 }
 
 /**
@@ -808,7 +808,7 @@ static int ipa_probe(struct platform_device *pdev)
 	/* Get configuration data early; needed for clock initialization */
 	data = of_device_get_match_data(dev);
 	if (!data) {
-		/* This is really IPA_VALIDATE (should never happen) */
+		/* This is really IPA_VALIDATION (should never happen) */
 		dev_err(dev, "matched hardware not supported\n");
 		return -ENODEV;
 	}
diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
index f25029b9ec857..6a239d49bb559 100644
--- a/drivers/net/ipa/ipa_mem.c
+++ b/drivers/net/ipa/ipa_mem.c
@@ -100,7 +100,7 @@ void ipa_mem_teardown(struct ipa *ipa)
 	/* Nothing to do */
 }
 
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 
 static bool ipa_mem_valid(struct ipa *ipa, enum ipa_mem_id mem_id)
 {
@@ -127,14 +127,14 @@ static bool ipa_mem_valid(struct ipa *ipa, enum ipa_mem_id mem_id)
 	return false;
 }
 
-#else /* !IPA_VALIDATE */
+#else /* !IPA_VALIDATION */
 
 static bool ipa_mem_valid(struct ipa *ipa, enum ipa_mem_id mem_id)
 {
 	return true;
 }
 
-#endif /*! IPA_VALIDATE */
+#endif /*! IPA_VALIDATION */
 
 /**
  * ipa_mem_config() - Configure IPA shared memory
diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
index 988f2c2886b95..aa8b3ce7e21d9 100644
--- a/drivers/net/ipa/ipa_table.c
+++ b/drivers/net/ipa/ipa_table.c
@@ -113,7 +113,7 @@
  */
 #define IPA_ZERO_RULE_SIZE		(2 * sizeof(__le32))
 
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 
 /* Check things that can be validated at build time. */
 static void ipa_table_validate_build(void)
@@ -225,13 +225,13 @@ bool ipa_filter_map_valid(struct ipa *ipa, u32 filter_map)
 	return true;
 }
 
-#else /* !IPA_VALIDATE */
+#else /* !IPA_VALIDATION */
 static void ipa_table_validate_build(void)
 
 {
 }
 
-#endif /* !IPA_VALIDATE */
+#endif /* !IPA_VALIDATION */
 
 /* Zero entry count means no table, so just return a 0 address */
 static dma_addr_t ipa_table_addr(struct ipa *ipa, bool filter_mask, u16 count)
diff --git a/drivers/net/ipa/ipa_table.h b/drivers/net/ipa/ipa_table.h
index 889c2e93b1223..6017d60fb870e 100644
--- a/drivers/net/ipa/ipa_table.h
+++ b/drivers/net/ipa/ipa_table.h
@@ -19,7 +19,7 @@ struct ipa;
 /* The maximum number of route table entries (IPv4, IPv6; hashed or not) */
 #define IPA_ROUTE_COUNT_MAX	15
 
-#ifdef IPA_VALIDATE
+#ifdef IPA_VALIDATION
 
 /**
  * ipa_table_valid() - Validate route and filter table memory regions
@@ -37,7 +37,7 @@ bool ipa_table_valid(struct ipa *ipa);
  */
 bool ipa_filter_map_valid(struct ipa *ipa, u32 filter_mask);
 
-#else /* !IPA_VALIDATE */
+#else /* !IPA_VALIDATION */
 
 static inline bool ipa_table_valid(struct ipa *ipa)
 {
@@ -49,7 +49,7 @@ static inline bool ipa_filter_map_valid(struct ipa *ipa, u32 filter_mask)
 	return true;
 }
 
-#endif /* !IPA_VALIDATE */
+#endif /* !IPA_VALIDATION */
 
 /**
  * ipa_table_hash_support() - Return true if hashed tables are supported
-- 
2.27.0

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

[Alex Elder]

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 <elder@linaro.org>
---
 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)
+
+/* 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

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

[Leon Romanovsky]

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

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

[Alex Elder]

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

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

[Leon Romanovsky]

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

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

[Alex Elder]

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

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

[Andrew Lunn]

> 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 net-next 4/4] net: ipa: activate some commented assertions

[Alex Elder]

Convert some commented assertion statements into real calls to
ipa_assert().  If the IPA device pointer is available, provide it,
otherwise pass NULL for that.

There are lots more places to convert, but this serves as an initial
verification of the new mechanism.  The assertions here implement
both runtime and build-time assertions, both with and without the
device pointer.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_reg.h   | 7 ++++---
 drivers/net/ipa/ipa_table.c | 5 ++++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
index 732e691e9aa62..d0de85de9f08d 100644
--- a/drivers/net/ipa/ipa_reg.h
+++ b/drivers/net/ipa/ipa_reg.h
@@ -9,6 +9,7 @@
 #include <linux/bitfield.h>
 
 #include "ipa_version.h"
+#include "ipa_assert.h"
 
 struct ipa;
 
@@ -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);
 
 	return 0x00000000;
 }
@@ -413,7 +414,7 @@ static inline u32 ipa_header_size_encoded(enum ipa_version version,
 
 	val = u32_encode_bits(size, HDR_LEN_FMASK);
 	if (version < IPA_VERSION_4_5) {
-		/* ipa_assert(header_size == size); */
+		ipa_assert(NULL, header_size == size);
 		return val;
 	}
 
@@ -433,7 +434,7 @@ static inline u32 ipa_metadata_offset_encoded(enum ipa_version version,
 
 	val = u32_encode_bits(off, HDR_OFST_METADATA_FMASK);
 	if (version < IPA_VERSION_4_5) {
-		/* ipa_assert(offset == off); */
+		ipa_assert(NULL, offset == off);
 		return val;
 	}
 
diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
index aa8b3ce7e21d9..7784b42fbaccc 100644
--- a/drivers/net/ipa/ipa_table.c
+++ b/drivers/net/ipa/ipa_table.c
@@ -23,6 +23,7 @@
 #include "ipa_cmd.h"
 #include "gsi.h"
 #include "gsi_trans.h"
+#include "ipa_assert.h"
 
 /**
  * DOC: IPA Filter and Route Tables
@@ -237,11 +238,13 @@ static void ipa_table_validate_build(void)
 static dma_addr_t ipa_table_addr(struct ipa *ipa, bool filter_mask, u16 count)
 {
 	u32 skip;
+	u32 max;
 
 	if (!count)
 		return 0;
 
-/* assert(count <= max_t(u32, IPA_FILTER_COUNT_MAX, IPA_ROUTE_COUNT_MAX)); */
+	max = max_t(u32, IPA_FILTER_COUNT_MAX, IPA_ROUTE_COUNT_MAX);
+	ipa_assert(&ipa->pdev->dev, max);
 
 	/* Skip over the zero rule and possibly the filter mask */
 	skip = filter_mask ? 1 : 2;
-- 
2.27.0

Re: [PATCH net-next 4/4] net: ipa: activate some commented assertions

[Leon Romanovsky]

On Thu, Mar 18, 2021 at 11:29:23PM -0500, Alex Elder wrote:
> Convert some commented assertion statements into real calls to
> ipa_assert().  If the IPA device pointer is available, provide it,
> otherwise pass NULL for that.
> 
> There are lots more places to convert, but this serves as an initial
> verification of the new mechanism.  The assertions here implement
> both runtime and build-time assertions, both with and without the
> device pointer.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/ipa_reg.h   | 7 ++++---
>  drivers/net/ipa/ipa_table.c | 5 ++++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
> index 732e691e9aa62..d0de85de9f08d 100644
> --- a/drivers/net/ipa/ipa_reg.h
> +++ b/drivers/net/ipa/ipa_reg.h
> @@ -9,6 +9,7 @@
>  #include <linux/bitfield.h>
>  
>  #include "ipa_version.h"
> +#include "ipa_assert.h"
>  
>  struct ipa;
>  
> @@ -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.

Thanks

Re: [PATCH net-next 4/4] net: ipa: activate some commented assertions

[Alex Elder]

On 3/19/21 12:00 AM, Leon Romanovsky wrote:
> On Thu, Mar 18, 2021 at 11:29:23PM -0500, Alex Elder wrote:
>> Convert some commented assertion statements into real calls to
>> ipa_assert().  If the IPA device pointer is available, provide it,
>> otherwise pass NULL for that.
>>
>> There are lots more places to convert, but this serves as an initial
>> verification of the new mechanism.  The assertions here implement
>> both runtime and build-time assertions, both with and without the
>> device pointer.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>   drivers/net/ipa/ipa_reg.h   | 7 ++++---
>>   drivers/net/ipa/ipa_table.c | 5 ++++-
>>   2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
>> index 732e691e9aa62..d0de85de9f08d 100644
>> --- a/drivers/net/ipa/ipa_reg.h
>> +++ b/drivers/net/ipa/ipa_reg.h
>> @@ -9,6 +9,7 @@
>>   #include <linux/bitfield.h>
>>   
>>   #include "ipa_version.h"
>> +#include "ipa_assert.h"
>>   
>>   struct ipa;
>>   
>> @@ -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.

					-Alex

> 
> Thanks
> 

Re: [PATCH net-next 4/4] net: ipa: activate some commented assertions

[Leon Romanovsky]

On Fri, Mar 19, 2021 at 07:40:21AM -0500, Alex Elder wrote:
> On 3/19/21 12:00 AM, Leon Romanovsky wrote:
> > On Thu, Mar 18, 2021 at 11:29:23PM -0500, Alex Elder wrote:
> > > Convert some commented assertion statements into real calls to
> > > ipa_assert().  If the IPA device pointer is available, provide it,
> > > otherwise pass NULL for that.
> > > 
> > > There are lots more places to convert, but this serves as an initial
> > > verification of the new mechanism.  The assertions here implement
> > > both runtime and build-time assertions, both with and without the
> > > device pointer.
> > > 
> > > Signed-off-by: Alex Elder <elder@linaro.org>
> > > ---
> > >   drivers/net/ipa/ipa_reg.h   | 7 ++++---
> > >   drivers/net/ipa/ipa_table.c | 5 ++++-
> > >   2 files changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
> > > index 732e691e9aa62..d0de85de9f08d 100644
> > > --- a/drivers/net/ipa/ipa_reg.h
> > > +++ b/drivers/net/ipa/ipa_reg.h
> > > @@ -9,6 +9,7 @@
> > >   #include <linux/bitfield.h>
> > >   #include "ipa_version.h"
> > > +#include "ipa_assert.h"
> > >   struct ipa;
> > > @@ -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?

> 
> 					-Alex
> 
> > 
> > Thanks
> > 
> 

Re: [PATCH net-next 4/4] net: ipa: activate some commented assertions

[Alex Elder]

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

Re: [PATCH net-next 4/4] net: ipa: activate some commented assertions

[Andrew Lunn]

> @@ -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);

Hi Alex

It is impossible to see just looking what the NULL means. And given
its the first parameter, it should be quite important. I find this API
pretty bad.

    Andrew

Re: [PATCH net-next 4/4] net: ipa: activate some commented assertions

[Alex Elder]

On 3/19/21 1:32 PM, Andrew Lunn 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);
> 
> Hi Alex
> 
> It is impossible to see just looking what the NULL means. And given
> its the first parameter, it should be quite important. I find this API
> pretty bad.

I actually don't like the first argument.  I would have
supplied the main IPA pointer, but that isn't always
visible or available (the GSI code doesn't have the
IPA pointer definition).  So I thought instead I could
at least supply the underlying device if available,
and use dev_err().

But I wouldn't mind just getting rid of the first
argument and having a failed assertion always call
pr_err() and not dev_err().

The thing of most value to me the asserted condition.

Thoughts?

					-Alex

Re: [PATCH net-next 4/4] net: ipa: activate some commented assertions

[Andrew Lunn]

On Fri, Mar 19, 2021 at 04:18:32PM -0500, Alex Elder wrote:
> On 3/19/21 1:32 PM, Andrew Lunn 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);
> > 
> > Hi Alex
> > 
> > It is impossible to see just looking what the NULL means. And given
> > its the first parameter, it should be quite important. I find this API
> > pretty bad.
> 
> I actually don't like the first argument.  I would have
> supplied the main IPA pointer, but that isn't always
> visible or available (the GSI code doesn't have the
> IPA pointer definition).  So I thought instead I could
> at least supply the underlying device if available,
> and use dev_err().
> 
> But I wouldn't mind just getting rid of the first
> argument and having a failed assertion always call
> pr_err() and not dev_err().
> 
> The thing of most value to me the asserted condition.

Hi Alex

What you really want to be looking at is adding a
WARN_ON_DEV(dev, condition) macro.

	 Andrew

Re: [PATCH net-next 0/4] net: ipa: fix validation

[Alex Elder]

On 3/18/21 11:29 PM, Alex Elder wrote:
> There is sanity checking code in the IPA driver that's meant to be
> enabled only during development.  This allows the driver to make
> certain assumptions, but not have to verify those assumptions are
> true at (operational) runtime.  This code is built conditional on
> IPA_VALIDATION, set (if desired) inside the IPA makefile.

Given the pushback on the ipa_assert() patch I will send
out version 2 of this series, omitting the two patches
that involve assertions.

I still think there's a case for my proposal, but I'm
going to move on for now and try to find other ways
to do what I want.  In some cases BUILD_BUG_ON() or
WARN_ON_DEV() could be used.  In other spots, I might
be able to use dev_dbg() for checking things only
while developing.  But there remain a few cases where
none of these options is quite right.

If I ever want to suggest an assertion again I'll do
it as an RFC and will copy Leon and Andrew, to make
sure they can provide input.

Thanks.

					-Alex

> Unfortunately, this validation code has some errors.  First, there
> are some mismatched arguments supplied to some dev_err() calls in
> ipa_cmd_table_valid() and ipa_cmd_header_valid(), and these are
> exposed if validation is enabled.  Second, the tag that enables
> this conditional code isn't used consistently (it's IPA_VALIDATE
> in some spots and IPA_VALIDATION in others).
> 
> This series fixes those two problems with the conditional validation
> code.
> 
> In addition, this series introduces some new assertion macros.  I
> have been meaning to add this for a long time.  There are comments
> indicating places where assertions could be checked throughout the
> code.
> 
> The macros are designed so that any asserted condition will be
> checked at compile time if possible.  Otherwise, the condition
> will be checked at runtime *only* if IPA_VALIDATION is enabled,
> and ignored otherwise.
> 
> NOTE:  The third patch produces two bogus (but understandable)
> warnings from checkpatch.pl.  It does not recognize that the "expr"
> argument passed to those macros aren't actually evaluated more than
> once.  In both cases, all but one reference is consumed by the
> preprocessor or compiler.
> 
> A final patch converts a handful of commented assertions into
> "real" ones.  Some existing validation code can done more simply
> with assertions, so over time such cases will be converted.  For
> now though, this series adds this assertion capability.
> 
> 					-Alex
> 
> Alex Elder (4):
>    net: ipa: fix init header command validation
>    net: ipa: fix IPA validation
>    net: ipa: introduce ipa_assert()
>    net: ipa: activate some commented assertions
> 
>   drivers/net/ipa/Makefile       |  2 +-
>   drivers/net/ipa/gsi_trans.c    |  8 ++---
>   drivers/net/ipa/ipa_assert.h   | 50 ++++++++++++++++++++++++++++++++
>   drivers/net/ipa/ipa_cmd.c      | 53 ++++++++++++++++++++++------------
>   drivers/net/ipa/ipa_cmd.h      |  6 ++--
>   drivers/net/ipa/ipa_endpoint.c |  6 ++--
>   drivers/net/ipa/ipa_main.c     |  6 ++--
>   drivers/net/ipa/ipa_mem.c      |  6 ++--
>   drivers/net/ipa/ipa_reg.h      |  7 +++--
>   drivers/net/ipa/ipa_table.c    | 11 ++++---
>   drivers/net/ipa/ipa_table.h    |  6 ++--
>   11 files changed, 115 insertions(+), 46 deletions(-)
>   create mode 100644 drivers/net/ipa/ipa_assert.h
> 

Re: [PATCH net-next 0/4] net: ipa: fix validation

[Alex Elder]

On 3/20/21 8:24 AM, Alex Elder wrote:
> On 3/18/21 11:29 PM, Alex Elder wrote:
>> There is sanity checking code in the IPA driver that's meant to be
>> enabled only during development.  This allows the driver to make
>> certain assumptions, but not have to verify those assumptions are
>> true at (operational) runtime.  This code is built conditional on
>> IPA_VALIDATION, set (if desired) inside the IPA makefile.
> 
> Given the pushback on the ipa_assert() patch I will send
> out version 2 of this series, omitting the two patches
> that involve assertions.

I posted version 2, but I think dropping two patches
without changing the subject might have messed up
the robots.  I don't know how to fix that and don't
want to make any more trouble by trying.

If there's something I can do, someone please tell me.

					-Alex

> I still think there's a case for my proposal, but I'm
> going to move on for now and try to find other ways
> to do what I want.  In some cases BUILD_BUG_ON() or
> WARN_ON_DEV() could be used.  In other spots, I might
> be able to use dev_dbg() for checking things only
> while developing.  But there remain a few cases where
> none of these options is quite right.
> 
> If I ever want to suggest an assertion again I'll do
> it as an RFC and will copy Leon and Andrew, to make
> sure they can provide input.
> 
> Thanks.
> 
>                      -Alex
> 
>> Unfortunately, this validation code has some errors.  First, there
>> are some mismatched arguments supplied to some dev_err() calls in
>> ipa_cmd_table_valid() and ipa_cmd_header_valid(), and these are
>> exposed if validation is enabled.  Second, the tag that enables
>> this conditional code isn't used consistently (it's IPA_VALIDATE
>> in some spots and IPA_VALIDATION in others).
>>
>> This series fixes those two problems with the conditional validation
>> code.
>>
>> In addition, this series introduces some new assertion macros.  I
>> have been meaning to add this for a long time.  There are comments
>> indicating places where assertions could be checked throughout the
>> code.
>>
>> The macros are designed so that any asserted condition will be
>> checked at compile time if possible.  Otherwise, the condition
>> will be checked at runtime *only* if IPA_VALIDATION is enabled,
>> and ignored otherwise.
>>
>> NOTE:  The third patch produces two bogus (but understandable)
>> warnings from checkpatch.pl.  It does not recognize that the "expr"
>> argument passed to those macros aren't actually evaluated more than
>> once.  In both cases, all but one reference is consumed by the
>> preprocessor or compiler.
>>
>> A final patch converts a handful of commented assertions into
>> "real" ones.  Some existing validation code can done more simply
>> with assertions, so over time such cases will be converted.  For
>> now though, this series adds this assertion capability.
>>
>>                     -Alex
>>
>> Alex Elder (4):
>>    net: ipa: fix init header command validation
>>    net: ipa: fix IPA validation
>>    net: ipa: introduce ipa_assert()
>>    net: ipa: activate some commented assertions
>>
>>   drivers/net/ipa/Makefile       |  2 +-
>>   drivers/net/ipa/gsi_trans.c    |  8 ++---
>>   drivers/net/ipa/ipa_assert.h   | 50 ++++++++++++++++++++++++++++++++
>>   drivers/net/ipa/ipa_cmd.c      | 53 ++++++++++++++++++++++------------
>>   drivers/net/ipa/ipa_cmd.h      |  6 ++--
>>   drivers/net/ipa/ipa_endpoint.c |  6 ++--
>>   drivers/net/ipa/ipa_main.c     |  6 ++--
>>   drivers/net/ipa/ipa_mem.c      |  6 ++--
>>   drivers/net/ipa/ipa_reg.h      |  7 +++--
>>   drivers/net/ipa/ipa_table.c    | 11 ++++---
>>   drivers/net/ipa/ipa_table.h    |  6 ++--
>>   11 files changed, 115 insertions(+), 46 deletions(-)
>>   create mode 100644 drivers/net/ipa/ipa_assert.h
>>
>