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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 11.07.2018 23:33, Florian Fainelli wrote:
> 
> 
> 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.
> 
OK, then I would poll for autoneg finished in phy_speed_down and
remove the polling option from phy_speed_up. I will do some tests
with this before submitting a v2.

> One nit, you might have to check for those functions that the PHY did
> have auto-negotiation enabled and was not forced.
> 
This I'm doing already, or do you mean something different?