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 <el...@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
