netdev - Re: [PATCH] net: phy: DP83822 initial driver submission

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]

Date:   Wed, 4 Oct 2017 11:22:26 -0500
From:   Dan Murphy <dmurphy@...com>
To:     Florian Fainelli <f.fainelli@...il.com>, <andrew@...n.ch>
CC:     <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: phy: DP83822 initial driver submission

Florian

On 10/04/2017 11:18 AM, Florian Fainelli wrote:
> On 10/04/2017 08:41 AM, Dan Murphy wrote:
>> Florian
>>
>> On 10/03/2017 01:31 PM, Florian Fainelli wrote:
>>> On 10/03/2017 11:03 AM, Dan Murphy wrote:
>>>> Florian
>>>>
>>>> Thanks for the review
>>>>
>>>> On 10/03/2017 12:15 PM, Florian Fainelli wrote:
>>>>>> +		} else {
>>>>>> +			value &= ~DP83822_WOL_SECURE_ON;
>>>>>> +		}
>>>>>> +
>>>>>> +		value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |
>>>>>> +			  DP83822_WOL_CLR_INDICATION);
>>>>>
>>>>> The extra parenthesis should not be required here.
>>>>
>>>> I did not code that in.  I had to add it after Checkpatch cribbed about it.
>>>> Let me know if you want me to remove it.
>>>
>>> Let's keep those, that does not change much.
>>>
>>>>
>>>>>
>>>>>> +		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
>>>>>> +			      value);
>>>>>> +	} else {
>>>>>> +		value =
>>>>>> +		    phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
>>>>>> +		value &= (~DP83822_WOL_EN);
>>>>>
>>>>> Same here, parenthesis should not be needed.
>>>>
>>>> There are three lines of code in the else.  This code all needs to be excuted in the else case.
>>>> I might reformat it to read better.  Lindent messed that one up.
>>>
>>> sorry, I meant to write that you don't need the parenthesis around
>>> DP83822_WOL_EN since that is just a single bit here.
>>>
>>> [snip]
>>>
>>>>>> +
>>>>>> +	mutex_unlock(&phydev->lock);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dp83822_resume(struct phy_device *phydev)
>>>>>> +{
>>>>>> +	int value;
>>>>>> +
>>>>>> +	mutex_lock(&phydev->lock);
>>>>>> +
>>>>>> +	value = phy_read(phydev, MII_BMCR);
>>>>>> +	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);
>>>>>
>>>>> And genphy_resume() here as well?
>>>>
>>>> genphy_resume does not have WoL.
>>>
>>> I should have been cleared, I meant using genphy_{suspend,resume} to
>>> avoid open coding the setting of the BMCR_PDOWN bit, conversely clearing
>>> of that bit. Because of the locking, maybe you could introduce unlocked
>>> versions of these two routines, or you acquire and release the lock
>>> outside of genphy_{suspend,resume}?
>>
>> OK I have addressed all of the open comments and will be posting v2 shortly.
>>
>> I do have a question on this request.
>> Are you indicating to exclusively call genphy_(suspend/resume) from within the over ridden
>> routine and then take care of the WoL bits in the phy specific code?
>>
>> Since the genphy code is duplicated here for the BMCR value that would make the most sense
>> to me.  The genphy code is exported so I can call it explicitly then do the WoL 
> 
> I would expect you to wrap your own calls around genphy_suspend and
> genphy_resume, such your functions become:
> 
> static int dp83822_suspend(struct phy_device *phydev)
> {
> 	int value;
> 
> 	mutex_lock(&phydev->lock);
> 	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
> 	mutex_unlock(&phydev->lock);
> 
> 	if (~value & DP83822_WOL_EN)
> 		genphy_suspend(phydev);
> }
> 
> static int dp83822_resume(struct phy_device *phydev)
> {
> 	int value;
> 
> 	genphy_resume(phydev);
> 
> 	mutex_lock(&phydev->lock);
> 	value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
> 
> 	phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |
> 		      DP83822_WOL_CLR_INDICATION);
> 
> 	mutex_unlock(&phydev->lock);
> 
> 	return 0;
> }
> 

Great thats exactly what I did.

Dan

>>
>> Dan
>>
>> [snip]-- 
>> ------------------
>> Dan Murphy
>>
> 
> 


-- 
------------------
Dan Murphy