-----Original Message-----
From: Thierry Reding <redacted>
Sent: 2021年4月14日 15:41
To: Joakim Zhang <redacted>
Cc: David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
Jon Hunter [off-list ref]; Giuseppe Cavallaro
[off-list ref]; Alexandre Torgue [off-list ref];
Jose Abreu [off-list ref]; netdev@vger.kernel.org; Linux Kernel
Mailing List [off-list ref]; linux-tegra
[off-list ref]
Subject: Re: Regression v5.12-rc3: net: stmmac: re-init rx buffers when mac
resume back

On Wed, Apr 14, 2021 at 02:18:58AM +0000, Joakim Zhang wrote:

quoted

quoted

-----Original Message-----
From: Thierry Reding <redacted>
Sent: 2021年4月14日 0:07
To: David S. Miller <davem@davemloft.net>; Jakub Kicinski
[off-list ref]
Cc: Joakim Zhang <redacted>; Jon Hunter
[off-list ref]; Giuseppe Cavallaro [off-list ref];
Alexandre Torgue [off-list ref]; Jose Abreu
[off-list ref]; netdev@vger.kernel.org; Linux Kernel Mailing
List [off-list ref]; linux-tegra
[off-list ref]
Subject: Re: Regression v5.12-rc3: net: stmmac: re-init rx buffers
when mac resume back

On Tue, Apr 13, 2021 at 12:13:01PM +0000, Joakim Zhang wrote:

quoted

Hi Jon,

quoted

-----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

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

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

quoted

So I guess if this page operate need IOMMU works when IOMMU
is

enabled.

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

#              | |         |   ||||      |         |
         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.

quoted

quoted

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.

quoted

For our EQOS MAC integrated in our SoC, Rx side logic depends on
RXC clock

from PHY, so we need phylink_start before MAC.

quoted

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

quoted

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

*/

quoted

quoted

quoted

quoted

                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.

quoted

When system resume back, stmmac_clear_descriptors() would change
this rx

descriptor to: 008 [0x00000000c4310080]: 0x0 0x40 0x0 0xb5010040, a
broken rx descriptor.

quoted

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.

quoted

quoted

quoted

quoted

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?

Hi Thierry, David, Jakub,

From my point of view, it is not a good choose to send a revert patch directly.

At my side, I have found the root cause. When system suspended, it is
possible that there are packets have not been received yet, such as:
008 [0x00000000c4310080]: 0x0 0x40 0x0 0x34010040.

After system resume, stmmac_clear_descriptors() clear the descriptor,
let it becomes below, it is a broken descriptor.
008 [0x00000000c4310080]: 0x0 0x40 0x0 0xb5010040

So it sounds like that is what needs to be fixed. Reallocating all buffers and
rewriting the descriptors seems more of a sledgehammer approach than a
proper fix to this problem.

quoted

I think it is a software bug there, and I don't know why others have
not reported it. This is a random issue, but there is a certain
probability that it will occur.

If this is really as rare as you say, I'm not completely surprised that nobody has
reported it.

quoted

My patch is a solution, may not a good solution, now it seems not a
workaround.

It's not an acceptable solution if it causes a regression.

quoted

At Joh's side, said it is related to IOMMU first, and then said
re-init rx buffers before PHY start also can fix it, and this patch
also only breaks one of their boards.

It's certainly possible that IOMMU has some sort of impact on the
reproducibility of the issue, but it's also a fact that before this patch the
systems that are now broken had been working.

Also, it's not relevant how many boards are broken. If a patch breaks a single
setup that used to work, that's a regression. What your patch does is basically
exchanging one working setup for another. And the regression is even worse
than the issue that you were trying to fix:
Jetson TX2 reliably fails to resume properly *every time*, whereas you
confirmed that you're only seeing this particular issue about once in
2000 suspend/resume cycles.

That's not how we do kernel development. Jon reported the regression 3 weeks
ago and nobody's come up with a fix that solves this properly and for everyone.
Given that we may only have 4 days left before the final release, the safest
course of action at this point is to revert and then we can try again for the next
cycle. Jon and I can help test any patches on the Tegra side.

quoted

This makes me think there is a specific integration in their SoCs.

Even if this was an integration issue, which I doubt, that's completely irrelevant.
What's relevant is that the setup was working before this patch.

quoted

I have not seen others report it is broken at their side.

Prior to your patch submission, did anybody report that suspend/resume was
broken for them? Also, if you are the only one seeing this issue, perhaps this is
an integration issue in your SoC?

As you can see this kind of argument makes us go in circles, hence why we have
the rule that when a patch causes a regression it either gets fixed or reverted.
Anything else leads to insanity.

quoted

Theoretically, at least, this patch should have no side effect.

Sorry, but that's not a valid argument. Practically this is causing a problem and
that counts more than theory.

quoted

In conclusion, we can revert this patch if we can find a better way to
fix this issue (packets have not received when system suspended).

Again, not how it works. The patch should be reverted to restore functionality
for setups that were previously working. Then we can can try to find a better
way to fix this issue.

I'm not confident that we can find that proper fix within the next few days, so
let's try again for the next release cycle.

Hi Thierry,

Thanks for your detailed explanation, please send a revert patch first for your urgent requirement.

Also please describe this issue I met and the reason for this revert. We can start another mail loop to discuss this issue.
My original thought is not to make regression, I am also sad about this. We need someone is much familiar about STMMAC driver,
who could point us a better solution.

Best Regards,
Joakim Zhang

Thierry