Message-ID: <DB8PR04MB679537972394A8096330C5A3E64E9@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Wed, 14 Apr 2021 08:06:33 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Thierry Reding <thierry.reding@...il.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


> -----Original Message-----
> From: Thierry Reding <thierry.reding@...il.com>
> Sent: 2021年4月14日 15:41
> 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; 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.

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

Powered by blists - more mailing lists