From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F2E54C25B78 for ; Tue, 28 May 2024 17:51:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=87okxSAHXvLh9abWyJo6Fm2h/mz/tgiYehPnf19kLe0=; b=alZYYqdZE8I7Yx 5v71eU1LxkUWL+01ZuDOMDfTFScnq7yQSjPsz3GUzXrCcClC26QFHjhr8KZHBJ4ezXYwxhU5wV3/n NXBO2+xTMDSW+yiiQZb9Zz2U8AKxjIb8hiaJTL9+BeiS63OHujCXxObUzW0imNz7ikak2J72i5t7T mlblzaY1td5lNzQ4aUvvVzFzSjEJ5ifZ2/uqGE3vG/LMk4KOKH7PBL/xw7POeIGKae/X/nfYZoqI0 c7CXzOZKfooG9ko0kRxWRIGt/12Crhv3YZRia4jK89VMrW0v5foo0vwhyNct5cky16PJKQ4RhQnjO JGQt8Z3fvilaRPyeZ4Jw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sC0ye-00000001bGX-2ieI; Tue, 28 May 2024 17:51:12 +0000 Received: from out-171.mta1.migadu.com ([2001:41d0:203:375::ab]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sC0yV-00000001bFt-1Ttx for linux-arm-kernel@lists.infradead.org; Tue, 28 May 2024 17:51:10 +0000 X-Envelope-To: linux@armlinux.org.uk DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1716918659; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=E4hwheoFrQ7bdjpnORVmrs5FpZ2wpl7LAQHlp/QppDc=; b=UyQRSNezUnHzdfcYU9YCUSRni+xPKZswLtYch39yEostjSju6y2qjYr+eoZL6R7/VnvxZM 523HyRHaxLFcu4fpQ5Nyg8fiWaXR5sACZOeO47/aO+krvW4qCkDn/Ux2y609398mCAbM+M R2t8Z3tlTW5w+CaMddiTH+X7Vfl2AZY= X-Envelope-To: andrew@lunn.ch X-Envelope-To: andi.shyti@kernel.org X-Envelope-To: netdev@vger.kernel.org X-Envelope-To: linux-i2c@vger.kernel.org X-Envelope-To: michal.simek@amd.com X-Envelope-To: hkallweit1@gmail.com X-Envelope-To: linux-arm-kernel@lists.infradead.org X-Envelope-To: linux-kernel@vger.kernel.org Message-ID: <90873b78-13ba-445e-890a-0b90a653721b@linux.dev> Date: Tue, 28 May 2024 13:50:54 -0400 MIME-Version: 1.0 Subject: Re: [BUG] SFP I2C timeout forces link down with PHY_ERROR To: "Russell King (Oracle)" Cc: Andrew Lunn , Andi Shyti , "netdev@vger.kernel.org" , linux-i2c@vger.kernel.org, Michal Simek , Heiner Kallweit , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" References: Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Sean Anderson In-Reply-To: X-Migadu-Flow: FLOW_OUT X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240528_105104_846336_9032E51E X-CRM114-Status: GOOD ( 41.08 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 >> >> [ 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel