Hi Jon,
> -----Original Message-----
> From: Jon Hunter <jonathanh@xxxxxxxxxx>
> Sent: 2021年4月13日 16:41
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; Giuseppe Cavallaro
> <peppe.cavallaro@xxxxxx>; Alexandre Torgue <alexandre.torgue@xxxxxx>;
> Jose Abreu <joabreu@xxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; Linux Kernel Mailing List
> <linux-kernel@xxxxxxxxxxxxxxx>; linux-tegra <linux-tegra@xxxxxxxxxxxxxxx>;
> Jakub Kicinski <kuba@xxxxxxxxxx>
> 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.
Best Regards,
Joakim Zhang
> Thanks
> Jon
>
> --
> nvpublic
