Received-SPF: pass (google.com: domain of linux-kernel+bounces-192807-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 28 May 2024 18:28:20 +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: <ZlYUNCRroM0up0xk@shell.armlinux.org.uk>
References: <ec7907f1-cb5a-41ab-824c-aa0b02440ada@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ec7907f1-cb5a-41ab-824c-aa0b02440ada@linux.dev>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

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.

My advice would be to investigate the hardware in the first instance.

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.

> I think some part of the stack should implement a retry mechanism, but
> I'm not sure which part. One idea could be to have mdio-i2c propagate
> negative errors instead of converting them to successful reads of
> 0xffff.

That would unfortunately break phylib's PHY probing.

> - Are I2C bus drivers supposed to be flaky like this? That is, are callers of
>   i2c_transfer expected to handle the occasional spurious error?

I2C transfers - to some extent - are supposed to have a number of
retries, but that's for the I2C device not responding to its address.
Otherwise, the bus is supposed to be reliable (there is no form of
error detection however - there's no CRCs or similar.)

The problem with merely retrying the transaction is a register read
from a PHY may have side-effects (such as the BMSR's LSTATUS bit
which is latched in link-fail state until the next read. Or a
register pointer could be incremented. So it's not simple to solve
at bus level.

> - Similarly, are MDIO bus drivers allowed to be flaky?

No.

I think the only realistic method would be for phylib to attempt to
reprogram the PHY, but that would need lots of changes to phylib.

Many drivers now do not check whether the PHY accesses they are
performing succeeded or not, and rely on the failure being permanent.

> Of course, the best option would be to fix cdns-i2c to not be buggy, but
> the hardware itself is buggy in at least one of the above cases so that
> may not be practical.

Well, I don't think there's much option. If I2C drivers are flakey maybe
its better to use GPIOs instead of the broken "inteligent" hardware.

Maybe Andrew has a different view however.

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