Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net-next 0/4] net: ipa: fix validation
From:   Alex Elder <elder@linaro.org>
To:     davem@davemloft.net, kuba@kernel.org
Cc:     bjorn.andersson@linaro.org, evgreen@chromium.org,
        cpratapa@codeaurora.org, elder@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20210319042923.1584593-1-elder@linaro.org>
 <5c6fabcf-88c7-29db-431e-01818321e9e7@linaro.org>
Message-ID: <9ef8e593-d6be-a936-7a02-0a08e0be51be@linaro.org>
Date:   Sat, 20 Mar 2021 09:23:36 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <5c6fabcf-88c7-29db-431e-01818321e9e7@linaro.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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