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

[Joakim Zhang <qiangqing.zhang@nxp.com>]

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2021年4月14日 16:07
> To: Thierry Reding <thierry.reding@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
> Jon Hunter <jonathanh@nvidia.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@st.com>;
> Jose Abreu <joabreu@synopsys.com>; netdev@vger.kernel.org; Linux Kernel
> Mailing List <linux-kernel@vger.kernel.org>; linux-tegra
> <linux-tegra@vger.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@gmail.com>
> > Sent: 2021年4月14日 15:41
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: David S. Miller <davem@davemloft.net>; Jakub Kicinski
> > <kuba@kernel.org>; Jon Hunter <jonathanh@nvidia.com>; Giuseppe
> > Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > <alexandre.torgue@st.com>; Jose Abreu <joabreu@synopsys.com>;
> > netdev@vger.kernel.org; Linux Kernel Mailing List
> > <linux-kernel@vger.kernel.org>; linux-tegra
> > <linux-tegra@vger.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@gmail.com>
> > > > Sent: 2021年4月14日 0:07
> > > > To: David S. Miller <davem@davemloft.net>; Jakub Kicinski
> > > > <kuba@kernel.org>
> > > > Cc: Joakim Zhang <qiangqing.zhang@nxp.com>; Jon Hunter
> > > > <jonathanh@nvidia.com>; Giuseppe Cavallaro
> > > > <peppe.cavallaro@st.com>; Alexandre Torgue
> > > > <alexandre.torgue@st.com>; Jose Abreu <joabreu@synopsys.com>;
> > > > netdev@vger.kernel.org; Linux Kernel Mailing List
> > > > <linux-kernel@vger.kernel.org>; linux-tegra
> > > > <linux-tegra@vger.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@nvidia.com>
> > > > > > Sent: 2021年4月13日 16:41
> > > > > > To: Joakim Zhang <qiangqing.zhang@nxp.com>; Giuseppe Cavallaro
> > > > > > <peppe.cavallaro@st.com>; Alexandre Torgue
> > > > > > <alexandre.torgue@st.com>; Jose Abreu <joabreu@synopsys.com>
> > > > > > Cc: netdev@vger.kernel.org; Linux Kernel Mailing List
> > > > > > <linux-kernel@vger.kernel.org>; linux-tegra
> > > > > > <linux-tegra@vger.kernel.org>; Jakub Kicinski
> > > > > > <kuba@kernel.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.

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