netdev - Re: [PATCH v2] net: fec: Refactor: #define magic constants

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] [day] [month] [year] [list]

Message-ID: <5a14a872-4d53-4804-b1ec-c60fcf438e09@prolan.hu>
Date: Mon, 12 Feb 2024 17:34:09 +0100
From: Csókás Bence <csokas.bence@...lan.hu>
To: Andrew Lunn <andrew@...n.ch>
CC: <netdev@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>, "David S.
 Miller" <davem@...emloft.net>, Wei Fang <wei.fang@....com>, Shenwei Wang
	<shenwei.wang@....com>, Clark Wang <xiaoning.wang@....com>, NXP Linux Team
	<linux-imx@....com>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
	<pabeni@...hat.com>, Francesco Dolcini <francesco.dolcini@...adex.com>, "Marc
 Kleine-Budde" <mkl@...gutronix.de>
Subject: Re: [PATCH v2] net: fec: Refactor: #define magic constants

2024. 02. 12. 17:03 keltezéssel, Andrew Lunn írta:
> On Mon, Feb 12, 2024 at 04:11:10PM +0100, Csókás Bence wrote:
>>
>>
>> 2024. 02. 12. 16:04 keltezéssel, Andrew Lunn írta:
>>> On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote:
>>>> Hi!
>>>>
>>>> 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta:
>>>>>> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
>>>>>>     	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
>>>>>>     	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
>>>>>>     	     ndev->phydev && ndev->phydev->pause)) {
>>>>>> -		rcntl |= FEC_ENET_FCE;
>>>>>> +		rcntl |= FEC_RCR_FLOWCTL;
>>>>>
>>>>> This immediately stood out to me while looking at the diff. Its not
>>>>> obvious why this is correct. Looking back, i see you removed
>>>>> FEC_ENET_FCE, not renamed it.
>>>>
>>>> What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it
>>>> obvious that it represents a bit in RCR (or `rcntl` as it is called on this
>>>> line). How is that not "renaming" it?
>>>
>>> Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was
>>> it wrong before? Is this actually a fix? Is it correct now, or is this
>>> a cut/paste typo? Looking at the rest of the patch there is no obvious
>>> answer. As i said, you deleted FEC_ENET_FCE, but there is no
>>> explanation why.
>>
>> The name `FEC_ENET_FCE` does not tell us that this is the FCE (Flow Control
>> Enable) bit (1 << 5) of the RCR (Receive Control Register). I added
>> FEC_RCR_* macros for all RCR bits, and I named BIT(5) FEC_RCR_FLOWCTL, a
>> much more descriptive name (in my opinion, at least).
> 
> Some form of that would be good in the commit message. It explains the
> 'Why?' of the change.

Ok. I split that change into a separate patch with a quick summary of 
this, in v4 of this patch. Hopefully it is more clear now.

>> So, a separate patch just for removing FEC_ENET_FCE and replacing all usages
>> with FEC_RCR_FLOWCTL? And the rest can stay as-is?
> 
> A few others made review comments as well. It could be addressing
> those comments also requires more small patches. Sometimes you can
> avoid review comments by thinking, what are reviewers going to ask,
> and putting the answer to those questions in the commit message. This
> might all seam like a lot of hassle now, but it will help getting your
> future patchsets merged if you follow this advice.
> 
>        Andrew

I believe I addressed all other comments already, but do enlighten me if 
that is not the case:
* removed the "fix FEC_ECR_EN1588 being cleared on link-down" half of 
the patch (v2) - your feedback
* removed `u32 ecntl` from `fec_stop()` (v3) - requested by Marc
* added net-next subject-prefix (v3) - suggested by Denis
* factored out the removal of FEC_ENET_FCE (v4) - your feedback

Did I miss something?

Bence