Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM entries

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 29 Oct 2024 16:30:54 +0100 Alexander Lobakin wrote:
> >> Please declare loop iterators right in loop declarations, we're GNU11
> >> for a couple years already.
> >>
> >> 	for (u32 i = 0; ...  
> > 
> > Why?  
> 
> Because we usually declare variables only inside the scopes within which
> they're used, IOW
> 
> 	for (...) {
> 		void *data;
> 
> 		data = ...
> 	}
> 
> is preferred over
> 
> 	void *data;
> 
> 	for (...) {
> 		data = ...
> 	}

Are you actually actively pointing that out in review?
If it was an important rule why is there no automation
to catch cases where variable is only used in a single
basic block but is declared at function scope.

> Here it's the same. `for (int` reduces the scope of the iterator.
> The iter is not used outside the loop.
> 
> > Please avoid giving people subjective stylistic feedback, especially  
> 
> I didn't say "You must do X" anywhere, only proposed some stuff, which
> from my PoV would improve the code.

You said "please do XYZ" which in English is pretty strong.

> And make the style more consistent. "Avoiding giving people subjective
> stylistic feedback" led to that it's not really consistent beyond the
> level of checkpatch's complaints.

checkpatch is obviously bad at its job but I don't think random people
giving subjective stylistic feedback will improve the situation.
We have a handful of reviewers who review maybe 1 in 10 patches.
The reviews are very much appreciated but since those reviewers are not
covering substantial portion of the code merged they should not come up
with guidelines of their own making.

I see plenty of cases where one patch gets nit picked to death on small
stylistic issues and another gets merged even tho its far below average.
Doesn't feel very fair.

> > when none of the maintainers have given such feedback in the past.  
> 
> I don't think my mission as a reviewer is to be a parrot?

Not what I'm saying. Please focus on functional review of the code,
and process / stylistic review only to the extent to which such
rules are widely applied. We even documented this in the netdev "FAQ".

> >> (+ don't use signed when it can't be < 0)  
> > 
> > Again, why. int is the most basic type in C, why is using a fixed side
> > kernel type necessary here?  
> 
> Because the negative part is not used at all here. Why not __u128 or
> double then if it doesn't matter?

We have plenty of bugs because someone decided to use an unsigned type 
and then decided to decrement as long as its >= 0..