Message-ID: <YHacfq+pWEbQDsZT@aiwendil>
Date:   Wed, 14 Apr 2021 09:40:46 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Joakim Zhang <qiangqing.zhang@....com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jon Hunter <jonathanh@...dia.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Jose Abreu <joabreu@...opsys.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-tegra <linux-tegra@...r.kernel.org>
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:
> 
> > -----Original Message-----
> > From: Thierry Reding <thierry.reding@...il.com>
> > Sent: 2021年4月14日 0:07
> > To: David S. Miller <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>
> > Cc: Joakim Zhang <qiangqing.zhang@....com>; Jon Hunter
> > <jonathanh@...dia.com>; Giuseppe Cavallaro <peppe.cavallaro@...com>;
> > Alexandre Torgue <alexandre.torgue@...com>; Jose Abreu
> > <joabreu@...opsys.com>; netdev@...r.kernel.org; Linux Kernel Mailing List
> > <linux-kernel@...r.kernel.org>; linux-tegra <linux-tegra@...r.kernel.org>
> > 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:
> > >
> > > Hi Jon,
> > >
> > > > -----Original Message-----
> > > > From: Jon Hunter <jonathanh@...dia.com>
> > > > Sent: 2021年4月13日 16:41
> > > > To: Joakim Zhang <qiangqing.zhang@....com>; Giuseppe Cavallaro
> > > > <peppe.cavallaro@...com>; Alexandre Torgue
> > > > <alexandre.torgue@...com>; Jose Abreu <joabreu@...opsys.com>
> > > > Cc: netdev@...r.kernel.org; Linux Kernel Mailing List
> > > > <linux-kernel@...r.kernel.org>; linux-tegra
> > > > <linux-tegra@...r.kernel.org>; Jakub Kicinski <kuba@...nel.org>
> > > > 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?
> 
> 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.

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

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

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

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

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

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

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

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists