Messages in this thread | | | Date | Wed, 23 Jun 2021 21:06:18 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next] net: phy: marvell10g: enable WoL for mv2110 |
| |
On Wed, Jun 23, 2021 at 09:09:29PM +0800, Ling Pei Lee wrote: > +static void mv2110_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > +{ > + int ret = 0;
This initialiser doesn't do anything.
> + > + wol->supported = WAKE_MAGIC; > + wol->wolopts = 0; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_WOL_CTRL); > + > + if (ret & MV_V2_WOL_MAGIC_PKT_EN) > + wol->wolopts |= WAKE_MAGIC;
You need to check whether "ret" is a negative number - if phy_read_mmd() returns an error, this test could be true or false. It would be better to have well defined behaviour (e.g. reporting that WOL is disabled?)
> + /* Reset the clear WOL status bit as it does not self-clear */ > + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, > + MV_V2_WOL_CTRL, > + MV_V2_WOL_CLEAR_STS); > + > + if (ret < 0) > + return ret; > + } else { > + /* Disable magic packet matching & reset WOL status bit */ > + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + MV_V2_WOL_CTRL, > + MV_V2_WOL_MAGIC_PKT_EN, > + MV_V2_WOL_CLEAR_STS); > + > + if (ret < 0) > + return ret; > + > + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, > + MV_V2_WOL_CTRL, > + MV_V2_WOL_CLEAR_STS); > + > + if (ret < 0) > + return ret;
This phy_clear_bits_mmd() is the same as the tail end of the other part of the if() clause. Consider moving it after the if () { } else { } statement...
> + } > + > + return ret;
and as all paths return "ret" just do:
return phy_clear_bits_mmd(...
I will also need to check whether this is the same as the 88x3310, but I'm afraid I don't have the energy this evening - please email me a remind to look at this tomorrow. Thanks.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |