> -----Original Message-----
> From: Thierry Reding <thierry.reding@xxxxxxxxx>
> Sent: 2021年4月14日 15:41
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Cc: David S. Miller <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
> Jon Hunter <jonathanh@xxxxxxxxxx>; Giuseppe Cavallaro
> <peppe.cavallaro@xxxxxx>; Alexandre Torgue <alexandre.torgue@xxxxxx>;
> Jose Abreu <joabreu@xxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; Linux Kernel
> Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; linux-tegra
> <linux-tegra@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> > > Sent: 2021年4月14日 0:07
> > > To: David S. Miller <davem@xxxxxxxxxxxxx>; Jakub Kicinski
> > > <kuba@xxxxxxxxxx>
> > > Cc: Joakim Zhang <qiangqing.zhang@xxxxxxx>; Jon Hunter
> > > <jonathanh@xxxxxxxxxx>; Giuseppe Cavallaro <peppe.cavallaro@xxxxxx>;
> > > Alexandre Torgue <alexandre.torgue@xxxxxx>; Jose Abreu
> > > <joabreu@xxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; Linux Kernel Mailing
> > > List <linux-kernel@xxxxxxxxxxxxxxx>; linux-tegra
> > > <linux-tegra@xxxxxxxxxxxxxxx>
> > > 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@xxxxxxxxxx>
> > > > > Sent: 2021年4月13日 16:41
> > > > > To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; Giuseppe Cavallaro
> > > > > <peppe.cavallaro@xxxxxx>; Alexandre Torgue
> > > > > <alexandre.torgue@xxxxxx>; Jose Abreu <joabreu@xxxxxxxxxxxx>
> > > > > Cc: netdev@xxxxxxxxxxxxxxx; Linux Kernel Mailing List
> > > > > <linux-kernel@xxxxxxxxxxxxxxx>; linux-tegra
> > > > > <linux-tegra@xxxxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>
> > > > > 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
