netdev - Re: [PATCH net-next v4 1/2] net: dsa: microchip: ksz8: Make flow control, speed, and duplex on CPU port configurable

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]

Date: Sun, 21 May 2023 13:28:09 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Woojung Huh <woojung.huh@...rochip.com>, Andrew Lunn <andrew@...n.ch>,
	Arun Ramadoss <arun.ramadoss@...rochip.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Simon Horman <simon.horman@...igine.com>,
	"Russell King (Oracle)" <linux@...linux.org.uk>,
	linux-kernel@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
	Paolo Abeni <pabeni@...hat.com>, kernel@...gutronix.de,
	netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
	UNGLinuxDriver@...rochip.com,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v4 1/2] net: dsa: microchip: ksz8: Make flow
 control, speed, and duplex on CPU port configurable

On Sun, May 21, 2023 at 06:38:41AM +0200, Oleksij Rempel wrote:
> Looks good, I like the idea with "wacky" registers!
> 
> Would you prefer that I start working on adapting your patch set to the
> KSZ8873? Or should I make a review to move forward the existing patch set?
> 
> Just a heads up, I don't have access to the KSZ87xx series switches, so
> I won't be able to test the changes on these models.
> 
> Let me know what you think and how we should proceed.

If we convert to regfields, the entire driver will need to be converted
(all switch families). I'd say make a best effort full conversion, and
if something breaks on the families which you could not test, surely
someone will jump to correct it. Since your KSZ8873 also has wacky registers
(btw, feel free to rename the concept to something more serious), I think
that not a lot can go wrong with a blind conversion as long as it's tested
on other hardware.

BTW, revisiting, I already found a bug in the conversion (patch 2/4):

+	} else if (mii_sel == bitval[P_RMII_SEL]) {
 		interface = PHY_INTERFACE_MODE_RGMII;
 	} else {
+		ret = ksz_regfields_read(dev, port, RF_RGMII_ID_IG_ENABLE, &ig);
+		if (WARN_ON(ret))
+			return PHY_INTERFACE_MODE_NA;
+
+		ret = ksz_regfields_read(dev, port, RF_RGMII_ID_IG_ENABLE, &eg);
						    ~~~~~~~~~~~~~~~~~~~~~
						    should have been RF_RGMII_ID_EG_ENABLE
+		if (WARN_ON(ret))
+			return PHY_INTERFACE_MODE_NA;
+
 		interface = PHY_INTERFACE_MODE_RGMII;
-		if (data8 & P_RGMII_ID_EG_ENABLE)
+		if (eg)
 			interface = PHY_INTERFACE_MODE_RGMII_TXID;
-		if (data8 & P_RGMII_ID_IG_ENABLE) {
+		if (ig) {
 			interface = PHY_INTERFACE_MODE_RGMII_RXID;
-			if (data8 & P_RGMII_ID_EG_ENABLE)
+			if (eg)
 				interface = PHY_INTERFACE_MODE_RGMII_ID;
 		}
 	}