Received-SPF: pass (google.com: domain of linux-kernel+bounces-166932-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 3 May 2024 00:26:12 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Andrew Halaney <ahalaney@redhat.com>, 
	"Russell King (Oracle)" <linux@armlinux.org.uk>, Andrew Lunn <andrew@lunn.ch>
Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-stm32@st-md-mailman.stormreply.com, netdev@vger.kernel.org, alexandre.torgue@foss.st.com, 
	joabreu@synopsys.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 
	pabeni@redhat.com, mcoquelin.stm32@gmail.com, hkallweit1@gmail.com
Subject: Re: racing ndo_open()/phylink*connect() with phy_probe()
Message-ID: <7723d4l2kqgrez3yfauvp2ueu6awbizkrq4otqpsqpytzp45q2@rju2nxmqu4ew>
References: <uz66kbjbxieof6vkliuwgpzhlrbcmeb2f5aeuourw2vqcoc4hv@2adpvba3zszx>
 <ZjFl4rql0UgsHp97@shell.armlinux.org.uk>
 <ykdqxnky7shebbhtucoiokbews2be5bml6raqafsfn4x6bp6h3@nqsn6akpajvp>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ykdqxnky7shebbhtucoiokbews2be5bml6raqafsfn4x6bp6h3@nqsn6akpajvp>

Hi all

On Thu, May 02, 2024 at 12:43:27PM -0500, Andrew Halaney wrote:
> On Tue, Apr 30, 2024 at 10:42:58PM +0100, Russell King (Oracle) wrote:
> > On Tue, Apr 30, 2024 at 04:02:19PM -0500, Andrew Halaney wrote:
> > > Basically, NetworkManager is setting both interfaces to up, and end1's
> > > phy doesn't seem to be ready when ndo_open() runs, returning
> > > -ENODEV in phylink_fwnode_phy_connect() and bubbling that back up. This doesn't
> > 
> > Let's get something clear - you're attributing phylink to this, but this
> > is not the case. phylink doesn't deal directly with PHYs, it makes use
> > of phylib for that, and merely passes back to its caller whatever status
> > it gets from phylib. It's also not fair to attribute this to phylib as
> > we will see later...
> 
> Sorry for the delay, I wanted to try and test with some extra logs in
> the legit setup (not my "simulate via EPROBE_DEFER delays" approach)
> which is tedious with the initramfs (plus I wasted time failing to
> ftrace some stuff :P) to reconvince me of old notes. Thanks for the
> explanation above on the nuances between phylink and phylib, I really
> appreciate it.
> 
> > 
> > There are a few reasons for phylink_fwnode_phy_connect() would return
> > -ENODEV:
> > 
> > 1) fwnode_get_phy_node() (a phylib function) returning an error,
> > basically meaning the phy node isn't found. This would be a persistent
> > error, so unlikely to be your issue.
> > 
> > 2) fwnode_phy_find_device() (another phylib function) not finding the
> > PHY device corresponding to the fwnode returned by the above on the
> > MDIO bus. This is possible if the PHY has not been detected on the
> > MDIO bus, but I suspect this is not the cause of your issue.
> 
> So I think we're in this case. I added some extra logs to see which
> of the cases we were hitting, as well as some extra logs in phy creation
> code etc to come to that conclusion:
> 
>     # end1 probe start (and finish)
>     [    1.424099] qcom-ethqos 23000000.ethernet: Adding to iommu group 2
>     ...
>     [    1.431267] qcom-ethqos 23000000.ethernet: Using 40/40 bits DMA host/device width
> 
>     # end0 probe start
>     [    1.440517] qcom-ethqos 23040000.ethernet: Adding to iommu group 3
>     ...
>     [    1.443502] qcom-ethqos 23040000.ethernet: Using 40/40 bits DMA host/device width
> 
>     # end0 starts making the mdio bus, and phy devices
>     [    1.443537] qcom-ethqos 23040000.ethernet: Before of_mdiobus_reg
> 
>     # create phy at addr 0x8, end0's phy
>     [    1.450118] Starting phy_create_device for addr: 8
> 
>     # NetworkManager up'ed end1! and again. But the device we're needing
>     # (0xa) isn't created yet
>     [    1.459743] qcom-ethqos 23000000.ethernet end1: Register MEM_TYPE_PAGE_POOL RxQ-0
>     ...
>     [    1.465168] Failed at fwnode_phy_find_device
>     [    1.465174] qcom-ethqos 23000000.ethernet end1: __stmmac_open: Cannot attach to PHY (error: -19)
>     [    1.473687] qcom-ethqos 23000000.ethernet end1: Register MEM_TYPE_PAGE_POOL RxQ-0
>     ...
>     [    1.477637] Failed at fwnode_phy_find_device
>     [    1.477643] qcom-ethqos 23000000.ethernet end1: __stmmac_open: Cannot attach to PHY (error: -19)
> 
>     # device created for 0x8, probe it
>     [    1.531617] Ending phy_create_device for addr: 8
>     [    1.627462] Marvell 88E1510 stmmac-0:08: Starting probe
>     [    1.627644] hwmon hwmon0: temp1_input not attached to any thermal zone
>     [    1.627650] Marvell 88E1510 stmmac-0:08: Ending probe
> 
>     # device created for 0xa, probe it
>     [    1.628992] Starting phy_create_device for addr: a
>     [    1.632615] Ending phy_create_device for addr: a
>     [    1.731552] Marvell 88E1510 stmmac-0:0a: Starting probe
>     [    1.731732] hwmon hwmon1: temp1_input not attached to any thermal zone
>     [    1.731738] Marvell 88E1510 stmmac-0:0a: Ending probe
> 
>     # end0 is done probing now
>     [    1.732804] qcom-ethqos 23040000.ethernet: After of_mdiobus_reg
>     [    1.820725] qcom-ethqos 23040000.ethernet end0: renamed from eth0
> 
>     # NetworkManager up's end0
>     [    1.851805] qcom-ethqos 23040000.ethernet end0: Register MEM_TYPE_PAGE_POOL RxQ-0
>     ...
>     [    1.914980] qcom-ethqos 23040000.ethernet end0: PHY [stmmac-0:08] driver [Marvell 88E1510] (irq=233)
>     ...
>     [    1.939432] qcom-ethqos 23040000.ethernet end0: configuring for phy/sgmii link mode
>     ...
>     [    4.451765] qcom-ethqos 23040000.ethernet end0: Link is Up - 1Gbps/Full - flow control rx/tx
> 
> So end1 is up'ed before end0 can finish making its mdio bus / phy
> devices, and therefore we fail to find it. I can easily simulate this
> situation as well by -EPROBE_DEFER'ing end0 for say 10 seconds.

AFAICS the problem is in the race between the end0 and end1 device
probes. Right?
If so then can't the order be fixed by adding the links between the
OF-devices?  As it's already done for various phandle-based references
like "clocks", "gpios", "phys", etc?

* Before this topic was raised I had thought it was working for any
phandle-based dependencies, but apparently it wasn't and the
supplier/consumer linkage was supposed to be implemented for each
particular case. The "phy-handle" property lacks that feature support
(see drivers/of/property.c:of_supplier_bindings and
of_fwnode_add_links() for details).

-Serge(y)

> [...]