| 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
| ||
|
Message-ID: <AM0PR04MB628950F03E423244198EFEF98F0BA@AM0PR04MB6289.eurprd04.prod.outlook.com>
Date: Wed, 2 Aug 2023 18:43:44 +0000
From: Leo Li <leoyang.li@....com>
To: Russell King <linux@...linux.org.uk>
CC: Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
David Bauer <mail@...id-bauer.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Viorel Suman <viorel.suman@....com>,
Wei Fang <wei.fang@....com>
Subject: RE: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions
> -----Original Message-----
> From: Russell King <linux@...linux.org.uk>
> Sent: Wednesday, August 2, 2023 5:22 AM
> To: Leo Li <leoyang.li@....com>
> Cc: Andrew Lunn <andrew@...n.ch>; Heiner Kallweit
> <hkallweit1@...il.com>; David S . Miller <davem@...emloft.net>; Jakub
> Kicinski <kuba@...nel.org>; David Bauer <mail@...id-bauer.net>;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Viorel Suman
> <viorel.suman@....com>; Wei Fang <wei.fang@....com>
> Subject: Re: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions
>
> On Fri, Jul 28, 2023 at 04:53:19PM -0500, Li Yang wrote:
> > In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"),
> > it seems not correct to use a wol_en bit in a 1588 Control Register
> > which is only available on AR8031/AR8033(share the same phy_id) to
> > determine if WoL is enabled. Change it back to use
> > AT803X_INTR_ENABLE_WOL for determining the WoL status which is
> > applicable on all chips supporting wol. Also update the
> > at803x_set_wol() function to only update the 1588 register on chips
> > having it. After this change, disabling wol at probe from commit
> > d7cd5e06c9dd ("net: phy: at803x: disable WOL at probe") is no longer
> needed. So that part is removed.
>
> Okay, having been through the AR8031, AR8033, and AR8035 datasheets that
> I have, this is what I've gathered:
>
> AR8031 and AR8033 are identical as far as WoL is concerned:
> In terms of hardware, these have a WOL_INT pin that is separate
> from the normal interrupt.
>
> MMD3 0x8012 (1588 register) bit 5 controls whether the WoL
> function is enabled or disabled. Defaults to enabled.
>
> BMCR in copper/fiber can be used to save more power.
>
> AR8035 details below also apply.
>
> AR8035:
> No WOL_INT pin.
>
> No MMD3 0x8012 register.
>
> WoL interrupt enable in C22 register 0x12 bit 0
> WoL interrupt status in C22 register 0x13 bit 0
> WoL MAC address programmed in MMD3 registers 0x804a (bits 47:32)
> 0x804b (bits 31:16) and 0x804c (bits 15:0)
>
> So, what this means is that AR8035, the only possibility for WoL is via the INT
> pin and the C22 interrupt enable/status registers.
>
> For AR8031 and AR8033, it depends how the hardware is wired.
>
> If WOL_INT is used to wake the system, then MMD3 0x8012 has to be used
> to enable or disable that functionality. From my reading of the datasheets,
> WOL_INT is unaffected by the C22 interrupt enable register settings.
>
> If INT is used to wake the system, then it behaves the same as AR8035.
> However, the datasheet doesn't make it clear whether MMD3 0x8012 bit 5
> also has an effect - although I would lean more towards it having an effect.
>
> So, given that:
>
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index
> > c1f307d90518..1d61f7190367 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -459,21 +459,27 @@ static int at803x_set_wol(struct phy_device
> *phydev,
> > phy_write_mmd(phydev, MDIO_MMD_PCS,
> offsets[i],
> > mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
> >
> > - /* Enable WOL function */
> > - ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL,
> > - 0, AT803X_WOL_EN);
> > - if (ret)
> > - return ret;
> > + /* Enable WOL function for 1588 */
> > + if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > + AT803X_PHY_MMD3_WOL_CTRL,
> > + 0, AT803X_WOL_EN);
> > + if (ret)
> > + return ret;
> > + }
> > /* Enable WOL interrupt */
> > ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0,
> AT803X_INTR_ENABLE_WOL);
> > if (ret)
> > return ret;
> > } else {
> > - /* Disable WoL function */
> > - ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL,
> > - AT803X_WOL_EN, 0);
> > - if (ret)
> > - return ret;
> > + /* Disable WoL function for 1588 */
> > + if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > + AT803X_PHY_MMD3_WOL_CTRL,
> > + AT803X_WOL_EN, 0);
> > + if (ret)
> > + return ret;
> > + }
> > /* Disable WOL interrupt */
> > ret = phy_modify(phydev, AT803X_INTR_ENABLE,
> AT803X_INTR_ENABLE_WOL, 0);
> > if (ret)
> > @@ -508,11 +514,11 @@ static void at803x_get_wol(struct phy_device
> *phydev,
> > wol->supported = WAKE_MAGIC;
> > wol->wolopts = 0;
> >
> > - value = phy_read_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL);
> > + value = phy_read(phydev, AT803X_INTR_ENABLE);
> > if (value < 0)
> > return;
> >
> > - if (value & AT803X_WOL_EN)
> > + if (value & AT803X_INTR_ENABLE_WOL)
> > wol->wolopts |= WAKE_MAGIC;
> > }
> >
>
> The above all looks correct to me.
>
> > @@ -858,9 +864,6 @@ static int at803x_probe(struct phy_device *phydev)
> > if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> > int mode_cfg;
> > - struct ethtool_wolinfo wol = {
> > - .wolopts = 0,
> > - };
> >
> > if (ccr < 0)
> > return ccr;
> > @@ -876,13 +879,6 @@ static int at803x_probe(struct phy_device *phydev)
> > priv->is_fiber = true;
> > break;
> > }
> > -
> > - /* Disable WOL by default */
> > - ret = at803x_set_wol(phydev, &wol);
> > - if (ret < 0) {
> > - phydev_err(phydev, "failed to disable WOL on
> probe: %d\n", ret);
> > - return ret;
> > - }
> > }
> >
> > return 0;
>
> This doesn't look correct to me, because in the case of AR8031 or
> AR8033 using WOL_INT, because MMD3 0x8012 bit 5 defaults on reset to
> being set, if we don't want WoL enabled after the PHY has been probed, we
> need to clear it.
The intent was to remove the code added by d7cd5e06c9d ("net: phy: at803x: disable WOL at probe") which claims to be needed after 7beecaf7d507b ("net: phy: at803x: improve the WOL feature"). Since I have changed the behavior back to before 7beecaf7d507b, it is natural that this part of d7cd5e06c9d is no longer needed.
But if we want MMD3 0x8012 bit 5 to be aligned with the WOL_INT on probe, we probably just need to clear the bit for AR8031/8033 instead of calling the set_wol for all chips.
Regards,
Leo
Powered by blists - more mailing lists