[PATCH net-next 0/2] net: phy: add functionality to speed down PHY when waiting for WoL packet

[PATCH net-next 0/2] net: phy: add functionality to speed down PHY when waiting for WoL packet

[Heiner Kallweit]

Some network drivers include functionality to speed down the PHY when
suspending and just waiting for a WoL packet because this saves energy.

This patch is based on our recent discussion about factoring out this
functionality to phylib. First user will be the r8169 driver.

Heiner Kallweit (2):
  net: phy: add helper phy_config_aneg
  net: phy: add phy_speed_down and phy_speed_up

 drivers/net/phy/phy.c | 91 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/phy.h   |  2 +
 2 files changed, 89 insertions(+), 4 deletions(-)

-- 
2.18.0

[PATCH net-next 1/2] net: phy: add helper phy_config_aneg

[Heiner Kallweit]

This functionality will also be needed in subsequent patches of this
series, therefore factor it out to a helper.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 537297d2..c4aa360d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -467,6 +467,14 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 }
 EXPORT_SYMBOL(phy_mii_ioctl);
 
+static int phy_config_aneg(struct phy_device *phydev)
+{
+	if (phydev->drv->config_aneg)
+		return phydev->drv->config_aneg(phydev);
+	else
+		return genphy_config_aneg(phydev);
+}
+
 /**
  * phy_start_aneg_priv - start auto-negotiation for this PHY device
  * @phydev: the phy_device struct
@@ -493,10 +501,7 @@ static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
 	/* Invalidate LP advertising flags */
 	phydev->lp_advertising = 0;
 
-	if (phydev->drv->config_aneg)
-		err = phydev->drv->config_aneg(phydev);
-	else
-		err = genphy_config_aneg(phydev);
+	err = phy_config_aneg(phydev);
 	if (err < 0)
 		goto out_unlock;
 
-- 
2.18.0

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

[Heiner Kallweit]

Some network drivers include functionality to speed down the PHY when
suspending and just waiting for a WoL packet because this saves energy.
This functionality is quite generic, therefore let's factor it out to
phylib.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 78 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h   |  2 ++
 2 files changed, 80 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c4aa360d..0547c603 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -551,6 +551,84 @@ int phy_start_aneg(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_start_aneg);
 
+static int phy_poll_aneg_done(struct phy_device *phydev)
+{
+	unsigned int retries = 100;
+	int ret;
+
+	do {
+		msleep(100);
+		ret = phy_aneg_done(phydev);
+	} while (!ret && --retries);
+
+	if (!ret)
+		return -ETIMEDOUT;
+
+	return ret < 0 ? ret : 0;
+}
+
+/**
+ * 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)
+{
+	u32 adv = phydev->lp_advertising & phydev->supported;
+	u32 adv_old = phydev->advertising;
+	int ret;
+
+	if (phydev->autoneg != AUTONEG_ENABLE)
+		return 0;
+
+	if (adv & PHY_10BT_FEATURES)
+		phydev->advertising &= ~(PHY_100BT_FEATURES |
+					 PHY_1000BT_FEATURES);
+	else if (adv & PHY_100BT_FEATURES)
+		phydev->advertising &= ~PHY_1000BT_FEATURES;
+
+	if (phydev->advertising == adv_old)
+		return 0;
+
+	ret = phy_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	return sync ? phy_poll_aneg_done(phydev) : 0;
+}
+EXPORT_SYMBOL_GPL(phy_speed_down);
+
+/**
+ * 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)
+{
+	u32 mask = PHY_10BT_FEATURES | PHY_100BT_FEATURES | PHY_1000BT_FEATURES;
+	u32 adv_old = phydev->advertising;
+	int ret;
+
+	if (phydev->autoneg != AUTONEG_ENABLE)
+		return 0;
+
+	phydev->advertising = (adv_old & ~mask) | (phydev->supported & mask);
+
+	if (phydev->advertising == adv_old)
+		return 0;
+
+	ret = phy_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	return sync ? phy_poll_aneg_done(phydev) : 0;
+}
+EXPORT_SYMBOL_GPL(phy_speed_up);
+
 /**
  * phy_start_machine - start PHY state machine tracking
  * @phydev: the phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6cd09098..275f528e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -942,6 +942,8 @@ void phy_start(struct phy_device *phydev);
 void phy_stop(struct phy_device *phydev);
 int phy_start_aneg(struct phy_device *phydev);
 int phy_aneg_done(struct phy_device *phydev);
+int phy_speed_down(struct phy_device *phydev, bool sync);
+int phy_speed_up(struct phy_device *phydev, bool sync);
 
 int phy_stop_interrupts(struct phy_device *phydev);
 int phy_restart_aneg(struct phy_device *phydev);
-- 
2.18.0

Re: [PATCH net-next 1/2] net: phy: add helper phy_config_aneg

[Andrew Lunn]

On Wed, Jul 11, 2018 at 10:30:27PM +0200, Heiner Kallweit wrote:
> This functionality will also be needed in subsequent patches of this
> series, therefore factor it out to a helper.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

> +/**
> + * 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.

     Andrew

Re: [PATCH net-next 1/2] net: phy: add helper phy_config_aneg

[Florian Fainelli]

On 07/11/2018 01:30 PM, Heiner Kallweit wrote:
> This functionality will also be needed in subsequent patches of this
> series, therefore factor it out to a helper.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Heiner Kallweit]

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.

>      Andrew
> 
Heiner

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

[Florian Fainelli]

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

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

[Heiner Kallweit]

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?

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

[Heiner Kallweit]

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

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

[Andrew Lunn]

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

Hi Heiner

Please put a big fat comment about the dangers of sync=false in the
function header. We want people to known it is dangerous by default,
and should only be used in special conditions, when it is known to be
safe.
	Andrew

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

[Heiner Kallweit]

On 12.07.2018 21:09, Andrew Lunn wrote:
>> 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.
> 
> Hi Heiner
> 
> Please put a big fat comment about the dangers of sync=false in the
> function header. We want people to known it is dangerous by default,
> and should only be used in special conditions, when it is known to be
> safe.
> 	Andrew
> 
OK ..

Heiner

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

[Florian Fainelli]

On 07/12/2018 12:10 PM, Heiner Kallweit wrote:
> On 12.07.2018 21:09, Andrew Lunn wrote:
>>> 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.
>>
>> Hi Heiner
>>
>> Please put a big fat comment about the dangers of sync=false in the
>> function header. We want people to known it is dangerous by default,
>> and should only be used in special conditions, when it is known to be
>> safe.
>> 	Andrew
>>
> OK ..

What part do you find dangerous? Magic Packets are UDP packets and they
are not routed (unless specifically taken care of) so there is already
some "lossy" behavior involved with waking-up an Ethernet MAC, I don't
think that is too bad to retry several times until the link comes up.
-- 
Florian

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

[Florian Fainelli]

On 07/12/2018 12:25 PM, Florian Fainelli wrote:
> 
> 
> On 07/12/2018 12:10 PM, Heiner Kallweit wrote:
>> On 12.07.2018 21:09, Andrew Lunn wrote:
>>>> 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.
>>>
>>> Hi Heiner
>>>
>>> Please put a big fat comment about the dangers of sync=false in the
>>> function header. We want people to known it is dangerous by default,
>>> and should only be used in special conditions, when it is known to be
>>> safe.
>>> 	Andrew
>>>
>> OK ..
> 
> What part do you find dangerous? Magic Packets are UDP packets and they
> are not routed (unless specifically taken care of) so there is already
> some "lossy" behavior involved with waking-up an Ethernet MAC, I don't
> think that is too bad to retry several times until the link comes up.

I see the concern with the comment from v2, and indeed you could get an
interrupt signaling the PHY auto-negotiated the link before or at the
time we are suspending causing potentially an early wake-up. Not that
this should be a problem though since there is usually a point of not
return past which you can't do early wake-up anyway.
-- 
Florian

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

[Heiner Kallweit]

On 12.07.2018 21:53, Florian Fainelli wrote:
> 
> 
> On 07/12/2018 12:25 PM, Florian Fainelli wrote:
>>
>>
>> On 07/12/2018 12:10 PM, Heiner Kallweit wrote:
>>> On 12.07.2018 21:09, Andrew Lunn wrote:
>>>>> 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.
>>>>
>>>> Hi Heiner
>>>>
>>>> Please put a big fat comment about the dangers of sync=false in the
>>>> function header. We want people to known it is dangerous by default,
>>>> and should only be used in special conditions, when it is known to be
>>>> safe.
>>>> 	Andrew
>>>>
>>> OK ..
>>
>> What part do you find dangerous? Magic Packets are UDP packets and they
>> are not routed (unless specifically taken care of) so there is already
>> some "lossy" behavior involved with waking-up an Ethernet MAC, I don't
>> think that is too bad to retry several times until the link comes up.
> 
> I see the concern with the comment from v2, and indeed you could get an
> interrupt signaling the PHY auto-negotiated the link before or at the
> time we are suspending causing potentially an early wake-up. Not that
> this should be a problem though since there is usually a point of not
> return past which you can't do early wake-up anyway.
> 
I think we should leave the comment in for the moment so that people
think twice about the described scenario. If we should find out that
the issue can't be triggered on all platforms then we still can remove
the comment.