Date:   Thu, 12 Jul 2018 21:00:37 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] net: phy: add phy_speed_down and
 phy_speed_up

On 11.07.2018 23:59, Heiner Kallweit wrote:
> 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.
> 
Like r8169 also tg3 driver doesn't wait for the speed-down-renegotiation
to finish. Therefore, even though I share Andrew's concerns, there seem
to be chips where it's safe to not wait for the renegotiation to finish
(e.g. because device is in PCI D3 already and can't generate an interrupt).
Having said that I'd keep the sync parameter for phy_speed_down so that
the driver can decide.
For phy_speed_up I'll remove the sync parameter as discussed.


>> 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?
> 

Powered by blists - more mailing lists