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

[PATCH net-next v2 0/2] 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.

Version 2 removes the two patches that introduced ipa_assert().  It
also modifies the description in the first patch so that it mentions
the changes made to ipa_cmd_table_valid().

					-Alex

Alex Elder (2):
  net: ipa: fix init header command validation
  net: ipa: fix IPA validation

 drivers/net/ipa/Makefile       |  2 +-
 drivers/net/ipa/gsi_trans.c    |  8 ++---
 drivers/net/ipa/ipa_cmd.c      | 54 ++++++++++++++++++++++------------
 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, 58 insertions(+), 42 deletions(-)

-- 
2.27.0

[PATCH net-next v2 1/2] 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.

Finally, update ipa_cmd_table_valid() so the format of messages
printed for errors matches what's done in ipa_cmd_header_valid().

Signed-off-by: Alex Elder <elder@linaro.org>
---
v2: - Updated description to mention ipa_cmd_table_valid() changes.

 drivers/net/ipa/ipa_cmd.c | 50 ++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ipa/ipa_cmd.c b/drivers/net/ipa/ipa_cmd.c
index 35e35852c25c5..d73b03a80ef89 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,36 @@ 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 size too large\n");
+		dev_err(dev, "    (0x%04x > 0x%08x)\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 v2 2/2] 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 d73b03a80ef89..2a4db902a728b 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,
@@ -317,7 +317,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

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

[Leon Romanovsky]

On Sat, Mar 20, 2021 at 09:17:29AM -0500, Alex Elder wrote:
> 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

Maybe netdev folks think differently here, but general rule that dead
code and closed code is such, is not acceptable to in Linux kernel.

<...>

>  
> -#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 */

If it is possible to supply those values, the check should be always and
not only under some closed config option.

>  
>  	/* 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 */

Same

<...>

>  {
> -#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 */

BUILD_BUG_ON()s are checked during compilation and not during runtime
like IPA_VALIDATION promised.

IMHO, the issue here is that this IPA code isn't release quality but
some debug drop variant and it is far from expected from submitted code.

Thanks

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

[Alex Elder]

On 3/21/21 3:21 AM, Leon Romanovsky wrote:
> On Sat, Mar 20, 2021 at 09:17:29AM -0500, Alex Elder wrote:
>> 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
> 
> Maybe netdev folks think differently here, but general rule that dead
> code and closed code is such, is not acceptable to in Linux kernel.
> 
> <...>

What is the purpose of CONFIG_KGDB?  Or CONFIG_DEBUG_KERNEL?
Would you prefer I expose this through a kconfig option?  I
intentionally did not do that, because I really intended it
to be only for development, so defined it in the Makefile.
But I have no objection to making it configurable that way.

>> -#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 */
> 
> If it is possible to supply those values, the check should be always and
> not only under some closed config option.

These are assertions.

There is no need to test them for working code.  If
I run the code successfully with these tests enabled
exactly once, and they are satisfied, then every time
the code is run thereafter they will pass.  So I want
to check them when debugging/developing only.  That
way there is a mistake, it gets caught, but otherwise
there's no pointless argument checking done.

I'll explain the first check; the others have similar
explanation.

In the current code, the passed size is sizeof(struct)
for three separate structures.
   - If the structure size changes, I want to be
     sure the constraint is still honored
   - The code will break of someone happens
     to pass a size of 0.  I don't expect that to
     ever happen, but this states that requirement.

This is an optimization, basically, but one that
allows the assumed conditions to be optionally
verified.

>>   	/* 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 */
> 
> Same
> 
> <...>
> 
>>   {
>> -#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 */
> 
> BUILD_BUG_ON()s are checked during compilation and not during runtime
> like IPA_VALIDATION promised.

So I should update the description.  But I'm not sure where
you are referring to.  Here is the first line of the patch
description:
   There are blocks of IPA code that sanity-check various
   values, at compile time where possible.

> IMHO, the issue here is that this IPA code isn't release quality but
> some debug drop variant and it is far from expected from submitted code.

Doesn't sound very humble, IMHO.

This code was found acceptable and merged for mainline a
year ago.  At that time it supported IPA on the SDM845 SoC
(IPA v3.5.1).  Had it not been merged, I would have continued
refining the code out-of-tree until it could be merged.  But
now, it's upstream, so anything I want to do to make it better
must be done upstream.

Since last year it has undergone considerable development,
including adding support for the SC7180 SoC (IPA v4.2).  I
am now in the process of getting things posted for review
so IPA versions 4.5, 4.9, and 4.11 are supported.  With any
luck all that will be done in this merge cycle; we'll see.

Most of what I've been doing is gradually transforming
things to support the new hardware.  But in the process
I'm also improving what's there so that it is better
organized, more consistent, more understandable, and
maintainable.

I have explained this previously, but this code was derived
from Qualcomm "downstream" code.  Much was done toward
getting it into the upstream kernel, including carving out
great deal of code, and removing functionality to focus on
just *one* target platform.

Now that it's upstream, the aim is to add back functionality,
ideally to support all current and future Qualcomm IPA hardware,
and eventually (this year) to support some of the features
(hardware filtering/routing/NAT) that were removed to make
the code simpler.

I'm doing a lot of development on this driver, yes.  But
it doesn't mean it's broken, it means it's improving.

					-Alex

> Thanks
> 

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

[Leon Romanovsky]

On Sun, Mar 21, 2021 at 08:21:24AM -0500, Alex Elder wrote:
> On 3/21/21 3:21 AM, Leon Romanovsky wrote:
> > On Sat, Mar 20, 2021 at 09:17:29AM -0500, Alex Elder wrote:
> > > 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
> > 
> > Maybe netdev folks think differently here, but general rule that dead
> > code and closed code is such, is not acceptable to in Linux kernel.
> > 
> > <...>
> 
> What is the purpose of CONFIG_KGDB?  Or CONFIG_DEBUG_KERNEL?
> Would you prefer I expose this through a kconfig option?  I
> intentionally did not do that, because I really intended it
> to be only for development, so defined it in the Makefile.
> But I have no objection to making it configurable that way.

I prefer you to follow netdev/linux kernel rules of development.
The upstream repository and drivers/net/* folder especially are not
the place to put code used for the development.

> 
> > > -#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 */
> > 
> > If it is possible to supply those values, the check should be always and
> > not only under some closed config option.
> 
> These are assertions.
> 
> There is no need to test them for working code.  If
> I run the code successfully with these tests enabled
> exactly once, and they are satisfied, then every time
> the code is run thereafter they will pass.  So I want
> to check them when debugging/developing only.  That
> way there is a mistake, it gets caught, but otherwise
> there's no pointless argument checking done.
> 
> I'll explain the first check; the others have similar
> explanation.
> 
> In the current code, the passed size is sizeof(struct)
> for three separate structures.
>   - If the structure size changes, I want to be
>     sure the constraint is still honored
>   - The code will break of someone happens
>     to pass a size of 0.  I don't expect that to
>     ever happen, but this states that requirement.
> 
> This is an optimization, basically, but one that
> allows the assumed conditions to be optionally
> verified.

Everything above as an outcome of attempting to mix constant vs. run-time
checks. If "size" is constant, the use of BUILD_BIG_ON() will help not only
you but other developers to catch the errors too. The assumption that you alone
are working on this code, can or can't be correct.

If "size" is not constant, you should check it always.

> 
> > >   	/* 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 */
> > 
> > Same
> > 
> > <...>
> > 
> > >   {
> > > -#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 */
> > 
> > BUILD_BUG_ON()s are checked during compilation and not during runtime
> > like IPA_VALIDATION promised.
> 
> So I should update the description.  But I'm not sure where
> you are referring to.  Here is the first line of the patch
> description:
>   There are blocks of IPA code that sanity-check various
>   values, at compile time where possible.

I'm suggesting to review if IPA_VALIDATION is truly needed.

> 
> > IMHO, the issue here is that this IPA code isn't release quality but
> > some debug drop variant and it is far from expected from submitted code.
> 
> Doesn't sound very humble, IMHO.

Sorry about that.

> 
> This code was found acceptable and merged for mainline a
> year ago.  At that time it supported IPA on the SDM845 SoC
> (IPA v3.5.1).  Had it not been merged, I would have continued
> refining the code out-of-tree until it could be merged.  But
> now, it's upstream, so anything I want to do to make it better
> must be done upstream.

The upstream just doesn't need to be your testing ground.

> 
> Since last year it has undergone considerable development,
> including adding support for the SC7180 SoC (IPA v4.2).  I
> am now in the process of getting things posted for review
> so IPA versions 4.5, 4.9, and 4.11 are supported.  With any
> luck all that will be done in this merge cycle; we'll see.
> 
> Most of what I've been doing is gradually transforming
> things to support the new hardware.  But in the process
> I'm also improving what's there so that it is better
> organized, more consistent, more understandable, and
> maintainable.
> 
> I have explained this previously, but this code was derived
> from Qualcomm "downstream" code.  Much was done toward
> getting it into the upstream kernel, including carving out
> great deal of code, and removing functionality to focus on
> just *one* target platform.
> 
> Now that it's upstream, the aim is to add back functionality,
> ideally to support all current and future Qualcomm IPA hardware,
> and eventually (this year) to support some of the features
> (hardware filtering/routing/NAT) that were removed to make
> the code simpler.
> 
> I'm doing a lot of development on this driver, yes.  But
> it doesn't mean it's broken, it means it's improving.

It is not what I said.

I said "some debug drop variant" and all those validation and custom
asserts for impossible flows are supporting my claim.

Thanks

> 
> 					-Alex
> 
> > Thanks
> > 
> 

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

[Alex Elder]

On 3/21/21 8:49 AM, Leon Romanovsky wrote:
> On Sun, Mar 21, 2021 at 08:21:24AM -0500, Alex Elder wrote:
>> On 3/21/21 3:21 AM, Leon Romanovsky wrote:
>>> On Sat, Mar 20, 2021 at 09:17:29AM -0500, Alex Elder wrote:
>>>> 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
>>>
>>> Maybe netdev folks think differently here, but general rule that dead
>>> code and closed code is such, is not acceptable to in Linux kernel.
>>>
>>> <...>
>>
>> What is the purpose of CONFIG_KGDB?  Or CONFIG_DEBUG_KERNEL?
>> Would you prefer I expose this through a kconfig option?  I
>> intentionally did not do that, because I really intended it
>> to be only for development, so defined it in the Makefile.
>> But I have no objection to making it configurable that way.
> 
> I prefer you to follow netdev/linux kernel rules of development.
> The upstream repository and drivers/net/* folder especially are not
> the place to put code used for the development.

How do I add support for new versions of the hardware as
it evolves?

What I started supporting (v3.5.1) was in some respects
relatively old.  Version 4.2 is newer, and the v4.5 and
beyond are for products that are relatively new on the
market.

Some updates to IPA (like 4.0+ after 3.5.1, or 4.5+
after 4.2) include substantial updates to the way the
hardware works.  The code can't support the new hardware
without being adapted and generalized to support both
old and new.

My goal is to get upstream support for IPA for all
Qualcomm SoCs that have it.  But the hardware design
is evolving; Qualcomm is actively developing their
architecture so they can support new technologies
(e.g. cellular 5G).  Development of the driver is
simply *necessary*.

The assertions I proposed and checks like this are
intended as an *aid* to the active development I
have been doing.

They may look like hacky debugging--checking errors
that can't happen.  They aren't that at all--they're
intended to the compiler help me develop correct code,
given I *know* it will be evolving.

But the assertions are gone, and I accept/agree that
these specific checks "look funny."  More below.

>>>> -#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 */
>>>
>>> If it is possible to supply those values, the check should be always and
>>> not only under some closed config option.
>>
>> These are assertions.
>>
>> There is no need to test them for working code.  If
>> I run the code successfully with these tests enabled
>> exactly once, and they are satisfied, then every time
>> the code is run thereafter they will pass.  So I want
>> to check them when debugging/developing only.  That
>> way there is a mistake, it gets caught, but otherwise
>> there's no pointless argument checking done.
>>
>> I'll explain the first check; the others have similar
>> explanation.
>>
>> In the current code, the passed size is sizeof(struct)
>> for three separate structures.
>>   - If the structure size changes, I want to be
>>     sure the constraint is still honored
>>   - The code will break of someone happens
>>     to pass a size of 0.  I don't expect that to
>>     ever happen, but this states that requirement.
>>
>> This is an optimization, basically, but one that
>> allows the assumed conditions to be optionally
>> verified.
> 
> Everything above as an outcome of attempting to mix constant vs. run-time
> checks. If "size" is constant, the use of BUILD_BIG_ON() will help not only
> you but other developers to catch the errors too. The assumption that you alone
> are working on this code, can or can't be correct.

Right now I am the only one doing substantive development.
I am listed as the maintainer, and I trust anything more
than simple fixes will await my review before being
merged.

> If "size" is not constant, you should check it always.

To do that I might need to make this function (and others
like it) inline, or maybe __always_inline.  Regardless,
I generally agree with your suggestion of defensively
testing the argument value.  But this is an *internal
interface*.  The only callers are inside the driver.

It's basically putting the burden on the caller to verify
parameters, because often the caller already knows.

I think this is more of a philosophical argument than
a technical one.  The check isn't *that* expensive.

>>>>   	/* 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 */
>>>
>>> Same
>>>
>>> <...>
>>>
>>>>   {
>>>> -#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 */
>>>
>>> BUILD_BUG_ON()s are checked during compilation and not during runtime
>>> like IPA_VALIDATION promised.
>>
>> So I should update the description.  But I'm not sure where
>> you are referring to.  Here is the first line of the patch
>> description:
>>   There are blocks of IPA code that sanity-check various
>>   values, at compile time where possible.
> 
> I'm suggesting to review if IPA_VALIDATION is truly needed.

*That* is a suggestion I can act on...

Right now, it's *there*.  These few patches were the beginning
of a side task to simplify and/or get rid of it.  The first
step is to get it so it's not fundamentally broken.  Then I
can work on getting rid of (or at least refactor) pieces.

The code I started with did lots of checks of these things
(including build-time checkable ones).  Many, many functions
needlessly returned values, just so these checks could be made.
The possibility of returning an error meant all callers had
to check for it, and that complicated things all the way up.

So I tried to gather such things into foo_validate() functions,
which just grouped these checks without having to clutter the
normal code path with them.  That way called functions could
have void return type, and calling functions would be simpler,
and so on.

So I guess to respond again to your comment, I really would
like to get rid of IPA_VALIDATION, or most of it.  As it is,
many things are checked with BUILD_BUG_ON(), but they need
not really be conditionally built.  That is a fix I intend
to make, but haven't yet.

But the code is there, and if I am going to fix it, I need
to do it with patches.  And I try to make my patches small
enough to be easily reviewable.

>>> IMHO, the issue here is that this IPA code isn't release quality but
>>> some debug drop variant and it is far from expected from submitted code.
>>
>> Doesn't sound very humble, IMHO.
> 
> Sorry about that.

I'd like to suggest a plan so I can begin to make progress,
but do so in a way you/others think is satisfactory.
- I would first like to fix the existing bugs, namely that
  if IPA_VALIDATION is defined there are build errors, and
  that IPA_VALIDATION is not consistently used.  That is
  this 2-patch series.
- I assure you that my goal is to simplify the code that
  does this sort of checking.  So here are some specific
  things I can implement in the coming weeks toward that:
    - Anything that can be checked at build time, will
      be checked with BUILD_BUG_ON().
    - Anything checked with BUILD_BUG_ON() will *not*
      be conditional.  I.e. it won't be inside an
      #ifdef IPA_VALIDATION block.
    - I will review all remaining VALIDATION code (which
      can't--or can't always--be checked at build time),
      If it looks prudent to make it *always* be checked,
      I will make it always be checked (not conditional
      on IPA_VALIDATION).
The result should clearly separate checks that can be done
at build time from those that can't.

And with what's left (especially on that third sub-bullet)
I might have some better examples with which to argue
for something different.  Or I might just concede that
you were right all along.

The IPA_VALIDATION stuff is a bit of an artifact of the
development process.  I want to make it better, and right
now it's upstream, so I have to do it in this forum.

>> This code was found acceptable and merged for mainline a
>> year ago.  At that time it supported IPA on the SDM845 SoC
>> (IPA v3.5.1).  Had it not been merged, I would have continued
>> refining the code out-of-tree until it could be merged.  But
>> now, it's upstream, so anything I want to do to make it better
>> must be done upstream.
> 
> The upstream just doesn't need to be your testing ground.

As I said, this is not how I view these checks, but
I understand why you're saying that.

Can you help me get closer to resolution?

Thank you for taking the time on this.  I have my
views on how I like to do things, but I really do
value your thoughts.

					-Alex

>> Since last year it has undergone considerable development,
>> including adding support for the SC7180 SoC (IPA v4.2).  I
>> am now in the process of getting things posted for review
>> so IPA versions 4.5, 4.9, and 4.11 are supported.  With any
>> luck all that will be done in this merge cycle; we'll see.
>>
>> Most of what I've been doing is gradually transforming
>> things to support the new hardware.  But in the process
>> I'm also improving what's there so that it is better
>> organized, more consistent, more understandable, and
>> maintainable.
>>
>> I have explained this previously, but this code was derived
>> from Qualcomm "downstream" code.  Much was done toward
>> getting it into the upstream kernel, including carving out
>> great deal of code, and removing functionality to focus on
>> just *one* target platform.
>>
>> Now that it's upstream, the aim is to add back functionality,
>> ideally to support all current and future Qualcomm IPA hardware,
>> and eventually (this year) to support some of the features
>> (hardware filtering/routing/NAT) that were removed to make
>> the code simpler.
>>
>> I'm doing a lot of development on this driver, yes.  But
>> it doesn't mean it's broken, it means it's improving.
> 
> It is not what I said.
> 
> I said "some debug drop variant" and all those validation and custom
> asserts for impossible flows are supporting my claim.
> 
> Thanks
> 
>>
>> 					-Alex
>>
>>> Thanks
>>>
>>

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

[Leon Romanovsky]

On Sun, Mar 21, 2021 at 12:19:02PM -0500, Alex Elder wrote:
> On 3/21/21 8:49 AM, Leon Romanovsky wrote:
> > On Sun, Mar 21, 2021 at 08:21:24AM -0500, Alex Elder wrote:
> >> On 3/21/21 3:21 AM, Leon Romanovsky wrote:
> >>> On Sat, Mar 20, 2021 at 09:17:29AM -0500, Alex Elder wrote:
> >>>> 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
> >>>
> >>> Maybe netdev folks think differently here, but general rule that dead
> >>> code and closed code is such, is not acceptable to in Linux kernel.
> >>>
> >>> <...>
> >>
> >> What is the purpose of CONFIG_KGDB?  Or CONFIG_DEBUG_KERNEL?
> >> Would you prefer I expose this through a kconfig option?  I
> >> intentionally did not do that, because I really intended it
> >> to be only for development, so defined it in the Makefile.
> >> But I have no objection to making it configurable that way.
> > 
> > I prefer you to follow netdev/linux kernel rules of development.
> > The upstream repository and drivers/net/* folder especially are not
> > the place to put code used for the development.
> 
> How do I add support for new versions of the hardware as
> it evolves?

Exactly like all other driver developers do. You are not different here.

1. Clean your driver to have stable base without dead/debug code.
2. Send patch series per-feature/hardware enablement on top of this base.

> 
> What I started supporting (v3.5.1) was in some respects
> relatively old.  Version 4.2 is newer, and the v4.5 and
> beyond are for products that are relatively new on the
> market.

I see that it was submitted in 2018, we have many large drivers
that by far older than IPA. For example, mlx5 supports more than 5
generations of hardware and was added in 2013.

> 
> Some updates to IPA (like 4.0+ after 3.5.1, or 4.5+
> after 4.2) include substantial updates to the way the
> hardware works.  The code can't support the new hardware
> without being adapted and generalized to support both
> old and new.

It is ok.

> 
> My goal is to get upstream support for IPA for all
> Qualcomm SoCs that have it.  But the hardware design
> is evolving; Qualcomm is actively developing their
> architecture so they can support new technologies
> (e.g. cellular 5G).  Development of the driver is
> simply *necessary*.

No argue here.

> 
> The assertions I proposed and checks like this are
> intended as an *aid* to the active development I
> have been doing.

They need to be local to your development environment.
It is perfectly fine if you keep extra debug patch internally.

> 
> They may look like hacky debugging--checking errors
> that can't happen.  They aren't that at all--they're
> intended to the compiler help me develop correct code,
> given I *know* it will be evolving.

It is wrong assumption that you are alone who are reading this code.
I presented my view as a casual developer who sometimes need to change
code that is not my expertise.

The extra checks, unreachable and/or for non-existing code are very similar
to the bad comments - they make simple tasks too complex, it causes to us
wonder why they exist, maybe I broke something, e.t.c.

Unreachable code and checks sometimes serve as a hint for code deletion
and this is exactly how I spotted your driver.

> 
> But the assertions are gone, and I accept/agree that
> these specific checks "look funny."  More below.
> 
> >>>> -#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 */
> >>>
> >>> If it is possible to supply those values, the check should be always and
> >>> not only under some closed config option.
> >>
> >> These are assertions.
> >>
> >> There is no need to test them for working code.  If
> >> I run the code successfully with these tests enabled
> >> exactly once, and they are satisfied, then every time
> >> the code is run thereafter they will pass.  So I want
> >> to check them when debugging/developing only.  That
> >> way there is a mistake, it gets caught, but otherwise
> >> there's no pointless argument checking done.

Like I said below, you are not alone who are reading this code.
The kernel has three ways to handle wrong flow:
1. if ... return; -- used for runtime checks of possible but wrong input, or userspace input
2. BUILD_BUG_ON() -- used for compilation checks
3. WARN_ON*()     -- used for runtime checks of not-possible kernel flows

You are trying to use something different which is not needed.

> >>
> >> I'll explain the first check; the others have similar
> >> explanation.
> >>
> >> In the current code, the passed size is sizeof(struct)
> >> for three separate structures.
> >>   - If the structure size changes, I want to be
> >>     sure the constraint is still honored
> >>   - The code will break of someone happens
> >>     to pass a size of 0.  I don't expect that to
> >>     ever happen, but this states that requirement.
> >>
> >> This is an optimization, basically, but one that
> >> allows the assumed conditions to be optionally
> >> verified.
> > 
> > Everything above as an outcome of attempting to mix constant vs. run-time
> > checks. If "size" is constant, the use of BUILD_BIG_ON() will help not only
> > you but other developers to catch the errors too. The assumption that you alone
> > are working on this code, can or can't be correct.
> 
> Right now I am the only one doing substantive development.
> I am listed as the maintainer, and I trust anything more
> than simple fixes will await my review before being
> merged.

It is wrong assumption too. Major changes are performed all the time and
not always maintainers have say, sometimes it is because of timing, sometimes
it is because right thing to do.

> 
> > If "size" is not constant, you should check it always.
> 
> To do that I might need to make this function (and others
> like it) inline, or maybe __always_inline.  Regardless,
> I generally agree with your suggestion of defensively
> testing the argument value.  But this is an *internal
> interface*.  The only callers are inside the driver.

This is checked with WARN_ON*().

> 
> It's basically putting the burden on the caller to verify
> parameters, because often the caller already knows.
> 
> I think this is more of a philosophical argument than
> a technical one.  The check isn't *that* expensive.

My complain is lack of readability and maintainability for random kernel
developers and not performance concerns. 

> 
> >>>>   	/* 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 */
> >>>
> >>> Same
> >>>
> >>> <...>
> >>>
> >>>>   {
> >>>> -#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 */
> >>>
> >>> BUILD_BUG_ON()s are checked during compilation and not during runtime
> >>> like IPA_VALIDATION promised.
> >>
> >> So I should update the description.  But I'm not sure where
> >> you are referring to.  Here is the first line of the patch
> >> description:
> >>   There are blocks of IPA code that sanity-check various
> >>   values, at compile time where possible.
> > 
> > I'm suggesting to review if IPA_VALIDATION is truly needed.
> 
> *That* is a suggestion I can act on...
> 
> Right now, it's *there*.  These few patches were the beginning
> of a side task to simplify and/or get rid of it.  The first
> step is to get it so it's not fundamentally broken.  Then I
> can work on getting rid of (or at least refactor) pieces.
> 
> The code I started with did lots of checks of these things
> (including build-time checkable ones).  Many, many functions
> needlessly returned values, just so these checks could be made.
> The possibility of returning an error meant all callers had
> to check for it, and that complicated things all the way up.
> 
> So I tried to gather such things into foo_validate() functions,
> which just grouped these checks without having to clutter the
> normal code path with them.  That way called functions could
> have void return type, and calling functions would be simpler,
> and so on.
> 
> So I guess to respond again to your comment, I really would
> like to get rid of IPA_VALIDATION, or most of it.  As it is,
> many things are checked with BUILD_BUG_ON(), but they need
> not really be conditionally built.  That is a fix I intend
> to make, but haven't yet.
> 
> But the code is there, and if I am going to fix it, I need
> to do it with patches.  And I try to make my patches small
> enough to be easily reviewable.
> 
> >>> IMHO, the issue here is that this IPA code isn't release quality but
> >>> some debug drop variant and it is far from expected from submitted code.
> >>
> >> Doesn't sound very humble, IMHO.
> > 
> > Sorry about that.
> 
> I'd like to suggest a plan so I can begin to make progress,
> but do so in a way you/others think is satisfactory.
> - I would first like to fix the existing bugs, namely that
>   if IPA_VALIDATION is defined there are build errors, and
>   that IPA_VALIDATION is not consistently used.  That is
>   this 2-patch series.

The thing is that IPA_VALIDATION is not defined in the upstream kernel.
There is nothing to be fixed in netdev repository

> - I assure you that my goal is to simplify the code that
>   does this sort of checking.  So here are some specific
>   things I can implement in the coming weeks toward that:
>     - Anything that can be checked at build time, will
>       be checked with BUILD_BUG_ON().

+1

>     - Anything checked with BUILD_BUG_ON() will *not*
>       be conditional.  I.e. it won't be inside an
>       #ifdef IPA_VALIDATION block.
>     - I will review all remaining VALIDATION code (which
>       can't--or can't always--be checked at build time),
>       If it looks prudent to make it *always* be checked,
>       I will make it always be checked (not conditional
>       on IPA_VALIDATION).

+1

> The result should clearly separate checks that can be done
> at build time from those that can't.
> 
> And with what's left (especially on that third sub-bullet)
> I might have some better examples with which to argue
> for something different.  Or I might just concede that
> you were right all along.

I hope so.

Thanks

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

[Alex Elder]

On 3/22/21 1:40 AM, Leon Romanovsky wrote:
>> I'd like to suggest a plan so I can begin to make progress,
>> but do so in a way you/others think is satisfactory.
>> - I would first like to fix the existing bugs, namely that
>>    if IPA_VALIDATION is defined there are build errors, and
>>    that IPA_VALIDATION is not consistently used.  That is
>>    this 2-patch series.
> The thing is that IPA_VALIDATION is not defined in the upstream kernel.
> There is nothing to be fixed in netdev repository
> 
>> - I assure you that my goal is to simplify the code that
>>    does this sort of checking.  So here are some specific
>>    things I can implement in the coming weeks toward that:
>>      - Anything that can be checked at build time, will
>>        be checked with BUILD_BUG_ON().
> +1
> 
>>      - Anything checked with BUILD_BUG_ON() will*not*
>>        be conditional.  I.e. it won't be inside an
>>        #ifdef IPA_VALIDATION block.
>>      - I will review all remaining VALIDATION code (which
>>        can't--or can't always--be checked at build time),
>>        If it looks prudent to make it*always*  be checked,
>>        I will make it always be checked (not conditional
>>        on IPA_VALIDATION).
> +1
> 
>> The result should clearly separate checks that can be done
>> at build time from those that can't.
>>
>> And with what's left (especially on that third sub-bullet)
>> I might have some better examples with which to argue
>> for something different.  Or I might just concede that
>> you were right all along.
> I hope so.

I came up with a solution last night that I'm going to try
to implement.  I will still do the things I mention above.

The solution is to create a user space tool inside the
drivers/net/ipa directory that will link with the kernel
source files and will perform all the basic one-time checks
I want to make.

Building, and then running that tool will be a build
dependency for the driver.  If it fails, the driver build
will fail.  If it passes, all the one-time checks will
have been satisfied and the driver will be built, but it
will not have all of these silly checks littered
throughout.  And all checks (even those that aren't
constant) will be made at build time.

I don't know if that passes your "casual developer"
criterion, but at least it will not be part of the
kernel code proper.

I'm going to withdraw this two-patch series, and post
it again along with others, so I the whole set of changes
can be done as a complete series.

It isn't easy to have something you value be rejected.
But I understand your concerns, and accept the reasoning
about non-standard coding patterns making it harder for
a casual reader to comprehend.  As I said, cleaning up
this validation code was already my goal, but I'll be
doing it differently now.  In the end, I might like the
new result better, which is always a nice outcome from
discussion during review.  Thank you.

					-Alex

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

[Alex Elder]

On 3/20/21 9:17 AM, 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.
> 
> 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.

After much back-and-forth with Leon Romanovsky:

	--> I retract this series <--

I will include these patches in a future series that will
do cleanup of this validation code more completely.

Thanks.

					-Alex

> Version 2 removes the two patches that introduced ipa_assert().  It
> also modifies the description in the first patch so that it mentions
> the changes made to ipa_cmd_table_valid().
> 
> 					-Alex
> 
> Alex Elder (2):
>    net: ipa: fix init header command validation
>    net: ipa: fix IPA validation
> 
>   drivers/net/ipa/Makefile       |  2 +-
>   drivers/net/ipa/gsi_trans.c    |  8 ++---
>   drivers/net/ipa/ipa_cmd.c      | 54 ++++++++++++++++++++++------------
>   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, 58 insertions(+), 42 deletions(-)
> 

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

[Leon Romanovsky]

On Mon, Mar 22, 2021 at 08:22:20AM -0500, Alex Elder wrote:
> On 3/20/21 9:17 AM, 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.
> > 
> > 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.
> 
> After much back-and-forth with Leon Romanovsky:
> 
> 	--> I retract this series <--
> 
> I will include these patches in a future series that will
> do cleanup of this validation code more completely.

Thanks a lot.

> 
> Thanks.
> 
> 					-Alex
> 
> > Version 2 removes the two patches that introduced ipa_assert().  It
> > also modifies the description in the first patch so that it mentions
> > the changes made to ipa_cmd_table_valid().
> > 
> > 					-Alex
> > 
> > Alex Elder (2):
> >    net: ipa: fix init header command validation
> >    net: ipa: fix IPA validation
> > 
> >   drivers/net/ipa/Makefile       |  2 +-
> >   drivers/net/ipa/gsi_trans.c    |  8 ++---
> >   drivers/net/ipa/ipa_cmd.c      | 54 ++++++++++++++++++++++------------
> >   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, 58 insertions(+), 42 deletions(-)
> > 
> 

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

[Leon Romanovsky]

On Mon, Mar 22, 2021 at 08:17:59AM -0500, Alex Elder wrote:
> On 3/22/21 1:40 AM, Leon Romanovsky wrote:
> > > I'd like to suggest a plan so I can begin to make progress,
> > > but do so in a way you/others think is satisfactory.
> > > - I would first like to fix the existing bugs, namely that
> > >    if IPA_VALIDATION is defined there are build errors, and
> > >    that IPA_VALIDATION is not consistently used.  That is
> > >    this 2-patch series.
> > The thing is that IPA_VALIDATION is not defined in the upstream kernel.
> > There is nothing to be fixed in netdev repository
> > 
> > > - I assure you that my goal is to simplify the code that
> > >    does this sort of checking.  So here are some specific
> > >    things I can implement in the coming weeks toward that:
> > >      - Anything that can be checked at build time, will
> > >        be checked with BUILD_BUG_ON().
> > +1
> > 
> > >      - Anything checked with BUILD_BUG_ON() will*not*
> > >        be conditional.  I.e. it won't be inside an
> > >        #ifdef IPA_VALIDATION block.
> > >      - I will review all remaining VALIDATION code (which
> > >        can't--or can't always--be checked at build time),
> > >        If it looks prudent to make it*always*  be checked,
> > >        I will make it always be checked (not conditional
> > >        on IPA_VALIDATION).
> > +1
> > 
> > > The result should clearly separate checks that can be done
> > > at build time from those that can't.
> > > 
> > > And with what's left (especially on that third sub-bullet)
> > > I might have some better examples with which to argue
> > > for something different.  Or I might just concede that
> > > you were right all along.
> > I hope so.
> 
> I came up with a solution last night that I'm going to try
> to implement.  I will still do the things I mention above.
> 
> The solution is to create a user space tool inside the
> drivers/net/ipa directory that will link with the kernel
> source files and will perform all the basic one-time checks
> I want to make.

We are not talking about putting this tool in upstream repo, right?
If yes, please get buy-in from David/Jakub first.

Thanks

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

[Alex Elder]

On 3/22/21 9:17 AM, Leon Romanovsky wrote:
> We are not talking about putting this tool in upstream repo, right?
> If yes, please get buy-in from David/Jakub first.

I'll make the user space sanity check tool either way.

It would be nice to have it upstream, the checks
continue to be made as the kernel moves forward.

If I need to keep it in my own repository it will be
fairly isolated, and easier to carry for my own use.

If David/Jakub comments here, I'll know.  Otherwise I'll
post it as an RFC initially when I've got something.

					-Alex

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

[Andrew Lunn]

> The solution is to create a user space tool inside the
> drivers/net/ipa directory that will link with the kernel
> source files and will perform all the basic one-time checks
> I want to make.

Hi Alex

Have you found any other driver doing this?  Where do they keep there
code?

Could this be a selftest, put somewhere in tools/testing/selftests.

Or can this be a test kernel module. Eg. we have crypt/testmsg.c which
runs a number of tests on the crypto subsystem,
./kernel/time/test_udelay.c which runs times on udelay.

Rather than inventing something new, please follow other examples
already in the kernel.

       Andrew 

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

[Alex Elder]

On 3/22/21 5:56 PM, Andrew Lunn wrote:
>> The solution is to create a user space tool inside the
>> drivers/net/ipa directory that will link with the kernel
>> source files and will perform all the basic one-time checks
>> I want to make.
> 
> Hi Alex
> 
> Have you found any other driver doing this?  Where do they keep there
> code?
> 
> Could this be a selftest, put somewhere in tools/testing/selftests.
> 
> Or can this be a test kernel module. Eg. we have crypt/testmsg.c which
> runs a number of tests on the crypto subsystem,
> ./kernel/time/test_udelay.c which runs times on udelay.
> 
> Rather than inventing something new, please follow other examples
> already in the kernel.

I will.  I did see the tools/testing directory and I'll
look at how people have done things there.

I need to try to get it working first, then I'll figure
out where it belongs.  I think I'll be able to do a user
space test, but it's a little tricky to be sure it's
actually testing what I want.  If that ends up being
too hard, I'll look into kernel test module.

Thanks for the suggestions.

					-Alex

>         Andrew
>