netdev - Re: [PATCH net-next 9/9] net: stmmac: convert to phylink managed EEE support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <05987b45-94b9-4744-a90d-9812cf3566d9@nvidia.com>
Date: Fri, 14 Feb 2025 10:58:55 +0000
From: Jon Hunter <jonathanh@...dia.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Andrew Lunn <andrew+netdev@...n.ch>,
 Bryan Whitehead <bryan.whitehead@...rochip.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, linux-arm-kernel@...ts.infradead.org,
 linux-stm32@...md-mailman.stormreply.com,
 Marcin Wojtas <marcin.s.wojtas@...il.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
 Paolo Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com,
 "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH net-next 9/9] net: stmmac: convert to phylink managed EEE
 support


On 13/02/2025 12:00, Russell King (Oracle) wrote:

...

>>> I have been tracking down a suspend regression on Tegra186 and bisect is
>>> pointing to this change. If I revert this on top of v6.14-rc2 then
>>> suspend is working again. This is observed on the Jetson TX2 board
>>> (specifically tegra186-p2771-0000.dts).
>>
>> Thanks for the report.
>>
>>> This device is using NFS for testing. So it appears that for this board
>>> networking does not restart and the board hangs. Looking at the logs I
>>> do see this on resume ...
>>>
>>> [   64.129079] dwc-eth-dwmac 2490000.ethernet: Failed to reset the dma
>>> [   64.133125] dwc-eth-dwmac 2490000.ethernet eth0: stmmac_hw_setup: DMA engine initialization failed
>>>
>>> My first thought was if 'dma_cap.eee' is not supported for this device,
>>> but from what I can see it is and 'dma_cap.eee' is true. Here are some
>>> more details on this device regarding the ethernet controller.
>>
>> Could you see whether disabling EEE through ethtool (maybe first try
>> turning tx-lpi off before using the "eee off") to see whether that
>> makes any difference please?
> 
> One thing that I'm wondering is - old code used to do:
> 
> -             phy_eee_rx_clock_stop(phy, !(priv->plat->flags &
> -                                          STMMAC_FLAG_RX_CLK_RUNS_IN_LPI));
> 
> The new code sets:
> 
> +     if (!(priv->plat->flags & STMMAC_FLAG_RX_CLK_RUNS_IN_LPI))
> +             priv->phylink_config.eee_rx_clk_stop_enable = true;
> 
> which does the same thing in phylink - phylink_bringup_phy() will call
> phy_eee_rx_clock_stop() when the PHY is attahed. So this happens at a
> different time.
> 
> We know that stmmac_reset() can fail when the PHY receive clock is
> stopped - at least with some cores.
> 
> So, I'm wondering whether I've inadvertently fixed another bug in stmmac
> which has uncovered a different bug - maybe the PHY clock must never be
> stopped even in LPI - or maybe we need to have a way of temporarily
> disabling the PHY's clock-stop ability during stmmac_reset().
> 
> In addition to what I asked previously, could you also intrument
> phy_eee_rx_clock_stop() and test before/after this patch to see
> (a) whether it gets called at all before this patch and (b) confirm
> the enable/disable state before and after.


Thanks for the feedback. So ...

1. I can confirm that suspend works if I disable EEE via ethtool
2. Prior to this change I do see phy_eee_rx_clock_stop being called
    to enable the clock resuming from suspend, but after this change
    it is not.

Prior to this change I see (note the prints around 389-392 are when
we resume from suspend) ...

[    4.654454] Broadcom BCM89610 stmmac-0:00: phy_eee_rx_clock_stop: clk_stop_enable 0
[    4.723123] dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii link mode
[    7.629652] Broadcom BCM89610 stmmac-0:00: phy_eee_rx_clock_stop: clk_stop_enable 1
[  389.086185] dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii link mode
[  392.863744] Broadcom BCM89610 stmmac-0:00: phy_eee_rx_clock_stop: clk_stop_enable 1

After this change I see ...

[    4.644614] Broadcom BCM89610 stmmac-0:00: phy_eee_rx_clock_stop: clk_stop_enable 1
[    4.679224] dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii link mode
[  191.219828] dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii link mode

So yes definitely related to the PHY clock.

Jon

-- 
nvpublic