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

[Alex Elder <elder@linaro.org>]

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

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

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

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

Thanks.

					-Alex

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