netdev - Re: [PATCH RFC 3/7] net: phy: resume PHY only if needed in, mdio_bus_phy

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3402d5b4-aad6-f5aa-ed17-b8c7e91a9d4b@gmail.com>
Date:   Thu, 15 Mar 2018 22:25:16 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     Geert Uytterhoeven <geert+renesas@...der.be>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 3/7] net: phy: resume PHY only if needed in,
 mdio_bus_phy_suspend

Am 15.03.2018 um 00:50 schrieb Florian Fainelli:
> On 03/14/2018 01:16 PM, Heiner Kallweit wrote:
>> Currently the PHY is unconditionally resumed in mdio_bus_phy_suspend().
>> In cases where the PHY was sleepinh before suspending or if somebody else
>> takes care of resuming later, this is not needed and wastes energy.
>>
>> Also start the state machine only if it's used by the driver (indicated
>> by the adjust_link callback being defined).
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/phy/phy_device.c | 33 +++++++++++++++++++++++----------
>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index a5691536f..c6fd79758 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -124,6 +124,18 @@ static bool phy_may_suspend(struct phy_device *phydev)
>>  }
>>  
>>  #ifdef CONFIG_PM
>> +
>> +static bool mdio_bus_phy_needs_start(struct phy_device *phydev)
>> +{
>> +	bool start;
> 
> How about needs_start? This is uber nitpicking but it seems to be more
> in line with what is being tested for here.
> 
Agree ..

>> +
>> +	mutex_lock(&phydev->lock);
>> +	start = phydev->state == PHY_UP && phydev->adjust_link;
> 
> You probably need to add an || phydev->phy_link_change here because that
> is what PHYLINK uses, it does not register an adjust_link callback, but
> would likely expect similar semantics. Even better, introduce a helper
> function that tests for both to avoid possible issues...
> 

phydev->phy_link_change is set in phy_attach_direct(). Therefore it's
always set if the device is attached. And mdio_bus_phy_needs_start()
is only used after we have verified that the device is attached.
Having said that I don't see when phydev->phy_link_change could
be NULL.

When talking about phydev->phy_link_change, why does it exist at all?
I found no driver setting an own callback, replacing the default
phy_link_change(). So we could use the default directly.
Or in which use case would a driver set an own callback?

>> +	mutex_unlock(&phydev->lock);
>> +
>> +	return start;
>> +}
>> +
>>  static int mdio_bus_phy_suspend(struct device *dev)
>>  {
>>  	struct phy_device *phydev = to_phy_device(dev);
>> @@ -142,25 +154,25 @@ static int mdio_bus_phy_suspend(struct device *dev)
>>  static int mdio_bus_phy_resume(struct device *dev)
>>  {
>>  	struct phy_device *phydev = to_phy_device(dev);
>> -	int ret;
>> +	int ret = 0;
>>  
>> -	ret = phy_resume(phydev);
>> -	if (ret < 0)
>> -		return ret;
>> +	if (!phydev->attached_dev)
>> +		return 0;
>>  
>> -	if (phydev->attached_dev && phydev->adjust_link)
>> -		phy_start_machine(phydev);
>> +	if (mdio_bus_phy_needs_start(phydev))
>> +		phy_start(phydev);
>> +	else if (!phydev->adjust_link)
>> +		ret = phy_resume(phydev);
> 
> Humm, under which conditions can you not have phydev->attached_dev and
> also not phydev->adjust_link being set? As mentioned earlier, you would
> likely need to test for phy_link_change too here.
> 
We come here only if phydev->attached_dev is set. If this is the case
and phydev->adjust_link is not set this indicates that the driver
doesn't use the phylib state machine.
And in this case I'd prefer to just call phy_resume().

>>  
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  static int mdio_bus_phy_restore(struct device *dev)
>>  {
>>  	struct phy_device *phydev = to_phy_device(dev);
>> -	struct net_device *netdev = phydev->attached_dev;
>>  	int ret;
>>  
>> -	if (!netdev)
>> +	if (!phydev->attached_dev)
>>  		return 0;
> 
> That does not seem to be making any functional difference, so I would
> just drop this for now.
> 
>>  
>>  	ret = phy_init_hw(phydev);
>> @@ -171,7 +183,8 @@ static int mdio_bus_phy_restore(struct device *dev)
>>  	phydev->link = 0;
>>  	phydev->state = PHY_UP;
>>  
>> -	phy_start_machine(phydev);
>> +	if (mdio_bus_phy_needs_start(phydev))
>> +		phy_start(phydev);
>>  
>>  	return 0;
>>  }
>>
> 
>