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

[Joakim Zhang <qiangqing.zhang@nxp.com>]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2021年4月28日 20:27
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
> joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org;
> f.fainelli@gmail.com; Jisheng.Zhang@synaptics.com; netdev@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.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