RE: Regression v5.12-rc3: net: stmmac: re-init rx buffers when mac resume back

From: Joakim Zhang <hidden>
Date: 2021-04-13 12:13:10
Also in: linux-tegra, lkml

Hi Jon,

-----Original Message-----
From: Jon Hunter <jonathanh@nvidia.com>
Sent: 2021年4月13日 16:41
To: Joakim Zhang <redacted>; Giuseppe Cavallaro
[off-list ref]; Alexandre Torgue [off-list ref];
Jose Abreu [off-list ref]
Cc: netdev@vger.kernel.org; Linux Kernel Mailing List
[off-list ref]; linux-tegra [off-list ref];
Jakub Kicinski [off-list ref]
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:

quoted

On 31/03/2021 12:41, Joakim Zhang wrote:

...

quoted

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.

quoted

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.

quoted

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?

quoted


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

quoted

         rtcwake-748     [000] ...1   536.735532:

arm_smmu_pm_suspend <-platform_pm_suspend

quoted

         rtcwake-748     [000] ...1   536.757290:

arm_smmu_pm_resume <-platform_pm_resume

quoted

         rtcwake-748     [003] ...1   536.856771:

stmmac_pltfr_resume <-platform_pm_resume

quoted


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.

quoted hunk ↗ jump to hunk

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help