On Tue, Apr 13, 2021 at 12:13:01PM +0000, Joakim Zhang wrote: > > Hi Jon, > > > -----Original Message----- > > From: Jon Hunter > > Sent: 2021年4月13日 16:41 > > To: Joakim Zhang ; Giuseppe Cavallaro > > ; Alexandre Torgue ; > > Jose Abreu > > Cc: netdev@vger.kernel.org; Linux Kernel Mailing List > > ; linux-tegra ; > > Jakub Kicinski > > Subject: Re: Regression v5.12-rc3: net: stmmac: re-init rx buffers when mac > > resume back > > > > > > On 01/04/2021 17:28, Jon Hunter wrote: > > > > > > On 31/03/2021 12:41, Joakim Zhang wrote: > > > > > > ... > > > > > >>> In answer to your question, resuming from suspend does work on this > > >>> board without your change. We have been testing suspend/resume now > > >>> on this board since Linux v5.8 and so we have the ability to bisect > > >>> such regressions. So it is clear to me that this is the change that caused > > this, but I am not sure why. > > >> > > >> Yes, I know this issue is regression caused by my patch. I just want to > > analyze the potential reasons. Due to the code change only related to the page > > recycle and reallocate. > > >> So I guess if this page operate need IOMMU works when IOMMU is enabled. > > Could you help check if IOMMU driver resume before STMMAC? Our common > > desire is to find the root cause, right? > > > > > > > > > Yes of course that is the desire here indeed. I had assumed that the > > > suspend/resume order was good because we have never seen any problems, > > > but nonetheless it is always good to check. Using ftrace I enabled > > > tracing of the appropriate suspend/resume functions and this is what I > > > see ... > > > > > > # tracer: function > > > # > > > # entries-in-buffer/entries-written: 4/4 #P:6 > > > # > > > # _-----=> irqs-off > > > # / _----=> need-resched > > > # | / _---=> hardirq/softirq > > > # || / _--=> preempt-depth > > > # ||| / delay > > > # TASK-PID CPU# |||| TIMESTAMP FUNCTION > > > # | | | |||| | | > > > rtcwake-748 [000] ...1 536.700777: > > stmmac_pltfr_suspend <-platform_pm_suspend > > > rtcwake-748 [000] ...1 536.735532: > > arm_smmu_pm_suspend <-platform_pm_suspend > > > rtcwake-748 [000] ...1 536.757290: > > arm_smmu_pm_resume <-platform_pm_resume > > > rtcwake-748 [003] ...1 536.856771: > > stmmac_pltfr_resume <-platform_pm_resume > > > > > > > > > So I don't see any ordering issues that could be causing this. > > > > > > Another thing I have found is that for our platform, if the driver for the ethernet > > PHY (in this case broadcom PHY) is enabled, then it fails to resume but if I > > disable the PHY in the kernel configuration, then resume works. I have found > > that if I move the reinit of the RX buffers to before the startup of the phy, then > > it can resume OK with the PHY enabled. > > > > Does the following work for you? Does your platform use a specific ethernet > > PHY driver? > > I am also looking into this issue these days, we use the Realtek RTL8211FDI PHY, driver is drivers/net/phy/realtek.c. > > For our EQOS MAC integrated in our SoC, Rx side logic depends on RXC clock from PHY, so we need phylink_start before MAC. > > I will test below code change tomorrow to see if it can work at my side, since it is only re-init memory, need not RXC clock. > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index 208cae344ffa..071d15d86dbe 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > @@ -5416,19 +5416,20 @@ int stmmac_resume(struct device *dev) > > return ret; > > } > > + rtnl_lock(); > > + mutex_lock(&priv->lock); > > + stmmac_reinit_rx_buffers(priv); > > + mutex_unlock(&priv->lock); > > + > > if (!device_may_wakeup(priv->device) || !priv->plat->pmt) { > > - rtnl_lock(); > > phylink_start(priv->phylink); > > /* We may have called phylink_speed_down before */ > > phylink_speed_up(priv->phylink); > > - rtnl_unlock(); > > } > > - rtnl_lock(); > > mutex_lock(&priv->lock); > > stmmac_reset_queues_param(priv); > > - stmmac_reinit_rx_buffers(priv); > > stmmac_free_tx_skbufs(priv); > > stmmac_clear_descriptors(priv); > > > > > > It is still not clear to us why the existing call to > > stmmac_clear_descriptors() is not sufficient to fix your problem. > > During suspend/resume stress test, I found rx descriptor may not refill when system suspended, rx descriptor could be: 008 [0x00000000c4310080]: 0x0 0x40 0x0 0x34010040. > When system resume back, stmmac_clear_descriptors() would change this rx descriptor to: 008 [0x00000000c4310080]: 0x0 0x40 0x0 0xb5010040, a broken rx descriptor. > So at my side, stmmac_clear_descriptors() seems to be chief culprit. I have a idea if there is way to ensure all rx descriptors are refilled when suspend MAC. > > > How often does the issue you see occur? > Suspend about 2000 times. Hi David, Jakub, given where we are in the release cycle, I think it'd be best to revert commit 9c63faaa931e ("net: stmmac: re-init rx buffers when mac resume back") for now. To summarize the discussion: the patch was meant as a workaround to fix an occasional suspend/resume failure on one board that was not fully root caused, and ends up causing fully reproducible suspend/resume failures on at least one other board. Joakim is looking at an alternative solution and Jon and I can provide testing from the Tegra side for any fixes. Do you want me to send a revert patch or can you revert directly on top of your tree? Thanks, Thierry