netdev - Re: [PATCH net-next v5 08/10] net: phy: use phy_read_poll

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] [day] [month] [year] [list]

Message-ID: <20200322191832.GW25745@shell.armlinux.org.uk>
Date:   Sun, 22 Mar 2020 19:18:32 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Dejin Zheng <zhengdejin5@...il.com>
Cc:     andrew@...n.ch, f.fainelli@...il.com, hkallweit1@...il.com,
        davem@...emloft.net, mchehab+samsung@...nel.org, corbet@....net,
        gregkh@...uxfoundation.org, broonie@...nel.org, tglx@...utronix.de,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v5 08/10] net: phy: use phy_read_poll_timeout()
 to simplify the code

On Mon, Mar 23, 2020 at 01:49:41AM +0800, Dejin Zheng wrote:
> use phy_read_poll_timeout() to replace the poll codes for
> simplify the code in phy_poll_reset() function.
> 
> Signed-off-by: Dejin Zheng <zhengdejin5@...il.com>
> ---
> v4 -> v5:
> 	- it can add msleep before call phy_read_poll_timeout()
> 	  to keep the code more similar. so add it.
> v3 -> v4:
> 	- drop it.
> v2 -> v3:
> 	- adapt to it after modifying the parameter order of the
> 	  newly added function
> v1 -> v2:
> 	- remove the handle of phy_read()'s return error.
> 
> 
>  drivers/net/phy/phy_device.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a585faf8b844..24297ee7f626 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1059,23 +1059,16 @@ EXPORT_SYMBOL(phy_disconnect);
>  static int phy_poll_reset(struct phy_device *phydev)
>  {
>  	/* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
> -	unsigned int retries = 12;
> -	int ret;
> -
> -	do {
> -		msleep(50);
> -		ret = phy_read(phydev, MII_BMCR);
> -		if (ret < 0)
> -			return ret;
> -	} while (ret & BMCR_RESET && --retries);
> -	if (ret & BMCR_RESET)
> -		return -ETIMEDOUT;
> +	int ret, val;
>  
> +	msleep(50);
> +	ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
> +				    50000, 550000);
>  	/* Some chips (smsc911x) may still need up to another 1ms after the
>  	 * BMCR_RESET bit is cleared before they are usable.
>  	 */
>  	msleep(1);
> -	return 0;
> +	return ret;

This isn't actually equivaent behaviour.  If the read timed out, the old
code didn't wait 1ms.  Your new code does.  However, since we've waited
about 600ms already, it probably doesn't matter.

These sorts of things should be documented in the commit log, IMHO, so
it's obvious that it's been considered.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up