Date:   Fri, 7 May 2021 10:59:12 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
        "alexandre.torgue@...com" <alexandre.torgue@...com>,
        "joabreu@...opsys.com" <joabreu@...opsys.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "Jisheng.Zhang@...aptics.com" <Jisheng.Zhang@...aptics.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH V4 net] net: stmmac: Fix MAC WoL not working if PHY does
 not support WoL


Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: 2021年5月7日 8:55
> To: Joakim Zhang <qiangqing.zhang@....com>
> Cc: peppe.cavallaro@...com; alexandre.torgue@...com;
> joabreu@...opsys.com; davem@...emloft.net; f.fainelli@...il.com;
> andrew@...n.ch; Jisheng.Zhang@...aptics.com; netdev@...r.kernel.org;
> dl-linux-imx <linux-imx@....com>
> Subject: Re: [PATCH V4 net] net: stmmac: Fix MAC WoL not working if PHY does
> not support WoL
> 
> On Thu,  6 May 2021 13:06:58 +0800 Joakim Zhang wrote:
> > Both get and set WoL will check device_can_wakeup(), if MAC supports
> > PMT, it will set device wakeup capability. After commit 1d8e5b0f3f2c ("net:
> > stmmac: Support WOL with phy"), device wakeup capability will be
> > overwrite in stmmac_init_phy() according to phy's Wol feature. If phy
> > doesn't support WoL, then MAC will lose wakeup capability.
> 
> Let's take a step back. Can we get a minimal fix for losing the config in
> stmmac_init_phy(), and then extend the support for WoL for devices which do
> support wake up themselves?

Sure, please review the V1, I think this is a minimal fix, then we can extend this as a new feature.
https://www.spinics.net/lists/netdev/msg733531.html

> > This patch combines WoL capabilities both MAC and PHY from
> > stmmac_get_wol(), set wakeup capability and give WoL priority to the
> > PHY in stmmac_set_wol() when enable WoL. What PHYs do implement is
> > WAKE_MAGIC, WAKE_UCAST, WAKE_MAGICSECURE and WAKE_BCAST.
> >
> > Fixes: commit 1d8e5b0f3f2c ("net: stmmac: Support WOL with phy")
> 
> Please remove "commit" from the fixes tag.

Yes.

> > Suggested-by: Andrew Lunn <andrew@...n.ch>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > index c5642985ef95..6d09908dec1f 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > @@ -629,35 +629,49 @@ static void stmmac_get_strings(struct net_device
> > *dev, u32 stringset, u8 *data)
> >  /* Currently only support WOL through Magic packet. */  static void
> > stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)  {
> > +	struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL };
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> >
> > -	if (!priv->plat->pmt)
> > -		return phylink_ethtool_get_wol(priv->phylink, wol);
> > -
> >  	mutex_lock(&priv->lock);
> > -	if (device_can_wakeup(priv->device)) {
> 
> Why remove the device_can_wakeup() check?

The original code logic is setting wakeup capability when check it supports PMT in stmmac_hw_init () at probe stage. After this patch,
we set wakeup capability when configure WoL in stmmac_set_wol(), so we change to check " priv->plat->pmt ", rather than " device_can_wakeup()".
 
> > +	if (priv->plat->pmt) {
> >  		wol->supported = WAKE_MAGIC | WAKE_UCAST;
> >  		if (priv->hw_cap_support && !priv->dma_cap.pmt_magic_frame)
> >  			wol->supported &= ~WAKE_MAGIC;
> > -		wol->wolopts = priv->wolopts;
> >  	}
> > +
> > +	phylink_ethtool_get_wol(priv->phylink, &wol_phy);
> > +
> > +	/* Combine WoL capabilities both PHY and MAC */
> > +	wol->supported |= wol_phy.supported;
> > +	wol->wolopts = priv->wolopts;
> > +
> >  	mutex_unlock(&priv->lock);
> >  }
> >
> >  static int stmmac_set_wol(struct net_device *dev, struct
> > ethtool_wolinfo *wol)  {
> > +	u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE |
> > +WAKE_BCAST;
> 
> Why this list?

Please see comments from Andrew: https://lore.kernel.org/netdev/YIgBJQi1H+f2VGWf@lunn.ch/T/#m00f11a84c1c43b3b4047dffcdfce57d534565a96
"What PHYs do implement is WAKE_MAGIC, WAKE_MAGICSEC, WAKE_UCAST, and WAKE_BCAST. So there is a clear overlap with what the MAC can do."

So this list is cover all the WoL sources both PHY and STMMAC.

> > +	struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL };
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> > -	u32 support = WAKE_MAGIC | WAKE_UCAST;
> 
> This list was the list of what the MAC supported, right?

Right.

> > +	int ret;
> >
> > -	if (!device_can_wakeup(priv->device))
> 
> Why remove this check?

Explain above.

> > +	if (wol->wolopts & ~support)
> >  		return -EOPNOTSUPP;
> >
> > -	if (!priv->plat->pmt) {
> > -		int ret = phylink_ethtool_set_wol(priv->phylink, wol);
> > -
> > -		if (!ret)
> > -			device_set_wakeup_enable(priv->device, !!wol->wolopts);
> > -		return ret;
> > +	/* First check if can WoL from PHY */
> > +	phylink_ethtool_get_wol(priv->phylink, &wol_phy);
> > +	if (wol->wolopts & wol_phy.supported) {
> 
> So if _any_ requests match PHY capabilities we'd use PHY?
> I think the check should be:
> 
> 	if ((wol->woltops & wol_phy.supported) == wol->woltops)
> 
> That said I'm not 100% sure what the semantics for WoL are.

Yes, you are right. Multiple wakeup sources can be enabled at the same time, from PHY or MAC, we give priority to PHY.

As Andrew said:
"If you are trying to save power, it is better if the PHY provides the WoL sources. If the PHY can provide all the required WoL sources, you
can turn the MAC off and save more power. So give priority to the PHY."

> > +		wol->wolopts &= wol_phy.supported;
> > +		ret = phylink_ethtool_set_wol(priv->phylink, wol);
> > +
> > +		if (!ret) {
> > +			pr_info("stmmac: phy wakeup enable\n");
> > +			device_set_wakeup_capable(priv->device, 1);
> > +			device_set_wakeup_enable(priv->device, 1);
> > +			goto wolopts_update;
> > +		} else {
> > +			return ret;
> > +		}
> >  	}
> >
> >  	/* By default almost all GMAC devices support the WoL via @@ -666,18
> > +680,21 @@ static int stmmac_set_wol(struct net_device *dev, struct
> ethtool_wolinfo *wol)
> >  	if ((priv->hw_cap_support) && (!priv->dma_cap.pmt_magic_frame))
> >  		wol->wolopts &= ~WAKE_MAGIC;
> >
> > -	if (wol->wolopts & ~support)
> > -		return -EINVAL;
> 
> Now you seem to not validate against MAC capabilities anywhere.

Yes, since we have combined WoL capabilities both PHY and MAC in stmmac_get_wol().

Best Regards,
Joakim Zhang
> > -	if (wol->wolopts) {
> > -		pr_info("stmmac: wakeup enable\n");
> > +	if (priv->plat->pmt && wol->wolopts) {
> > +		pr_info("stmmac: mac wakeup enable\n");
> > +		device_set_wakeup_capable(priv->device, 1);
> >  		device_set_wakeup_enable(priv->device, 1);
> >  		enable_irq_wake(priv->wol_irq);
> > -	} else {
> > +		goto wolopts_update;
> > +	}
> > +
> > +	if (!wol->wolopts) {
> > +		device_set_wakeup_capable(priv->device, 0);
> >  		device_set_wakeup_enable(priv->device, 0);
> >  		disable_irq_wake(priv->wol_irq);
> >  	}
> >
> > +wolopts_update:
> >  	mutex_lock(&priv->lock);
> >  	priv->wolopts = wol->wolopts;
> >  	mutex_unlock(&priv->lock);

Powered by blists - more mailing lists