netdev - Re: [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read

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]

Date:   Thu, 9 Mar 2023 15:31:26 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Russell King - ARM Linux <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in
 lan87xx_read_status

On Wed, Mar 08, 2023 at 09:11:02PM +0100, Heiner Kallweit wrote:
> Simplify the code by using phy_clear/sert_bits().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> ---
>  drivers/net/phy/smsc.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)

The phy_clear/sert_bits changes lookg good.
But I have a few nit-pick comments.

> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index af89f3ef1..5965a8afa 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev)
>  static int lan87xx_read_status(struct phy_device *phydev)
>  {
>  	struct smsc_phy_priv *priv = phydev->priv;
> +	int rc;
>  
> -	int err = genphy_read_status(phydev);
> +	rc = genphy_read_status(phydev);
> +	if (rc)
> +		return rc;

nit: this seems like a separate change, possibly a fix.

>  
>  	if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {
>  		/* Disable EDPD to wake up PHY */
> -		int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> -		if (rc < 0)
> -			return rc;
> -
> -		rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
> -			       rc & ~MII_LAN83C185_EDPWRDOWN);
> +		rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS,
> +				    MII_LAN83C185_EDPWRDOWN);
>  		if (rc < 0)
>  			return rc;
>  
> @@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev)
>  		 * an actual error.
>  		 */
>  		read_poll_timeout(phy_read, rc,
> -				  rc & MII_LAN83C185_ENERGYON || rc < 0,
> +				  rc < 0 || rc & MII_LAN83C185_ENERGYON,

nit: this also seems like a separate change.

>  				  10000, 640000, true, phydev,
>  				  MII_LAN83C185_CTRL_STATUS);
>  		if (rc < 0)
>  			return rc;
>  
>  		/* Re-enable EDPD */
> -		rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> -		if (rc < 0)
> -			return rc;
> -
> -		rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
> -			       rc | MII_LAN83C185_EDPWRDOWN);
> +		rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS,
> +				  MII_LAN83C185_EDPWRDOWN);
>  		if (rc < 0)
>  			return rc;
>  	}
>  
> -	return err;
> +	return 0;
>  }
>  
>  static int smsc_get_sset_count(struct phy_device *phydev)
> -- 
> 2.39.2
>