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

[Alex Elder <elder@linaro.org>]

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

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

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

					-Alex

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