[BUG] SFP I2C timeout forces link down with PHY_ERROR

[Sean Anderson]

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.

>> 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.

OK...

>> - 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.

Would it? Maybe we just need something like

if (err == -ENOLINK) {
	phy_init_hw(phydev);
	needs_aneg = true;
	phydev->state = PHY_UP;
	err = 0;
}

in the phy_state_machine switch statement under PHY_NOLINK and
PHY_RUNNING. The phy_init_hw wouldn't even be necessary for this case
(but would probably be a good idea in the general case where
master/slave resolution fails).

> 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. 

>> 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.

The CPU on this device is already underpowered, so I'd rather not resort
to bitbanging.

--Sean