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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241029172645.61935736@kernel.org>
Date: Tue, 29 Oct 2024 17:26:45 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: Mohsin Bashir <mohsin.bashr@...il.com>, <netdev@...r.kernel.org>,
 <alexanderduyck@...com>, <andrew@...n.ch>, <andrew+netdev@...n.ch>,
 <davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
 <kernel-team@...a.com>, <sanmanpradhan@...a.com>, <sdf@...ichev.me>,
 <vadim.fedorenko@...ux.dev>
Subject: Re: [PATCH net-next v2] eth: fbnic: Add support to write TCE TCAM
 entries

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