Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation

From: Alex Elder <elder@linaro.org>
To: Leon Romanovsky <leon@kernel.org>
Cc: davem@davemloft.net, kuba@kernel.org, andrew@lunn.ch,
	bjorn.andersson@linaro.org, evgreen@chromium.org,
	cpratapa@codeaurora.org, subashab@codeaurora.org,
	elder@kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation
Date: Mon, 22 Mar 2021 08:17:59 -0500	[thread overview]
Message-ID: <f152c274-6fe0-37a1-3723-330b7bfe249a@linaro.org> (raw)
In-Reply-To: <YFg7yHUeYvQZt+/Z@unreal>

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