netdev - Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register

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: <20230524165701.pbrcs4e74juzb4r3@skbuf>
Date: Wed, 24 May 2023 19:57:01 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: arinc9.unal@...il.com
Cc: Sean Wang <sean.wang@...iatek.com>,
	Landen Chao <Landen.Chao@...iatek.com>,
	DENG Qingfang <dqfext@...il.com>,
	Daniel Golle <daniel@...rotopia.org>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Russell King <linux@...linux.org.uk>,
	Arınç ÜNAL <arinc.unal@...nc9.com>,
	Richard van Schagen <richard@...terhints.com>,
	Richard van Schagen <vschagen@...com>,
	Frank Wunderlich <frank-w@...lic-files.de>,
	Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
	erkin.bozoglu@...ont.com, mithat.guner@...ont.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from
 correct register

On Mon, May 22, 2023 at 03:15:07PM +0300, arinc9.unal@...il.com wrote:
> From: Arınç ÜNAL <arinc.unal@...nc9.com>
> 
> On commit 7ef6f6f8d237 ("net: dsa: mt7530: Add MT7621 TRGMII mode support")
> macros for reading the crystal frequency were added under the MT7530_HWTRAP
> register. However, the value given to the xtal variable on
> mt7530_pad_clk_setup() is read from the MT7530_MHWTRAP register instead.
> 
> Although the document MT7621 Giga Switch Programming Guide v0.3 states that
> the value can be read from both registers, use the register where the
> macros were defined under.
> 
> Tested-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> ---

I'm sorry, but I refuse this patch, mainly as a matter of principle -
because that's just not how we do things, and you need to understand why.

The commit title ("read XTAL value from correct register") claims that
the process of reading a field which cannot be changed by software is
any more correct when it is read from HWTRAP rather than MHWTRAP
(modified HWTRAP).

Your justification is that it's confusing to you if two registers have
the same layout, and the driver has a single set of macros to decode the
fields from both. You seem to think it's somehow not correct to decode
fields from the MHWTRAP register using macros which have just HWTRAP in
the name.

While in this very particular case there should be no negative effect
caused by the change (*because* XTAL_FSEL is read-only), your approach
absolutely does not scale to the other situations that you will be faced
with.

If it was any other r/w field from HWTRAP vs MHWTRAP, you would have
needed to get used to that coding pattern (or do something about the
coding pattern itself), and not just decide to change the register to
what you think is correct - which is a *behavior* change and not just
a *coding style* change. You don't change the *behavior* when you don't
like the *coding style* !!! because that's not a punctual fix to your
problem.

I'm sorry that it is like this, but you can't expect the Linux kernel to
be written for the level of understanding of a beginner C programmer.
It's simply too hard for everyone to change, and much easier, and more
beneficial overall, for you to change.

I understand that you're poking sticks at everything in order to become
more familiar with the driver. That's perfectly normal and fine. But not
everything that comes as a result of that is of value for sharing back
to the mainline kernel's mailing lists.

Seriously, please first share these small rewrites with someone more
senior than you, and ask for a preliminary second opinion.

As they say, "on the Internet, nobody knows you're a dog". If reviewers
don't take into account that you're a newbie with C (which is a badge
that you don't carry everywhere - because you don't have to), it's easy
for patches like this (and most of this series) to come across as:
"I have no consideration for the fact that the existing code works, and
the way in which it works, I'm just gonna rewrite all of it according to
my own sensibilities and subjective justifications, and throw baseless
words at it such as: fix this, correct that, when in fact all I'm doing
is make silly changes with no effect to the observable behavior".

Because I know that the above isn't the case, I try to be as polite as
possible expressing my frustration that I am looking at a large volume
of worthless and misguided refactoring.