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 <[email protected]>
>> ---
>> 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;
>> }
>>
>
>
