Re: [PATCH net-next v2 2/4] net: stmmac: stm32: add WoL from PHY support

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Sat, Sep 27, 2025 at 02:04:15PM -0700, Florian Fainelli wrote:
> 
> 
> On 9/26/2025 12:05 PM, Florian Fainelli wrote:
> > 
> > 
> > On 9/26/2025 10:59 AM, Russell King (Oracle) wrote:
> > > On Wed, Sep 17, 2025 at 05:31:16PM +0100, Russell King (Oracle) wrote:
> > > > On Wed, Sep 17, 2025 at 05:36:37PM +0200, Gatien Chevallier wrote:
> > > > > If the "st,phy-wol" property is present in the device tree node,
> > > > > set the STMMAC_FLAG_USE_PHY_WOL flag to use the WoL capability of
> > > > > the PHY.
> > > > > 
> > > > > Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> > > > > ---
> > > > >   drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c | 5 +++++
> > > > >   1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git
> > > > > a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/
> > > > > drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> > > > > index 77a04c4579c9dbae886a0b387f69610a932b7b9e..6f197789cc2e8018d6959158b795e4bca46869c5
> > > > > 100644
> > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> > > > > @@ -106,6 +106,7 @@ struct stm32_dwmac {
> > > > >       u32 speed;
> > > > >       const struct stm32_ops *ops;
> > > > >       struct device *dev;
> > > > > +    bool phy_wol;
> > > > >   };
> > > > >   struct stm32_ops {
> > > > > @@ -433,6 +434,8 @@ static int stm32_dwmac_parse_data(struct
> > > > > stm32_dwmac *dwmac,
> > > > >           }
> > > > >       }
> > > > > +    dwmac->phy_wol = of_property_read_bool(np, "st,phy-wol");
> > > > > +
> > > > >       return err;
> > > > >   }
> > > > > @@ -557,6 +560,8 @@ static int stm32_dwmac_probe(struct
> > > > > platform_device *pdev)
> > > > >       plat_dat->bsp_priv = dwmac;
> > > > >       plat_dat->suspend = stm32_dwmac_suspend;
> > > > >       plat_dat->resume = stm32_dwmac_resume;
> > > > > +    if (dwmac->phy_wol)
> > > > > +        plat_dat->flags |= STMMAC_FLAG_USE_PHY_WOL;
> > > > 
> > > > I would much rather we found a different approach, rather than adding
> > > > custom per-driver DT properties to figure this out.
> > > > 
> > > > Andrew has previously suggested that MAC drivers should ask the PHY
> > > > whether WoL is supported, but this pre-supposes that PHY drivers are
> > > > coded correctly to only report WoL capabilities if they are really
> > > > capable of waking the system. As shown in your smsc PHY driver patch,
> > > > this may not be the case.
> > > > 
> > > > Given that we have historically had PHY drivers reporting WoL
> > > > capabilities without being able to wake the system, we can't
> > > > implement Andrew's suggestion easily.
> > > > 
> > > > The only approach I can think that would allow us to transition is
> > > > to add:
> > > > 
> > > > static inline bool phy_can_wakeup(struct phy_device *phy_dev)
> > > > {
> > > >     return device_can_wakeup(&phy_dev->mdio.dev);
> > > > }
> > > > 
> > > > to include/linux/phy.h, and a corresponding wrapper for phylink.
> > > > This can then be used to determine whether to attempt to use PHY-based
> > > > Wol in stmmac_get_wol() and rtl8211f_set_wol(), falling back to
> > > > PMT-based WoL if supported at the MAC.
> > > > 
> > > > So, maybe something like:
> > > > 
> > > > static u32 stmmac_wol_support(struct stmmac_priv *priv)
> > > > {
> > > >     u32 support = 0;
> > > > 
> > > >     if (priv->plat->pmt && device_can_wakeup(priv->device)) {
> > > >         support = WAKE_UCAST;
> > > >         if (priv->hw_cap_support && priv->dma_cap.pmt_magic_frame)
> > > >             support |= WAKE_MAGIC;
> > > >     }
> > > > 
> > > >     return support;
> > > > }
> > > > 
> > > > static void stmmac_get_wol(struct net_device *dev, struct
> > > > ethtool_wolinfo *wol)
> > > > {
> > > >     struct stmmac_priv *priv = netdev_priv(dev);
> > > >     int err;
> > > > 
> > > >     /* Check STMMAC_FLAG_USE_PHY_WOL for legacy */
> > > >     if (phylink_can_wakeup(priv->phylink) ||
> > > >         priv->plat->flags & STMMAC_FLAG_USE_PHY_WOL) {
> > > >         err = phylink_ethtool_get_wol(priv->phylink, wol);
> > > >         if (err != 0 && err != -EOPNOTSUPP)
> > > >             return;
> > > >     }
> > > > 
> > > >     wol->supported |= stmmac_wol_support(priv);
> > > > 
> > > >     /* A read of priv->wolopts is single-copy atomic. Locking
> > > >      * doesn't add any benefit.
> > > >      */
> > > >     wol->wolopts |= priv->wolopts;
> > > > }
> > > > 
> > > > static int stmmac_set_wol(struct net_device *dev, struct
> > > > ethtool_wolinfo *wol)
> > > > {
> > > >     struct stmmac_priv *priv = netdev_priv(dev);
> > > >     u32 support, wolopts;
> > > >     int err;
> > > > 
> > > >     wolopts = wol->wolopts;
> > > > 
> > > >     /* Check STMMAC_FLAG_USE_PHY_WOL for legacy */
> > > >     if (phylink_can_wakeup(priv->phylink) ||
> > > >         priv->plat->flags & STMMAC_FLAG_USE_PHY_WOL) {
> > > >         struct ethtool_wolinfo w;
> > > > 
> > > >         err = phylink_ethtool_set_wol(priv->phylink, wol);
> > > >         if (err != -EOPNOTSUPP)
> > > >             return err;
> > > > 
> > > >         /* Remove the WoL modes that the PHY is handling */
> > > >         if (!phylink_ethtool_get_wol(priv->phylink, &w))
> > > >             wolopts &= ~w.wolopts;
> > > >     }
> > > > 
> > > >     support = stmmac_wol_support(priv);
> > > > 
> > > >     mutex_lock(&priv->lock);
> > > >     priv->wolopts = wolopts & support;
> > > >     device_set_wakeup_enable(priv->device, !!priv->wolopts);
> > > >     mutex_unlock(&priv->lock);
> > > > 
> > > >     return 0;
> > > > }
> > > > 
> > > > ... and now I'm wondering whether this complexity is something that
> > > > phylink should handle internally, presenting a mac_set_wol() method
> > > > to configure the MAC-side WoL settings. What makes it difficult to
> > > > just move into phylink is the STMMAC_FLAG_USE_PHY_WOL flag, but
> > > > that could be a "force_phy_wol" flag in struct phylink_config as
> > > > a transitionary measure... so long as PHY drivers get fixed.
> > > 
> > > I came up with this as an experiment - I haven't tested it beyond
> > > running it through the compiler (didn't let it get to the link stage
> > > yet.) Haven't even done anything with it for stmmac yet.
> > > 
> > 
> > I like the direction this is going, we could probably take one step
> > further and extract the logic present in bcmgenet_wol.c and make those
> > helper functions for other drivers to get the overlay of PHY+MAC WoL
> > options/password consistent across all drivers. What do you think?
> 
> +		if (wolopts & WAKE_MAGIC)
> +			changed |= !!memcmp(wol->sopass, pl->wol_sopass,
> +					    sizeof(wol->sopass));
> 
> 
> Should not the hunk above be wolopts & WAKE_MAGICSECURE?

Yes, and there's a few other bits missing as well. The series has
progressed, and stmmac is converted and tested on my Jetson platform.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!