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

On 3/22/21 1:40 AM, Leon Romanovsky wrote:

quoted

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

quoted

- 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

quoted

     - 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

quoted

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