netdev - Re: [PATCH net 1/2] net: phy: DP83822: Fix WoL in config init to be disabled

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: <54778ab0-faf0-9432-c5b8-755f1401d193@ti.com>
Date:   Thu, 23 Apr 2020 12:49:11 -0500
From:   Dan Murphy <dmurphy@...com>
To:     Heiner Kallweit <hkallweit1@...il.com>, <andrew@...n.ch>,
        <f.fainelli@...il.com>
CC:     <linux@...linux.org.uk>, <davem@...emloft.net>,
        <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
        <afd@...com>
Subject: Re: [PATCH net 1/2] net: phy: DP83822: Fix WoL in config init to be
 disabled

Heiner

On 4/23/20 12:02 PM, Heiner Kallweit wrote:
> On 23.04.2020 18:39, Dan Murphy wrote:
>> The WoL feature should be disabled when config_init is called and the
>> feature should turned on or off  when set_wol is called.
>>
>> In addition updated the calls to modify the registers to use the set_bit
>> and clear_bit function calls.
>>
>> Fixes: 3b427751a9d0 ("net: phy: DP83822 initial driver submission")
>> Signed-off-by: Dan Murphy <dmurphy@...com>
>> ---
>>   drivers/net/phy/dp83822.c | 30 ++++++++++++++++--------------
>>   1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
>> index fe9aa3ad52a7..40fdfd043947 100644
>> --- a/drivers/net/phy/dp83822.c
>> +++ b/drivers/net/phy/dp83822.c
>> @@ -137,16 +137,19 @@ static int dp83822_set_wol(struct phy_device *phydev,
>>   			value &= ~DP83822_WOL_SECURE_ON;
>>   		}
>>   
>> -		value |= (DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL |
>> -			  DP83822_WOL_CLR_INDICATION);
>> -		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
>> -			      value);
>> +		/* Clear any pending WoL interrupt */
>> +		phy_read(phydev, MII_DP83822_MISR2);
>> +
>> +		value |= DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL |
>> +			DP83822_WOL_CLR_INDICATION;
>> +
>> +		return phy_set_bits_mmd(phydev, DP83822_DEVADDR,
>> +					MII_DP83822_WOL_CFG, value);
> The switch to phy_set_bits_mmd() doesn't seem to be correct here.
> So far bit DP83822_WOL_MAGIC_EN is cleared if WAKE_MAGIC isn't set.
> Similar for bit DP83822_WOL_SECURE_ON. With your change they don't
> get cleared any longer.


Thanks for the review.  I was not watching those bits during my testing 
I will revert back to the original code.

>>   	} else {
>> -		value = phy_read_mmd(phydev, DP83822_DEVADDR,
>> -				     MII_DP83822_WOL_CFG);
>> -		value &= ~DP83822_WOL_EN;
>> -		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
>> -			      value);
>> +		value = DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION;
>> +
> You clear one bit more than before. The reason for this may be worth a note
> in the commit message.

Thanks this was an artifact from debugging.  I can remove the CLR as it 
was cleared in set_wol enablement.

Dan