[PATCH] r8169: Enable suspend when device is idle from boot.

[PATCH] r8169: Enable suspend when device is idle from boot.

[Todd Broch]

The r8169 driver supports power management and correctly transitions
from active to suspend when link transitions from connected to
disconnected.  However, if link is not connected at boot the device
remains active after the initial probe.

This change adds a check of the link status to the idle pm_op that
will schedule a suspend if inactive.

Signed-off-by: Todd Broch <tbroch@chromium.org>
---
 drivers/net/ethernet/realtek/r8169.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 67bf078..3dbb5fd 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6137,6 +6137,7 @@ static int rtl8169_runtime_idle(struct device *device)
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct rtl8169_private *tp = netdev_priv(dev);
 
+	__rtl8169_check_link_status(dev, tp, tp->mmio_addr, true);
 	return tp->TxDescArray ? -EBUSY : 0;
 }
 
-- 
1.7.3.1

Re: [PATCH] r8169: Enable suspend when device is idle from boot.

[David Miller]

From: Todd Broch <tbroch@chromium.org>
Date: Thu, 29 Dec 2011 10:33:14 -0800

> The r8169 driver supports power management and correctly transitions
> from active to suspend when link transitions from connected to
> disconnected.  However, if link is not connected at boot the device
> remains active after the initial probe.
> 
> This change adds a check of the link status to the idle pm_op that
> will schedule a suspend if inactive.
> 
> Signed-off-by: Todd Broch <tbroch@chromium.org>

Francois, what would you like me to do with this patch?

Thanks.

Re: [PATCH] r8169: Enable suspend when device is idle from boot.

[Francois Romieu]

David Miller <davem@davemloft.net> :
[...]
> Francois, what would you like me to do with this patch?

I have not tested it yet. I have no objection if a fix must go in now.

There is a slot for some sanity testing this evening.

The description of the patch implies that the initial power management
state is not right. I would be more inclined to set it correctly when
the device goes up instead of checking repeatedly for a loss of sync
through rtl8169_runtime_idle. Todd, any comment ?

-- 
Ueimor

Re: [PATCH] r8169: Enable suspend when device is idle from boot.

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 31 Dec 2011 13:17:04 +0100

> David Miller <davem@davemloft.net> :
> [...]
>> Francois, what would you like me to do with this patch?
> 
> I have not tested it yet. I have no objection if a fix must go in now.

There is no rush with this, I just was seeking your opinion :-)

Re: [PATCH] r8169: Enable suspend when device is idle from boot.

[Francois Romieu]

Francois Romieu <romieu@fr.zoreil.com> :
[...]
> The description of the patch implies that the initial power management
> state is not right. I would be more inclined to set it correctly when
> the device goes up instead of checking repeatedly for a loss of sync
> through rtl8169_runtime_idle. Todd, any comment ?

... which could be as simple as (completely untested) :

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index c8f47f1..e9fbd41 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4309,7 +4309,7 @@ static int rtl8169_open(struct net_device *dev)
 	tp->saved_wolopts = 0;
 	pm_runtime_put_noidle(&pdev->dev);
 
-	rtl8169_check_link_status(dev, tp, ioaddr);
+	__rtl8169_check_link_status(dev, tp, ioaddr, true);
 out:
 	return retval;
 

Re: [PATCH] r8169: Enable suspend when device is idle from boot.

[Olof Johansson]

On Tue, Jan 3, 2012 at 3:30 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Francois Romieu <romieu@fr.zoreil.com> :
> [...]
>> The description of the patch implies that the initial power management
>> state is not right. I would be more inclined to set it correctly when
>> the device goes up instead of checking repeatedly for a loss of sync
>> through rtl8169_runtime_idle. Todd, any comment ?
>
> ... which could be as simple as (completely untested) :

That change is just to the code path of open(), which means that for
the power savings to kick in, you have to bring the interface up. The
point of the original patch is to check even if the interface has
never been touched.

Perhaps call check_link_status at the end if init_one instead to
activate pm then?


-Olof

Re: [PATCH] r8169: Enable suspend when device is idle from boot.

[Francois Romieu]

Olof Johansson <olof@lixom.net> :
> On Tue, Jan 3, 2012 at 3:30 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> > Francois Romieu <romieu@fr.zoreil.com> :
> > [...]
> >> The description of the patch implies that the initial power management
> >> state is not right. I would be more inclined to set it correctly when
> >> the device goes up instead of checking repeatedly for a loss of sync
> >> through rtl8169_runtime_idle. Todd, any comment ?
> >
> > ... which could be as simple as (completely untested) :
> 
> That change is just to the code path of open(), which means that for
> the power savings to kick in, you have to bring the interface up. The
> point of the original patch is to check even if the interface has
> never been touched.

Fair enough.

If so the original patch should not be needed as the driver core is
supposed to invoke the idle callback after probe (aka rtl8169_init_one).
After probe the device is in TxDescArray == 0 state and rtl8169_runtime_idle
returns 0. pm_runtime_suspend should thus be called [*]... I must have
missed something. :o/

[*] Not sure if it always imply a transition to D3 though.

> Perhaps call check_link_status at the end if init_one instead to
> activate pm then ?

It should not be needed, see above.

Moreover one should not care for the link status until the device is
brought up. It may seem a bit counterintuitive but the device and its
PHY are tightly coupled here. You do not manage them separately.

You should forget it anyway: PHY init happens late in the r8169 driver
and it can not be done sooner as it sometimes depends on firmware loading.

-- 
Ueimor

Re: [PATCH] r8169: Enable suspend when device is idle from boot.

[Francois Romieu]

Todd Broch <tbroch@chromium.org> :
> Thanks for suggestion and patch.  Tried it on system I have and it did not
> place device in runtime suspend (D3) from initialization.  It appears that
> pm_schedule_suspend returns an error (-11, EAGAIN?).  Going to investigate
> more today and will comment further then.

Did you find anything new ?

If I insert the r8169 module then enable runtime PM control on an unused
r8169 device (no cable, never brought up), rtl8169_runtime_suspend is 
called. Same thing when runtime PM control is enabled before the module
is inserted.

I would not mind http://patchwork.ozlabs.org/patch/133590/ going
'Not Applicable' :o)

-- 
Ueimor