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

Message-ID: <35e1541d-1cad-8d72-300f-b9c4d0c27f11@gmail.com>
Date:   Wed, 4 Oct 2017 09:18:39 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Dan Murphy <dmurphy@...com>, andrew@...n.ch
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] net: phy: DP83822 initial driver submission

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

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


-- 
Florian