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]

Message-ID: <a7ae8bf8-7521-ebbd-82dc-6338f766b59c@gmail.com>
Date:   Fri, 10 Mar 2023 21:51:37 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Simon Horman <simon.horman@...igine.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 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.

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

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