Received-SPF: pass (google.com: domain of linux-kernel+bounces-64122-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <65cbbacb.050a0220.b81d1.eb5b@mx.google.com>
Date: Tue, 13 Feb 2024 19:53:58 +0100
From: Christian Marangi <ansuelsmth@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Robert Marko <robimarko@gmail.com>,
	"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [net-next RFC PATCH 0/2] net: phy: aquantia: fix system
 interface provision
References: <20240213182415.17223-1-ansuelsmth@gmail.com>
 <ee00b11a-4679-4ba4-be42-10f15d5e9f65@lunn.ch>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ee00b11a-4679-4ba4-be42-10f15d5e9f65@lunn.ch>

On Tue, Feb 13, 2024 at 07:46:45PM +0100, Andrew Lunn wrote:
> On Tue, Feb 13, 2024 at 07:24:10PM +0100, Christian Marangi wrote:
> > Posting this as RFC as I think this require some discussion on the topic.
> > 
> > There is currently a problem. OEM multiple time provision Aquantia FW
> > with random and wrong data that may apply for one board but doesn't for
> > another. And at the same time OEM use the same broken FW for multiple
> > board and apply fixup at runtime.
> > 
> > This is the common case for AQR112 where downstream (uboot, OEM sdk,
> > openwrt to have the port correctly working) hack patch are used to fixup
> > broken system interface provision from the FW.
> > 
> > The downstream patch do one simple thing, they setup the SERDES startup
> > rate (that the FW may wrongly not init) and overwrite the
> > global system config for each rate to default values for the rwquested PHY
> > interface.
> > 
> > Now setting the SERDES startup value is SAFE, and this can be implemented
> > right away.
> > 
> > Overwriting the SERDES modes for each rate tho might pose some question
> > on how this is correct or wrong.
> > 
> > Reality is that probably every user an Aquantia PHY in one way or another
> > makes use of the SDK and have this patch in use making any kind of
> > provision on the FW ignored, (since the default values are always applied
> > at runtime) making the introduction of this change safe and restoring
> > correct functionality of AQR112 in the case of a broken FW loaded.
> 
> This is part of the discussion i had with Aquantia about
> provisioning. Basically, you cannot trust any register to contain a
> known value, e.g the value the data sheet indicates the reset value
> should be, or that the 802.3 standard says it should be.
> 
> So in effect, the driver needs to write every single register it
> depends on.
>

Well if that's the case then this RFC patch is a must. With a
misconfigured System Interface configuration, the PHY can't comunicate
with the MAC.

> > This might be the safest change but again would not give us 100% idea that
> > the thing provision by the FW are correct.
> 
> I would say, we have to assume provision is 100% wrong. Write every
> single register with the needed value.
> 
> Is the provisioning information available? Can it be read from the
> flash? Can it be dumped from firmware we have on disk? Dumping it for
> a number of devices could give a list of register values which are
> highly suspect, ones that OEMs typically mess with. We could start by
> always setting those registers.
>

We know where they are stored in the FW but it's not documented how the
provision values are stored in the FW. (the format, how they are
organized...) I can waste some time trying to reverse it and produce a
tool to parse them if needed.

Would love also some comments by Russell about this, there was a patch
adding support for WoL where another user was messing with these regs
and he was with the idea of being careful with overwriting the provision
values.

-- 
	Ansuel