netdev - RE: [PATCH] net: phy: at803x: Improve hibernation support on start up

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]
Date: Wed, 9 Aug 2023 02:27:12 +0000
From: Wei Fang <wei.fang@....com>
To: Marek Vasut <marex@...x.de>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
	Eric Dumazet <edumazet@...gle.com>, Heiner Kallweit <hkallweit1@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Oleksij Rempel <linux@...pel-privat.de>,
	Paolo Abeni <pabeni@...hat.com>, Russell King <linux@...linux.org.uk>
Subject: RE: [PATCH] net: phy: at803x: Improve hibernation support on start up

> >> Toggle hibernation mode OFF and ON to wake the PHY up and make it
> >> generate clock on RX_CLK pin for about 10 seconds.
> >> These clock are needed during start up by MACs like DWMAC in NXP
> >> i.MX8M Plus to release their DMA from reset. After the MAC has
> >> started up, the PHY can enter hibernation and disable the RX_CLK
> >> clock, this poses no problem for the MAC.
> >>
> >> Originally, this issue has been described by NXP in commit
> >> 9ecf04016c87 ("net: phy: at803x: add disable hibernation mode
> >> support") but this approach fully disables the hibernation support
> >> and takes away any power saving benefit. This patch instead makes the
> >> PHY generate the clock on start up for 10 seconds, which should be
> >> long enough for the EQoS MAC to release DMA from reset.
> >>
> >> Before this patch on i.MX8M Plus board with AR8031 PHY:
> >> "
> >> $ ifconfig eth1 up
> >> [   25.576734] imx-dwmac 30bf0000.ethernet eth1: Register
> >> MEM_TYPE_PAGE_POOL RxQ-0
> >> [   25.658916] imx-dwmac 30bf0000.ethernet eth1: PHY [stmmac-1:00]
> >> driver [Qualcomm Atheros AR8031/AR8033] (irq=38)
> >> [   26.670276] imx-dwmac 30bf0000.ethernet: Failed to reset the dma
> >> [   26.676322] imx-dwmac 30bf0000.ethernet eth1: stmmac_hw_setup:
> >> DMA engine initialization failed
> >> [   26.685103] imx-dwmac 30bf0000.ethernet eth1: __stmmac_open:
> Hw
> >> setup failed
> >> ifconfig: SIOCSIFFLAGS: Connection timed out "
> >>
> >
> > Have you reproduced this issue based on the upstream net-next or net
> tree?
> 
> On current linux-next next-20230808 so 6.5.0-rc5 . As far as I can tell,
> net-next is merged into this tree too.
> 

> > If so, can this issue be reproduced? The reason why I ask this is
> > because when I tried to reproduce this problem on net-next 6.3.0
> > version, I found that it could not be reproduced (I did not disable
> > hibernation mode when I reproduced this issue ). So I guess maybe other
> patches in eqos driver fixed the issue.
> 
> This is what I use for testing:
> 
> - Make sure "qca,disable-hibernation-mode" is NOT present in PHY DT node
Yes, I deleted this property when I reproduced this issue.

> - Boot the machine with NO ethernet cable plugged into the affected port
>    (i.e. the EQoS port), this is important
> - Make sure the EQoS MAC is not brought up e.g. by systemd-networkd or
>    whatever other tool, I use busybox initramfs for testing with plain
>    script as init (it mounts the various filesystems and runs /bin/sh)

It looks like something has been changed since I submitted the "disable hibernation
mode " patch. In previous test, I only need to unplug the cable and then use ifconfig
cmd to disable the interface, wait more than 10 seconds, then use ifconfig cmd to
enable the interface.

> - Wait longer than 10 seconds
> - If possible, measure AR8031 PHY pin 33 RX_CLK, wait for the RX_CLK to
>    be turned OFF by the PHY (means PHY entered hibernation)
> - ifconfig ethN up -- try to bring up the EQoS MAC <observe failure>
> 
> [...]

For the patch, I think your approach is better than mine, but I have a suggestion,
is the following modification more appropriate?

--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -991,12 +991,28 @@ static int at8031_pll_config(struct phy_device *phydev)
 static int at803x_hibernation_mode_config(struct phy_device *phydev)
 {
        struct at803x_priv *priv = phydev->priv;
+       int ret;

        /* The default after hardware reset is hibernation mode enabled. After
         * software reset, the value is retained.
         */
-       if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
-               return 0;
+       if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE)) {
+               /* Toggle hibernation mode OFF and ON to wake the PHY up and
+                * make it generate clock on RX_CLK pin for about 10 seconds.
+                * These clock are needed during start up by MACs like DWMAC
+                * in NXP i.MX8M Plus to release their DMA from reset. After
+                * the MAC has started up, the PHY can enter hibernation and
+                * disable the RX_CLK clock, this poses no problem for the MAC.
+                */
+               ret = at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+                                           AT803X_DEBUG_HIB_CTRL_PS_HIB_EN, 0);
+               if (ret < 0)
+                       return ret;
+
+               return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+                                            AT803X_DEBUG_HIB_CTRL_PS_HIB_EN,
+                                            AT803X_DEBUG_HIB_CTRL_PS_HIB_EN);
+       }

        return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,