linux-kernel - Re: [PATCH net-next] net: phy: marvell10g: enable WoL for mv2110

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: <20210623200618.GO22278@shell.armlinux.org.uk>
Date:   Wed, 23 Jun 2021 21:06:18 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Ling Pei Lee <pei.lee.ling@...el.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>, davem@...emloft.net,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Marek Behun <marek.behun@....cz>,
        weifeng.voon@...el.com, vee.khee.wong@...ux.intel.com,
        vee.khee.wong@...el.com
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!