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

Message-ID: <ZA8QZBfrg5S4NOc4@corigine.com>
Date:   Mon, 13 Mar 2023 13:00:36 +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 Fri, Mar 10, 2023 at 09:51:37PM +0100, Heiner Kallweit wrote:
> On 09.03.2023 15:31, Simon Horman wrote:
> > 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.
> > 
> There's no known problem with the current code, so the need for a fix
> may be questionable. But you're right, it's a separate change.
> IMO it just wasn't worth it to provide it as a separate patch.

Ok, I don't feel strongly about this.
Though perhaps it would be nice to mention in the patch
description if a v2 materialises.

In any case, I am happy with the correctness of this patch.

Signed-off-by: Simon Horman <simon.horman@...igine.com>

> >>  
> >>  	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.
> > 
> Same as for the remark before.

Ack.

> >>  				  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
> >>
>