netdev - RE: [PATCH V3 net] net: stmmac: fix MAC WoL unwork if PHY doesn't support WoL

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]

Message-ID: <DB8PR04MB6795286E3C03699616C8C5C4E65F9@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Thu, 29 Apr 2021 07:09:47 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Andrew Lunn <andrew@...n.ch>
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>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "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 V3 net] net: stmmac: fix MAC WoL unwork if PHY doesn't
 support WoL


> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: 2021年4月28日 20:27
> To: Joakim Zhang <qiangqing.zhang@....com>
> Cc: peppe.cavallaro@...com; alexandre.torgue@...com;
> joabreu@...opsys.com; davem@...emloft.net; kuba@...nel.org;
> f.fainelli@...il.com; Jisheng.Zhang@...aptics.com; netdev@...r.kernel.org;
> dl-linux-imx <linux-imx@....com>
> Subject: Re: [PATCH V3 net] net: stmmac: fix MAC WoL unwork if PHY doesn't
> support WoL
> 
> >  static int stmmac_set_wol(struct net_device *dev, struct
> > ethtool_wolinfo *wol)  {
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> > -	u32 support = WAKE_MAGIC | WAKE_UCAST;
> > +	struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL };
> > +	u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE |
> > +WAKE_BCAST;
> 
> Reverse christmass tree please.
Ok

> >
> > -	if (!device_can_wakeup(priv->device))
> > -		return -EOPNOTSUPP;
> > +	if (wol->wolopts & ~support)
> > +		return -EINVAL;
> 
> Maybe -EOPNOTSUPP would be better.
Ok

> >
> > -	if (!priv->plat->pmt) {
> > +	/* First check if can WoL from PHY */
> > +	phylink_ethtool_get_wol(priv->phylink, &wol_phy);
> 
> This could return an error. In which case, you probably should not trust
> wol_phy.
phylink_ethtool_get_wol() is a void function, there is no return value. I think we can trust wol_phy, if PHY driver does not implement
get_wol(), wol_phy.supported is 0; if PHY driver implement it, then it will fill the wol_phy.supported field. Please point me if I am misunderstanding.

> > +	if (wol->wolopts & wol_phy.supported) {
> 
> This returns true if the PHY supports one or more of the requested WoL
> sources.
>
> >  		int ret = phylink_ethtool_set_wol(priv->phylink, wol);
> 
> and here you request the PHY to enable all the requested WoL sources. If it
> only supports a subset, it is likely to return -EOPNOTSUPP, or -EINVAL, and do
> nothing. So here you only want to enable those sources the PHY actually
> supports. And let the MAC implement the rest.
Yes, you are right! It should be:
	wol->wolopts &= wol_phy.supported;
	ret = phylink_ethtool_set_wol(priv->phylink, wol);

Best Regards,
Joakim Zhang
> 	  Andrew