Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and phy_speed_up

[Florian Fainelli <f.fainelli@gmail.com>]

On 07/11/2018 02:08 PM, Heiner Kallweit wrote:
> On 11.07.2018 22:55, Andrew Lunn wrote:
>>> +/**
>>> + * phy_speed_down - set speed to lowest speed supported by both link partners
>>> + * @phydev: the phy_device struct
>>> + * @sync: perform action synchronously
>>> + *
>>> + * Description: Typically used to save energy when waiting for a WoL packet
>>> + */
>>> +int phy_speed_down(struct phy_device *phydev, bool sync)
>>
>> This sync parameter needs some more thought. I'm not sure it is safe.
>>
>> How does a PHY trigger a WoL wake up? I guess some use the interrupt
>> pin. How does a PHY indicate auto-neg has completed? It triggers an
>> interrupt. So it seems like there is a danger here we suspend, and
>> then wake up 2 seconds later when auto-neg has completed.
>>
>> I'm not sure we can safely suspend until auto-neg has completed.
>>
>>> +/**
>>> + * phy_speed_up - (re)set advertised speeds to all supported speeds
>>> + * @phydev: the phy_device struct
>>> + * @sync: perform action synchronously
>>> + *
>>> + * Description: Used to revert the effect of phy_speed_down
>>> + */
>>> +int phy_speed_up(struct phy_device *phydev, bool sync)
>>
>> And here, i'm thinking the opposite. A MAC driver needs to be ready
>> for the PHY state to change at any time. So why do we need to wait?
>> Just let the normal mechanisms inform the MAC when the link is up.
>>
> I see your points, thanks for the feedback. In my case WoL triggers
> a PCI PME and the code works as expected, but I agree this may be
> different in other setups (external PHY).
> 
> The sync parameter was inspired by following comment from Florian:
> "One thing that bothers me a bit is that this should ideally be
> offered as both blocking and non-blocking options"
> So let's see which comments he may have before preparing a v2.

What I had in mind is that you would be able to register a callback that
would tell you when auto-negotiation completes, and not register one if
you did not want to have that information.

As Andrew points out though, with PHY using interrupts, this might be a
bit challenging to do because you will get an interrupt about "something
has changed" and you would have to run the callback from the PHY state
machine to determine this was indeed a result of triggering
auto-negotiation. Maybe polling for auto-negotiation like you do here is
good enough.

One nit, you might have to check for those functions that the PHY did
have auto-negotiation enabled and was not forced.
-- 
Florian