-----Original Message-----
From: Joakim Zhang <redacted>
Sent: 2021年4月14日 16:07
To: Thierry Reding <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

quoted

-----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
[off-list ref]; 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.

quoted

quoted

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.

quoted

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

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.

quoted

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.

quoted

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.

Hi Thierry,

After you revert that patch, could you help test below code change if possible? I check the stmmac driver, from the first version, it will clear both rx and tx descriptors.
I change the behavior to only clear tx descriptors, not sure if it also have ang other risks, need more people review it. Now I am also doing the overnight stress test.

--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

@@ -5412,9 +5412,12 @@ static void stmmac_reset_queues_param(struct stmmac_priv *priv)
                tx_q->mss = 0;

                netdev_tx_reset_queue(netdev_get_tx_queue(priv->dev, queue));
+
+               stmmac_clear_tx_descriptors(priv, queue);
        }
 }

 /**
  * stmmac_resume - resume callback
  * @dev: device pointer

@@ -5470,9 +5473,7 @@ int stmmac_resume(struct device *dev)
        mutex_lock(&priv->lock);

        stmmac_reset_queues_param(priv);
-       stmmac_reinit_rx_buffers(priv);
        stmmac_free_tx_skbufs(priv);
-       stmmac_clear_descriptors(priv);

        stmmac_hw_setup(ndev, false);
        stmmac_init_coalesce(priv);

Best Regards,
Joakim Zhang

quoted

Thierry