Received-SPF: pass (google.com: domain of linux-kernel+bounces-192866-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 28 May 2024 19:22:54 +0100
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Sean Anderson <sean.anderson@linux.dev>
Cc: Andrew Lunn <andrew@lunn.ch>, Andi Shyti <andi.shyti@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	linux-i2c@vger.kernel.org, Michal Simek <michal.simek@amd.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [BUG] SFP I2C timeout forces link down with PHY_ERROR
Message-ID: <ZlYg/hDhy6q/Olpr@shell.armlinux.org.uk>
References: <ec7907f1-cb5a-41ab-824c-aa0b02440ada@linux.dev>
 <ZlYUNCRroM0up0xk@shell.armlinux.org.uk>
 <90873b78-13ba-445e-890a-0b90a653721b@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <90873b78-13ba-445e-890a-0b90a653721b@linux.dev>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Tue, May 28, 2024 at 01:50:54PM -0400, Sean Anderson wrote:
> On 5/28/24 13:28, Russell King (Oracle) wrote:
> > First, note that phylib's policy is if it loses comms with the PHY,
> > then the link will be forced down. This is out of control of the SFP
> > or phylink code.
> > 
> > I've seen bugs with the I2C emulation on some modules resulting in
> > problems with various I2C controllers.
> > 
> > Sometimes the problem is due to a bad I2C level shifter. Some I2C
> > level shifter manufacturers will swear blind that their shifter
> > doesn't lock up, but strangely, one can prove with an osciloscope
> > that it _does_ lock up - and in a way that the only way to recover
> > was to possibly unplug the module or poewr cycle the platform.
> 
> Well, I haven't seen any case where the bus locks up. I've been able to
> recover just by doing
> 
> 	ip link set net0 down
> 	ip link set net0 up
> 
> which suggests that this is just a transient problem.
> 
> > My advice would be to investigate the hardware in the first instance.
> 
> I'll try to keep this in mind, but it's pretty infrequent and I probably
> won't be able to test anything until I can reproduce it better.
> 
> > On Tue, May 28, 2024 at 12:57:25PM -0400, Sean Anderson wrote:
> >> Hi,
> >> 
> >> I saw the following warning [1] twice when testing 1000Base-T SFP
> >> modules:
> >> 
> >> [ 1481.682501] cdns-i2c ff030000.i2c: timeout waiting on completion
> >> [ 1481.692010] Marvell 88E1111 i2c:sfp-ge3:16: Master/Slave resolution failed
> >> [ 1481.699910] ------------[ cut here ]------------
> >> [ 1481.705459] phy_check_link_status+0x0/0xe8: returned: -67
> >> [ 1481.711448] WARNING: CPU: 2 PID: 67 at drivers/net/phy/phy.c:1233 phy_state_machine+0xac/0x2ec
> >> <snip>
> >> [ 1481.904544] macb ff0c0000.ethernet net1: Link is Down
> >> 
> >> and a second time with some other errors too:
> >> 
> >> [   64.972751] cdns-i2c ff030000.i2c: xfer_size reg rollover. xfer aborted!
> >> [   64.979478] cdns-i2c ff030000.i2c: xfer_size reg rollover. xfer aborted!
> > 
> > I2C driver bug? From what I can see, this occurs when there is further
> > data to be read, and id->recv_count hits zero. The I2C controller is
> > entirely in control of how many bytes are transferred from the remote
> > device, and it should raise a NAK on the last byte before signalling a
> > STOP condition during a read.
> 
> Commit bbf967b223b3 ("i2c: cadence: Handle transfer_size rollover")
> makes it seem like a hardware error. E.g. Linux thinks we're done but
> the hardware thinks there's still more data. I've added Alex to CC;
> maybe he can comment.

See https://www.ti.com/lit/an/slva704/slva704.pdf figure 9 and the
text immediately above it. On a read, the controller is entirely
in control of how many bytes are transferred from the connected
device, and the controller has the responsibility to generate the
ACK after each byte read from the device _if_ it wants another
byte, or a NAK if it doesn't.

So, if the controller has been programmed to transfer e.g. 2 bytes,
but decides to ACK the 2nd byte and proceed to receive a 3rd byte,
that's nothing to do with the bus or the device, it's entirely down
to the controller being silly when it knows we only want 2 bytes.

> > Many drivers now do not check whether the PHY accesses they are
> > performing succeeded or not, and rely on the failure being permanent.
> 
> Well, this driver does, which is how the error gets propagated all the
> way up to phy_state_machine. 

While the Marvell driver is good (probably because phylib maintainers
look after it!), this isn't true of all drivers, and I don't think we
should add a kind of recovery to the core without sorting out the
other drivers first.

Maybe it needs to be something that PHY drivers opt into.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!