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: <b44d50d8-23a2-47d6-99f7-856539e1de69@intel.com>
Date: Tue, 29 Oct 2024 16:30:54 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
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

From: Jakub Kicinski <kuba@...nel.org>
Date: Mon, 28 Oct 2024 16:35:54 -0700

> On Fri, 25 Oct 2024 17:19:03 +0200 Alexander Lobakin wrote:
>>> +static void fbnic_clear_tce_tcam_entry(struct fbnic_dev *fbd, unsigned int idx)
>>> +{
>>> +	int i;
>>> +
>>> +	/* Invalidate entry and clear addr state info */
>>> +	for (i = 0; i <= FBNIC_TCE_TCAM_WORD_LEN; i++)  
>>
>> 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 = ...
	}

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

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

> 
>> (+ 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?

Thanks,
Olek