| Message ID | 20230508184309.1628108-2-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support for Wake-on-LAN for Broadcom PHYs | expand |
On Mon, May 08, 2023 at 11:43:07AM -0700, Florian Fainelli wrote: > A few PHY drivers are currently attempting to not suspend the PHY when > Wake-on-LAN is enabled, however that code is not currently executing at > all due to an early check in phy_suspend(). > > This prevents PHY drivers from making an appropriate decisions and put > the hardware into a low power state if desired. > > In order to allow the PHY framework to always call into the PHY driver's > ->suspend routine whether Wake-on-LAN is enabled or not, provide a > phydev::wol_enabled boolean that tracks whether the PHY or the attached > MAC has Wake-on-LAN enabled. > > If phydev::wol_enabled then the PHY shall not prevent its own > Wake-on-LAN detection logic from working and shall not prevent the > Ethernet MAC from receiving packets for matching. Hi Florian Did you look at using late_suspend for this? Then there would not be any need to change all these drivers which are happy as they are. It actually looks like late_suspend is what you want anyway, when you say you want to turn the matching hardware on as late as possible. Andrew
On 5/8/23 12:02, Andrew Lunn wrote: > On Mon, May 08, 2023 at 11:43:07AM -0700, Florian Fainelli wrote: >> A few PHY drivers are currently attempting to not suspend the PHY when >> Wake-on-LAN is enabled, however that code is not currently executing at >> all due to an early check in phy_suspend(). >> >> This prevents PHY drivers from making an appropriate decisions and put >> the hardware into a low power state if desired. >> >> In order to allow the PHY framework to always call into the PHY driver's >> ->suspend routine whether Wake-on-LAN is enabled or not, provide a >> phydev::wol_enabled boolean that tracks whether the PHY or the attached >> MAC has Wake-on-LAN enabled. >> >> If phydev::wol_enabled then the PHY shall not prevent its own >> Wake-on-LAN detection logic from working and shall not prevent the >> Ethernet MAC from receiving packets for matching. > > Hi Florian > > Did you look at using late_suspend for this? Then there would not be > any need to change all these drivers which are happy as they are. I did not know its existence until you mentioned it, this would require plumbing all the way from the MDIO bus driver down to the PHY driver level, which could be done, but for a single driver? The way Linux suspends devices currently, and the fact that the interrupt is driven level low, it does not give much room if at all for missing the interrupt AFAICT. phy_suspend() is called both from the system suspend path, but also whenever the PHY is unused, and this is a nice property because we do not really need to differentiate these paths usually, that includes Wake-on-LAN. Besides there are drivers like drivers/net/phy/at803x.c that wish to isolate the PHY if WoL is enabled and which are currently not doing it because this never gets called.
On 5/8/23 12:17, Florian Fainelli wrote: > On 5/8/23 12:02, Andrew Lunn wrote: >> On Mon, May 08, 2023 at 11:43:07AM -0700, Florian Fainelli wrote: >>> A few PHY drivers are currently attempting to not suspend the PHY when >>> Wake-on-LAN is enabled, however that code is not currently executing at >>> all due to an early check in phy_suspend(). >>> >>> This prevents PHY drivers from making an appropriate decisions and put >>> the hardware into a low power state if desired. >>> >>> In order to allow the PHY framework to always call into the PHY driver's >>> ->suspend routine whether Wake-on-LAN is enabled or not, provide a >>> phydev::wol_enabled boolean that tracks whether the PHY or the attached >>> MAC has Wake-on-LAN enabled. >>> >>> If phydev::wol_enabled then the PHY shall not prevent its own >>> Wake-on-LAN detection logic from working and shall not prevent the >>> Ethernet MAC from receiving packets for matching. >> >> Hi Florian >> >> Did you look at using late_suspend for this? Then there would not be >> any need to change all these drivers which are happy as they are. > > I did not know its existence until you mentioned it, this would require > plumbing all the way from the MDIO bus driver down to the PHY driver > level, which could be done, but for a single driver? The way Linux > suspends devices currently, and the fact that the interrupt is driven > level low, it does not give much room if at all for missing the > interrupt AFAICT. I suppose that a middle ground could be to introduce a specific callback or flag that says: please call my suspend routine and I will do what's necessary. > > phy_suspend() is called both from the system suspend path, but also > whenever the PHY is unused, and this is a nice property because we do > not really need to differentiate these paths usually, that includes > Wake-on-LAN. > > Besides there are drivers like drivers/net/phy/at803x.c that wish to > isolate the PHY if WoL is enabled and which are currently not doing it > because this never gets called.
diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c index 334a6904ca5a..ffe4e8f16c07 100644 --- a/drivers/net/phy/aquantia_main.c +++ b/drivers/net/phy/aquantia_main.c @@ -691,6 +691,9 @@ static int aqr107_suspend(struct phy_device *phydev) { int err; + if (phydev->wol_enabled) + return 0; + err = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, MDIO_CTRL1, MDIO_CTRL1_LPOWER); if (err) diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 656136628ffd..acd941beeb8e 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -569,6 +569,13 @@ static int at803x_suspend(struct phy_device *phydev) int value; int wol_enabled; + /* We cannot isolate if the attached network device has + * Wake-on-LAN enabled since we would not be passing + * packets through it. + */ + if (phydev->attached_dev->wol_enabled) + return 0; + value = phy_read(phydev, AT803X_INTR_ENABLE); wol_enabled = value & AT803X_INTR_ENABLE_WOL; @@ -1701,6 +1708,9 @@ static int qca83xx_suspend(struct phy_device *phydev) { u16 mask = 0; + if (phydev->wol_enabled) + return 0; + /* Only QCA8337 support actual suspend. * QCA8327 cause port unreliability when phy suspend * is set. diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c index 06be71ecd2f8..f060a5783694 100644 --- a/drivers/net/phy/bcm7xxx.c +++ b/drivers/net/phy/bcm7xxx.c @@ -747,6 +747,9 @@ static int bcm7xxx_suspend(struct phy_device *phydev) }; unsigned int i; + if (phydev->wol_enabled) + return 0; + for (i = 0; i < ARRAY_SIZE(bcm7xxx_suspend_cfg); i++) { ret = phy_write(phydev, bcm7xxx_suspend_cfg[i].reg, diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index ad71c88c87e7..3f142dc266a9 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -443,6 +443,9 @@ static int bcm54xx_suspend(struct phy_device *phydev) bcm54xx_ptp_stop(phydev); + if (phydev->wol_enabled) + return 0; + /* We cannot use a read/modify/write here otherwise the PHY gets into * a bad state where its LEDs keep flashing, thus defeating the purpose * of low power mode. @@ -770,6 +773,9 @@ static int brcm_fet_suspend(struct phy_device *phydev) { int reg, err, err2, brcmtest; + if (phydev->wol_enabled) + return 0; + /* We cannot use a read/modify/write here otherwise the PHY continues * to drive LEDs which defeats the purpose of low power mode. */ diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c index b7cb71817780..2ab0cbd1a60f 100644 --- a/drivers/net/phy/dp83822.c +++ b/drivers/net/phy/dp83822.c @@ -573,7 +573,7 @@ static int dp83822_suspend(struct phy_device *phydev) value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG); - if (!(value & DP83822_WOL_EN)) + if (!(value & DP83822_WOL_EN) && !phydev->wol_enabled) genphy_suspend(phydev); return 0; diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index d75f526a20a4..8f2b19e57f90 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -708,6 +708,9 @@ static int dp83867_suspend(struct phy_device *phydev) dp83867_config_intr(phydev); } + if (phydev->wol_enabled) + return 0; + return genphy_suspend(phydev); } diff --git a/drivers/net/phy/dp83tc811.c b/drivers/net/phy/dp83tc811.c index 7ea32fb77190..2106f9ea15f5 100644 --- a/drivers/net/phy/dp83tc811.c +++ b/drivers/net/phy/dp83tc811.c @@ -368,7 +368,7 @@ static int dp83811_suspend(struct phy_device *phydev) value = phy_read_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_CFG); - if (!(value & DP83811_WOL_EN)) + if (!(value & DP83811_WOL_EN) && !phydev->wol_enabled) genphy_suspend(phydev); return 0; diff --git a/drivers/net/phy/marvell-88x2222.c b/drivers/net/phy/marvell-88x2222.c index f83cae64585d..9c13e4dd1807 100644 --- a/drivers/net/phy/marvell-88x2222.c +++ b/drivers/net/phy/marvell-88x2222.c @@ -458,6 +458,9 @@ static int mv2222_resume(struct phy_device *phydev) static int mv2222_suspend(struct phy_device *phydev) { + /* No need to check for phydev->wol_enabled since we only disable the + * transmitter + */ return mv2222_tx_disable(phydev); } diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 43b6cb725551..c08ad1c3f4c3 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -1741,6 +1741,9 @@ static int marvell_suspend(struct phy_device *phydev) { int err; + if (phydev->wol_enabled) + return 0; + /* Suspend the fiber mode first */ if (linkmode_test_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported)) { diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 55d9d7acc32e..6d45a94e5bea 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -559,6 +559,9 @@ static void mv3310_remove(struct phy_device *phydev) static int mv3310_suspend(struct phy_device *phydev) { + if (phydev->wol_enabled) + return 0; + return mv3310_power_down(phydev); } diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 3f81bb8dac44..fe71ab8926fc 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -1826,6 +1826,9 @@ static int kszphy_suspend(struct phy_device *phydev) phydev->drv->config_intr(phydev); } + if (phydev->wol_enabled) + return 0; + return genphy_suspend(phydev); } diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c index 0b88635f4fbc..5290fd684c93 100644 --- a/drivers/net/phy/microchip.c +++ b/drivers/net/phy/microchip.c @@ -74,10 +74,8 @@ static irqreturn_t lan88xx_handle_interrupt(struct phy_device *phydev) static int lan88xx_suspend(struct phy_device *phydev) { - struct lan88xx_priv *priv = phydev->priv; - /* do not power down PHY when WOL is enabled */ - if (!priv->wolopts) + if (!phydev->wol_enabled) genphy_suspend(phydev); return 0; diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c index 2fa5a90e073b..50472708c15e 100644 --- a/drivers/net/phy/motorcomm.c +++ b/drivers/net/phy/motorcomm.c @@ -1414,7 +1414,7 @@ static int yt8521_suspend(struct phy_device *phydev) return wol_config; /* if wol enable, do nothing */ - if (wol_config & YTPHY_WCR_ENABLE) + if ((wol_config & YTPHY_WCR_ENABLE) || phydev->wol_enabled) return 0; return yt8521_modify_utp_fiber_bmcr(phydev, 0, BMCR_PDOWN); diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index fee514b96ab1..355e78c441a8 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -64,6 +64,9 @@ EXPORT_SYMBOL_GPL(genphy_c45_pma_resume); */ int genphy_c45_pma_suspend(struct phy_device *phydev) { + if (phydev->wol_enabled) + return 0; + if (!genphy_c45_pma_can_sleep(phydev)) return -EOPNOTSUPP; diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 17d0d0555a79..ae86e169a578 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1862,8 +1862,8 @@ int phy_suspend(struct phy_device *phydev) /* If the device has WOL enabled, we cannot suspend the PHY */ phy_ethtool_get_wol(phydev, &wol); - if (wol.wolopts || (netdev && netdev->wol_enabled)) - return -EBUSY; + phydev->wol_enabled = !!(wol.wolopts || (netdev && + netdev->wol_enabled)); if (!phydrv || !phydrv->suspend) return 0; @@ -2687,6 +2687,9 @@ EXPORT_SYMBOL(genphy_write_mmd_unsupported); int genphy_suspend(struct phy_device *phydev) { + if (phydev->wol_enabled) + return 0; + return phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN); } EXPORT_SYMBOL(genphy_suspend); diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index 3d99fd6664d7..316f9e868d84 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -482,6 +482,9 @@ static int rtl8211e_config_init(struct phy_device *phydev) static int rtl8211b_suspend(struct phy_device *phydev) { + if (phydev->wol_enabled) + return 0; + phy_write(phydev, MII_MMD_DATA, BIT(9)); return genphy_suspend(phydev); diff --git a/include/linux/phy.h b/include/linux/phy.h index c5a0dc829714..e36dfc8236b4 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -548,6 +548,8 @@ struct macsec_ops; * @downshifted_rate: Set true if link speed has been downshifted. * @is_on_sfp_module: Set true if PHY is located on an SFP module. * @mac_managed_pm: Set true if MAC driver takes of suspending/resuming PHY + * @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN + * enabled. * @state: State of the PHY for management purposes * @dev_flags: Device-specific flags used by the PHY driver. * @@ -644,6 +646,7 @@ struct phy_device { unsigned downshifted_rate:1; unsigned is_on_sfp_module:1; unsigned mac_managed_pm:1; + unsigned wol_enabled:1; unsigned autoneg:1; /* The most recently read link state */
A few PHY drivers are currently attempting to not suspend the PHY when Wake-on-LAN is enabled, however that code is not currently executing at all due to an early check in phy_suspend(). This prevents PHY drivers from making an appropriate decisions and put the hardware into a low power state if desired. In order to allow the PHY framework to always call into the PHY driver's ->suspend routine whether Wake-on-LAN is enabled or not, provide a phydev::wol_enabled boolean that tracks whether the PHY or the attached MAC has Wake-on-LAN enabled. If phydev::wol_enabled then the PHY shall not prevent its own Wake-on-LAN detection logic from working and shall not prevent the Ethernet MAC from receiving packets for matching. The check for -EBUSY is also removed since it would have prevented a PHY being in an always on power domain and thus retaining its Wake-on-LAN configuration from allowing the system to suspend. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/phy/aquantia_main.c | 3 +++ drivers/net/phy/at803x.c | 10 ++++++++++ drivers/net/phy/bcm7xxx.c | 3 +++ drivers/net/phy/broadcom.c | 6 ++++++ drivers/net/phy/dp83822.c | 2 +- drivers/net/phy/dp83867.c | 3 +++ drivers/net/phy/dp83tc811.c | 2 +- drivers/net/phy/marvell-88x2222.c | 3 +++ drivers/net/phy/marvell.c | 3 +++ drivers/net/phy/marvell10g.c | 3 +++ drivers/net/phy/micrel.c | 3 +++ drivers/net/phy/microchip.c | 4 +--- drivers/net/phy/motorcomm.c | 2 +- drivers/net/phy/phy-c45.c | 3 +++ drivers/net/phy/phy_device.c | 7 +++++-- drivers/net/phy/realtek.c | 3 +++ include/linux/phy.h | 3 +++ 17 files changed, 55 insertions(+), 8 deletions(-)