Re: [BUG] SFP I2C timeout forces link down with PHY_ERROR

[Sean Anderson <sean.anderson@linux.dev>]

(forgot to CC Alex)

On 5/28/24 13:50, 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.
> 
>>> 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