* [PATCH net-next v2 0/2] net: ipa: fix validation
@ 2021-03-20 14:17 Alex Elder
2021-03-20 14:17 ` [PATCH net-next v2 1/2] net: ipa: fix init header command validation Alex Elder
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Alex Elder @ 2021-03-20 14:17 UTC (permalink / raw)
To: davem, kuba
Cc: leon, andrew, bjorn.andersson, evgreen, cpratapa, subashab,
elder, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH net-next v2 1/2] net: ipa: fix init header command validation 2021-03-20 14:17 [PATCH net-next v2 0/2] net: ipa: fix validation Alex Elder @ 2021-03-20 14:17 ` Alex Elder 2021-03-20 14:17 ` [PATCH net-next v2 2/2] net: ipa: fix IPA validation Alex Elder 2021-03-22 13:22 ` [PATCH net-next v2 0/2] net: ipa: fix validation Alex Elder 2 siblings, 0 replies; 15+ messages in thread From: Alex Elder @ 2021-03-20 14:17 UTC (permalink / raw) To: davem, kuba Cc: leon, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-20 14:17 [PATCH net-next v2 0/2] net: ipa: fix validation Alex Elder 2021-03-20 14:17 ` [PATCH net-next v2 1/2] net: ipa: fix init header command validation Alex Elder @ 2021-03-20 14:17 ` Alex Elder 2021-03-21 8:21 ` Leon Romanovsky 2021-03-22 13:22 ` [PATCH net-next v2 0/2] net: ipa: fix validation Alex Elder 2 siblings, 1 reply; 15+ messages in thread From: Alex Elder @ 2021-03-20 14:17 UTC (permalink / raw) To: davem, kuba Cc: leon, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-20 14:17 ` [PATCH net-next v2 2/2] net: ipa: fix IPA validation Alex Elder @ 2021-03-21 8:21 ` Leon Romanovsky 2021-03-21 13:21 ` Alex Elder 0 siblings, 1 reply; 15+ messages in thread From: Leon Romanovsky @ 2021-03-21 8:21 UTC (permalink / raw) To: Alex Elder Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-21 8:21 ` Leon Romanovsky @ 2021-03-21 13:21 ` Alex Elder 2021-03-21 13:49 ` Leon Romanovsky 0 siblings, 1 reply; 15+ messages in thread From: Alex Elder @ 2021-03-21 13:21 UTC (permalink / raw) To: Leon Romanovsky Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-21 13:21 ` Alex Elder @ 2021-03-21 13:49 ` Leon Romanovsky 2021-03-21 17:19 ` Alex Elder 0 siblings, 1 reply; 15+ messages in thread From: Leon Romanovsky @ 2021-03-21 13:49 UTC (permalink / raw) To: Alex Elder Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-21 13:49 ` Leon Romanovsky @ 2021-03-21 17:19 ` Alex Elder 2021-03-22 6:40 ` Leon Romanovsky 0 siblings, 1 reply; 15+ messages in thread From: Alex Elder @ 2021-03-21 17:19 UTC (permalink / raw) To: Leon Romanovsky Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 >>> >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-21 17:19 ` Alex Elder @ 2021-03-22 6:40 ` Leon Romanovsky 2021-03-22 13:17 ` Alex Elder 0 siblings, 1 reply; 15+ messages in thread From: Leon Romanovsky @ 2021-03-22 6:40 UTC (permalink / raw) To: Alex Elder Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-22 6:40 ` Leon Romanovsky @ 2021-03-22 13:17 ` Alex Elder 2021-03-22 14:17 ` Leon Romanovsky 2021-03-22 22:56 ` Andrew Lunn 0 siblings, 2 replies; 15+ messages in thread From: Alex Elder @ 2021-03-22 13:17 UTC (permalink / raw) To: Leon Romanovsky Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-22 13:17 ` Alex Elder @ 2021-03-22 14:17 ` Leon Romanovsky 2021-03-22 15:06 ` Alex Elder 2021-03-22 22:56 ` Andrew Lunn 1 sibling, 1 reply; 15+ messages in thread From: Leon Romanovsky @ 2021-03-22 14:17 UTC (permalink / raw) To: Alex Elder Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-22 14:17 ` Leon Romanovsky @ 2021-03-22 15:06 ` Alex Elder 0 siblings, 0 replies; 15+ messages in thread From: Alex Elder @ 2021-03-22 15:06 UTC (permalink / raw) To: Leon Romanovsky Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-22 13:17 ` Alex Elder 2021-03-22 14:17 ` Leon Romanovsky @ 2021-03-22 22:56 ` Andrew Lunn 2021-03-23 0:03 ` Alex Elder 1 sibling, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2021-03-22 22:56 UTC (permalink / raw) To: Alex Elder Cc: Leon Romanovsky, davem, kuba, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation 2021-03-22 22:56 ` Andrew Lunn @ 2021-03-23 0:03 ` Alex Elder 0 siblings, 0 replies; 15+ messages in thread From: Alex Elder @ 2021-03-23 0:03 UTC (permalink / raw) To: Andrew Lunn Cc: Leon Romanovsky, davem, kuba, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 0/2] net: ipa: fix validation 2021-03-20 14:17 [PATCH net-next v2 0/2] net: ipa: fix validation Alex Elder 2021-03-20 14:17 ` [PATCH net-next v2 1/2] net: ipa: fix init header command validation Alex Elder 2021-03-20 14:17 ` [PATCH net-next v2 2/2] net: ipa: fix IPA validation Alex Elder @ 2021-03-22 13:22 ` Alex Elder 2021-03-22 14:16 ` Leon Romanovsky 2 siblings, 1 reply; 15+ messages in thread From: Alex Elder @ 2021-03-22 13:22 UTC (permalink / raw) To: davem, kuba Cc: leon, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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(-) > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v2 0/2] net: ipa: fix validation 2021-03-22 13:22 ` [PATCH net-next v2 0/2] net: ipa: fix validation Alex Elder @ 2021-03-22 14:16 ` Leon Romanovsky 0 siblings, 0 replies; 15+ messages in thread From: Leon Romanovsky @ 2021-03-22 14:16 UTC (permalink / raw) To: Alex Elder Cc: davem, kuba, andrew, bjorn.andersson, evgreen, cpratapa, subashab, elder, netdev, linux-kernel 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(-) > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-03-23 0:04 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-20 14:17 [PATCH net-next v2 0/2] net: ipa: fix validation Alex Elder 2021-03-20 14:17 ` [PATCH net-next v2 1/2] net: ipa: fix init header command validation Alex Elder 2021-03-20 14:17 ` [PATCH net-next v2 2/2] net: ipa: fix IPA validation Alex Elder 2021-03-21 8:21 ` Leon Romanovsky 2021-03-21 13:21 ` Alex Elder 2021-03-21 13:49 ` Leon Romanovsky 2021-03-21 17:19 ` Alex Elder 2021-03-22 6:40 ` Leon Romanovsky 2021-03-22 13:17 ` Alex Elder 2021-03-22 14:17 ` Leon Romanovsky 2021-03-22 15:06 ` Alex Elder 2021-03-22 22:56 ` Andrew Lunn 2021-03-23 0:03 ` Alex Elder 2021-03-22 13:22 ` [PATCH net-next v2 0/2] net: ipa: fix validation Alex Elder 2021-03-22 14:16 ` Leon Romanovsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).