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

From: Joakim Zhang
Date: Tue Apr 13 2021 - 22:19:04 EST


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

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.
My patch is a solution, may not a good solution, now it seems not a workaround.

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.
This makes me think there is a specific integration in their SoCs. I have not seen others report it is broken at their side. Theoretically, at least, this patch should have
no side effect.

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

Best Regards,
Joakim Zhang
> Thanks,
> Thierry